💬 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?
(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)
(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>
...
(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
(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
(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?
(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}}$
(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.
(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)
(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.
(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'
```
(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
...
(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.
(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
(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?
(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
...
(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.
(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"
(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
(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?
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3513328432)
@maflcko, what happened to our friend, @DrahtBot?