Merge bitcoin/bitcoin#33078: kernel: improve BlockChecked ownership semantics

1d9f1cb kernel: improve BlockChecked ownership semantics (stickies-v)
9ba1fff kernel: refactor: ConnectTip to pass block pointer by value (stickies-v)

Pull request description:

  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. By using a const-ref shared_ptr, no atomic reference count cost is incurred if a subscriber does not require block ownership.

  For example: in  #30595, this would allow us to drop the `kernel_BlockPointer` handle entirely, and generalize everything into `kernel_Block`. This PoC is implemented in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/.

  ---

  ### Performance

  I have added a benchmark in a [separate branch](https://github.com/stickies-v/bitcoin/commits/2025-07/validation-interface-ownership-benched/), to ensure this change does not lead to a problematic performance regression. Since most of the overhead comes from the subscribers, I have added scenarios for `One`, `Two`, and `Ten` subscribers. From these results, it appears there is no meaningful performance difference on my machine.

  When `BlockChecked()` takes a `const CBlock&` reference _(master)_:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |              170.09 |        5,879,308.26 |    0.3% |      0.01 | `BlockCheckedOne`
  |            1,603.95 |          623,460.10 |    0.5% |      0.01 | `BlockCheckedTen`
  |              336.00 |        2,976,173.37 |    1.1% |      0.01 | `BlockCheckedTwo`

  When `BlockChecked()` takes a `const std::shared_ptr<const CBlock>&` _(this PR)_:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |              172.20 |        5,807,155.33 |    0.1% |      0.01 | `BlockCheckedOne`
  |            1,596.79 |          626,254.52 |    0.0% |      0.01 | `BlockCheckedTen`
  |              333.38 |        2,999,603.17 |    0.3% |      0.01 | `BlockCheckedTwo`

ACKs for top commit:
  achow101:
    ACK 1d9f1cb
  w0xlt:
    reACK bitcoin/bitcoin@1d9f1cb
  ryanofsky:
    Code review ACK 1d9f1cb. These all seem like simple changes that make sense
  TheCharlatan:
    ACK 1d9f1cb
  yuvicc:
    Code Review ACK 1d9f1cb

Tree-SHA512: 7ed0cccb7883dbb1885917ef749ab7cae5d60ee803b7e3145b2954d885e81ba8c9d5ab1edb9694ce6b308235c621094c029024eaf99f1aab1b47311c40958095

Author: achow101

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<const CBlock>& block, const BlockValidationState& stateIn) override
             {
-                if (block.GetHash() != hash)
+                if (block->GetHash() != hash)
                     return;
                 found = true;
                 state = stateIn;

src/net_processing.cpp

@@ -513,7 +513,7 @@ class PeerManagerImpl final : public PeerManager
         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<const CBlock>& block, const BlockValidationState& state) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
@@ -2071,11 +2071,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<const CBlock>& block, const BlockValidationState& state)
 {
     LOCK(cs_main);
 
-    const uint256 hash(block.GetHash());
+    const uint256 hash(block->GetHash());
     std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
 
     // If the block failed validation, we know where it came from and we're still connected

src/rpc/mining.cpp

@@ -1030,9 +1030,9 @@ class submitblock_StateCatcher final : public CValidationInterface
     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<const CBlock>& block, const BlockValidationState& stateIn) override
+    {
+        if (block->GetHash() != hash) return;
         found = true;
         state = stateIn;
     }

src/test/util/mining.cpp

@@ -18,6 +18,7 @@
 #include <versionbits.h>
 
 #include <algorithm>
+#include <memory>
 
 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<const CBlock>& block, const BlockValidationState& state) override
     {
-        if (block.GetHash() != m_hash) return;
+        if (block->GetHash() != m_hash) return;
         m_state = state;
     }
 };

src/test/validationinterface_tests.cpp

@@ -12,11 +12,12 @@
 #include <validationinterface.h>
 
 #include <atomic>
+#include <memory>
 
 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 CBlock>&, 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<const CBlock>(), state_dummy);
         }
     }};
 
@@ -60,15 +60,14 @@ class TestInterface : public CValidationInterface
     {
         if (m_on_destroy) m_on_destroy();
     }
-    void BlockChecked(const CBlock& block, const BlockValidationState& state) override
+    void BlockChecked(const std::shared_ptr<const CBlock>& 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<const CBlock>(), state);
     }
     std::function<void()> m_on_call;
     std::function<void()> m_on_destroy;

src/validation.cpp

@@ -3108,26 +3108,28 @@ class ConnectTrace {
  *
  * The block is added to connectTrace if connection succeeds.
  */
-bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
+bool Chainstate::ConnectTip(
+    BlockValidationState& state,
+    CBlockIndex* pindexNew,
+    std::shared_ptr<const CBlock> block_to_connect,
+    ConnectTrace& connectTrace,
+    DisconnectedBlockTransactions& disconnectpool)
 {
     AssertLockHeld(cs_main);
     if (m_mempool) AssertLockHeld(m_mempool->cs);
 
     assert(pindexNew->pprev == m_chain.Tip());
     // Read block from disk.
     const auto time_1{SteadyClock::now()};
-    std::shared_ptr<const CBlock> pthisBlock;
-    if (!pblock) {
+    if (!block_to_connect) {
         std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
         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;
@@ -3137,9 +3139,9 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
              Ticks<MillisecondsDouble>(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())
@@ -3175,8 +3177,8 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
              Ticks<MillisecondsDouble>(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);
@@ -3202,7 +3204,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;
 }
 
@@ -4515,7 +4517,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& 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;

src/validation.h

@@ -799,7 +799,12 @@ class Chainstate
 
 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 ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
+    bool ConnectTip(
+        BlockValidationState& state,
+        CBlockIndex* pindexNew,
+        std::shared_ptr<const CBlock> 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);

src/validationinterface.cpp

@@ -17,6 +17,7 @@
 #include <util/task_runner.h>
 
 #include <future>
+#include <memory>
 #include <unordered_map>
 #include <utility>
 
@@ -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<const CBlock>& 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); });
 }
 

src/validationinterface.h

@@ -150,7 +150,7 @@ class CValidationInterface {
      * 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 CBlock>&, 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 @@ class ValidationSignals {
     void BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex);
     void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
     void ChainStateFlushed(ChainstateRole, const CBlockLocator &);
-    void BlockChecked(const CBlock&, const BlockValidationState&);
+    void BlockChecked(const std::shared_ptr<const CBlock>&, const BlockValidationState&);
     void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr<const CBlock>&);
 };