Merge #2924: [Bug] Fix validation interface data race

ddbd4b0 fix BlockStateCatcher data races (ale)
b01c3df let validationinterface take shared_ptr (ale)

Pull request description:

  first commit:
  Partially backport bitcoin PR bitcoin#18338 which introduce the functions
  `RegisterSharedValidationInterface` and `UnregisterSharedValidationInterface`.  See that PR why using normal pointers is problematic, and how `shared_ptr` solve the issue
  (In a few words the problem is that it can happen that the pointed memory is freed before all signals have been processed, while with shared pointer we are sure that  memory will be freed after the last signal is handled)

  second commit:
  Utilize the new functions to the validation interface `BlockStateCatcher`.  In order to keep everywhere the logic unchanged (Registering on creation and Unregistering when the object goes out of scope) I created the wrapper class `BlockStateCatcherWrapper` which contains a `shared_ptr` to `BlockStatecatcher`.

  Those two commits solve the following data race where a `BlockStateCatcher` pointer is dereferenced after the pointed memory is freed

  ```
  WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=6423)
    Write of size 8 at 0x7fc3d7c2d570 by thread T20:
      #0 CValidationInterface::~CValidationInterface() validationinterface.h:75 (pivxd+0xeacac)
      #1 BlockStateCatcher::~BlockStateCatcher() util/blockstatecatcher.h:23 (pivxd+0xeacac)
      #2 generateBlocks(Consensus::Params const&, CWallet*, bool, int, int, int, CScript*) rpc/mining.cpp:78 (pivxd+0x1c1a98)
      #3 generate(JSONRPCRequest const&) rpc/mining.cpp:140 (pivxd+0x1c2215)
      ...

    Previous read of size 8 at 0x7fc3d7c2d570 by thread T35 (mutexes: write M133270, write M132847):
      #0 void std::__invoke_impl<void, void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>(std::__invoke_memfun_deref, void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*&&) /usr/include/c++/12/bits/invoke.h:74 (pivxd+0x2eef14)
      #1 std::__invoke_result<void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>::type std::__invoke<void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*>(void (CValidationInterface::*&)(CConnman*), CValidationInterface*&, CConnman*&&) /usr/include/c++/12/bits/invoke.h:96 (pivxd+0x2eefbb)
      #2 void std::_Bind<void (CValidationInterface::*(CValidationInterface*, std::_Placeholder<1>))(CConnman*)>::__call<void, CConnman*&&, 0ul, 1ul>(std::tuple<CConnman*&&>&&, std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/functional:484 (pivxd+0x2eefbb)
      ...
     ```

ACKs for top commit: ddbd4b0
  Duddino:
    utACK ddbd4b0
  Liquid369:
    tACK ddbd4b0

Tree-SHA512: 94d55effc11f14f755f27c30ef1b6801a235bc314cc34312ebb14b75525824d3b11bb7812389d9977c30a0929d62fb745bd5c1a15249708014bd573132a5e55c

src/miner.cpp

@@ -78,10 +78,10 @@ bool ProcessBlockFound(const std::shared_ptr<const CBlock>& pblock, CWallet& wal
         reservekey->KeepKey();
 
     // Process this block the same as if we had received it from another node
-    BlockStateCatcher sc(pblock->GetHash());
+    BlockStateCatcherWrapper sc(pblock->GetHash());
     sc.registerEvent();
     bool res = ProcessNewBlock(pblock, nullptr);
-    if (!res || sc.stateErrorFound()) {
+    if (!res || sc.get().stateErrorFound()) {
         return error("PIVXMiner : ProcessNewBlock, block not accepted");
     }
 

src/rpc/mining.cpp

@@ -36,7 +36,7 @@ UniValue generateBlocks(const Consensus::Params& consensus,
 {
     UniValue blockHashes(UniValue::VARR);
 
-    BlockStateCatcher sc(UINT256_ZERO);
+    BlockStateCatcherWrapper sc(UINT256_ZERO);
     sc.registerEvent();
     while (nHeight < nHeightEnd && !ShutdownRequested()) {
 
@@ -57,9 +57,9 @@ UniValue generateBlocks(const Consensus::Params& consensus,
             if (!SolveBlock(pblock, nHeight + 1)) continue;
         }
 
-        sc.setBlockHash(pblock->GetHash());
+        sc.get().setBlockHash(pblock->GetHash());
         bool res = ProcessNewBlock(pblock, nullptr);
-        if (!res || sc.stateErrorFound())
+        if (!res || sc.get().stateErrorFound())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
 
         ++nHeight;
@@ -772,19 +772,19 @@ UniValue submitblock(const JSONRPCRequest& request)
         }
     }
 
-    BlockStateCatcher sc(block.GetHash());
+    BlockStateCatcherWrapper sc(block.GetHash());
     sc.registerEvent();
     bool fAccepted = ProcessNewBlock(blockptr, nullptr);
     if (fBlockPresent) {
-        if (fAccepted && !sc.found)
+        if (fAccepted && !sc.get().found)
             return "duplicate-inconclusive";
         return "duplicate";
     }
     if (fAccepted) {
-        if (!sc.found)
+        if (!sc.get().found)
             return "inconclusive";
     }
-    return BIP22ValidationResult(sc.state);
+    return BIP22ValidationResult(sc.get().state);
 }
 
 UniValue estimatefee(const JSONRPCRequest& request)

src/test/miner_tests.cpp

@@ -199,11 +199,11 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
             txFirst.emplace_back(pblock->vtx[0]);
         pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
         pblock->nNonce = blockinfo[i].nonce;
-        BlockStateCatcher stateCatcher(pblock->GetHash());
+        BlockStateCatcherWrapper stateCatcher(pblock->GetHash());
         stateCatcher.registerEvent();
         BOOST_CHECK(ProcessNewBlock(pblock, nullptr));
         SyncWithValidationInterfaceQueue();
-        BOOST_CHECK(stateCatcher.found && stateCatcher.state.IsValid());
+        BOOST_CHECK(stateCatcher.get().found && stateCatcher.get().state.IsValid());
         pblock->hashPrevBlock = pblock->GetHash();
     }
 

src/test/mnpayments_tests.cpp

@@ -245,11 +245,11 @@ BOOST_FIXTURE_TEST_CASE(mnwinner_test, TestChain100Setup)
     {
         auto pBadBlock = std::make_shared<CBlock>(badBlock);
         SolveBlock(pBadBlock, nextBlockHeight);
-        BlockStateCatcher sc(pBadBlock->GetHash());
+        BlockStateCatcherWrapper sc(pBadBlock->GetHash());
         sc.registerEvent();
         ProcessNewBlock(pBadBlock, nullptr);
-        BOOST_CHECK(sc.found && !sc.state.IsValid());
-        BOOST_CHECK_EQUAL(sc.state.GetRejectReason(), "bad-cb-payee");
+        BOOST_CHECK(sc.get().found && !sc.get().state.IsValid());
+        BOOST_CHECK_EQUAL(sc.get().state.GetRejectReason(), "bad-cb-payee");
     }
     BOOST_CHECK(WITH_LOCK(cs_main, return chainActive.Tip()->GetBlockHash();) != badBlock.GetHash());
 

src/test/util/blocksutil.cpp

@@ -14,11 +14,11 @@ void ProcessBlockAndCheckRejectionReason(std::shared_ptr<CBlock>& pblock,
                                          const std::string& blockRejectionReason,
                                          int expectedChainHeight)
 {
-    BlockStateCatcher stateCatcher(pblock->GetHash());
+    BlockStateCatcherWrapper stateCatcher(pblock->GetHash());
     stateCatcher.registerEvent();
     ProcessNewBlock(pblock, nullptr);
-    BOOST_CHECK(stateCatcher.found);
-    CValidationState state = stateCatcher.state;
+    BOOST_CHECK(stateCatcher.get().found);
+    CValidationState state = stateCatcher.get().state;
     BOOST_CHECK_EQUAL(WITH_LOCK(cs_main, return chainActive.Height();), expectedChainHeight);
     BOOST_CHECK(!state.IsValid());
     BOOST_CHECK_EQUAL(state.GetRejectReason(), blockRejectionReason);

src/test/validation_block_tests.cpp

@@ -137,18 +137,18 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
                 ProcessNewBlock(block, nullptr);
             }
 
-            BlockStateCatcher sc(UINT256_ZERO);
+            BlockStateCatcherWrapper sc(UINT256_ZERO);
             sc.registerEvent();
             // to make sure that eventually we process the full chain - do it here
             for (const auto& block : blocks) {
                 if (block->vtx.size() == 1) {
-                    sc.setBlockHash(block->GetHash());
+                    sc.get().setBlockHash(block->GetHash());
                     bool processed = ProcessNewBlock(block, nullptr);
                     // Future to do: "prevblk-not-found" here is the only valid reason to not check processed flag.
-                    std::string stateReason = sc.state.GetRejectReason();
-                    if (sc.found && (stateReason == "duplicate" || stateReason == "prevblk-not-found" ||
-                        stateReason == "bad-prevblk" || stateReason == "blk-out-of-order")) continue;
-                    ASSERT_WITH_MSG(processed,  ("Error: " + sc.state.GetRejectReason()).c_str());
+                    std::string stateReason = sc.get().state.GetRejectReason();
+                    if (sc.get().found && (stateReason == "duplicate" || stateReason == "prevblk-not-found" ||
+                                              stateReason == "bad-prevblk" || stateReason == "blk-out-of-order")) continue;
+                    ASSERT_WITH_MSG(processed, ("Error: " + sc.get().state.GetRejectReason()).c_str());
                 }
             }
         });

src/test/validation_tests.cpp

@@ -98,11 +98,11 @@ void CheckBlockZcRejection(std::shared_ptr<CBlock>& pblock, int nHeight, CMutabl
 {
     pblock->vtx.emplace_back(MakeTransactionRef(mtx));
     BOOST_CHECK(SolveBlock(pblock, nHeight));
-    BlockStateCatcher stateCatcher(pblock->GetHash());
+    BlockStateCatcherWrapper stateCatcher(pblock->GetHash());
     stateCatcher.registerEvent();
     BOOST_CHECK(!ProcessNewBlock(pblock, nullptr));
-    BOOST_CHECK(stateCatcher.found && !stateCatcher.state.IsValid());
-    BOOST_CHECK_EQUAL(stateCatcher.state.GetRejectReason(), expected_msg);
+    BOOST_CHECK(stateCatcher.get().found && !stateCatcher.get().state.IsValid());
+    BOOST_CHECK_EQUAL(stateCatcher.get().state.GetRejectReason(), expected_msg);
     BOOST_CHECK(WITH_LOCK(cs_main, return chainActive.Tip()->GetBlockHash(); ) != pblock->GetHash());
 }
 

src/util/blockstatecatcher.h

@@ -17,11 +17,7 @@ class BlockStateCatcher : public CValidationInterface
     uint256 hash;
     bool found;
     CValidationState state;
-    bool isRegistered{false};
-
     BlockStateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state(){};
-    ~BlockStateCatcher() { if (isRegistered) UnregisterValidationInterface(this); }
-    void registerEvent() { RegisterValidationInterface(this); isRegistered = true; }
     void setBlockHash(const uint256& _hash) { clear(); hash = _hash; }
     void clear() { hash.SetNull(); found = false; state = CValidationState(); }
     bool stateErrorFound() { return found && state.IsError(); }
@@ -34,4 +30,30 @@ class BlockStateCatcher : public CValidationInterface
     };
 };
 
+class BlockStateCatcherWrapper
+{
+private:
+    std::shared_ptr<BlockStateCatcher> stateCatcher = nullptr;
+    bool isRegistered = false;
+
+public:
+    explicit BlockStateCatcherWrapper(const uint256& hashIn)
+    {
+        stateCatcher = std::make_shared<BlockStateCatcher>(hashIn);
+    }
+    ~BlockStateCatcherWrapper()
+    {
+        if (isRegistered) UnregisterSharedValidationInterface(stateCatcher);
+    }
+    void registerEvent()
+    {
+        RegisterSharedValidationInterface(stateCatcher);
+        isRegistered = true;
+    }
+    BlockStateCatcher& get() const
+    {
+        return *stateCatcher;
+    }
+};
+
 #endif //PIVX_BLOCKSTATECATCHER_H

src/validation.cpp

@@ -3964,7 +3964,7 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
     int64_t nStart = GetTimeMillis();
 
     // Block checked event listener
-    BlockStateCatcher stateCatcher(UINT256_ZERO);
+    BlockStateCatcherWrapper stateCatcher(UINT256_ZERO);
     stateCatcher.registerEvent();
 
     int nLoaded = 0;
@@ -4025,11 +4025,11 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
                 // process in case the block isn't known yet
                 if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) {
                     std::shared_ptr<const CBlock> block_ptr = std::make_shared<const CBlock>(block);
-                    stateCatcher.setBlockHash(block_ptr->GetHash());
+                    stateCatcher.get().setBlockHash(block_ptr->GetHash());
                     if (ProcessNewBlock(block_ptr, dbp)) {
                         nLoaded++;
                     }
-                    if (stateCatcher.stateErrorFound()) {
+                    if (stateCatcher.get().stateErrorFound()) {
                         break;
                     }
                 } else if (hash != Params().GetConsensus().hashGenesisBlock && pindex->nHeight % 1000 == 0) {

src/validationinterface.cpp

@@ -93,10 +93,11 @@ CMainSignals& GetMainSignals()
 {
     return g_signals;
 }
-
-void RegisterValidationInterface(CValidationInterface* pwalletIn)
+void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn)
 {
-    ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn];
+    // Each connection captures pwalletIn to ensure that each callback is
+    // executed before pwalletIn is destroyed. For more details see bitcoin #18338
+    ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[pwalletIn.get()];
     conns.AcceptedBlockHeader = g_signals.m_internals->AcceptedBlockHeader.connect(std::bind(&CValidationInterface::AcceptedBlockHeader, pwalletIn, std::placeholders::_1));
     conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
     conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1));
@@ -108,6 +109,12 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn)
     conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2));
     conns.NotifyMasternodeListChanged = g_signals.m_internals->NotifyMasternodeListChanged.connect(std::bind(&CValidationInterface::NotifyMasternodeListChanged, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
 }
+void RegisterValidationInterface(CValidationInterface* pwalletIn)
+{
+    // Create a shared_ptr with a no-op deleter - CValidationInterface lifecycle
+    // is managed by the caller.
+    RegisterSharedValidationInterface({pwalletIn, [](CValidationInterface*) {}});
+}
 
 void UnregisterValidationInterface(CValidationInterface* pwalletIn)
 {
@@ -116,6 +123,11 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn)
     }
 }
 
+void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn)
+{
+    UnregisterValidationInterface(pwalletIn.get());
+}
+
 void UnregisterAllValidationInterfaces()
 {
     if (!g_signals.m_internals) {

src/validationinterface.h

@@ -34,6 +34,14 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn);
 void UnregisterValidationInterface(CValidationInterface* pwalletIn);
 /** Unregister all wallets from core */
 void UnregisterAllValidationInterfaces();
+
+// Alternate registration functions that release a shared_ptr after the last
+// notification is sent. These are useful for race-free cleanup, since
+// unregistration is nonblocking and can return before the last notification is
+// processed.
+void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn);
+void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn);
+
 /**
  * Pushes a function to callback onto the notification queue, guaranteeing any
  * callbacks generated prior to now are finished when the function is called.
@@ -147,7 +155,7 @@ class CValidationInterface {
     /** Tells listeners to broadcast their data. */
     virtual void ResendWalletTransactions(CConnman* connman) {}
     virtual void BlockChecked(const CBlock&, const CValidationState&) {}
-    friend void ::RegisterValidationInterface(CValidationInterface*);
+    friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>);
     friend void ::UnregisterValidationInterface(CValidationInterface*);
     friend void ::UnregisterAllValidationInterfaces();
     /** Notifies listeners of updated deterministic masternode list */
@@ -159,7 +167,7 @@ class CMainSignals {
 private:
     std::unique_ptr<MainSignalsInstance> m_internals;
 
-    friend void ::RegisterValidationInterface(CValidationInterface*);
+    friend void ::RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>);
     friend void ::UnregisterValidationInterface(CValidationInterface*);
     friend void ::UnregisterAllValidationInterfaces();
     friend void ::CallFunctionInValidationInterfaceQueue(std::function<void ()> func);

src/wallet/test/pos_validations_tests.cpp

@@ -305,12 +305,12 @@ BOOST_FIXTURE_TEST_CASE(created_on_fork_tests, TestPoSChainSetup)
     pindexPrev = mapBlockIndex.at(pblockE2->GetHash());
     std::shared_ptr<CBlock> pblock5Forked = CreateBlockInternal(pwalletMain.get(), {F2_tx1},
                                                                 pindexPrev, {pblockD1, pblockE2});
-    BlockStateCatcher stateCatcher(pblock5Forked->GetHash());
+    BlockStateCatcherWrapper stateCatcher(pblock5Forked->GetHash());
     stateCatcher.registerEvent();
     BOOST_CHECK(!ProcessNewBlock(pblock5Forked, nullptr));
-    BOOST_CHECK(stateCatcher.found);
-    BOOST_CHECK(!stateCatcher.state.IsValid());
-    BOOST_CHECK_EQUAL(stateCatcher.state.GetRejectReason(), "bad-txns-inputs-spent-fork-post-split");
+    BOOST_CHECK(stateCatcher.get().found);
+    BOOST_CHECK(!stateCatcher.get().state.IsValid());
+    BOOST_CHECK_EQUAL(stateCatcher.get().state.GetRejectReason(), "bad-txns-inputs-spent-fork-post-split");
 
     // #############################################
     // ### 4) coins created in D and spent in E3 ###
@@ -329,12 +329,12 @@ BOOST_FIXTURE_TEST_CASE(created_on_fork_tests, TestPoSChainSetup)
 
     pindexPrev = mapBlockIndex.at(pblockD3->GetHash());
     std::shared_ptr<CBlock> pblockE3 = CreateBlockInternal(pwalletMain.get(), {E3_tx1}, pindexPrev, {pblockD3});
-    stateCatcher.clear();
-    stateCatcher.setBlockHash(pblockE3->GetHash());
+    stateCatcher.get().clear();
+    stateCatcher.get().setBlockHash(pblockE3->GetHash());
     BOOST_CHECK(!ProcessNewBlock(pblockE3, nullptr));
-    BOOST_CHECK(stateCatcher.found);
-    BOOST_CHECK(!stateCatcher.state.IsValid());
-    BOOST_CHECK_EQUAL(stateCatcher.state.GetRejectReason(), "bad-txns-inputs-created-post-split");
+    BOOST_CHECK(stateCatcher.get().found);
+    BOOST_CHECK(!stateCatcher.get().state.IsValid());
+    BOOST_CHECK_EQUAL(stateCatcher.get().state.GetRejectReason(), "bad-txns-inputs-created-post-split");
 
     // ####################################################################
     // ### 5) coins create in D, spent in F and then double spent in F3 ###