From 1a8494d16c7b1c21dec384438c18ac08a469bb61 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 29 Nov 2025 14:02:11 -0500 Subject: [PATCH 1/2] Rework CTxMemPool::GetChildren() to not use epochs This is likely slightly slower, but this was the last place we were using epochs instead of sets to deduplicate, and this is only used by the RPC code and in tests, and should not be CPU-performance critical. Eliminating this allows us to save 8 bytes in CTxMemPoolEntry. Co-Authored-By: Pieter Wuille --- src/kernel/mempool_entry.h | 2 -- src/txmempool.cpp | 13 ++++++++----- src/txmempool.h | 23 +---------------------- 3 files changed, 9 insertions(+), 29 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 3053a77c3e..58824f7a80 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -12,7 +12,6 @@ #include #include #include -#include #include #include @@ -138,7 +137,6 @@ public: bool GetSpendsCoinbase() const { return spendsCoinbase; } mutable size_t idx_randomized; //!< Index in mempool's txns_randomized - mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms }; using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4dc692bfd8..2486270bc0 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -56,15 +56,18 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) std::vector CTxMemPool::GetChildren(const CTxMemPoolEntry& entry) const { - LOCK(cs); std::vector ret; - WITH_FRESH_EPOCH(m_epoch); - auto iter = mapNextTx.lower_bound(COutPoint(entry.GetTx().GetHash(), 0)); - for (; iter != mapNextTx.end() && iter->first->hash == entry.GetTx().GetHash(); ++iter) { - if (!visited(iter->second)) { + const auto& hash = entry.GetTx().GetHash(); + { + LOCK(cs); + auto iter = mapNextTx.lower_bound(COutPoint(hash, 0)); + for (; iter != mapNextTx.end() && iter->first->hash == hash; ++iter) { ret.emplace_back(*(iter->second)); } } + std::ranges::sort(ret, CompareIteratorByHash{}); + auto removed = std::ranges::unique(ret, [](auto& a, auto& b) noexcept { return &a.get() == &b.get(); }); + ret.erase(removed.begin(), removed.end()); return ret; } diff --git a/src/txmempool.h b/src/txmempool.h index 996a1fe081..9feeee3e31 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -20,7 +20,6 @@ #include #include #include -#include #include #include #include @@ -197,7 +196,6 @@ protected: mutable int64_t lastRollingFeeUpdate GUARDED_BY(cs){GetTime()}; mutable bool blockSinceLastRollingFeeBump GUARDED_BY(cs){false}; mutable double rollingMinimumFeeRate GUARDED_BY(cs){0}; //!< minimum fee to get into the pool, decreases exponentially - mutable Epoch m_epoch GUARDED_BY(cs){}; // In-memory counter for external mempool tracking purposes. // This number is incremented once every time a transaction @@ -394,7 +392,7 @@ public: * @param[in] vHashesToUpdate The set of txids from the * disconnected block that have been accepted back into the mempool. */ - void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); + void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); std::vector GetFeerateDiagram() const EXCLUSIVE_LOCKS_REQUIRED(cs); FeePerWeight GetMainChunkFeerate(const CTxMemPoolEntry& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs) { @@ -592,25 +590,6 @@ private: /* Removal from the mempool also triggers removal of the entry's Ref from txgraph. */ void removeUnchecked(txiter entry, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); public: - /** visited marks a CTxMemPoolEntry as having been traversed - * during the lifetime of the most recently created Epoch::Guard - * and returns false if we are the first visitor, true otherwise. - * - * An Epoch::Guard must be held when visited is called or an assert will be - * triggered. - * - */ - bool visited(const txiter it) const EXCLUSIVE_LOCKS_REQUIRED(cs, m_epoch) - { - return m_epoch.visited(it->m_epoch_marker); - } - - bool visited(std::optional it) const EXCLUSIVE_LOCKS_REQUIRED(cs, m_epoch) - { - assert(m_epoch.guarded()); // verify guard even when it==nullopt - return !it || visited(*it); - } - /* * CTxMemPool::ChangeSet: * From 40735450c00b10baa03e3a7f1e2bee439077e356 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 29 Nov 2025 14:07:23 -0500 Subject: [PATCH 2/2] Remove unused epochguard.h --- src/util/epochguard.h | 102 ------------------------------------------ 1 file changed, 102 deletions(-) delete mode 100644 src/util/epochguard.h diff --git a/src/util/epochguard.h b/src/util/epochguard.h deleted file mode 100644 index e908b2816a..0000000000 --- a/src/util/epochguard.h +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-present The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_UTIL_EPOCHGUARD_H -#define BITCOIN_UTIL_EPOCHGUARD_H - -#include -#include - -#include - -/** Epoch: 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 traversals 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 incompatible 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 LOCKABLE Epoch -{ -private: - uint64_t m_raw_epoch = 0; - bool m_guarded = false; - -public: - Epoch() = default; - Epoch(const Epoch&) = delete; - Epoch& operator=(const Epoch&) = delete; - Epoch(Epoch&&) = delete; - Epoch& operator=(Epoch&&) = delete; - ~Epoch() = default; - - bool guarded() const { return m_guarded; } - - class Marker - { - private: - uint64_t m_marker = 0; - - // only allow modification via Epoch member functions - friend class Epoch; - Marker& operator=(const Marker&) = delete; - - public: - Marker() = default; - Marker(const Marker&) = default; - Marker(Marker&&) = default; - Marker& operator=(Marker&&) = default; - ~Marker() = default; - }; - - class SCOPED_LOCKABLE Guard - { - private: - Epoch& m_epoch; - - public: - explicit Guard(Epoch& epoch) EXCLUSIVE_LOCK_FUNCTION(epoch) : m_epoch(epoch) - { - assert(!m_epoch.m_guarded); - ++m_epoch.m_raw_epoch; - m_epoch.m_guarded = true; - } - ~Guard() UNLOCK_FUNCTION() - { - assert(m_epoch.m_guarded); - ++m_epoch.m_raw_epoch; // ensure clear separation between epochs - m_epoch.m_guarded = false; - } - }; - - bool visited(Marker& marker) const EXCLUSIVE_LOCKS_REQUIRED(*this) - { - assert(m_guarded); - if (marker.m_marker < m_raw_epoch) { - // marker is from a previous epoch, so this is its first visit - marker.m_marker = m_raw_epoch; - return false; - } else { - return true; - } - } -}; - -#define WITH_FRESH_EPOCH(epoch) const Epoch::Guard UNIQUE_NAME(epoch_guard_)(epoch) - -#endif // BITCOIN_UTIL_EPOCHGUARD_H