💬 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
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542843754)
I don't have Windows, it's why I asked if instead of adding a new method we can just delete the conversions, somewhat similarly to the mentioned https://github.com/bitcoin/bitcoin/pull/33550/files#diff-69423eb01bf14b3bd0d930c0b3e1fd6f4f061ffefacab579053eaa734fc22f38R65 (since the error seemed superficially similar to me).
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542843754)
I don't have Windows, it's why I asked if instead of adding a new method we can just delete the conversions, somewhat similarly to the mentioned https://github.com/bitcoin/bitcoin/pull/33550/files#diff-69423eb01bf14b3bd0d930c0b3e1fd6f4f061ffefacab579053eaa734fc22f38R65 (since the error seemed superficially similar to me).
👍 l0rinc approved a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3484132142)
untested code review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa
This is a continuation of https://github.com/bitcoin/bitcoin/pull/30660/files#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR326, separating the existing `TimeoutError` to add extra details for `ConnectionResetError` as well.
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3484132142)
untested code review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa
This is a continuation of https://github.com/bitcoin/bitcoin/pull/30660/files#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR326, separating the existing `TimeoutError` to add extra details for `ConnectionResetError` as well.
💬 fanquake commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3557317662)
> This [branch](https://github.com/hebasto/bitcoin/tree/251119-qt6.9) fails to cross-compile on Debian Bookworm with GCC 12.2.0:
That's the same issue we work around in CMake, which is fixed by just turning off stack-clash-protection: https://github.com/fanquake/bitcoin/tree/qt_6_9_no_stack_clash_win. However it looks like there are other issues, possibly due to https://github.com/qt/qtbase/commit/d25d9f2c2673bc287590d9a83bd7ef1357d7021a#diff-4ae1a340d7e79a6402009cb635708b4950b1633f4bc850aba4
...
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3557317662)
> This [branch](https://github.com/hebasto/bitcoin/tree/251119-qt6.9) fails to cross-compile on Debian Bookworm with GCC 12.2.0:
That's the same issue we work around in CMake, which is fixed by just turning off stack-clash-protection: https://github.com/fanquake/bitcoin/tree/qt_6_9_no_stack_clash_win. However it looks like there are other issues, possibly due to https://github.com/qt/qtbase/commit/d25d9f2c2673bc287590d9a83bd7ef1357d7021a#diff-4ae1a340d7e79a6402009cb635708b4950b1633f4bc850aba4
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549698974)
I also found it confusing at first, but it's a branchless structure that we're using in a [few](https://github.com/bitcoin/bitcoin/blob/1fe851a4781a6af6264f1ba9d93c3281b59a6cac/src/net_processing.cpp#L1632) [other](https://github.com/bitcoin/bitcoin/blob/fa9ca13f35be0a023aeed78775ad66f95717b28b/src/util/feefrac.h#L75) [places](https://github.com/bitcoin/bitcoin/pull/33512/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R684) [already](https://github.com/bitcoin/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549698974)
I also found it confusing at first, but it's a branchless structure that we're using in a [few](https://github.com/bitcoin/bitcoin/blob/1fe851a4781a6af6264f1ba9d93c3281b59a6cac/src/net_processing.cpp#L1632) [other](https://github.com/bitcoin/bitcoin/blob/fa9ca13f35be0a023aeed78775ad66f95717b28b/src/util/feefrac.h#L75) [places](https://github.com/bitcoin/bitcoin/pull/33512/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R684) [already](https://github.com/bitcoin/bitcoin
...
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549969970)
is this removal related to the mac cleanup?
it seems this was before the Mac section originally https://github.com/bitcoin/bitcoin/commit/33dd764984def9371f324d3add19ee894a0260bf#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0aL87-L88
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549969970)
is this removal related to the mac cleanup?
it seems this was before the Mac section originally https://github.com/bitcoin/bitcoin/commit/33dd764984def9371f324d3add19ee894a0260bf#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0aL87-L88
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2551029734)
nit: With the latest push (https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c#diff-d6d633592a40f5f3d8b03863e41547de8751b874c1d20f129a616b9dd719b999), we are testing the same thing on two lines.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2551029734)
nit: With the latest push (https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c#diff-d6d633592a40f5f3d8b03863e41547de8751b874c1d20f129a616b9dd719b999), we are testing the same thing on two lines.
💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3576053570)
Rebased after https://github.com/bitcoin/bitcoin/commit/4f65a1c5db84426b9d814a18f63ac46632691ab2#diff-f14e9423bd43609969886b2e42751c55606344e0b71780564cdb23515802cec0R12, ready for review again.
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3576053570)
Rebased after https://github.com/bitcoin/bitcoin/commit/4f65a1c5db84426b9d814a18f63ac46632691ab2#diff-f14e9423bd43609969886b2e42751c55606344e0b71780564cdb23515802cec0R12, ready for review again.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3581723693)
`ade7fcb6a0...12dfb1cb7c`: use `std::ranges::max_element()` instead of manual find, inspired by https://github.com/l0rinc/bitcoin/commit/d42c922e9256625b166ab388ff234ba9bc272233#diff-ad1d5d83183a5facaf06e2b44c42a3e942a261b7506576720588bf6b158e4c3dR37
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3581723693)
`ade7fcb6a0...12dfb1cb7c`: use `std::ranges::max_element()` instead of manual find, inspired by https://github.com/l0rinc/bitcoin/commit/d42c922e9256625b166ab388ff234ba9bc272233#diff-ad1d5d83183a5facaf06e2b44c42a3e942a261b7506576720588bf6b158e4c3dR37
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568598322)
We’re [moving towards Flush not returning a value](https://github.com/bitcoin/bitcoin/pull/33866/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR257) – can we avoid adding new code that relies on its return? Not sure it’s possible before that other PR lands, but worth keeping in mind.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568598322)
We’re [moving towards Flush not returning a value](https://github.com/bitcoin/bitcoin/pull/33866/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR257) – can we avoid adding new code that relies on its return? Not sure it’s possible before that other PR lands, but worth keeping in mind.