validation: Explicitly move blocks to validation signals

sedited commented at 9:07 am on March 1, 2026: contributor

This enforces behaviour that is currently already implicit: The destructor for blocks runs mostly in the scheduler thread. 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 moved to a dedicated connect trace. Once connect trace 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.

DrahtBot added the label Validation on Mar 1, 2026

DrahtBot commented at 9:07 am on March 1, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, arejula27

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34708 (validation: refactor: remove ConnectTrace by stickies-v)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/validationinterface.cpp:165 in 76d1bc9bd3

162 #define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...)           \
163     do {                                                       \
164         auto local_name = (name);                              \
165         LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__);  \
166-        m_internals->m_task_runner->insert([=] { \
167+        m_internals->m_task_runner->insert([=, event = std::move(event)] { \

stickies-v commented at 5:47 am on March 2, 2026:

I think the ENQUEUE_AND_LOG_EVENT change could use its own commit, since this macro is used by many more functions than just BlockConnected

I’m also a bit concerned about a macro opaquely making event an rvalue. It’s more verbose, but I think letting the caller move it would be safer?

(the static_assert is optional)

 0diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
 1index 85827115a3..1cbbd43d35 100644
 2--- a/src/validationinterface.cpp
 3+++ b/src/validationinterface.cpp
 4@@ -157,15 +157,17 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
 5 // Use a macro instead of a function for conditional logging to prevent
 6 // evaluating arguments when logging is not enabled.
 7 //
 8-// NOTE: The lambda captures the event by move and all other variables by value.
 9-#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...)           \
10-    do {                                                       \
11-        auto local_name = (name);                              \
12-        LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__);  \
13-        m_internals->m_task_runner->insert([=, event = std::move(event)] { \
14-            LOG_EVENT(fmt, local_name, __VA_ARGS__);           \
15-            event();                                           \
16-        });                                                    \
17+// NOTE: The lambda captures all local variables by value.
18+#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...)                    \
19+    do {                                                                \
20+        static_assert(std::is_rvalue_reference_v<decltype((event))>,    \
21+                      "event must be passed as an rvalue");             \
22+        auto local_name = (name);                                       \
23+        LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__);           \
24+        m_internals->m_task_runner->insert([=, local_event = (event)] { \
25+            LOG_EVENT(fmt, local_name, __VA_ARGS__);                    \
26+            local_event();                                              \
27+        });                                                             \
28     } while (0)
29 
30 #define LOG_EVENT(fmt, ...) \
31@@ -179,7 +181,7 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo
32     auto event = [pindexNew, pindexFork, fInitialDownload, this] {
33         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });
34     };
35-    ENQUEUE_AND_LOG_EVENT(event, "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__,
36+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__,
37                           pindexNew->GetBlockHash().ToString(),
38                           pindexFork ? pindexFork->GetBlockHash().ToString() : "null",
39                           fInitialDownload);
40@@ -196,7 +198,7 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf
41     auto event = [tx, mempool_sequence, this] {
42         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); });
43     };
44-    ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
45+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s", __func__,
46                           tx.info.m_tx->GetHash().ToString(),
47                           tx.info.m_tx->GetWitnessHash().ToString());
48 }
49@@ -205,7 +207,7 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx,
50     auto event = [tx, reason, mempool_sequence, this] {
51         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); });
52     };
53-    ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s reason=%s", __func__,
54+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s reason=%s", __func__,
55                           tx->GetHash().ToString(),
56                           tx->GetWitnessHash().ToString(),
57                           RemovalReasonToString(reason));
58@@ -217,7 +219,7 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p
59     auto event = [role, pblock = std::move(pblock), pindex, this] {
60         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); });
61     };
62-    ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__,
63+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__,
64                           block_hash,
65                           pindex->nHeight);
66 }
67@@ -227,7 +229,7 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem
68     auto event = [txs_removed_for_block, nBlockHeight, this] {
69         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); });
70     };
71-    ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs removed=%s", __func__,
72+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block height=%s txs removed=%s", __func__,
73                           nBlockHeight,
74                           txs_removed_for_block.size());
75 }
76@@ -238,7 +240,7 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock,
77     auto event = [pblock = std::move(pblock), pindex, this] {
78         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); });
79     };
80-    ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__,
81+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__,
82                           block_hash,
83                           pindex->nHeight);
84 }
85@@ -248,7 +250,7 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo
86     auto event = [role, locator, this] {
87         m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); });
88     };
89-    ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__,
90+    ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s", __func__,
91                           locator.IsNull() ? "null" : locator.vHave.front().ToString());
92 }
93

stickies-v commented at 7:33 am on March 2, 2026: contributor

Concept ACK

I think the current commit message wordings are not very easy to understand. Best-effort attempt at an alternative for the first one:

 0validation: Move block into BlockConnected signal
 1
 2Transfer unique ownership of the block's shared_ptr from the
 3validation thread to the scheduler thread through std::move, rather
 4than copying the shared_ptr and relying on refcount timing to
 5determine which thread deallocates the block.
 6
 7Previously, both the caller (via ConnectTrace) and the queued event
 8lambda held copies of the shared_ptr. The block would typically be
 9freed on the scheduler thread — but only because ConnectTrace usually
10went out of scope before the scheduler ran. If the scheduler ran
11first, the block would instead be freed on the validation thread.
12
13Now, ownership is transferred at each step: ConnectTrace yields via
14std::move, BlockConnected takes by value, and the event lambda
15move-captures the shared_ptr. This guarantees the block is always
16freed on the scheduler thread, avoiding potentially expensive
17deallocation on the validation thread.

Also, I think it would be natural for this PR to update ActivateBestChainStep to move the block into the connect trace?

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index df26ef23cc..d824044443 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -3219,7 +3219,7 @@ void Chainstate::PruneBlockIndexCandidates() {
 5  *
 6  * [@returns](/bitcoin-bitcoin/contributor/returns/) true unless a system error occurred
 7  */
 8-bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
 9+bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, std::shared_ptr<const CBlock> pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
10 {
11     AssertLockHeld(cs_main);
12     if (m_mempool) AssertLockHeld(m_mempool->cs);
13@@ -3264,7 +3264,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
14 
15         // Connect new blocks.
16         for (CBlockIndex* pindexConnect : vpindexToConnect | std::views::reverse) {
17-            if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) {
18+            if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? std::move(pblock) : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) {
19                 if (state.IsInvalid()) {
20                     // The block violates a consensus rule.
21                     if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
22@@ -3414,7 +3414,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
23                 // in case snapshot validation is completed during ActivateBestChainStep, the
24                 // result of GetRole() changes from BACKGROUND to NORMAL.
25                const ChainstateRole chainstate_role{this->GetRole()};
26-                if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) {
27+                if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? std::move(pblock) : nullBlockPtr, fInvalidFound, connectTrace)) {
28                     // A system error occurred
29                     return false;
30                 }
31diff --git a/src/validation.h b/src/validation.h
32index 482772c0d6..d3ad21c9ac 100644
33--- a/src/validation.h
34+++ b/src/validation.h
35@@ -847,7 +847,7 @@ public:
36     std::pair<int, int> GetPruneRange(int last_height_can_prune) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
37 
38 protected:
39-    bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
40+    bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, std::shared_ptr<const CBlock> pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
41     bool ConnectTip(
42         BlockValidationState& state,
43         CBlockIndex* pindexNew,

sedited commented at 8:00 am on March 2, 2026: contributor

Re #34704#pullrequestreview-3874178574

Also, I think it would be natural for this PR to update ActivateBestChainStep to move the block into the connect trace?

When we call ActivateBestChain we take a copy of a block, meaning we in all likelihood increment its refcount from 1 to 2. So we are just passing around a block whose lifetime is already ambiguous. I’m guessing your motivation is that this would allow us to skip one additional refcount increment when we are passing it to ConnectTip? Moving in the nested while loop in ActivateBestChain also seems dangerous (though I guess the ternary condition protects against that).

mzumsande commented at 9:05 am on March 2, 2026: contributor

The destructor for blocks runs mostly in the scheduler thread.

When we call ActivateBestChain we take a copy of a block, meaning we in all likelihood increment its refcount from 1 to 2.

This is just the case during IBD when NewPoWValidBlock signals aren’t sent, right? I would expect that when synced to the tip, m_most_recent_block would keep pointing to the block, and the block would be destroyed whenever the next block arrives, minutes after connection. I feel like this should be mentioned somewhere.

sedited commented at 9:11 am on March 2, 2026: contributor

This is just the case during IBD when NewPoWValidBlock signals aren’t sent, right? I would expect that when synced to the tip, m_most_recent_block would keep pointing to the block, and the block would be destroyed whenever the next block arrives, minutes after connection. I feel like this should be mentioned somewhere.

That is my read too. Will make this explicit in the PR description.

validation: Move validation signal events to task runner

This avoids the variables captured by the event being copied again and
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>

35ee6d4c26

validation: Move block into BlockConnected signal

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: ConnectTrace 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.

74e9322a42

validation: Move block into BlockDisconnected signal

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.

40c519bd7b

sedited force-pushed on Mar 2, 2026

sedited commented at 9:56 am on March 2, 2026: contributor

Updated 76d1bc9bd3881da73c0691404b3fda7799d3214b -> 40c519bd7be3944fcb5b8c1d4eddcff8f6bb19f3 (block_connected_move_0 -> block_connected_move_1, compare)

Addressed @stickies-v’s comment, improving the commit messages.
Took @stickies-v’s suggestion, adding a separate commit for moving the events and making the move explicit for the callers.

stickies-v approved

stickies-v commented at 1:38 pm on March 2, 2026: contributor

ACK 40c519bd7be3944fcb5b8c1d4eddcff8f6bb19f3

I’m guessing your motivation is that this would allow us to skip one additional refcount increment when we are passing it to ConnectTip?

Yeah, that’s what triggered me to look into it, but…

Moving in the nested while loop in ActivateBestChain also seems dangerous (though I guess the ternary condition protects against that).

I agree, I missed that, better to leave as-is.

arejula27 commented at 3:55 pm on March 6, 2026: none

ACK [40c519bd7be3944fcb5b8c1d4eddcff8f6bb19f3]

I like the removal of an unnecessary shared ownership. With the move, the reference count never needs to increment just to be decremented once iterations ends.

Not part of your changes, but while reviewing the PR I noticed a typo in src/validationinterface.h at lines 111–112, there is a duplicated as:

0* Notifies listeners of transactions removed from the mempool as
1* as a result of new block being connected.

Since you already edited the file, would you mind fixing it here? Opening a separate PR myself feels a bit like sneaking into the contributors list on a one-word fix.

validation: Explicitly move blocks to validation signals #34704

Reviews

Conflicts