From 9ba1fff29e4794615c599e59ef453848a9bdb880 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 28 Jul 2025 13:42:13 +0100 Subject: [PATCH 1/2] kernel: refactor: ConnectTip to pass block pointer by value By passing by value, we can remove the need to allocate a new pointer if the callsite uses move semantics. In the process, simplify naming. --- src/validation.cpp | 24 +++++++++++++----------- src/validation.h | 7 ++++++- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b62691ca4c0..e339fa774f0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3145,7 +3145,12 @@ public: * * The block is added to connectTrace if connection succeeds. */ -bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) +bool Chainstate::ConnectTip( + BlockValidationState& state, + CBlockIndex* pindexNew, + std::shared_ptr block_to_connect, + ConnectTrace& connectTrace, + DisconnectedBlockTransactions& disconnectpool) { AssertLockHeld(cs_main); if (m_mempool) AssertLockHeld(m_mempool->cs); @@ -3153,18 +3158,15 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, assert(pindexNew->pprev == m_chain.Tip()); // Read block from disk. const auto time_1{SteadyClock::now()}; - std::shared_ptr pthisBlock; - if (!pblock) { + if (!block_to_connect) { std::shared_ptr pblockNew = std::make_shared(); if (!m_blockman.ReadBlock(*pblockNew, *pindexNew)) { return FatalError(m_chainman.GetNotifications(), state, _("Failed to read block.")); } - pthisBlock = pblockNew; + block_to_connect = std::move(pblockNew); } else { LogDebug(BCLog::BENCH, " - Using cached block\n"); - pthisBlock = pblock; } - const CBlock& blockConnecting = *pthisBlock; // Apply the block atomically to the chain state. const auto time_2{SteadyClock::now()}; SteadyClock::time_point time_3; @@ -3174,9 +3176,9 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, Ticks(time_2 - time_1)); { CCoinsViewCache view(&CoinsTip()); - bool rv = ConnectBlock(blockConnecting, state, pindexNew, view); + bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view); if (m_chainman.m_options.signals) { - m_chainman.m_options.signals->BlockChecked(blockConnecting, state); + m_chainman.m_options.signals->BlockChecked(*block_to_connect, state); } if (!rv) { if (state.IsInvalid()) @@ -3212,8 +3214,8 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, Ticks(m_chainman.time_chainstate) / m_chainman.num_blocks_total); // Remove conflicting transactions from the mempool.; if (m_mempool) { - m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight); - disconnectpool.removeForBlock(blockConnecting.vtx); + m_mempool->removeForBlock(block_to_connect->vtx, pindexNew->nHeight); + disconnectpool.removeForBlock(block_to_connect->vtx); } // Update m_chain & related variables. m_chain.SetTip(*pindexNew); @@ -3239,7 +3241,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, m_chainman.MaybeCompleteSnapshotValidation(); } - connectTrace.BlockConnected(pindexNew, std::move(pthisBlock)); + connectTrace.BlockConnected(pindexNew, std::move(block_to_connect)); return true; } diff --git a/src/validation.h b/src/validation.h index c25dd2de2d9..acdd53cc2a2 100644 --- a/src/validation.h +++ b/src/validation.h @@ -787,7 +787,12 @@ public: protected: bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); - bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); + bool ConnectTip( + BlockValidationState& state, + CBlockIndex* pindexNew, + std::shared_ptr block_to_connect, + ConnectTrace& connectTrace, + DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main); From 1d9f1cb4bd6b119e1d56cbdd7f6ce4d4521fffa3 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 28 Jul 2025 13:57:52 +0100 Subject: [PATCH 2/2] kernel: improve BlockChecked ownership semantics Subscribers to the BlockChecked validation interface event may need access to the block outside of the callback scope. Currently, this is only possible by copying the block, which makes exposing this validation interface event publicly either cumbersome or with significant copy overhead. By using shared_ptr, we make the shared ownership explicit and allow users to safely use the block outside of the callback scope. --- src/bitcoin-chainstate.cpp | 4 ++-- src/net_processing.cpp | 6 +++--- src/rpc/mining.cpp | 6 +++--- src/test/util/mining.cpp | 5 +++-- src/test/validationinterface_tests.cpp | 11 +++++------ src/validation.cpp | 4 ++-- src/validationinterface.cpp | 6 ++++-- src/validationinterface.h | 4 ++-- 8 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 47ddfaad8de..4ac6a6f41a1 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -200,9 +200,9 @@ int main(int argc, char* argv[]) explicit submitblock_StateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state() {} protected: - void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override + void BlockChecked(const std::shared_ptr& block, const BlockValidationState& stateIn) override { - if (block.GetHash() != hash) + if (block->GetHash() != hash) return; found = true; state = stateIn; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c4a89c44cb..e5c2a86d429 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -513,7 +513,7 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - void BlockChecked(const CBlock& block, const BlockValidationState& state) override + void BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); @@ -2103,11 +2103,11 @@ void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlock * Handle invalid block rejection and consequent peer discouragement, maintain which * peers announce compact blocks. */ -void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationState& state) +void PeerManagerImpl::BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) { LOCK(cs_main); - const uint256 hash(block.GetHash()); + const uint256 hash(block->GetHash()); std::map>::iterator it = mapBlockSource.find(hash); // If the block failed validation, we know where it came from and we're still connected diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index f11305f16b8..8b45a8c1293 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1030,9 +1030,9 @@ public: explicit submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), state() {} protected: - void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override { - if (block.GetHash() != hash) - return; + void BlockChecked(const std::shared_ptr& block, const BlockValidationState& stateIn) override + { + if (block->GetHash() != hash) return; found = true; state = stateIn; } diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp index 1cd8f8ee718..0dacf71b684 100644 --- a/src/test/util/mining.cpp +++ b/src/test/util/mining.cpp @@ -18,6 +18,7 @@ #include #include +#include using node::BlockAssembler; using node::NodeContext; @@ -83,9 +84,9 @@ struct BlockValidationStateCatcher : public CValidationInterface { m_state{} {} protected: - void BlockChecked(const CBlock& block, const BlockValidationState& state) override + void BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) override { - if (block.GetHash() != m_hash) return; + if (block->GetHash() != m_hash) return; m_state = state; } }; diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index a46cfc30292..8a24b282458 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -12,11 +12,12 @@ #include #include +#include BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, ChainTestingSetup) struct TestSubscriberNoop final : public CValidationInterface { - void BlockChecked(const CBlock&, const BlockValidationState&) override {} + void BlockChecked(const std::shared_ptr&, const BlockValidationState&) override {} }; BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) @@ -25,10 +26,9 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) // Start thread to generate notifications std::thread gen{[&] { - const CBlock block_dummy; BlockValidationState state_dummy; while (generate) { - m_node.validation_signals->BlockChecked(block_dummy, state_dummy); + m_node.validation_signals->BlockChecked(std::make_shared(), state_dummy); } }}; @@ -60,15 +60,14 @@ public: { if (m_on_destroy) m_on_destroy(); } - void BlockChecked(const CBlock& block, const BlockValidationState& state) override + void BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) override { if (m_on_call) m_on_call(); } void Call() { - CBlock block; BlockValidationState state; - m_signals.BlockChecked(block, state); + m_signals.BlockChecked(std::make_shared(), state); } std::function m_on_call; std::function m_on_destroy; diff --git a/src/validation.cpp b/src/validation.cpp index e339fa774f0..bd2963b496f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3178,7 +3178,7 @@ bool Chainstate::ConnectTip( CCoinsViewCache view(&CoinsTip()); bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view); if (m_chainman.m_options.signals) { - m_chainman.m_options.signals->BlockChecked(*block_to_connect, state); + m_chainman.m_options.signals->BlockChecked(block_to_connect, state); } if (!rv) { if (state.IsInvalid()) @@ -4554,7 +4554,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr& blo } if (!ret) { if (m_options.signals) { - m_options.signals->BlockChecked(*block, state); + m_options.signals->BlockChecked(block, state); } LogError("%s: AcceptBlock FAILED (%s)\n", __func__, state.ToString()); return false; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index f1ac9305ff2..313f730ea09 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -245,9 +246,10 @@ void ValidationSignals::ChainStateFlushed(ChainstateRole role, const CBlockLocat locator.IsNull() ? "null" : locator.vHave.front().ToString()); } -void ValidationSignals::BlockChecked(const CBlock& block, const BlockValidationState& state) { +void ValidationSignals::BlockChecked(const std::shared_ptr& block, const BlockValidationState& state) +{ LOG_EVENT("%s: block hash=%s state=%s", __func__, - block.GetHash().ToString(), state.ToString()); + block->GetHash().ToString(), state.ToString()); m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockChecked(block, state); }); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 5a50876809c..e0a88ad0f21 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -150,7 +150,7 @@ protected: * is guaranteed to be the current best block at the time the * callback was generated (not necessarily now). */ - virtual void BlockChecked(const CBlock&, const BlockValidationState&) {} + virtual void BlockChecked(const std::shared_ptr&, const BlockValidationState&) {} /** * Notifies listeners that a block which builds directly on our current tip * has been received and connected to the headers tree, though not validated yet. @@ -224,7 +224,7 @@ public: void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); void ChainStateFlushed(ChainstateRole, const CBlockLocator &); - void BlockChecked(const CBlock&, const BlockValidationState&); + void BlockChecked(const std::shared_ptr&, const BlockValidationState&); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); };