💬 achow101 commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511292626)
In 26442fec572da263765c576c080186c84cee1615 "bench: Add wallet encryption benchmark"

We've generally not had "measure overhead" in the benchmarks. If this is because encryption is actually a very small amount of time, I would suggest generating significantly more keys that need to actually be encrypted.

💬 achow101 commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511269902)
In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a "refactor: Generalize derivation target calculation"

This comment does not seem helpful, I'm not sure what information it is trying to convey.

💬 achow101 commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511273130)
In 26442fec572da263765c576c080186c84cee1615 "bench: Add wallet encryption benchmark"

This part of setup is unnecessary. Encryption does not encrypt destinations or transactions. Generating new destinations does not result in more things to be encrypted.

If the idea was to have a bunch of keys to be encrypted for the benchmark, then several independent descriptors would need to be imported.

💬 fanquake commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3512823435)
> Thanks. I've pulled those two in here.

Looking at the CI. Those commits are not going to work as implemented (essentially anywhere we are using Clang).

💬 achow101 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-3512825537)
> Needs rebase.

Doesn't seem like it does?

🚀 achow101 merged a pull request: "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response"
(https://github.com/bitcoin/bitcoin/pull/32517)

⚠️ hebasto opened an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(https://github.com/bitcoin/bitcoin/issues/33846)
```
$ sudo apt update
$ sudo apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
$ git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin
$ cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j $(nproc)
<snip>
[ 76%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp:17:10: fatal error: format: No such file or directory
17 | #include <format>

...

💬 hebasto commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3512866067)
cc @TheCharlatan

💬 achow101 commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3512874868)
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0

💬 fanquake commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3512913726)
Forgot to re-add the fuzz test from #33843 (https://github.com/bitcoin/bitcoin/pull/33843/commits/8e4084f7b6d51b3fd1e5f299c5e7531f5885b2cd) back here?

💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2511344463)
https://en.wikipedia.org/wiki/Ratio#Notation_and_terminology ([archive link](https://web.archive.org/web/20250918113648/https://en.wikipedia.org/wiki/Ratio#Notation_and_terminology))

This comment explains why the assignment below is correct. This can be read as "target_iterations is to elapsed_iterations as target_time is to elapsed_time", this is mostly equivalent to: $\frac{\text{targetiterations}}{\text{elapsediterations}} = \frac{\text{targettime}}{\text{elapsedtime}}$

💬 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
...