Merge bitcoin/bitcoin#34384: Remove epoch logic from mempool

40735450c00b10baa03e3a7f1e2bee439077e356 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c7b1c21dec384438c18ac08a469bb61 Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)

Pull request description:

  Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.

  This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.

ACKs for top commit:
  ajtowns:
    ACK 40735450c00b10baa03e3a7f1e2bee439077e356
  instagibbs:
    ACK 40735450c00b10baa03e3a7f1e2bee439077e356
  l0rinc:
    code review ACK 40735450c00b10baa03e3a7f1e2bee439077e356

Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0

src/kernel/mempool_entry.h

@@ -12,7 +12,6 @@
 #include <policy/settings.h>
 #include <primitives/transaction.h>
 #include <txgraph.h>
-#include <util/epochguard.h>
 #include <util/overflow.h>
 
 #include <chrono>
@@ -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;

src/txmempool.cpp

@@ -56,15 +56,18 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
 
 std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> CTxMemPool::GetChildren(const CTxMemPoolEntry& entry) const
 {
-    LOCK(cs);
     std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> 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;
 }
 

src/txmempool.h

@@ -20,7 +20,6 @@
 #include <primitives/transaction_identifier.h>
 #include <sync.h>
 #include <txgraph.h>
-#include <util/epochguard.h>
 #include <util/feefrac.h>
 #include <util/hasher.h>
 #include <util/result.h>
@@ -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<Txid>& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch);
+    void UpdateTransactionsFromBlock(const std::vector<Txid>& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
 
     std::vector<FeePerWeight> 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<txiter> it) const EXCLUSIVE_LOCKS_REQUIRED(cs, m_epoch)
-    {
-        assert(m_epoch.guarded()); // verify guard even when it==nullopt
-        return !it || visited(*it);
-    }
-
     /*
      * CTxMemPool::ChangeSet:
      *

src/util/epochguard.h

@@ -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 <threadsafety.h>
-#include <util/macros.h>
-
-#include <cassert>
-
-/** 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