đ darosior approved a pull request: "[28.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/33535#pullrequestreview-3300089164)
utACK 06fe49dc88638e2ad21f1b7d0dd87661de384517.
(https://github.com/bitcoin/bitcoin/pull/33535#pullrequestreview-3300089164)
utACK 06fe49dc88638e2ad21f1b7d0dd87661de384517.
đŦ andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3366595565)
Removed `m_batch_size`. Each thread now increments the atomic counter by 1.
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3366595565)
Removed `m_batch_size`. Each thread now increments the atomic counter by 1.
đ¤ danielabrozzoni reviewed a pull request: "test: addrman: check isTerrible when time is more than 10min in the future"
(https://github.com/bitcoin/bitcoin/pull/33533#pullrequestreview-3300148395)
tACK 8e47ed6906d5e381498681e2cab9f2e318597705
I verified that mutating the isTerrible condition did not cause any tests to fail on master, while this PR correctly triggers a test failure.
(https://github.com/bitcoin/bitcoin/pull/33533#pullrequestreview-3300148395)
tACK 8e47ed6906d5e381498681e2cab9f2e318597705
I verified that mutating the isTerrible condition did not cause any tests to fail on master, while this PR correctly triggers a test failure.
đ theStack approved a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3299741455)
Code-review ACK 652424ad162b63d73ecb6bd65bd26946e90c617f :flags:
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3299741455)
Code-review ACK 652424ad162b63d73ecb6bd65bd26946e90c617f :flags:
đŦ theStack commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2402803342)
nit: `operator<` would be sufficient (currently only needed in the transaction tests, where a [`std::set` of script flag combinations](https://github.com/bitcoin/bitcoin/blob/86eaa4d6cd5c428f6635a8d44fa6b5a9545ea909/src/test/transaction_tests.cpp#L183) is used), I doubt that the other ones would ever have a use-case
```diff
diff --git a/src/script/verify_flags.h b/src/script/verify_flags.h
index 95a55d2c79..e14a329ace 100644
--- a/src/script/verify_flags.h
+++ b/src/script/verify_flags.h
@
...
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2402803342)
nit: `operator<` would be sufficient (currently only needed in the transaction tests, where a [`std::set` of script flag combinations](https://github.com/bitcoin/bitcoin/blob/86eaa4d6cd5c428f6635a8d44fa6b5a9545ea909/src/test/transaction_tests.cpp#L183) is used), I doubt that the other ones would ever have a use-case
```diff
diff --git a/src/script/verify_flags.h b/src/script/verify_flags.h
index 95a55d2c79..e14a329ace 100644
--- a/src/script/verify_flags.h
+++ b/src/script/verify_flags.h
@
...
đŦ theStack commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2402485646)
nit: while touching, could switch to the more modern `.contains`
```suggestion
if (!mapFlagNames.contains(word)) {
```
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2402485646)
nit: while touching, could switch to the more modern `.contains`
```suggestion
if (!mapFlagNames.contains(word)) {
```
đ¤ marcofleon reviewed a pull request: "test: addrman: check isTerrible when time is more than 10min in the future"
(https://github.com/bitcoin/bitcoin/pull/33533#pullrequestreview-3300206362)
Nice, code review ACK 8e47ed6906d5e381498681e2cab9f2e318597705
(https://github.com/bitcoin/bitcoin/pull/33533#pullrequestreview-3300206362)
Nice, code review ACK 8e47ed6906d5e381498681e2cab9f2e318597705
đ glozow merged a pull request: "[29.x] Finalise 29.2rc2"
(https://github.com/bitcoin/bitcoin/pull/33534)
(https://github.com/bitcoin/bitcoin/pull/33534)
đŦ davidgumberg commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3366820377)
reACK https://github.com/bitcoin/bitcoin/commit/14ae71f323dd011c6d51470ea15cf00750970f65
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3366820377)
reACK https://github.com/bitcoin/bitcoin/commit/14ae71f323dd011c6d51470ea15cf00750970f65
đ¤ marcofleon reviewed a pull request: "[30.x] Backports & rc3"
(https://github.com/bitcoin/bitcoin/pull/33473#pullrequestreview-3300410005)
lgtm ACK 4e869a67aa7415f9c756bf6463e3437ae0a3ec44
The diff looks fine and I did a (light) code review of every PR commit.
(https://github.com/bitcoin/bitcoin/pull/33473#pullrequestreview-3300410005)
lgtm ACK 4e869a67aa7415f9c756bf6463e3437ae0a3ec44
The diff looks fine and I did a (light) code review of every PR commit.
đ¤ mzumsande reviewed a pull request: "Improve LastCommonAncestor performance + add tests"
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3300473697)
Code Review ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
The added test is slightly faster with the changes.
> I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
I think it would require really long forked chains for this to become relevant, so basically in consensus split scenarios?!
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3300473697)
Code Review ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
The added test is slightly faster with the changes.
> I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
I think it would require really long forked chains for this to become relevant, so basically in consensus split scenarios?!
đŦ mzumsande commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2403037390)
If I understand it correctly, we don't have to worry about dereferencing a `nullptr` here, because `pskip` is only `nullptr` for genesis, and if one of the blocks was genesis, we couldn't get to this spot.
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2403037390)
If I understand it correctly, we don't have to worry about dereferencing a `nullptr` here, because `pskip` is only `nullptr` for genesis, and if one of the blocks was genesis, we couldn't get to this spot.
đŦ andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2403069813)
Removed the batch size đ
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2403069813)
Removed the batch size đ
đ fanquake merged a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121)
(https://github.com/bitcoin/bitcoin/pull/33121)
đ fanquake merged a pull request: "test: addrman: check isTerrible when time is more than 10min in the future"
(https://github.com/bitcoin/bitcoin/pull/33533)
(https://github.com/bitcoin/bitcoin/pull/33533)
đŦ ryanofsky commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3367170341)
<!-- begin push-3 -->
Updated 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5 -> c25a5e670b27d3b6eb958ce437dbe89678bd1511 ([`pr/sigwait.2`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.2) -> [`pr/sigwait.3`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/sigwait.2..pr/sigwait.3))<!-- end --> fixing regression in previous version that caused Qt shutdown to hang during wait calls from the GUI console
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3367170341)
<!-- begin push-3 -->
Updated 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5 -> c25a5e670b27d3b6eb958ce437dbe89678bd1511 ([`pr/sigwait.2`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.2) -> [`pr/sigwait.3`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/sigwait.2..pr/sigwait.3))<!-- end --> fixing regression in previous version that caused Qt shutdown to hang during wait calls from the GUI console
đ ryanofsky's pull request is ready for review: "init: Fix Ctrl-C shutdown hangs during wait calls"
(https://github.com/bitcoin/bitcoin/pull/33511)
(https://github.com/bitcoin/bitcoin/pull/33511)
đ ryanofsky approved a pull request: "multiprocess: Fix high overhead from message logging"
(https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3300929927)
Partial code review ACK d6167af132febba14bd0d86d7465aef490c58fea (just reviewed the Bitcoin code changes in last two commits, not the earlier libmultiprocess changes which will disappear when rebased), but this all looks good
(https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3300929927)
Partial code review ACK d6167af132febba14bd0d86d7465aef490c58fea (just reviewed the Bitcoin code changes in last two commits, not the earlier libmultiprocess changes which will disappear when rebased), but this all looks good
đŦ ryanofsky commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2403330641)
In commit "multiprocess: update multiprocess EventLoop construction to use options" (7c0015d543b4a6141520a02895342b4a316b60fe)
This is fine to keep if intentional, but could consider passing unnamed temporary like `mp::LogOptions{.log_fn = IpcLogFn}` to simply code and avoid need for std::move, and not keep an empty options object on the stack for the duration of the thread.
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2403330641)
In commit "multiprocess: update multiprocess EventLoop construction to use options" (7c0015d543b4a6141520a02895342b4a316b60fe)
This is fine to keep if intentional, but could consider passing unnamed temporary like `mp::LogOptions{.log_fn = IpcLogFn}` to simply code and avoid need for std::move, and not keep an empty options object on the stack for the duration of the thread.
đŦ blocktraveler commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-3367229312)
âšī¸ I've released the [Core Wallet Migration Tools](https://github.com/blocktraveler/Core-Wallet-Migration-Tools) in Python for this purpose which could be implemented into Core with a simple light-weight wrapper over the existing RPCs `getdescriptorinfo` and `importdescriptors`, see [Proposal: Add importprivkeys RPC (helper for WIF â descriptor import)](https://gist.github.com/blocktraveler/3e6198c698a272bd8b13b16e0f13d390) for details (also shared with the mailing list with subject `Add importp
...
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-3367229312)
âšī¸ I've released the [Core Wallet Migration Tools](https://github.com/blocktraveler/Core-Wallet-Migration-Tools) in Python for this purpose which could be implemented into Core with a simple light-weight wrapper over the existing RPCs `getdescriptorinfo` and `importdescriptors`, see [Proposal: Add importprivkeys RPC (helper for WIF â descriptor import)](https://gist.github.com/blocktraveler/3e6198c698a272bd8b13b16e0f13d390) for details (also shared with the mailing list with subject `Add importp
...