fix BlockStateCatcher data races

PIVX-Project · Apr 7, 2024 · ddbd4b0 · ddbd4b0
1 parent b01c3df
commit ddbd4b0
Show file tree

Hide file tree

Showing 10 changed files with 64 additions and 42 deletions.
diff --git a/src/miner.cpp b/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");
     }
 

diff --git a/src/rpc/mining.cpp b/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)

diff --git a/src/test/miner_tests.cpp b/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();
     }
 

diff --git a/src/test/mnpayments_tests.cpp b/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());
 

diff --git a/src/test/util/blocksutil.cpp b/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);

diff --git a/src/test/validation_block_tests.cpp b/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());
                 }
             }
         });

diff --git a/src/test/validation_tests.cpp b/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());
 }
 

diff --git a/src/util/blockstatecatcher.h b/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
diff --git a/src/validation.cpp b/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) {

diff --git a/src/wallet/test/pos_validations_tests.cpp b/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 ###