💬 TheBlueMatt commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3478062912)
I was misled by https://github.com/bitcointranscripts/bitcointranscripts/pull/593/files#diff-e25cced7d45bde5a991f8ac638162335638ea1ba39b21a5cb76d5e5e31b31345R9-R10
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3478062912)
I was misled by https://github.com/bitcointranscripts/bitcointranscripts/pull/593/files#diff-e25cced7d45bde5a991f8ac638162335638ea1ba39b21a5cb76d5e5e31b31345R9-R10
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3478064586)
> I was misled by https://github.com/bitcointranscripts/bitcointranscripts/pull/593/files#diff-e25cced7d45bde5a991f8ac638162335638ea1ba39b21a5cb76d5e5e31b31345R9-R10
Ok, I see, that could have been more clear as either "depends on `rpki-client`" (the only dependency outside of python) or "depends on RPKI, IRR and Routeviews downloads" or something. I will clarify in the PR.
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3478064586)
> I was misled by https://github.com/bitcointranscripts/bitcointranscripts/pull/593/files#diff-e25cced7d45bde5a991f8ac638162335638ea1ba39b21a5cb76d5e5e31b31345R9-R10
Ok, I see, that could have been more clear as either "depends on `rpki-client`" (the only dependency outside of python) or "depends on RPKI, IRR and Routeviews downloads" or something. I will clarify in the PR.
💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486365567)
Generally yes, but here I wanted to copy the [original code](https://github.com/bitcoin/bitcoin/pull/33637/files#diff-2e16a7ac99a65c1617112c3c326fba499b0010fe6c48b92b59bbc9696d883585L104) as closely as possible
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486365567)
Generally yes, but here I wanted to copy the [original code](https://github.com/bitcoin/bitcoin/pull/33637/files#diff-2e16a7ac99a65c1617112c3c326fba499b0010fe6c48b92b59bbc9696d883585L104) as closely as possible
💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487174964)
http://github.com/bitcoin/bitcoin/pull/32604/commits/d541409a64c60d127ff912dad9dea949d45dbd8c#diff-44fd50b51e8fc6799d38f193237fb921ec9d34306c448f64837524a17bac06eeR471-R472 introduced prefixing all messages with `[*]` to indicate that ratelimiting is currently being applied.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487174964)
http://github.com/bitcoin/bitcoin/pull/32604/commits/d541409a64c60d127ff912dad9dea949d45dbd8c#diff-44fd50b51e8fc6799d38f193237fb921ec9d34306c448f64837524a17bac06eeR471-R472 introduced prefixing all messages with `[*]` to indicate that ratelimiting is currently being applied.
💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498620712)
I'm also not a fan of globally disabling these, they could hide [real problems](https://github.com/bitcoin/bitcoin/pull/32313/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248).
Doing a `git grep -n "const auto&.*Assert("` shows there are only a few of these
```
src/index/coinstatsindex.cpp:204: const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
src/init.cpp:2006: const auto& tip{*Assert(chainman.ActiveTip())};
src/node/miner.c
...
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498620712)
I'm also not a fan of globally disabling these, they could hide [real problems](https://github.com/bitcoin/bitcoin/pull/32313/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248).
Doing a `git grep -n "const auto&.*Assert("` shows there are only a few of these
```
src/index/coinstatsindex.cpp:204: const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
src/init.cpp:2006: const auto& tip{*Assert(chainman.ActiveTip())};
src/node/miner.c
...
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503646384)
> Sorry, but one more rebase is needed.
I think I've rebased correctly here -- I removed an additional `SKIP_BUILD_RPATH OFF` override that seemed like it would no longer be needed, https://github.com/bitcoin/bitcoin/commit/a02282d20c87471a3399e4061c7edad7ecdb391f#diff-2a07660db8ebdbbf967cf42f6b4da3ffc8558a322f45a3a08ce292c9e91767b8L13
Guix build for a02282d20c87:
a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0e31a0ed9 guix-build-a02282d20c87/output/aarch64-linux-gnu/SHA256SUMS
...
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503646384)
> Sorry, but one more rebase is needed.
I think I've rebased correctly here -- I removed an additional `SKIP_BUILD_RPATH OFF` override that seemed like it would no longer be needed, https://github.com/bitcoin/bitcoin/commit/a02282d20c87471a3399e4061c7edad7ecdb391f#diff-2a07660db8ebdbbf967cf42f6b4da3ffc8558a322f45a3a08ce292c9e91767b8L13
Guix build for a02282d20c87:
a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0e31a0ed9 guix-build-a02282d20c87/output/aarch64-linux-gnu/SHA256SUMS
...
🤔 hebasto reviewed a pull request: "refactor: Add missing include in bitcoinkernel_wrapper.h"
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.
[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
-#include <kern
...
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.
[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
-#include <kern
...
💬 l0rinc commented on pull request "scripted-diff: Remove obsolete comment":
(https://github.com/bitcoin/bitcoin/pull/33826#issuecomment-3506770544)
code review ACK 36724205fc1f226d7b5493ed0212c336e7f2ae84
Looks like this is a leftover from [before the cmake migration](https://github.com/bitcoin/bitcoin/commit/6ce50fd9d0ae6850d54bf883e7a7c1bcb6912c5c#diff-7533d479b9e3ad56dcfbf6daaa21317373e8233da5ea13714554115c66057aa2L22-L132).
Checked locally, other Python 3 references seem unrelated.
(https://github.com/bitcoin/bitcoin/pull/33826#issuecomment-3506770544)
code review ACK 36724205fc1f226d7b5493ed0212c336e7f2ae84
Looks like this is a leftover from [before the cmake migration](https://github.com/bitcoin/bitcoin/commit/6ce50fd9d0ae6850d54bf883e7a7c1bcb6912c5c#diff-7533d479b9e3ad56dcfbf6daaa21317373e8233da5ea13714554115c66057aa2L22-L132).
Checked locally, other Python 3 references seem unrelated.
💬 maflcko commented on pull request "refactor: Add missing include in bitcoinkernel_wrapper.h":
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511768897)
> [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511768897)
> [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
💬 hebasto commented on pull request "refactor: Add missing include in bitcoinkernel_wrapper.h":
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511843037)
> > [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
>
> I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
It's here: https://github.com/hebasto/bitcoin/actions/runs/19194191546/job/54872770781:
```
<snip>
2025-11-08T14:21:35.1873049Z (/home/runner/work/_temp/src/kernel/bitcoinkernel_wrapper.h
...
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511843037)
> > [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
>
> I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
It's here: https://github.com/hebasto/bitcoin/actions/runs/19194191546/job/54872770781:
```
<snip>
2025-11-08T14:21:35.1873049Z (/home/runner/work/_temp/src/kernel/bitcoinkernel_wrapper.h
...
💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521439654)
> Since when is this "unused"?
Did some digging, https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 inlined `ComputeMerkleRoot` in one case, leaving the `proot` and `pmutated` values unused in `MerkleComputation` (cc: @sipa, @Sjors).
And `BlockWitnessMerkleRoot` was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already
...
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521439654)
> Since when is this "unused"?
Did some digging, https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 inlined `ComputeMerkleRoot` in one case, leaving the `proot` and `pmutated` values unused in `MerkleComputation` (cc: @sipa, @Sjors).
And `BlockWitnessMerkleRoot` was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already
...
💬 rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527400928)
In d1a2e278572f70d0d68a21312b2cc7de27f326df "test: replace class ValidWitnessMalleatedTx with function"
Unlike the `malleate_tx_to_invalid_witness` function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn't seem generic enough to be present in an util file. Few points regarding the function I noticed:
1. The name doesn't reflect the implementation, the fu
...
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527400928)
In d1a2e278572f70d0d68a21312b2cc7de27f326df "test: replace class ValidWitnessMalleatedTx with function"
Unlike the `malleate_tx_to_invalid_witness` function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn't seem generic enough to be present in an util file. Few points regarding the function I noticed:
1. The name doesn't reflect the implementation, the fu
...