From b01c3df1bdd083be2e7723e0361bffe396a9337b Mon Sep 17 00:00:00 2001 From: ale Date: Sat, 6 Apr 2024 15:40:53 +0200 Subject: [PATCH 1/2] let validationinterface take shared_ptr --- src/validationinterface.cpp | 18 +++++++++++++++--- src/validationinterface.h | 12 ++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c37b61d59ba08..2859edd5cf7e6 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -93,10 +93,11 @@ CMainSignals& GetMainSignals() { return g_signals; } - -void RegisterValidationInterface(CValidationInterface* pwalletIn) +void RegisterSharedValidationInterface(std::shared_ptr 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 pwalletIn) +{ + UnregisterValidationInterface(pwalletIn.get()); +} + void UnregisterAllValidationInterfaces() { if (!g_signals.m_internals) { diff --git a/src/validationinterface.h b/src/validationinterface.h index de6f7ed044ddf..18ab38356b563 100644 --- a/src/validationinterface.h +++ b/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 pwalletIn); +void UnregisterSharedValidationInterface(std::shared_ptr 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); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); /** Notifies listeners of updated deterministic masternode list */ @@ -159,7 +167,7 @@ class CMainSignals { private: std::unique_ptr m_internals; - friend void ::RegisterValidationInterface(CValidationInterface*); + friend void ::RegisterSharedValidationInterface(std::shared_ptr); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); friend void ::CallFunctionInValidationInterfaceQueue(std::function func); From ddbd4b01549999efa183373e349fc963d0f76aa2 Mon Sep 17 00:00:00 2001 From: ale Date: Sat, 6 Apr 2024 16:50:08 +0200 Subject: [PATCH 2/2] fix BlockStateCatcher data races --- src/miner.cpp | 4 +-- src/rpc/mining.cpp | 14 +++++------ src/test/miner_tests.cpp | 4 +-- src/test/mnpayments_tests.cpp | 6 ++--- src/test/util/blocksutil.cpp | 6 ++--- src/test/validation_block_tests.cpp | 12 ++++----- src/test/validation_tests.cpp | 6 ++--- src/util/blockstatecatcher.h | 30 ++++++++++++++++++++--- src/validation.cpp | 6 ++--- src/wallet/test/pos_validations_tests.cpp | 18 +++++++------- 10 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index ce42f821a6721..abce96ae79f68 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -78,10 +78,10 @@ bool ProcessBlockFound(const std::shared_ptr& 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 index b582fe2d554d5..9b44d4a44d58b 100644 --- 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 index 061f15213dbfc..58ffd048dfe79 100644 --- 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 index 9baee5f922390..7053115b92109 100644 --- 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(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 index 15d141f4383f3..8043665b89417 100644 --- a/src/test/util/blocksutil.cpp +++ b/src/test/util/blocksutil.cpp @@ -14,11 +14,11 @@ void ProcessBlockAndCheckRejectionReason(std::shared_ptr& 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 index c53a999ddb02b..6a5a02fed10c9 100644 --- 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 index f71fde210681c..820e187556ae3 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -98,11 +98,11 @@ void CheckBlockZcRejection(std::shared_ptr& 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 index d5c60a50fb20d..c08779f19706b 100644 --- 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 stateCatcher = nullptr; + bool isRegistered = false; + +public: + explicit BlockStateCatcherWrapper(const uint256& hashIn) + { + stateCatcher = std::make_shared(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 index 9c02ceecce5a4..9c174c7d54e7e 100644 --- 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 block_ptr = std::make_shared(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 index e07d89d1c5882..38b436f7df947 100644 --- 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 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 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 ###