From 8b0fb64c0243fa06d97a394dfb4500f97ef05f7b Mon Sep 17 00:00:00 2001 From: sedited Date: Mon, 2 Mar 2026 09:42:18 +0100 Subject: [PATCH 1/3] validation: Move validation signal events to task runner 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 --- src/validationinterface.cpp | 78 +++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c7be6abc3a1..3c142f8db28 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -156,33 +156,39 @@ 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 all local 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([=] { \ - LOG_EVENT(fmt, local_name, __VA_ARGS__); \ - event(); \ - }); \ +#define ENQUEUE_AND_LOG_EVENT(event, log_msg) \ + do { \ + static_assert(std::is_rvalue_reference_v, \ + "event must be passed as an rvalue"); \ + static_assert(std::is_rvalue_reference_v, \ + "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([local_log_msg = std::move(enqueue_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 "\n", __VA_ARGS__) + 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 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__, + 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] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); + }; + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) @@ -193,61 +199,67 @@ void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool 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] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; - ENQUEUE_AND_LOG_EVENT(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), std::move(log_msg)); } 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(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(), tx->GetWitnessHash().ToString(), 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), std::move(log_msg)); } void ValidationSignals::BlockConnected(const ChainstateRole& role, const std::shared_ptr& 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, 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__, - pblock->GetHash().ToString(), - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector& 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] { 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__, - nBlockHeight, - txs_removed_for_block.size()); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::BlockDisconnected(const std::shared_ptr& 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, pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s block height=%d", __func__, - pblock->GetHash().ToString(), - pindex->nHeight); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(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] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); }; - ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__, - locator.IsNull() ? "null" : locator.vHave.front().ToString()); + ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } void ValidationSignals::BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) From 4d02d2b316a41119e77448056339cb8fb9ae7b6a Mon Sep 17 00:00:00 2001 From: sedited Date: Fri, 30 Jan 2026 12:48:04 +0100 Subject: [PATCH 2/3] 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: 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. --- src/validation.cpp | 4 ++-- src/validationinterface.cpp | 4 ++-- src/validationinterface.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1efe109b6f6..f9cc7d78e1b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3391,9 +3391,9 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< } pindexNewTip = m_chain.Tip(); - for (const auto& [index, block] : connected_blocks) { + for (auto& [index, block] : std::move(connected_blocks)) { if (m_chainman.m_options.signals) { - m_chainman.m_options.signals->BlockConnected(chainstate_role, Assert(block), Assert(index)); + m_chainman.m_options.signals->BlockConnected(chainstate_role, std::move(Assert(block)), Assert(index)); } } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 3c142f8db28..3e939de964e 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -219,12 +219,12 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } -void ValidationSignals::BlockConnected(const ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindex) +void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_ptr 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, pindex, this] { + 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), std::move(log_msg)); diff --git a/src/validationinterface.h b/src/validationinterface.h index 4777e8dca8d..641afd555be 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -223,7 +223,7 @@ public: void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); - void BlockConnected(const kernel::ChainstateRole&, const std::shared_ptr&, const CBlockIndex* pindex); + void BlockConnected(const kernel::ChainstateRole&, std::shared_ptr, const CBlockIndex* pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); void ChainStateFlushed(const kernel::ChainstateRole&, const CBlockLocator&); void BlockChecked(const std::shared_ptr&, const BlockValidationState&); From d6f680b4275296e4a7f9e6ea693149e59800e495 Mon Sep 17 00:00:00 2001 From: sedited Date: Sat, 28 Feb 2026 12:13:53 +0100 Subject: [PATCH 3/3] 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. --- src/validation.cpp | 2 +- src/validationinterface.cpp | 4 ++-- src/validationinterface.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index f9cc7d78e1b..00c1bab501f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2982,7 +2982,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: if (m_chainman.m_options.signals) { - m_chainman.m_options.signals->BlockDisconnected(pblock, pindexDelete); + m_chainman.m_options.signals->BlockDisconnected(std::move(pblock), pindexDelete); } return true; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 3e939de964e..45f38b3740b 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -241,12 +241,12 @@ void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector& pblock, const CBlockIndex* pindex) +void ValidationSignals::BlockDisconnected(std::shared_ptr 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, pindex, this] { + auto event = [pblock = std::move(pblock), pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockDisconnected(pblock, pindex); }); }; ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); diff --git a/src/validationinterface.h b/src/validationinterface.h index 641afd555be..8cfe41380ec 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -224,7 +224,7 @@ public: void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(const kernel::ChainstateRole&, std::shared_ptr, const CBlockIndex* pindex); - void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); + void BlockDisconnected(std::shared_ptr, const CBlockIndex* pindex); void ChainStateFlushed(const kernel::ChainstateRole&, const CBlockLocator&); void BlockChecked(const std::shared_ptr&, const BlockValidationState&); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&);