💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348447122)
https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: I think it's best to take out this TODO from the commit. You should also adjust the commit message to focus on what this commit is adding specifically.

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348463061)
https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: A comment here will also be beneficial.

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348538334)
https://github.com/bitcoin/bitcoin/pull/33336/commits/63dc937eea297332ec85c1abbcf3b94b8f74aa85:
should be "block not on best header chain"

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348503453)
https://github.com/bitcoin/bitcoin/pull/33336/commits/6222d8cced4828b83a9dde20b9da9df44848b9bd:
Why not `AssumeValid::DISABLED` instead of `AssumeValid::CHECKED` and `AssumeValid::ENABLED` instead of `AssumeValid:SKIPPED`?
I find the current names to be confusing. I think using `AssumeValid::ENABLED` to indicate that assume_valid is enabled is better.

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348449770)
https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: This should probably still be set to `3` since the 4th node is not used in this commit.

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348462197)
https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: A comment explaining this check will be beneficial.

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348540085)
https://github.com/bitcoin/bitcoin/pull/33336/commits/63dc937eea297332ec85c1abbcf3b94b8f74aa85:
consider "block not part of assumevalid chain"

💬 Eunovo commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2348292328)
https://github.com/bitcoin/bitcoin/pull/33336/commits/63dc937eea297332ec85c1abbcf3b94b8f74aa85:
+1 on returning "unknown reason" instead of aborting.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3291494499)
Rebased on master and added a comment explaining why we bother to skip minimumchainwork, even though we expect minimumchainwork to match assumevalid block height most times.

💬 0xB10C commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3291550859)
Running v30.0rc1 on a couple of [peer.observer](https:///public.peer.observer) nodes - will report if I see something suspicious.

👍 ismaelsadeeq approved a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210#pullrequestreview-3224092444)
Code review ACK c9a7a198d9e81e99de99a2aaff1687d13d6674e8
with a comment and nit happy to reACK when you amend my suggestion

💬 ismaelsadeeq commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2348652999)
In "fuzz: mock CBlockPolicyEstimator in wallet_fuzz" ff10a37e99271125a9ece92bae571f7b78fb9e22

It would be nice if `FuzzedBlockPolicyEstimator` were renamed to `MockedBlockPolicyEstimator`.
It could then be moved to `src/test/util/` so that it can be reused by other components that might need a fee rate estimate as well.

💬 ismaelsadeeq commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2348701041)
In "fuzz: create FeeEstimatorTestingSetup to set fee_estimator" adf67eb21baf39a222b65480e45ae76f093e8f66

nit: It will be nice if we don't have to set the fee estimator and just initialize it in the `FeeEstimatorTestingSetup` constructor, but we have to find a way to pass the fuzz data provider as well.

💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348807001)
re: https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348144204

I think it makes sense if method order order in `.capnp` files just matches the method order in `.h` files so the declarations are easy to compare.

Since we haven't needed binary compatibility before, we've just renumbered when making any changes, so numbers have been in order up to this point. But if we want to start having binary compatibility, we won't be able to reassign numbers anymore, and it'll be normal for th
...

⚠️ ryanofsky reopened an issue: "Assertion hit on shutdown of bitcoin-node with connected mining interface client"
(https://github.com/bitcoin/bitcoin/issues/33387)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Running c0894a0a2be032cd9a5d5945643689230ab10255 `bitcoin-node` with Sjor's [sv2-tp] connected resulted in an unclean shutdown:
```
2025-09-14T10:24:34Z CreateNewBlock(): block weight: 460455 txs: 371 fees: 230840 sigops 1336
2025-09-14T10:25:05Z CreateNewBlock(): block weight: 513153 txs: 441 fees: 290027 sigops 1489
2025-09-14T10:25:36Z CreateNewBlock(): block weight: 635048 txs: 541 f
...

💬 ryanofsky commented on issue "Assertion hit on shutdown of bitcoin-node with connected mining interface client":
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3291934337)
Will reopen since I think this is probably worth following up on, even if it may not be an urgent priority.

If I'm understanding the problem correctly, it might not be to hard to reproduce with a functional test calling IPC methods during shutdown, though the test would be inherently racy. It might be easier to reproduce reliably with a unit test.

👍 ryanofsky approved a pull request: "mining: add applySolution() to interface"
(https://github.com/bitcoin/bitcoin/pull/33374#pullrequestreview-3224409033)
Concept ACK, this does seem like a more general solution than 33372.

💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348861453)
> re: [#33374 (comment)](https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348144204)

Looking at the PR more, since this is just adding a new method and should be backwards compatible, probably nothing in my comment above applies here, other than that I think it's reasonable for applySolution to follow submitSolution in the .capnp file like it does in the .h file, and for the methods to not be renumbered or sorted by number.

💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348937808)
> Another thing that's not great here is that `m_block_template->block` is mutated.

It does seem nicer to not to mutate the block template. And as long as entire block is being serialized and sent over the socket anyway, probably the cost of copying it is not significant.

But one thought is that maybe serializing whole block is not necessary, and this could just return CBlockHeader?

Another idea is that this could be changed to use output parameters, and the client could decide what inf
...

📝 purpleKarrot opened a pull request: "cmake: exclude secp256k1 from all"
(https://github.com/bitcoin/bitcoin/pull/33390)
Instead of setting the EXCLUDE_FROM_ALL target property, pass EXCLUDE_FROM_ALL to `add_subdirectory()`.

This has the following advanteges:

* It is shorter (obviously).
* Target properties are set only in the `CMakeLists.txt` file that defines the target.
* Install rules defined in the subdirectory are excluded as well. This is what we want, because secp256k1 is linked statically.