From eeee1104d78eb59a582ee1709ff4ac2c33ee1190 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 19 Jul 2020 09:07:54 +0200 Subject: [PATCH 1/3] Remove mempool global from init Can be reviewed with the git diff options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space --- src/init.cpp | 24 ++++++++++++------------ src/validation.cpp | 8 ++++++++ src/validation.h | 3 +++ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index bb93f5c7975..60042c682b0 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -187,7 +187,7 @@ void Shutdown(NodeContext& node) /// Be sure that anything that writes files or flushes caches only does this if the respective /// module was initialized. util::ThreadRename("shutoff"); - mempool.AddTransactionsUpdated(1); + if (node.mempool) node.mempool->AddTransactionsUpdated(1); StopHTTPRPC(); StopREST(); @@ -231,8 +231,8 @@ void Shutdown(NodeContext& node) node.connman.reset(); node.banman.reset(); - if (::mempool.IsLoaded() && node.args->GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { - DumpMempool(::mempool); + if (node.mempool && node.mempool->IsLoaded() && node.args->GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { + DumpMempool(*node.mempool); } if (fFeeEstimatesInitialized) @@ -738,10 +738,7 @@ static void ThreadImport(ChainstateManager& chainman, std::vector vImp return; } } // End scope of CImportingNow - if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { - LoadMempool(::mempool); - } - ::mempool.SetIsLoaded(!ShutdownRequested()); + chainman.ActiveChainstate().LoadMempool(args); } /** Sanity checks @@ -1054,11 +1051,6 @@ bool AppInitParameterInteraction(const ArgsManager& args) } } - // Checkmempool and checkblockindex default to true in regtest mode - int ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); - if (ratio != 0) { - mempool.setSanityCheck(1.0 / ratio); - } fCheckBlockIndex = args.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks()); fCheckpointsEnabled = args.GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED); @@ -1368,10 +1360,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA node.banman = MakeUnique(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); node.connman = MakeUnique(GetRand(std::numeric_limits::max()), GetRand(std::numeric_limits::max()), args.GetBoolArg("-networkactive", true)); + // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads, // which are all started after this, may use it from the node context. assert(!node.mempool); node.mempool = &::mempool; + if (node.mempool) { + int ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); + if (ratio != 0) { + node.mempool->setSanityCheck(1.0 / ratio); + } + } + assert(!node.chainman); node.chainman = &g_chainman; ChainstateManager& chainman = *Assert(node.chainman); diff --git a/src/validation.cpp b/src/validation.cpp index 58af8744d90..684cfc807ac 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4227,6 +4227,14 @@ bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& ch return true; } +void CChainState::LoadMempool(const ArgsManager& args) +{ + if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { + ::LoadMempool(m_mempool); + } + m_mempool.SetIsLoaded(!ShutdownRequested()); +} + bool CChainState::LoadChainTip(const CChainParams& chainparams) { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index cac9473c7a5..17a0adb0835 100644 --- a/src/validation.h +++ b/src/validation.h @@ -671,6 +671,9 @@ public: */ void CheckBlockIndex(const Consensus::Params& consensusParams); + /** Load the persisted mempool from disk */ + void LoadMempool(const ArgsManager& args); + /** Update the chain tip based on database information, i.e. CoinsTip()'s best block. */ bool LoadChainTip(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From fa0359c5b30730744aa8a7cd9ffab79ded91041f Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 19 Jul 2020 14:52:50 +0200 Subject: [PATCH 2/3] Remove mempool global from p2p --- src/net_processing.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8d6ecf2779b..15508683cea 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1710,7 +1710,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c } //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). -CTransactionRef static FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main) +static CTransactionRef FindTxForGetData(const CTxMemPool& mempool, const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main) { auto txinfo = mempool.info(gtxid); if (txinfo.tx) { @@ -1766,7 +1766,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm continue; } - CTransactionRef tx = FindTxForGetData(pfrom, ToGenTxid(inv), mempool_req, now); + CTransactionRef tx = FindTxForGetData(mempool, pfrom, ToGenTxid(inv), mempool_req, now); if (tx) { // WTX and WITNESS_TX imply we serialize with witness int nSendFlags = (inv.IsMsgTx() ? SERIALIZE_TRANSACTION_NO_WITNESS : 0); @@ -2732,7 +2732,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } } else if (inv.IsGenTxMsg()) { const GenTxid gtxid = ToGenTxid(inv); - const bool fAlreadyHave = AlreadyHaveTx(gtxid, mempool); + const bool fAlreadyHave = AlreadyHaveTx(gtxid, m_mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); pfrom.AddKnownTx(inv.hash); @@ -3003,7 +3003,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty std::list lRemovedTxn; - // We do the AlreadyHave() check using wtxid, rather than txid - in the + // We do the AlreadyHaveTx() check using wtxid, rather than txid - in the // absence of witness malleation, this is strictly better, because the // recent rejects filter may contain the wtxid but rarely contains // the txid of a segwit transaction that has been rejected. From fafb381af8279b2d2ca768df0bf68d7eb036a2f9 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 19 Jul 2020 20:30:46 +0200 Subject: [PATCH 3/3] Remove mempool global --- src/init.cpp | 6 +++--- src/node/context.cpp | 1 + src/node/context.h | 2 +- src/rest.cpp | 4 ++-- src/rpc/rawtransaction.cpp | 2 +- src/test/util/setup_common.cpp | 6 +++--- src/validation.cpp | 1 - src/validation.h | 1 - 8 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 60042c682b0..633dd8cefcb 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -302,7 +302,7 @@ void Shutdown(NodeContext& node) GetMainSignals().UnregisterBackgroundSignalScheduler(); globalVerifyHandle.reset(); ECC_Stop(); - node.mempool = nullptr; + node.mempool.reset(); node.chainman = nullptr; node.scheduler.reset(); @@ -1364,7 +1364,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads, // which are all started after this, may use it from the node context. assert(!node.mempool); - node.mempool = &::mempool; + node.mempool = MakeUnique(&::feeEstimator); if (node.mempool) { int ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); if (ratio != 0) { @@ -1559,7 +1559,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA chainman.m_total_coinstip_cache = nCoinCacheUsage; chainman.m_total_coinsdb_cache = nCoinDBCache; - UnloadBlockIndex(node.mempool); + UnloadBlockIndex(node.mempool.get()); // new CBlockTreeDB tries to delete the existing file, which // fails if it's still open from the previous loop. Close it first: diff --git a/src/node/context.cpp b/src/node/context.cpp index 0238aab0d98..49d0c372357 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -9,6 +9,7 @@ #include #include #include +#include NodeContext::NodeContext() {} NodeContext::~NodeContext() {} diff --git a/src/node/context.h b/src/node/context.h index 793c9dfc343..d9d0750951c 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -35,7 +35,7 @@ class WalletClient; //! be used without pulling in unwanted dependencies or functionality. struct NodeContext { std::unique_ptr connman; - CTxMemPool* mempool{nullptr}; // Currently a raw pointer because the memory is not managed by this struct + std::unique_ptr mempool; std::unique_ptr peer_logic; ChainstateManager* chainman{nullptr}; // Currently a raw pointer because the memory is not managed by this struct std::unique_ptr banman; diff --git a/src/rest.cpp b/src/rest.cpp index 7130625d5c8..f0bcbe55f92 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -102,7 +102,7 @@ static CTxMemPool* GetMemPool(const util::Ref& context, HTTPRequest* req) RESTERR(req, HTTP_NOT_FOUND, "Mempool disabled or instance not found"); return nullptr; } - return node->mempool; + return node->mempool.get(); } static RetFormat ParseDataFormat(std::string& param, const std::string& strReq) @@ -393,7 +393,7 @@ static bool rest_tx(const util::Ref& context, HTTPRequest* req, const std::strin const NodeContext* const node = GetNodeContext(context, req); if (!node) return false; uint256 hashBlock = uint256(); - const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, node->mempool, hash, Params().GetConsensus(), hashBlock); + const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, node->mempool.get(), hash, Params().GetConsensus(), hashBlock); if (!tx) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index f46dee82588..93e8357e868 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -191,7 +191,7 @@ static UniValue getrawtransaction(const JSONRPCRequest& request) } uint256 hash_block; - const CTransactionRef tx = GetTransaction(blockindex, node.mempool, hash, Params().GetConsensus(), hash_block); + const CTransactionRef tx = GetTransaction(blockindex, node.mempool.get(), hash, Params().GetConsensus(), hash_block); if (!tx) { std::string errmsg; if (blockindex) { diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 8a88e75892d..536a131313e 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -141,7 +141,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(&::feeEstimator); m_node.mempool->setSanityCheck(1.0); m_node.chainman = &::g_chainman; @@ -187,8 +187,8 @@ TestingSetup::~TestingSetup() m_node.connman.reset(); m_node.banman.reset(); m_node.args = nullptr; - UnloadBlockIndex(m_node.mempool); - m_node.mempool = nullptr; + UnloadBlockIndex(m_node.mempool.get()); + m_node.mempool.reset(); m_node.scheduler.reset(); m_node.chainman->Reset(); m_node.chainman = nullptr; diff --git a/src/validation.cpp b/src/validation.cpp index 684cfc807ac..bdbae66511a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -148,7 +148,6 @@ arith_uint256 nMinimumChainWork; CFeeRate minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); CBlockPolicyEstimator feeEstimator; -CTxMemPool mempool(&feeEstimator); // Internal stuff namespace { diff --git a/src/validation.h b/src/validation.h index 17a0adb0835..53c2dd65e5f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -113,7 +113,6 @@ enum class SynchronizationState { extern RecursiveMutex cs_main; extern CBlockPolicyEstimator feeEstimator; -extern CTxMemPool mempool; typedef std::unordered_map BlockMap; extern Mutex g_best_block_mutex; extern std::condition_variable g_best_block_cv;