💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480568701)
`remote_block_before` is set before the two `submitBlock` calls above. I improved the wording a bit and I moved the `getchaintips()[0]["height"]` down to the next test, because that was probably confusing.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3471963619)
Pushed [7d57c4e](https://github.com/bitcoin/bitcoin/commit/7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4) to [2fc95ca](https://github.com/bitcoin/bitcoin/commit/2fc95ca731b52ca419215109c568e89bf683a94b): [compare](https://github.com/bitcoin/bitcoin/compare/7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4..2fc95ca731b52ca419215109c568e89bf683a94b). No behavior change.

I added a patch the to PR description that reproduces the original issue.

Expanded the `submitSolution` documentation to indicate that a
...

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480606043)
Added a patch to the PR description which reproduces the issue.

📝 A-Manning opened a pull request: "rest: Query predecessor headers using negative count param"
(https://github.com/bitcoin/bitcoin/pull/33752)
There is currently no mechanism by which to batch-query headers from tip, backwards towards genesis.
This PR extends the REST Blockheaders API to support batch-querying predecessor headers, by providing a negative count parameter.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472031005)
@ryanofsky wrote:

> I do think more improvements could be made here:

These could be followups, so will comment here:

> - documentation could be updated to say what requirements on submitSolution inputs are

Not sure what to write there, basically whatever consensus requires. Previously mining software would the whole coinbase from scratch and we never really documented that.

The Stratum v2 spec does specify what their `SubmitSolution` should look like: https://stratumprotocol.org/
...

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480649764)
I kept all three, but as I explained in https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3471963619 two of them are already `false`.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480669020)
If you apply the patch in the description and also drop the `sync_all` call, you'll see that the second node see the block as `headers-only`.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472081330)
cc @ajtowns

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480717926)
I think this was a copy-paste thing from elsewhere in the code (this "dummy" comment appears at line 111 as well, and seems to have been introduced when this fuzz test was first created) -- I presume the origin is just that the MemPoolRemovalReason doesn't matter for how `removeRecursive()` behaves, it's just passed on to callbacks.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480723944)
Incorporated in 0097138011b6

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480726400)
Done in d00d8a8a84e43cb79e81482a2a85532209dd9ab2

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480727366)
Done in d00d8a8a84e43cb79e81482a2a85532209dd9ab2

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480729561)
Done in 74654eaf1bef5b578b97111af20b3de39c39cec6

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480730805)
Done in 74654eaf1bef5b578b97111af20b3de39c39cec6

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2480733194)
Done in 0097138011b6e502bfbae528304a6a7e6b6462ad

💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3472140702)
> Guix build for 1e6aa74b4b628e8acafe6a3d2aaf2a6aa0d9d290:

Thanks. However you should do a build for all `HOSTS`. As this is changing code that effects all `HOSTS`.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3472188061)
I've addressed most of the review feedback, other than the mutation testing notes from @brunoerg which I still need to review (thank you for the analysis!), and some documentation items that were brought up [here](https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440825043) and [here](https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440904975).

I'll work on incorporating the documentation improvements in #33591.

👍 dergoegge approved a pull request: "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn"
(https://github.com/bitcoin/bitcoin/pull/33743#pullrequestreview-3403414040)
utACK fa4b52bd16189d40761c5976b8427e30779aba23

🤔 stickies-v reviewed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3403430467)
Concept ACK: reindex validation behaviour being different to IBD validation behaviour is undesirable and should be avoided.

Slight approach NACK: I think this is probably the smallest diff we can get to improve reindex behaviour, but I think there are drawbacks that outweigh the benefits:
1) it still doesn't make IBD and reindex validation behaviour the same: with the current approach blocks that are NOT part of the assumevalid chain will still be validated as assumevalid. While it doesn't s
...

✅ fanquake closed an issue: "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null"
(https://github.com/bitcoin/bitcoin/issues/33643)