From 9d4b4b2c2c49774523de740d6492ee5b1ee15e74 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 20 Oct 2020 20:09:55 +0200 Subject: [PATCH 1/3] refactor: Avoid double to int cast for nCheckFrequency Use a ratio instead of a frequency that requires a double to int cast for determining how often a mempool sanity check should run. --- src/init.cpp | 6 ++---- src/txmempool.cpp | 10 ++++------ src/txmempool.h | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1387d6b982..51a4943478 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1394,10 +1394,8 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA assert(!node.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) { - node.mempool->setSanityCheck(1.0 / ratio); - } + int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); + node.mempool->setSanityCheck(check_ratio); } assert(!node.chainman); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 0c2b731967..78af3bf833 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -339,7 +339,7 @@ CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) // Sanity checks off by default for performance, because otherwise // accepting transactions becomes O(N^2) where N is the number // of transactions in the pool - nCheckFrequency = 0; + m_check_ratio = 0; } bool CTxMemPool::isSpent(const COutPoint& outpoint) const @@ -523,7 +523,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem if (it2 != mapTx.end()) continue; const Coin &coin = pcoins->AccessCoin(txin.prevout); - if (nCheckFrequency != 0) assert(!coin.IsSpent()); + if (m_check_ratio != 0) assert(!coin.IsSpent()); if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { txToRemove.insert(it); break; @@ -620,11 +620,9 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m void CTxMemPool::check(const CCoinsViewCache *pcoins) const { LOCK(cs); - if (nCheckFrequency == 0) - return; + if (m_check_ratio == 0) return; - if (GetRand(std::numeric_limits::max()) >= nCheckFrequency) - return; + if (GetRand(m_check_ratio) >= 1) return; LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size()); diff --git a/src/txmempool.h b/src/txmempool.h index f513f14af6..38abc65c5d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -488,7 +488,7 @@ public: class CTxMemPool { private: - uint32_t nCheckFrequency GUARDED_BY(cs); //!< Value n means that n times in 2^32 we check. + uint32_t m_check_ratio GUARDED_BY(cs); //!< Value n means that 1 times in n we check. std::atomic nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation CBlockPolicyEstimator* minerPolicyEstimator; @@ -611,7 +611,7 @@ public: * check does nothing. */ void check(const CCoinsViewCache *pcoins) const; - void setSanityCheck(double dFrequency = 1.0) { LOCK(cs); nCheckFrequency = static_cast(dFrequency * 4294967295.0); } + void setSanityCheck(int check_ratio = 0) { LOCK(cs); m_check_ratio = check_ratio; } // addUnchecked must updated state for all ancestors of a given transaction, // to track size/count of descendant transactions. First version of From e3310692d0e9720e960b9785274ce1f0b58b4cd7 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 20 Oct 2020 20:42:47 +0200 Subject: [PATCH 2/3] refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument Since m_check_ratio is only set once and since the CTxMemPool object is no longer a global variable, m_check_ratio can be passed into the constructor of CTxMemPool. Since it is only read from after initialization, m_check_ratio can also be made a const and hence no longer needs to be guarded by the cs mutex. --- src/init.cpp | 9 ++------- src/test/util/setup_common.cpp | 3 +-- src/txmempool.cpp | 11 +++-------- src/txmempool.h | 11 ++++++++--- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 51a4943478..1ba34945c2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1389,14 +1389,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA 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 = MakeUnique(&::feeEstimator); - if (node.mempool) { - int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); - node.mempool->setSanityCheck(check_ratio); - } + int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); + node.mempool = MakeUnique(&::feeEstimator, check_ratio); assert(!node.chainman); node.chainman = &g_chainman; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2d3137e1e2..adf5970206 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -141,8 +141,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(&::feeEstimator); - m_node.mempool->setSanityCheck(1.0); + m_node.mempool = MakeUnique(&::feeEstimator, 1); m_node.chainman = &::g_chainman; m_node.chainman->InitializeChainstate(*m_node.mempool); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 78af3bf833..ca7021443e 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -331,15 +331,10 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, assert(int(nSigOpCostWithAncestors) >= 0); } -CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) - : nTransactionsUpdated(0), minerPolicyEstimator(estimator), m_epoch(0), m_has_epoch_guard(false) +CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio) + : m_check_ratio(check_ratio), nTransactionsUpdated(0), minerPolicyEstimator(estimator), m_epoch(0), m_has_epoch_guard(false) { _clear(); //lock free clear - - // Sanity checks off by default for performance, because otherwise - // accepting transactions becomes O(N^2) where N is the number - // of transactions in the pool - m_check_ratio = 0; } bool CTxMemPool::isSpent(const COutPoint& outpoint) const @@ -619,11 +614,11 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m void CTxMemPool::check(const CCoinsViewCache *pcoins) const { - LOCK(cs); if (m_check_ratio == 0) return; if (GetRand(m_check_ratio) >= 1) return; + LOCK(cs); LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size()); uint64_t checkTotal = 0; diff --git a/src/txmempool.h b/src/txmempool.h index 38abc65c5d..da071576ac 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -488,7 +488,7 @@ public: class CTxMemPool { private: - uint32_t m_check_ratio GUARDED_BY(cs); //!< Value n means that 1 times in n we check. + const int m_check_ratio; //!< Value n means that 1 times in n we check. std::atomic nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation CBlockPolicyEstimator* minerPolicyEstimator; @@ -601,8 +601,14 @@ public: std::map mapDeltas; /** 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 + * in the pool. + * + * @param[in] estimator is used to estimate appropriate transaction fees. + * @param[in] check_ratio is the ratio used to determine how often sanity checks will run. */ - explicit CTxMemPool(CBlockPolicyEstimator* estimator = nullptr); + explicit CTxMemPool(CBlockPolicyEstimator* estimator = nullptr, int check_ratio = 0); /** * If sanity-checking is turned on, check makes sure the pool is @@ -611,7 +617,6 @@ public: * check does nothing. */ void check(const CCoinsViewCache *pcoins) const; - void setSanityCheck(int check_ratio = 0) { LOCK(cs); m_check_ratio = check_ratio; } // addUnchecked must updated state for all ancestors of a given transaction, // to track size/count of descendant transactions. First version of From f15e780b9e57554c723bc02aa41150ecf3e3a8c9 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 20 Oct 2020 21:25:07 +0200 Subject: [PATCH 3/3] refactor: Clean up CTxMemPool initializer list Shorten the CTxMemPool initializer list using default initialization for members that dont depend on the constuctor parameters. --- src/txmempool.cpp | 2 +- src/txmempool.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ca7021443e..d18182c07d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -332,7 +332,7 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, } CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio) - : m_check_ratio(check_ratio), nTransactionsUpdated(0), minerPolicyEstimator(estimator), m_epoch(0), m_has_epoch_guard(false) + : m_check_ratio(check_ratio), minerPolicyEstimator(estimator) { _clear(); //lock free clear } diff --git a/src/txmempool.h b/src/txmempool.h index da071576ac..78ad62aae6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -489,7 +489,7 @@ class CTxMemPool { private: const int m_check_ratio; //!< Value n means that 1 times in n we check. - std::atomic nTransactionsUpdated; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation + std::atomic nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation CBlockPolicyEstimator* minerPolicyEstimator; uint64_t totalTxSize; //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141. @@ -498,8 +498,8 @@ private: mutable int64_t lastRollingFeeUpdate; mutable bool blockSinceLastRollingFeeBump; mutable double rollingMinimumFeeRate; //!< minimum fee to get into the pool, decreases exponentially - mutable uint64_t m_epoch; - mutable bool m_has_epoch_guard; + mutable uint64_t m_epoch{0}; + mutable bool m_has_epoch_guard{false}; // In-memory counter for external mempool tracking purposes. // This number is incremented once every time a transaction