validation: Explicitly move blocks to validation signals by sedited · Pull Request #34704 · bitcoin/bitcoin

sedited · 2026-03-01T09:07:28Z

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

DrahtBot · 2026-03-01T09:07:49Z

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	maflcko, stickies-v, frankomosh
Stale ACK	arejula27, rkrux

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34803 (mempool: asynchronous mempool fee rate diagram updates via validation interface by ismaelsadeeq)
#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.

stickies-v

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:

validation: Move block into BlockConnected signal

Transfer unique ownership of the block's shared_ptr from the
validation thread to the scheduler thread through std::move, rather
than copying the shared_ptr and relying on refcount timing to
determine which thread deallocates the block.

Previously, both the caller (via ConnectTrace) and the queued event
lambda held copies of the shared_ptr. The block would typically be
freed on the scheduler thread — but only because ConnectTrace usually
went out of scope before the scheduler ran. If the scheduler ran
first, the block would instead be freed on the validation thread.

Now, ownership is transferred at each step: ConnectTrace yields via
std::move, BlockConnected takes by value, and the event lambda
move-captures the shared_ptr. This guarantees the block is always
freed on the scheduler thread, avoiding potentially expensive
deallocation 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?

git diff on 76d1bc9

diff --git a/src/validation.cpp b/src/validation.cpp
index df26ef23cc..d824044443 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3219,7 +3219,7 @@ void Chainstate::PruneBlockIndexCandidates() {
  *
  * @returns true unless a system error occurred
  */
-bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
+bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, std::shared_ptr<const CBlock> pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
 {
     AssertLockHeld(cs_main);
     if (m_mempool) AssertLockHeld(m_mempool->cs);
@@ -3264,7 +3264,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
 
         // Connect new blocks.
         for (CBlockIndex* pindexConnect : vpindexToConnect | std::views::reverse) {
-            if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) {
+            if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? std::move(pblock) : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) {
                 if (state.IsInvalid()) {
                     // The block violates a consensus rule.
                     if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
@@ -3414,7 +3414,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                 // in case snapshot validation is completed during ActivateBestChainStep, the
                 // result of GetRole() changes from BACKGROUND to NORMAL.
                const ChainstateRole chainstate_role{this->GetRole()};
-                if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) {
+                if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? std::move(pblock) : nullBlockPtr, fInvalidFound, connectTrace)) {
                     // A system error occurred
                     return false;
                 }
diff --git a/src/validation.h b/src/validation.h
index 482772c0d6..d3ad21c9ac 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -847,7 +847,7 @@ public:
     std::pair<int, int> GetPruneRange(int last_height_can_prune) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
 protected:
-    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);
+    bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, std::shared_ptr<const CBlock> pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
     bool ConnectTip(
         BlockValidationState& state,
         CBlockIndex* pindexNew,

stickies-v · 2026-03-02T05:47:00Z

src/validationinterface.cpp

        auto local_name = (name);                              \
        LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__);  \
-        m_internals->m_task_runner->insert([=] { \
+        m_internals->m_task_runner->insert([=, event = std::move(event)] { \


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)

git diff on 76d1bc9

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 85827115a3..1cbbd43d35 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -157,15 +157,17 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() // Use a macro instead of a function for conditional logging to prevent // evaluating arguments when logging is not enabled. // -// NOTE: The lambda captures the event by move and all other variables by value. -#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...) \ - do { \ - auto local_name = (name); \ - LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__); \ - m_internals->m_task_runner->insert([=, event = std::move(event)] { \ - LOG_EVENT(fmt, local_name, __VA_ARGS__); \ - event(); \ - }); \ +// NOTE: The lambda captures all local variables by value. +#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...) \ + do { \ + static_assert(std::is_rvalue_reference_v<decltype((event))>, \ + "event must be passed as an rvalue"); \ + auto local_name = (name); \ + LOG_EVENT("Enqueuing " fmt, local_name, __VA_ARGS__); \ + m_internals->m_task_runner->insert([=, local_event = (event)] { \ + LOG_EVENT(fmt, local_name, __VA_ARGS__); \ + local_event(); \ + }); \ } while (0) #define LOG_EVENT(fmt, ...) \ @@ -179,7 +181,7 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo auto event = [pindexNew, pindexFork, fInitialDownload, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, pindexNew->GetBlockHash().ToString(), pindexFork ? pindexFork->GetBlockHash().ToString() : "null", fInitialDownload); @@ -196,7 +198,7 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf auto event = [tx, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s", __func__, tx.info.m_tx->GetHash().ToString(), tx.info.m_tx->GetWitnessHash().ToString()); } @@ -205,7 +207,7 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, auto event = [tx, reason, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s reason=%s", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s reason=%s", __func__, tx->GetHash().ToString(), tx->GetWitnessHash().ToString(), RemovalReasonToString(reason)); @@ -217,7 +219,7 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p auto event = [role, pblock = std::move(pblock), pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, block_hash, pindex->nHeight); } @@ -227,7 +229,7 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem auto event = [txs_removed_for_block, nBlockHeight, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs removed=%s", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block height=%s txs removed=%s", __func__, nBlockHeight, txs_removed_for_block.size()); } @@ -238,7 +240,7 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, auto event = [pblock = std::move(pblock), pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, block_hash, pindex->nHeight); } @@ -248,7 +250,7 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo auto event = [role, locator, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__, + ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s", __func__, locator.IsNull() ? "null" : locator.vHave.front().ToString()); }

The block would typically be
freed on the scheduler thread — but only because ConnectTrace usually
went out of scope before the scheduler ran. If the scheduler ran
first, the block would instead be freed on the validation thread.

I have not been able to completely understand as to why the block would be freed on the validation thread even if the scheduler ran first.

From what I understand: these two threads are executing in parallel (on multicore systems) and it's highly likely that the block in the caller in the validation thread would go out of scope before the corresponding event handler function finishes in the scheduler thread, making the scheduler thread responsible for freeing the block.

Yes, your description is what is happening currently. The change here just enforces existing behavior to ensure that a developer reading the code understands this and protects against regressions of it in case a future change seeks to extend the lifetime of the block in the thread triggering the validation signal.

sedited · 2026-03-02T08:00:09Z

Re #34704 (review)

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

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 · 2026-03-02T09:11:08Z

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.

sedited · 2026-03-02T09:56:38Z

Updated 76d1bc9 -> 40c519b (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

ACK 40c519b

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 · 2026-03-06T15:55:19Z

ACK [40c519b]

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:

* Notifies listeners of transactions removed from the mempool as
* 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.

frankomosh

CrACK 40c519b.

When #34708 goes in, the std::move at the BlockConnected call site gets replaced by Assert(block) under a const auto& binding. Would this regress the ownership transfer that changes in this PR aim to establish?`

rkrux · 2026-03-12T08:33:03Z

When #34708 goes in, the std::move at the BlockConnected call site gets replaced by Assert(block) under a const auto& binding. Would this regress the ownership transfer that changes in this PR aim to establish?`

I suppose the other PR will rebase over master when this PR is merged, accommodating the move changes of this PR in that line.

sedited · 2026-03-12T11:12:33Z

Rebased 40c519b -> 95e3359 (block_connected_move_1 -> block_connected_move_2, compare)

Fixed conflict with validation: refactor: remove ConnectTrace #34708

I suppose the other PR will rebase over master when this PR is merged, accommodating the move changes of this PR in that line.

Or the other way around :)

frankomosh · 2026-03-12T11:23:21Z

Re-ACK 95e3359

stickies-v · 2026-03-12T14:48:21Z

re-ACK 95e3359, no changes except addressing merge conflict

nit: dabdcfa commit message still references ConnectTrace

rkrux · 2026-03-13T10:00:12Z

The destructor for blocks runs mostly in the scheduler thread.

I've been analysing the flamegraph to verify this claim. I did notice around ~50 biilion samples of libc.so::cfree calls in the scheduler thread and didn't find enough in the msghand thread. However, I did find ~5 billion samples of the cfree calls in msghand but they were on top of ConnectBlock - don't think it's related to deallocation of the block.

edit: I had expected to find some on top of CConman::ThreadMessageHandler because thats where the reference count of CBlock is init'ed to 1 (via ProcessMessage) but didn't - either these are not there in significant amount or maybe my expectation is incorrect.

Regardless, these observations don't seem contradictory to what's mentioned in the PR description and commits.

rkrux

ACK 95e3359

I do like the explicit intent this PR conveys. I did some analysis of the shared flamegraph and tracked the shared pointer usage & copies in the block connected & disconnected flows - while the latter is quite straightforward, the former is more involved.

maflcko

lgtm. I think this is a nice change, removing some redundant copies in the first commit and documenting that the scheduler thread can act as a "garbage collector thread" in the other two commits.

I've left some nits, and a follow-up idea.

Happy to re-review if you decide to take any of the nits. They seem easy enough to be in-scope and not require a follow-up.

review ACK 95e3359 🗯

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 95e3359967e1c2ae7147867e4bae6c317f0dbfc8 🗯
kPHnbmRJ44LliSgdavOUNWwIYrCw/8+MC38SHgkWdcIlcKvEcBtV1uq3Y6rPzmlOoovPQIlFmhxHhQ1vYuHjDg==

src/validationinterface.cpp

maflcko · 2026-03-16T11:13:16Z

src/validation.cpp

                pindexNewTip = m_chain.Tip();

-                for (const auto& [index, block] : connected_blocks) {
+                for (auto& [index, block] : connected_blocks) {


nit in dabdcfa: Should this not be

for (auto& [index, block] : std::move(connected_blocks)) {

Otherwise, clang-tidy use-after-move won't see that the content of the vector was moved?

Also, unrelated: I don't understand the comment on std::vector<ConnectedBlock> connected_blocks; // Destructed before cs_main is unlocked. I guess it is not wrong, but I fail to see how it is relevant.

Also, unrelated: I don't understand the comment // Lock transaction pool for at least as long as it takes for connected_blocks to be consumed. I guess this should have been removed in #34708?

Also, unrelated: it could make sense to reduce the scope either in this commit or in a new commit:

diff --git a/src/validation.cpp b/src/validation.cpp index 1efe109b6f..fedc3d84a3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3356,3 +3356,2 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< { - // Lock transaction pool for at least as long as it takes for connected_blocks to be consumed LOCK(MempoolMutex()); @@ -3364,3 +3363,2 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // (with the exception of shutdown due to hardware issues, low disk space, etc). - std::vector<ConnectedBlock> connected_blocks; // Destructed before cs_main is unlocked @@ -3381,2 +3379,3 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< const ChainstateRole chainstate_role{this->GetRole()}; + std::vector<ConnectedBlock> connected_blocks; if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connected_blocks)) {

Happy to push the second unrelated diff as a separate pull request. I shouldn't conflict, I guess.

maflcko · 2026-03-16T11:19:58Z

src/validationinterface.cpp

+void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex)
 {
-    auto event = [role, pblock, pindex, this] {
+    auto block_hash{pblock->GetHash().ToString()};


nit in dabdcfa: I guess you are doing this to avoid a third copy of pblock, and instead do a copy of block_hash now? Not sure which one is better. I think it may be best to leave this as-is and create a redundant third copy of pblock, until the logging is fixed to not create any new copies at all.

Otherwise, when the logging is eventually fixed, there will be a possibly redundant calculation and copy of block_hash.

I think I prefer keeping this (with the changes to only calculate if we are indeed logging). Copying the block instead would leave it to be destructed at the end of the function's scope, which imo runs counter to the PRs goals. Or do you have a different approach in mind that could retain that?

Sry, I actually missed that the block is moved into the event first, before the block hash is used to construct the log message. So I guess the copy of the hash is needed, unless you want to restructure how the log message is constructed.

I've implemented that, and while it is a minimally more verbose, I think it is overall nicer, because:

Async events and sync events equally format the log message as the first step. Async ones via LOG_MSG and sync ones via LOG_EVENT

There is no need to duplicate the ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? into several functions and macros. Only one place does this thing what feels a bit like a layer violation

The ENQUEUE_AND_LOG_EVENT macro is easier to follow, because it no longer deals with __VA_ARGS__ formatting at all, and only has two arguments.

Here is the diff on top of this pull request. (Note that I've only compiled it with clang-tidy -p=./bld-cmake/ src/validationinterface.cpp and I have not run clang-format yet. If you agree with the diff, it could make sense to apply it on top of the first commit instead of the top of this pull).

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 202eac4ff3..381f9dd870 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -159,4 +159,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() // -// NOTE: The lambda captures the event by move -#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...) \ +#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ do { \ @@ -164,9 +163,7 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() "event must be passed as an rvalue"); \ - auto local_name = (name); \ - auto log_msg = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) \ - ? tfm::format(fmt, local_name, __VA_ARGS__) \ - : std::string{}; \ + static_assert(!std::is_const_v<std::remove_reference_t<decltype((log_msg))>>, \ + "log_msg must be mutable reference");\ LOG_EVENT("Enqueuing %s", log_msg); \ - m_internals->m_task_runner->insert([log_msg = std::move(log_msg), local_event = (event)] { \ - LOG_EVENT("%s", log_msg); \ + m_internals->m_task_runner->insert([local_log_msg = std::move(log_msg), local_event = (event)] { \ + LOG_EVENT("%s", local_log_msg); \ local_event(); \ @@ -175,2 +172,8 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() +#define LOG_MSG(fmt, ...) \ + ( ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) \ + ? tfm::format((fmt), __VA_ARGS__) \ + : std::string{} \ + ) + #define LOG_EVENT(fmt, ...) \ @@ -183,6 +186,3 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo - auto event = [pindexNew, pindexFork, fInitialDownload, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, + auto log_msg = LOG_MSG( "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, pindexNew->GetBlockHash().ToString(), @@ -190,2 +190,6 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo fInitialDownload); + auto event = [pindexNew, pindexFork, fInitialDownload, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); + }; + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -200,2 +204,5 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf { + auto log_msg = LOG_MSG( "%s: txid=%s wtxid=%s", __func__, + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); auto event = [tx, mempool_sequence, this] { @@ -203,5 +210,3 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s", __func__, - tx.info.m_tx->GetHash().ToString(), - tx.info.m_tx->GetWitnessHash().ToString()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -209,6 +214,3 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { - auto event = [tx, reason, mempool_sequence, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s reason=%s", __func__, + auto log_msg = LOG_MSG( "%s: txid=%s wtxid=%s reason=%s", __func__, tx->GetHash().ToString(), @@ -216,2 +218,6 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, RemovalReasonToString(reason)); + auto event = [tx, reason, mempool_sequence, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); + }; + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -220,3 +226,5 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p { - std::string block_hash = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? pblock->GetHash().ToString() : ""; + auto log_msg = LOG_MSG( "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); auto event = [role, pblock = std::move(pblock), pindex, this] { @@ -224,5 +232,3 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, - block_hash, - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -231,2 +237,5 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem { + auto log_msg = LOG_MSG( "%s: block height=%s txs removed=%s", __func__, + nBlockHeight, + txs_removed_for_block.size()); auto event = [txs_removed_for_block, nBlockHeight, this] { @@ -234,5 +243,3 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block height=%s txs removed=%s", __func__, - nBlockHeight, - txs_removed_for_block.size()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -241,3 +248,5 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, { - std::string block_hash = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? pblock->GetHash().ToString() : ""; + auto log_msg = LOG_MSG( "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); auto event = [pblock = std::move(pblock), pindex, this] { @@ -245,5 +254,3 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, - block_hash, - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -252,2 +259,4 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo { + auto log_msg = LOG_MSG( "%s: block hash=%s", __func__, + locator.IsNull() ? "null" : locator.vHave.front().ToString()); auto event = [role, locator, this] { @@ -255,4 +264,3 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s", __func__, - locator.IsNull() ? "null" : locator.vHave.front().ToString()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); }

Is it important that we log the entire message when enqueueing? Because if not, and we log e.g. just the function name, I think we don't really need any of these macros?

git diff on 8c4439a

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index cc1abcd6cd..038e9d220b 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -154,123 +154,100 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() promise.get_future().wait(); } -// Use a macro instead of a function for conditional logging to prevent -// evaluating arguments when logging is not enabled. -// -// NOTE: The lambda captures the event by move -#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ - do { \ - static_assert(std::is_rvalue_reference_v<decltype((event))>, \ - "event must be passed as an rvalue"); \ - static_assert(!std::is_const_v<std::remove_reference_t<decltype((log_msg))>>, \ - "log_msg must be mutable reference"); \ - LOG_EVENT("Enqueuing %s", log_msg); \ - m_internals->m_task_runner->insert([local_log_msg = std::move(log_msg), local_event = (event)] { \ - LOG_EVENT("%s", local_log_msg); \ - local_event(); \ - }); \ - } while (0) - -#define LOG_MSG(fmt, ...) \ - (ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? tfm::format((fmt), __VA_ARGS__) : std::string{}) - -#define LOG_EVENT(fmt, ...) \ - LogDebug(BCLog::VALIDATION, fmt, __VA_ARGS__) - void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) { // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which // the chain actually updates. One way to ensure this is for the caller to invoke this signal // in the same critical section where the chain is updated - auto log_msg = LOG_MSG("%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, - pindexNew->GetBlockHash().ToString(), - pindexFork ? pindexFork->GetBlockHash().ToString() : "null", - fInitialDownload); - auto event = [pindexNew, pindexFork, fInitialDownload, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([pindexNew, pindexFork, fInitialDownload, this] { + LogDebug(BCLog::VALIDATION, "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, + pindexNew->GetBlockHash().ToString(), + pindexFork ? pindexFork->GetBlockHash().ToString() : "null", + fInitialDownload); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) { - LOG_EVENT("%s: new block hash=%s block height=%d", __func__, new_tip.GetBlockHash().ToString(), new_tip.nHeight); + LogDebug(BCLog::VALIDATION, "%s: new block hash=%s block height=%d", __func__, new_tip.GetBlockHash().ToString(), new_tip.nHeight); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ActiveTipChange(new_tip, is_ibd); }); } void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) { - auto log_msg = LOG_MSG("%s: txid=%s wtxid=%s", __func__, - tx.info.m_tx->GetHash().ToString(), - tx.info.m_tx->GetWitnessHash().ToString()); - auto event = [tx, mempool_sequence, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([tx, mempool_sequence, this] { + LogDebug(BCLog::VALIDATION, "%s: txid=%s wtxid=%s", __func__, + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { - auto log_msg = LOG_MSG("%s: txid=%s wtxid=%s reason=%s", __func__, - tx->GetHash().ToString(), - tx->GetWitnessHash().ToString(), - RemovalReasonToString(reason)); - auto event = [tx, reason, mempool_sequence, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([tx, reason, mempool_sequence, this] { + LogDebug(BCLog::VALIDATION, "%s: txid=%s wtxid=%s reason=%s", __func__, + tx->GetHash().ToString(), + tx->GetWitnessHash().ToString(), + RemovalReasonToString(reason)); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex) { - auto log_msg = LOG_MSG("%s: block hash=%s block height=%d", __func__, - pblock->GetHash().ToString(), - pindex->nHeight); - auto event = [role, pblock = std::move(pblock), pindex, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([role, pblock = std::move(pblock), pindex, this] { + LogDebug(BCLog::VALIDATION, "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) { - auto log_msg = LOG_MSG("%s: block height=%s txs removed=%s", __func__, - nBlockHeight, - txs_removed_for_block.size()); - auto event = [txs_removed_for_block, nBlockHeight, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([txs_removed_for_block, nBlockHeight, this] { + LogDebug(BCLog::VALIDATION, "%s: block height=%d txs removed=%d", __func__, + nBlockHeight, + txs_removed_for_block.size()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex) { - auto log_msg = LOG_MSG("%s: block hash=%s block height=%d", __func__, - pblock->GetHash().ToString(), - pindex->nHeight); - auto event = [pblock = std::move(pblock), pindex, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([pblock = std::move(pblock), pindex, this] { + LogDebug(BCLog::VALIDATION, "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlockLocator& locator) { - auto log_msg = LOG_MSG("%s: block hash=%s", __func__, - locator.IsNull() ? "null" : locator.vHave.front().ToString()); - auto event = [role, locator, this] { + LogDebug(BCLog::VALIDATION, "Enqueuing %s event", __func__); + m_internals->m_task_runner->insert([role, locator, this] { + LogDebug(BCLog::VALIDATION, "%s: block hash=%s", __func__, + locator.IsNull() ? "null" : locator.vHave.front().ToString()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + }); } void ValidationSignals::BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state) { - LOG_EVENT("%s: block hash=%s state=%s", __func__, - block->GetHash().ToString(), state.ToString()); + LogDebug(BCLog::VALIDATION, "%s: block hash=%s state=%s", __func__, + block->GetHash().ToString(), state.ToString()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockChecked(block, state); }); } void ValidationSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) { - LOG_EVENT("%s: block hash=%s", __func__, block->GetHash().ToString()); + LogDebug(BCLog::VALIDATION, "%s: block hash=%s", __func__, block->GetHash().ToString()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NewPoWValidBlock(pindex, block); }); }

Is it important that we log the entire message when enqueueing?

I'd say so, you probably want to match the exact event with it being processed in the callback, not just the function.

I think for most cases, it is enough to just log when processing and a log on enqueing is not needed. However for stuff like transactions removed from mempool, it is helpful while debugging to see when the event for this txid was enqueued, and when it was executed.

Sure, the queue is single-threaded, so one can guess the matching from the order, but this seems tedious and may break if this is ever made multi-threaded.

So my preference would be to leave this as-is for now.

src/validationinterface.cpp

sedited · 2026-03-16T17:24:14Z

Thanks for the review @maflcko,

Updated 95e3359 -> d62c6e9 (block_connected_move_2 -> block_connected_move_3, compare)

Addressed @maflcko's comment, pre-formatting the string if it will be logged to avoid copying the passed in data again.
Addressed @maflcko's comment, use std::move when iterating through connected_blocks.

maflcko

Thx. The new push looks nice, because it also fixes the three redundant copies of all validation interface args.

I've left more nits, and I am happy to re-review after them, but they are not blocking.

lgtm d62c6e9 🍧

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm d62c6e93a3f3777fa1227ae8775a652c315bf98d 🍧
m0ORBaxAhyghv4mHEwa94UOk63PsD13EDdjUKbt3DwihP/8pHAkEC5xwsmfuHCAplx7TIlxd9lI4S6Helcz9AQ==

src/validationinterface.cpp

maflcko · 2026-03-17T09:17:53Z

src/validationinterface.cpp

+void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex)
 {
-    auto event = [role, pblock, pindex, this] {
+    auto block_hash{pblock->GetHash().ToString()};


Sry, I actually missed that the block is moved into the event first, before the block hash is used to construct the log message. So I guess the copy of the hash is needed, unless you want to restructure how the log message is constructed.

I've implemented that, and while it is a minimally more verbose, I think it is overall nicer, because:

Async events and sync events equally format the log message as the first step. Async ones via LOG_MSG and sync ones via LOG_EVENT

There is no need to duplicate the ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? into several functions and macros. Only one place does this thing what feels a bit like a layer violation

The ENQUEUE_AND_LOG_EVENT macro is easier to follow, because it no longer deals with __VA_ARGS__ formatting at all, and only has two arguments.

Here is the diff on top of this pull request. (Note that I've only compiled it with clang-tidy -p=./bld-cmake/ src/validationinterface.cpp and I have not run clang-format yet. If you agree with the diff, it could make sense to apply it on top of the first commit instead of the top of this pull).

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 202eac4ff3..381f9dd870 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -159,4 +159,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() // -// NOTE: The lambda captures the event by move -#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...) \ +#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ do { \ @@ -164,9 +163,7 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() "event must be passed as an rvalue"); \ - auto local_name = (name); \ - auto log_msg = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) \ - ? tfm::format(fmt, local_name, __VA_ARGS__) \ - : std::string{}; \ + static_assert(!std::is_const_v<std::remove_reference_t<decltype((log_msg))>>, \ + "log_msg must be mutable reference");\ LOG_EVENT("Enqueuing %s", log_msg); \ - m_internals->m_task_runner->insert([log_msg = std::move(log_msg), local_event = (event)] { \ - LOG_EVENT("%s", log_msg); \ + m_internals->m_task_runner->insert([local_log_msg = std::move(log_msg), local_event = (event)] { \ + LOG_EVENT("%s", local_log_msg); \ local_event(); \ @@ -175,2 +172,8 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() +#define LOG_MSG(fmt, ...) \ + ( ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) \ + ? tfm::format((fmt), __VA_ARGS__) \ + : std::string{} \ + ) + #define LOG_EVENT(fmt, ...) \ @@ -183,6 +186,3 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo - auto event = [pindexNew, pindexFork, fInitialDownload, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, + auto log_msg = LOG_MSG( "%s: new block hash=%s fork block hash=%s (in IBD=%s)", __func__, pindexNew->GetBlockHash().ToString(), @@ -190,2 +190,6 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo fInitialDownload); + auto event = [pindexNew, pindexFork, fInitialDownload, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); + }; + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -200,2 +204,5 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf { + auto log_msg = LOG_MSG( "%s: txid=%s wtxid=%s", __func__, + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); auto event = [tx, mempool_sequence, this] { @@ -203,5 +210,3 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s", __func__, - tx.info.m_tx->GetHash().ToString(), - tx.info.m_tx->GetWitnessHash().ToString()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -209,6 +214,3 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { - auto event = [tx, reason, mempool_sequence, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); - }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: txid=%s wtxid=%s reason=%s", __func__, + auto log_msg = LOG_MSG( "%s: txid=%s wtxid=%s reason=%s", __func__, tx->GetHash().ToString(), @@ -216,2 +218,6 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, RemovalReasonToString(reason)); + auto event = [tx, reason, mempool_sequence, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); + }; + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -220,3 +226,5 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p { - std::string block_hash = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? pblock->GetHash().ToString() : ""; + auto log_msg = LOG_MSG( "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); auto event = [role, pblock = std::move(pblock), pindex, this] { @@ -224,5 +232,3 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, - block_hash, - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -231,2 +237,5 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem { + auto log_msg = LOG_MSG( "%s: block height=%s txs removed=%s", __func__, + nBlockHeight, + txs_removed_for_block.size()); auto event = [txs_removed_for_block, nBlockHeight, this] { @@ -234,5 +243,3 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block height=%s txs removed=%s", __func__, - nBlockHeight, - txs_removed_for_block.size()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -241,3 +248,5 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, { - std::string block_hash = ShouldLog(BCLog::VALIDATION, BCLog::Level::Debug) ? pblock->GetHash().ToString() : ""; + auto log_msg = LOG_MSG( "%s: block hash=%s block height=%d", __func__, + pblock->GetHash().ToString(), + pindex->nHeight); auto event = [pblock = std::move(pblock), pindex, this] { @@ -245,5 +254,3 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s block height=%d", __func__, - block_hash, - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); } @@ -252,2 +259,4 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo { + auto log_msg = LOG_MSG( "%s: block hash=%s", __func__, + locator.IsNull() ? "null" : locator.vHave.front().ToString()); auto event = [role, locator, this] { @@ -255,4 +264,3 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo }; - ENQUEUE_AND_LOG_EVENT(std::move(event), "%s: block hash=%s", __func__, - locator.IsNull() ? "null" : locator.vHave.front().ToString()); + ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); }

sedited · 2026-03-17T10:49:06Z

Thanks for the suggestion @maflcko,

Updated d62c6e9 -> f9c9fe7 (block_connected_move_3 -> block_connected_move_4, compare)

Took @maflcko's suggestion

Updated f9c9fe7 -> 8c4439a (block_connected_move_4 -> block_connected_move_5, compare)

Addressed @stickies-v's comment, renamed mentions for `ConnectTrace'

maflcko · 2026-03-18T10:39:37Z

re-review ACK 8c4439a 🏵

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 8c4439a5d13f3572c8ec9c3c5eb336e805b5cd33 🏵
rf+5s+7Qn1gi0Ta5aoxrSKB28/xBsKyd44leVHA5wYFbfyW0kXoeTBF4+SSGO8scM+0iG12WdJYfXj9LYa55Cg==

stickies-v · 2026-03-18T11:52:04Z

src/validationinterface.cpp

+    auto event = [pindexNew, pindexFork, fInitialDownload, this] {
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); });
+    };
+    ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg);


I think it's really awkward that even though ENQUEUE_AND_LOG_EVENT consumes both, event must be an rvalue and log_msg must not be. Why not just require them both to be rvalue? I also think the NOTE: The lambda captures the event by move docstring is no longer relevant.

git diff on 8c4439a

diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index cc1abcd6cd..124040d516 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -156,19 +156,18 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() // Use a macro instead of a function for conditional logging to prevent // evaluating arguments when logging is not enabled. -// -// NOTE: The lambda captures the event by move -#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ - do { \ - static_assert(std::is_rvalue_reference_v<decltype((event))>, \ - "event must be passed as an rvalue"); \ - static_assert(!std::is_const_v<std::remove_reference_t<decltype((log_msg))>>, \ - "log_msg must be mutable reference"); \ - LOG_EVENT("Enqueuing %s", log_msg); \ - m_internals->m_task_runner->insert([local_log_msg = std::move(log_msg), local_event = (event)] { \ - LOG_EVENT("%s", local_log_msg); \ - local_event(); \ - }); \ +#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ + do { \ + static_assert(std::is_rvalue_reference_v<decltype((event))>, \ + "event must be passed as an rvalue"); \ + static_assert(std::is_rvalue_reference_v<decltype((log_msg))>, \ + "log_msg must be passed as an rvalue"); \ + auto enqueue_log_msg = (log_msg); \ + LOG_EVENT("Enqueuing %s", enqueue_log_msg); \ + m_internals->m_task_runner->insert([event_log_msg = std::move(enqueue_log_msg), local_event = (event)]() mutable { \ + LOG_EVENT("%s", event_log_msg); \ + local_event(); \ + }); \ } while (0) #define LOG_MSG(fmt, ...) \ @@ -189,7 +188,7 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo auto event = [pindexNew, pindexFork, fInitialDownload, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) @@ -206,10 +205,11 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf auto event = [tx, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } -void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { +void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) +{ auto log_msg = LOG_MSG("%s: txid=%s wtxid=%s reason=%s", __func__, tx->GetHash().ToString(), tx->GetWitnessHash().ToString(), @@ -217,7 +217,7 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, auto event = [tx, reason, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex) @@ -228,7 +228,7 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p auto event = [role, pblock = std::move(pblock), pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) @@ -239,7 +239,7 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector<Rem auto event = [txs_removed_for_block, nBlockHeight, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, const CBlockIndex* pindex) @@ -250,7 +250,7 @@ void ValidationSignals::BlockDisconnected(std::shared_ptr<const CBlock> pblock, auto event = [pblock = std::move(pblock), pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlockLocator& locator) @@ -260,7 +260,7 @@ void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlo auto event = [role, locator, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); }; - ENQUEUE_AND_LOG_EVENT(std::move(event), log_msg); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state)

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 <[email protected]>

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.

sedited · 2026-03-18T12:45:03Z

Updated 8c4439a -> d6f680b (block_connected_move_5 -> block_connected_move_6, compare)

Took @stickies-v's suggestion, passing the log message to the macro as an rvalue.

maflcko · 2026-03-18T12:52:40Z

re-lgtm. Only change is calling the std::string move constructor once more for each function call, which my patch skipped, but I guess it is fast and harmless. Anything should be fine here.

re-review ACK d6f680b 🔌

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK d6f680b4275296e4a7f9e6ea693149e59800e495 🔌
CEHnyUbbBkwJ6/iA99iSBpEdEyQ3VDejMzvrL+/H8nGB3B5y0evMQCLMsqg0i/HqX8J3kor7eT/s+LLUS2d7Bw==

stickies-v · 2026-03-18T13:17:08Z

re-ACK d6f680b

frankomosh · 2026-03-18T15:35:59Z

Re-ACK d6f680b

DrahtBot added the Validation label Mar 1, 2026

DrahtBot mentioned this pull request Mar 2, 2026

fees: Introduce Mempool Based Fee Estimation to reduce overestimation #34075

Open

stickies-v reviewed Mar 2, 2026

View reviewed changes

mzumsande reviewed Mar 2, 2026

View reviewed changes

sedited force-pushed the block_connected_move branch from 76d1bc9 to 40c519b Compare March 2, 2026 09:56

stickies-v approved these changes Mar 2, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 2, 2026

validation: refactor: remove ConnectTrace #34708

Merged

frankomosh reviewed Mar 12, 2026

View reviewed changes

sedited force-pushed the block_connected_move branch from 40c519b to 95e3359 Compare March 12, 2026 11:12

DrahtBot requested a review from stickies-v March 12, 2026 11:23

rkrux reviewed Mar 13, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 14, 2026

mempool: asynchronous mempool fee rate diagram updates via validation interface #34803

Open

maflcko approved these changes Mar 16, 2026

View reviewed changes

sedited force-pushed the block_connected_move branch from 95e3359 to d62c6e9 Compare March 16, 2026 17:23

maflcko approved these changes Mar 17, 2026

View reviewed changes

sedited force-pushed the block_connected_move branch from d62c6e9 to f9c9fe7 Compare March 17, 2026 10:48

sedited force-pushed the block_connected_move branch from f9c9fe7 to 8c4439a Compare March 17, 2026 10:52

DrahtBot added the CI failed label Mar 17, 2026

DrahtBot removed the CI failed label Mar 17, 2026

DrahtBot requested review from frankomosh and rkrux March 18, 2026 10:39

stickies-v reviewed Mar 18, 2026

View reviewed changes

DrahtBot requested a review from stickies-v March 18, 2026 12:07

sedited and others added 3 commits March 18, 2026 13:34

sedited force-pushed the block_connected_move branch from 8c4439a to d6f680b Compare March 18, 2026 12:44

Conversation

sedited commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited commented Mar 2, 2026

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

sedited commented Mar 2, 2026

Uh oh!

sedited commented Mar 2, 2026

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

arejula27 commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankomosh left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux commented Mar 12, 2026

Uh oh!

sedited commented Mar 12, 2026

Uh oh!

frankomosh commented Mar 12, 2026

Uh oh!

stickies-v commented Mar 12, 2026

Uh oh!

rkrux commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

sedited commented Mar 16, 2026

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

sedited commented Mar 1, 2026 •

edited

Loading

DrahtBot commented Mar 1, 2026 •

edited

Loading

rkrux Mar 10, 2026 •

edited

Loading

arejula27 commented Mar 6, 2026 •

edited

Loading

rkrux commented Mar 13, 2026 •

edited

Loading

sedited commented Mar 17, 2026 •

edited

Loading