[pull] master from bitcoin:master#706
Merged
pull[bot] merged 8 commits intoorngr:masterfrom Mar 19, 2026
Merged
Conversation
sync.h is low-level and should not require any other subsystems. Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself.
Currently arguments passed through the validation interface are copied three times. Once on capture in the event, again when the event itself is copied into the task runner lambda, and when the various arguments used by the logging statement are copied into the lambda. This change avoids the variables captured by the event being copied again. Next to avoiding needless copies, this is done in preparation of the following two commits, which seek to clarify the ownership semantics of the blocks passed through the validation interface. Co-authored-by: stickies-v <stickies-v@protonmail.com>
This makes existing behaviour of the block's destructor triggering on the scheduler thread more explicit by moving it to the thread. The scheduler thread doing so is useful, since it does not block the thread doing validation while releasing a block's memory. Previously, both the caller and the queued event lambda held copies of the shared_ptr. The block would typically be freed on the scheduler thread - but only because it went out of scope before the queued event on the scheduler thread ran. If the scheduler ran first, the block would instead be freed on the validation thread. Now, ownership is transferred at each step when invoking the BlockConnected signal: connected_blocks yields via std::move, BlockConnected takes by value, and the event lambda move-captures the shared_ptr. Though it is possible that this only decrements the block's reference count, blocks are also read from disk in `ConnectTip`, which now explicitly results in their memory being released on the scheduler thread.
This makes existing behaviour of the block's destructor triggering on the scheduler thread more explicit by moving it to the thread. The scheduler thread doing so is useful, since it does not block the thread doing validation while releasing a block's memory. DisconnectTip already creates and destroys the block itself, so moving it into the validation signals is well scoped.
79467e3 threading: never require logging from sync.h (Cory Fields) Pull request description: This is an updated version of #34793 after stickies-v [pointed out that the approach there was broken](#34793 (comment)). sync.h is low-level and should not require any other subsystems. Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself. ACKs for top commit: stickies-v: re-ACK 79467e3 , thanks for doing the modernization right away 👍 sedited: ACK 79467e3 Tree-SHA512: 53e4b19cbf6ec81ce6aee06092a07e56877e2247a2614a148ba25c276dc5cf00d68445f9cd43dbd71e4481da74b788e3e20d3634484760d037f94d44bf13ea9c
779e782 fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg) Pull request description: This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because: 1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s); 2) Mocking would require lots of refactors. This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDescriptor`. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for #31452, for example. ACKs for top commit: frankomosh: Code Review ACK 779e782 marcofleon: reACK 779e782 Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e
d6f680b validation: Move block into BlockDisconnected signal (sedited) 4d02d2b validation: Move block into BlockConnected signal (sedited) 8b0fb64 validation: Move validation signal events to task runner (sedited) Pull request description: This enforces behaviour that is currently already implicit: The destructor for blocks runs mostly in the [scheduler thread](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-176/20472174834/mainnet-default-instrumented-head-flamegraph.svg?x=2762391536960&y=684). The change should make it a bit clearer what the ownership semantics for these validation signals are. `BlockConnected` already takes a reference to a block that is emplaced in `connected_blocks`. Once `connected_blocks` is iterated through, it is not reused. Similarly `BlockDisconnected` currently takes a reference to a block that is discarded after the call to it. Note that this does not give the guarantee that blocks' lifetimes are extended by other means once they are connected. For example after IBD, the block's lifetime is extended in net_processing's `m_most_recent_block` and `ActivateBestChain` itself takes a copy of the block's shared pointer, meaning its caller may delay de-allocation. ACKs for top commit: maflcko: re-review ACK d6f680b 🔌 stickies-v: re-ACK d6f680b frankomosh: Re-ACK d6f680b Tree-SHA512: 9209a7d23e7af0d76fa70dff958b1329f38ef29ccc49b5a32bcf9f349d59cc2bf70464ebdb130d26077c0ff9362ce9211472231d375ff1c9c89c0ec3020eac80
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )