From 3fba3d5deec6d7bae33823b8da7682f9b03d9deb Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 24 Jan 2024 10:28:55 +0100 Subject: [PATCH] [refactor] Make signals optional in mempool and chainman This is done in preparation for the next two commits, where the CMainSignals are de-globalized. This avoids adding new constructor arguments to the ChainstateManager and CTxMemPool classes over the next two commits. This could also allow future tests that are only interested in the internal behaviour of the classes to forgo instantiating the signals. --- .../devtools/test_deterministic_coverage.sh | 26 ++++----- src/bitcoin-chainstate.cpp | 1 + src/init.cpp | 2 + src/kernel/chainstatemanager_opts.h | 2 + src/kernel/mempool_options.h | 4 ++ src/test/util/setup_common.cpp | 1 + src/test/util/txmempool.cpp | 2 + .../validation_chainstatemanager_tests.cpp | 1 + src/txmempool.cpp | 11 ++-- src/txmempool.h | 3 + src/validation.cpp | 58 ++++++++++++------- 11 files changed, 72 insertions(+), 39 deletions(-) diff --git a/contrib/devtools/test_deterministic_coverage.sh b/contrib/devtools/test_deterministic_coverage.sh index 8501c72f04a..23c260b529f 100755 --- a/contrib/devtools/test_deterministic_coverage.sh +++ b/contrib/devtools/test_deterministic_coverage.sh @@ -17,21 +17,21 @@ NON_DETERMINISTIC_TESTS=( "blockfilter_index_tests/blockfilter_index_initial_sync" # src/checkqueue.h: In CCheckQueue::Loop(): while (queue.empty()) { ... } "coinselector_tests/knapsack_solver_test" # coinselector_tests.cpp: if (equal_sets(setCoinsRet, setCoinsRet2)) "fs_tests/fsbridge_fstream" # deterministic test failure? - "miner_tests/CreateNewBlock_validity" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) + "miner_tests/CreateNewBlock_validity" # validation.cpp: if (signals.CallbacksPending() > 10) "scheduler_tests/manythreads" # scheduler.cpp: CScheduler::serviceQueue() "scheduler_tests/singlethreadedscheduler_ordered" # scheduler.cpp: CScheduler::serviceQueue() - "txvalidationcache_tests/checkinputs_test" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "txvalidationcache_tests/tx_mempool_block_doublespend" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "txindex_tests/txindex_initial_sync" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "txvalidation_tests/tx_mempool_reject_coinbase" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "validation_block_tests/processnewblock_signals_ordering" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/coin_mark_dirty_immature_credit" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/dummy_input_size_test" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/importmulti_rescan" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/importwallet_rescan" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/ListCoins" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/scan_for_wallet_transactions" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) - "wallet_tests/wallet_disableprivkeys" # validation.cpp: if (GetMainSignals().CallbacksPending() > 10) + "txvalidationcache_tests/checkinputs_test" # validation.cpp: if (signals.CallbacksPending() > 10) + "txvalidationcache_tests/tx_mempool_block_doublespend" # validation.cpp: if (signals.CallbacksPending() > 10) + "txindex_tests/txindex_initial_sync" # validation.cpp: if (signals.CallbacksPending() > 10) + "txvalidation_tests/tx_mempool_reject_coinbase" # validation.cpp: if (signals.CallbacksPending() > 10) + "validation_block_tests/processnewblock_signals_ordering" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/coin_mark_dirty_immature_credit" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/dummy_input_size_test" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/importmulti_rescan" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/importwallet_rescan" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/ListCoins" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/scan_for_wallet_transactions" # validation.cpp: if (signals.CallbacksPending() > 10) + "wallet_tests/wallet_disableprivkeys" # validation.cpp: if (signals.CallbacksPending() > 10) ) TEST_BITCOIN_BINARY="src/test/test_bitcoin" diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index c1a71ed7498..577f398c1ae 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -118,6 +118,7 @@ int main(int argc, char* argv[]) .chainparams = *chainparams, .datadir = abs_datadir, .notifications = *notifications, + .signals = &GetMainSignals(), }; const node::BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, diff --git a/src/init.cpp b/src/init.cpp index 988daefeec8..735177bfc00 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1449,6 +1449,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) .chainparams = chainparams, .datadir = args.GetDataDirNet(), .notifications = *node.notifications, + .signals = &GetMainSignals(), }; Assert(ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction @@ -1478,6 +1479,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CTxMemPool::Options mempool_opts{ .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, + .signals = &GetMainSignals(), }; auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; if (!result) { diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index 864aac336ed..766a812d91b 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -18,6 +18,7 @@ #include class CChainParams; +class CMainSignals; static constexpr bool DEFAULT_CHECKPOINTS_ENABLED{true}; static constexpr auto DEFAULT_MAX_TIP_AGE{24h}; @@ -44,6 +45,7 @@ struct ChainstateManagerOpts { DBOptions coins_db{}; CoinsViewOptions coins_view{}; Notifications& notifications; + CMainSignals* signals{nullptr}; //! Number of script check worker threads. Zero means no parallel verification. int worker_threads_num{0}; }; diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index 753aebd4553..1f69df95ff2 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -13,6 +13,8 @@ #include #include +class CMainSignals; + /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -maxmempool when blocksonly is set */ @@ -56,6 +58,8 @@ struct MemPoolOptions { bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF}; bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT}; MemPoolLimits limits{}; + + CMainSignals* signals{nullptr}; }; } // namespace kernel diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 9f587d7ec0b..b9c26592212 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -185,6 +185,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto .datadir = m_args.GetDataDirNet(), .check_block_index = true, .notifications = *m_node.notifications, + .signals = &GetMainSignals(), .worker_threads_num = 2, }; const BlockManager::Options blockman_opts{ diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 3b4161ddd3c..8c568c2eea7 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -13,6 +13,7 @@ #include #include #include +#include using node::NodeContext; @@ -22,6 +23,7 @@ CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node) // Default to always checking mempool regardless of // chainparams.DefaultConsistencyChecks for tests .check_ratio = 1, + .signals = &GetMainSignals(), }; const auto result{ApplyArgsManOptions(*node.args, ::Params(), mempool_opts)}; Assert(result); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index a33e71d50ee..4bc0de6d9bf 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -383,6 +383,7 @@ struct SnapshotTestSetup : TestChain100Setup { .chainparams = ::Params(), .datadir = chainman.m_options.datadir, .notifications = *m_node.notifications, + .signals = &GetMainSignals(), }; const BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, diff --git a/src/txmempool.cpp b/src/txmempool.cpp index de340f6b6d7..0bee27c2b21 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -406,7 +406,8 @@ CTxMemPool::CTxMemPool(const Options& opts) m_require_standard{opts.require_standard}, m_full_rbf{opts.full_rbf}, m_persist_v1_dat{opts.persist_v1_dat}, - m_limits{opts.limits} + m_limits{opts.limits}, + m_signals{opts.signals} { } @@ -487,12 +488,12 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); - if (reason != MemPoolRemovalReason::BLOCK) { + if (reason != MemPoolRemovalReason::BLOCK && m_signals) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. - GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); + m_signals->TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -643,7 +644,9 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } - GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); + if (m_signals) { + m_signals->MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); + } lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } diff --git a/src/txmempool.h b/src/txmempool.h index b98355c65f1..5e0f22229b1 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -40,6 +40,7 @@ #include class CChain; +class CMainSignals; /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */ static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF; @@ -447,6 +448,8 @@ public: const Limits m_limits; + CMainSignals* const m_signals; + /** Create a new CTxMemPool. * Sanity checks will be off by default for performance, because otherwise * accepting transactions becomes O(N^2) where N is the number of transactions diff --git a/src/validation.cpp b/src/validation.cpp index 0552bde6658..b3a53a537da 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1224,13 +1224,14 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); + if (!m_pool.m_signals) continue; const CTransaction& tx = *ws.m_ptx; const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, ws.m_vsize, ws.m_entry->GetHeight(), args.m_bypass_limits, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); - GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); + m_pool.m_signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } return all_submitted; } @@ -1238,7 +1239,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) { AssertLockHeld(cs_main); - LOCK(m_pool.cs); // mempool "read lock" (held through GetMainSignals().TransactionAddedToMempool()) + LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_signals->TransactionAddedToMempool()) Workspace ws(ptx); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; @@ -1273,13 +1274,15 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); } - const CTransaction& tx = *ws.m_ptx; - const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, - ws.m_vsize, ws.m_entry->GetHeight(), - args.m_bypass_limits, args.m_package_submission, - IsCurrentForFeeEstimation(m_active_chainstate), - m_pool.HasNoInputsOf(tx)); - GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); + if (m_pool.m_signals) { + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + m_pool.m_signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); + } return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); @@ -2691,9 +2694,9 @@ bool Chainstate::FlushStateToDisk( (bool)fFlushForPrune); } } - if (full_flush_completed) { + if (full_flush_completed && m_chainman.m_options.signals) { // Update best block in wallet (so we can detect restored wallets). - GetMainSignals().ChainStateFlushed(this->GetRole(), m_chain.GetLocator()); + m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), m_chain.GetLocator()); } } catch (const std::runtime_error& e) { return FatalError(m_chainman.GetNotifications(), state, std::string("System error while flushing: ") + e.what()); @@ -2860,7 +2863,9 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra UpdateTip(pindexDelete->pprev); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: - GetMainSignals().BlockDisconnected(pblock, pindexDelete); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->BlockDisconnected(pblock, pindexDelete); + } return true; } @@ -2945,7 +2950,9 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, { CCoinsViewCache view(&CoinsTip()); bool rv = ConnectBlock(blockConnecting, state, pindexNew, view); - GetMainSignals().BlockChecked(blockConnecting, state); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->BlockChecked(blockConnecting, state); + } if (!rv) { if (state.IsInvalid()) InvalidBlockFound(pindexNew, state); @@ -3206,10 +3213,10 @@ static bool NotifyHeaderTip(ChainstateManager& chainman) LOCKS_EXCLUDED(cs_main) return fNotify; } -static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) { +static void LimitValidationInterfaceQueue(CMainSignals& signals) LOCKS_EXCLUDED(cs_main) { AssertLockNotHeld(cs_main); - if (GetMainSignals().CallbacksPending() > 10) { + if (signals.CallbacksPending() > 10) { SyncWithValidationInterfaceQueue(); } } @@ -3248,7 +3255,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // Note that if a validationinterface callback ends up calling // ActivateBestChain this may lead to a deadlock! We should // probably have a DEBUG_LOCKORDER test for this in the future. - LimitValidationInterfaceQueue(); + if (m_chainman.m_options.signals) LimitValidationInterfaceQueue(*m_chainman.m_options.signals); { LOCK(cs_main); @@ -3287,7 +3294,9 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(this->GetRole(), trace.pblock, trace.pindex); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->BlockConnected(this->GetRole(), trace.pblock, trace.pindex); + } } // This will have been toggled in @@ -3313,7 +3322,9 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected if (this == &m_chainman.ActiveChainstate() && pindexFork != pindexNewTip) { // Notify ValidationInterface subscribers - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, still_in_ibd); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->UpdatedBlockTip(pindexNewTip, pindexFork, still_in_ibd); + } // Always notify the UI if a new block tip was connected if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip(GetSynchronizationState(still_in_ibd), *pindexNewTip))) { @@ -3447,7 +3458,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde if (m_chainman.m_interrupt) break; // Make sure the queue of validation callbacks doesn't grow unboundedly. - LimitValidationInterfaceQueue(); + if (m_chainman.m_options.signals) LimitValidationInterfaceQueue(*m_chainman.m_options.signals); LOCK(cs_main); // Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is @@ -4155,8 +4166,9 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, // Header is valid/has work, merkle tree and segwit merkle tree are good...RELAY NOW // (but if it does not build on our best tip, let the SendMessages loop relay it) - if (!IsInitialBlockDownload() && ActiveTip() == pindex->pprev) - GetMainSignals().NewPoWValidBlock(pindex, pblock); + if (!IsInitialBlockDownload() && ActiveTip() == pindex->pprev && m_options.signals) { + m_options.signals->NewPoWValidBlock(pindex, pblock); + } // Write block to history file if (fNewBlock) *fNewBlock = true; @@ -4209,7 +4221,9 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr& blo ret = AcceptBlock(block, state, &pindex, force_processing, nullptr, new_block, min_pow_checked); } if (!ret) { - GetMainSignals().BlockChecked(*block, state); + if (m_options.signals) { + m_options.signals->BlockChecked(*block, state); + } return error("%s: AcceptBlock FAILED (%s)", __func__, state.ToString()); } }