💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511348221)
Fair enough, I think this is adding a little more information than the comment above the for loop, but I'll remove when/if retouching.

🚀 achow101 merged a pull request: "refactor: inline constant return values from `dbwrapper` write methods"
(https://github.com/bitcoin/bitcoin/pull/33042)

💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511359877)
I responded here: https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511344463

I'll mark this resolved and we can discuss there.

💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511369749)
Really? I couldn't find this in `doc/` and I find a lot of counter-examples when doing:

```
git grep -C 2 "// .*\." '*.cpp'
```

🤔 ismaelsadeeq reviewed a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3444353769)
Code review and tested ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704

This looks really good. I have verified that previously, we would indeed accept a block with an invalid witness commitment because we did not check for it. The new toggle of the cached flags to `false` in the first commit prevents us from doing so.
The added documentation also looks good.

Before the patch in the first commit, the added test in the second commit failed
```terminal
2025-11-10T17:14:58.517226Z TestFramework (IN
...

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3513030399)
Rebased, ready for review.

💬 optout21 commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3513147182)
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

💬 frankomosh commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2511549180)
> adding an error for TOTAL_TRIES, similar to `ErrorMaxWeightExceeded()` ought to be straightforward.
>
Would be better for a follow-up PR though?

🤔 Crypt-iQ reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3444281200)
Reviewed 32bfd6136134e7194ea0b56ec08498f66829fc40 and left some test-related comments. I think if the use-case brought up by @ajtowns is not a concern, then I can ACK.

Separately, I've been thinking about how a refactor would look. I'm still at a loss because unifying the headers and the compact blocks processing logic seems difficult because sometimes the compact block can be processed as a headers message (i.e. calls `ProcessHeadersMessage`) and there are compact blocks-specific checks that
...

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2511530460)
Since `stalling_peer` is low-bandwidth, the compact block is dropped and the full block is requested (taking up the first slot) in `SendMessages` since `self.nodes[0]` has processed the header. The test still passes, but I think what this is testing has changed slightly from the original. The original behavior can be maintained if `stalling_peer` sends the header first and then sends over the compact block.

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2511329708)
Commit message of 42e281dba0f0807cc08d8c50a9d19906e27e2129 can change to say "non-HB peers sending ~~un~~solicited invalid"

💬 frankomosh commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#issuecomment-3513298652)
Code review ACK b189a34

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3513328432)
@maflcko, what happened to our friend, @DrahtBot?

💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-3513450348)
I guess not. Just tried a merge with current master, which builds fine and tests pass.

💬 janb84 commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3513471282)
i'm wondering if this build uses more than 38 GiB, my guix build just failed for this PR. it just has slightly more than 38 GiB available so it passes the check but just failed on no more space on disk.

Will do another run to check :)

💬 yancyribbens commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2511732349)
> Would be better for a follow-up PR though?

Well currently the only reason to add it would be to make the test nicer, which I don't think is a good enough reason. The rust version is meant for any wallet implementation, and I could imagine a wallet maybe wanting to try a retry strategy (with less inputs) if the total tries are hit. So far, the core wallet doesn't do anything fancy like that afaik.

💬 willcl-ark commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3513567925)
My build:

```
x86_64
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe72f917be109a68a2103e915183ed4fe01947 guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu.tar.gz
73a257f64eb6
...

👍 willcl-ark approved a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3444844067)
ACK f06c6e18983139dd63873b3537d2f87b8c6ec752

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511014559)
In commit "Allow moving CTxMemPoolEntry objects, disallow copying"

Ultranit: `push_back` instead of `emplace_back` (no real downside to using emplace_back, but push_back makes it more clear that nothing is really being constructed in-place).

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511137437)
In commit "Add sigops adjusted weight calculator"

This can be done more accurately (avoiding the rounding):
```c++
return {feerate.fee * WITNESS_SCALE_FACTOR, feerate.size};
```

Would that be desirable? It's used inside `BlockAssembler::addChunks` where it would be useful I think, but also in `TxMempool` where it's less clear to me if sticking to the old rounding-up behavior may be necessary.

One potential pitfall is that the resulting `size` member variable of the returned `FeePerVS
...