From f3e5ca703cc62bb0e1e4299dd67e7045e0e7aebd Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Tue, 14 Jan 2020 11:31:36 -0800 Subject: [PATCH] bitcoin#17925: Improve UpdateTransactionsFromBlock with Epochs --- src/txmempool.cpp | 48 ++++++++++++++++++++++++++++++------------- src/txmempool.h | 52 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 5decd7b2ad..4ff75e3b1d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -29,7 +29,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFe int64_t _nTime, unsigned int _entryHeight, bool _spendsCoinbase, unsigned int _sigOps, LockPoints lp): tx(_tx), nFee(_nFee), nTime(_nTime), entryHeight(_entryHeight), - spendsCoinbase(_spendsCoinbase), sigOpCount(_sigOps), lockPoints(lp) + spendsCoinbase(_spendsCoinbase), sigOpCount(_sigOps), lockPoints(lp), m_epoch(0) { nTxSize = ::GetSerializeSize(*_tx, SER_NETWORK, PROTOCOL_VERSION); nUsageSize = RecursiveDynamicUsage(tx); @@ -126,8 +126,6 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes // setMemPoolChildren will be updated, an assumption made in // UpdateForDescendants. for (const uint256 &hash : reverse_iterate(vHashesToUpdate)) { - // we cache the in-mempool children to avoid duplicate updates - setEntries setChildren; // calculate children from mapNextTx txiter it = mapTx.find(hash); if (it == mapTx.end()) { @@ -136,17 +134,21 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes auto iter = mapNextTx.lower_bound(COutPoint(hash, 0)); // First calculate the children, and update setMemPoolChildren to // include them, and update their setMemPoolParents to include this tx. - for (; iter != mapNextTx.end() && iter->first->hash == hash; ++iter) { - const uint256 &childHash = iter->second->GetHash(); - txiter childIter = mapTx.find(childHash); - assert(childIter != mapTx.end()); - // We can skip updating entries we've encountered before or that - // are in the block (which are already accounted for). - if (setChildren.insert(childIter).second && !setAlreadyIncluded.count(childHash)) { - UpdateChild(it, childIter, true); - UpdateParent(childIter, it, true); + // we cache the in-mempool children to avoid duplicate updates + { + const auto epoch = GetFreshEpoch(); + for (; iter != mapNextTx.end() && iter->first->hash == hash; ++iter) { + const uint256 &childHash = iter->second->GetHash(); + txiter childIter = mapTx.find(childHash); + assert(childIter != mapTx.end()); + // We can skip updating entries we've encountered before or that + // are in the block (which are already accounted for). + if (!visited(childIter) && !setAlreadyIncluded.count(childHash)) { + UpdateChild(it, childIter, true); + UpdateParent(childIter, it, true); + } } - } + } // release epoch guard for UpdateForDescendants UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); } } @@ -329,7 +331,7 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, } CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) : - nTransactionsUpdated(0), minerPolicyEstimator(estimator) + nTransactionsUpdated(0), minerPolicyEstimator(estimator), m_epoch(0), m_has_epoch_guard(false) { _clear(); //lock free clear @@ -1591,4 +1593,22 @@ void CTxMemPool::GetTransactionAncestry(const uint256& txid, size_t& ancestors, } } +CTxMemPool::EpochGuard CTxMemPool::GetFreshEpoch() const +{ + return EpochGuard(*this); +} +CTxMemPool::EpochGuard::EpochGuard(const CTxMemPool& in) : pool(in) +{ + assert(!pool.m_has_epoch_guard); + ++pool.m_epoch; + pool.m_has_epoch_guard = true; +} + +CTxMemPool::EpochGuard::~EpochGuard() +{ + // prevents stale results being used + ++pool.m_epoch; + pool.m_has_epoch_guard = false; +} + SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits::max())), k1(GetRand(std::numeric_limits::max())) {} diff --git a/src/txmempool.h b/src/txmempool.h index 44ab0b4b71..c6c0883ffc 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -136,6 +136,7 @@ public: // If this is a proTx, this will be the hash of the key for which this ProTx was valid mutable uint256 validForProTxKey; mutable bool isKeyChangeProTx{false}; + mutable uint64_t m_epoch; //!< epoch when last touched, useful for graph algorithms }; // Helpers for modifying CTxMemPool::mapTx, which is a boost multi_index. @@ -456,6 +457,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; void trackPackageRemoved(const CFeeRate& rate) EXCLUSIVE_LOCKS_REQUIRED(cs); @@ -731,6 +734,55 @@ private: * removal. */ void removeUnchecked(txiter entry, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN) EXCLUSIVE_LOCKS_REQUIRED(cs); +public: + /** EpochGuard: RAII-style guard for using epoch-based graph traversal algorithms. + * When walking ancestors or descendants, we generally want to avoid + * visiting the same transactions twice. Some traversal algorithms use + * std::set (or setEntries) to deduplicate the transaction we visit. + * However, use of std::set is algorithmically undesirable because it both + * adds an asymptotic factor of O(log n) to traverals cost and triggers O(n) + * more dynamic memory allocations. + * In many algorithms we can replace std::set with an internal mempool + * counter to track the time (or, "epoch") that we began a traversal, and + * check + update a per-transaction epoch for each transaction we look at to + * determine if that transaction has not yet been visited during the current + * traversal's epoch. + * Algorithms using std::set can be replaced on a one by one basis. + * Both techniques are not fundamentally incomaptible across the codebase. + * Generally speaking, however, the remaining use of std::set for mempool + * traversal should be viewed as a TODO for replacement with an epoch based + * traversal, rather than a preference for std::set over epochs in that + * algorithm. + */ + class EpochGuard { + const CTxMemPool& pool; + public: + EpochGuard(const CTxMemPool& in); + ~EpochGuard(); + }; + // N.B. GetFreshEpoch modifies mutable state via the EpochGuard construction + // (and later destruction) + EpochGuard GetFreshEpoch() const EXCLUSIVE_LOCKS_REQUIRED(cs); + + /** visited marks a CTxMemPoolEntry as having been traversed + * during the lifetime of the most recently created EpochGuard + * and returns false if we are the first visitor, true otherwise. + * + * An EpochGuard must be held when visited is called or an assert will be + * triggered. + * + */ + bool visited(txiter it) const EXCLUSIVE_LOCKS_REQUIRED(cs) { + assert(m_has_epoch_guard); + bool ret = it->m_epoch >= m_epoch; + it->m_epoch = std::max(it->m_epoch, m_epoch); + return ret; + } + + bool visited(boost::optional it) const EXCLUSIVE_LOCKS_REQUIRED(cs) { + assert(m_has_epoch_guard); + return !it || visited(*it); + } }; /**