[refactor] txmempool: split epoch logic into class

Summary: This is a backport of [[bitcoin/bitcoin#18017 | core#18017]]

Test Plan:
With clang and  debug:
`ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11308

src/txmempool.cpp

@@ -32,7 +32,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef &_tx, const Amount _nFee,
     : tx(_tx), nFee(_nFee), nTxSize(tx->GetTotalSize()),
       nUsageSize(RecursiveDynamicUsage(tx)), nTime(_nTime),
       entryHeight(_entryHeight), spendsCoinbase(_spendsCoinbase),
-      sigOpCount(_sigOpsCount), lockPoints(lp), m_epoch(0) {
+      sigOpCount(_sigOpsCount), lockPoints(lp) {
     nCountWithDescendants = 1;
     nSizeWithDescendants = GetTxSize();
     nSigOpCountWithDescendants = sigOpCount;
@@ -164,7 +164,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(
         // include them, and update their CTxMemPoolEntry::m_parents to include
         // this tx. we cache the in-mempool children to avoid duplicate updates
         {
-            const auto epoch = GetFreshEpoch();
+            WITH_FRESH_EPOCH(m_epoch);
             for (; iter != mapNextTx.end() && iter->first->GetTxId() == txid;
                  ++iter) {
                 const TxId &childTxId = iter->second->GetId();
@@ -1457,19 +1457,3 @@ void DisconnectedBlockTransactions::updateMempoolForReorg(
                    std::chrono::hours{
                        gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
 }
-
-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;
-}

src/txmempool.h

@@ -13,6 +13,7 @@
 #include <primitives/transaction.h>
 #include <salteduint256hasher.h>
 #include <sync.h>
+#include <util/epochguard.h>
 
 #include <boost/multi_index/hashed_index.hpp>
 #include <boost/multi_index/ordered_index.hpp>
@@ -187,7 +188,7 @@ class CTxMemPoolEntry {
     //! Index in mempool's vTxHashes
     mutable size_t vTxHashesIdx;
     //! epoch when last touched, useful for graph algorithms
-    mutable uint64_t m_epoch;
+    mutable Epoch::Marker m_epoch_marker;
 };
 
 // Helpers for modifying CTxMemPool::mapTx, which is a boost multi_index.
@@ -510,8 +511,7 @@ class CTxMemPool {
     mutable bool blockSinceLastRollingFeeBump GUARDED_BY(cs);
     //! minimum fee to get into the pool, decreases exponentially
     mutable double rollingMinimumFeeRate GUARDED_BY(cs);
-    mutable uint64_t m_epoch{0};
-    mutable bool m_has_epoch_guard{false};
+    mutable Epoch m_epoch GUARDED_BY(cs);
 
     // In-memory counter for external mempool tracking purposes.
     // This number is incremented once every time a transaction
@@ -708,7 +708,7 @@ class CTxMemPool {
      * disconnected block that have been accepted back into the mempool.
      */
     void UpdateTransactionsFromBlock(const std::vector<TxId> &txidsToUpdate)
-        EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+        EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch);
 
     /**
      * Try to calculate all in-mempool ancestors of entry.
@@ -886,55 +886,23 @@ class CTxMemPool {
         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 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 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
+     * during the lifetime of the most recently created Epoch::Guard
      * 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
+     * An Epoch::Guard 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(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) {
-        assert(m_has_epoch_guard);
+    bool visited(std::optional<txiter> it) const
+        EXCLUSIVE_LOCKS_REQUIRED(cs, m_epoch) {
+        // verify guard even when it==nullopt
+        assert(m_epoch.guarded());
         return !it || visited(*it);
     }
 };

src/util/epochguard.h

@@ -0,0 +1,89 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2020 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 <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;
+
+    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;
+    };
+
+    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);
+            // ensure clear separation between epochs
+            ++m_epoch.m_raw_epoch;
+            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 PASTE2(epoch_guard_, __COUNTER__)(epoch)
+
+#endif // BITCOIN_UTIL_EPOCHGUARD_H