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.

Author: stickies-v

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);
@@ -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<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

@@ -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<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/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>&);
 };