Bitcoin Core Github
44 subscribers
120K links
Download Telegram
👍 darosior approved a pull request: "[29.x] Finalise 29.2rc2"
(https://github.com/bitcoin/bitcoin/pull/33534#pullrequestreview-3300035989)
utACK 513cef75ee06bc5d310a22d366a5f3c815aa1499. Changes look good to me, but i have not been through the process of regenerating the doc myself.
👍 darosior approved a pull request: "[28.x] More backports"
(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.
🤔 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.
👍 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:
💬 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
@
...
💬 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)) {
```
🤔 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
🚀 glozow merged a pull request: "[29.x] Finalise 29.2rc2"
(https://github.com/bitcoin/bitcoin/pull/33534)
🤔 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.
🤔 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?!
💬 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.
💬 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 🎉
🚀 fanquake merged a pull request: "test: fix p2p_leak_tx.py"
(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)
💬 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
👋 ryanofsky's pull request is ready for review: "init: Fix Ctrl-C shutdown hangs during wait calls"
(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
💬 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.