[Bug] Fix validation interface data race

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 ###