Merge bitcoin/bitcoin#17786: refactor: Nuke policy/fees->mempool circular dependencies

glozow · glozow · commit d0b1f613c2a2 · 2022-11-18T17:04:49.000-08:00
c8dc0e3 refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov) 75bbe59 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov) Pull request description: This PR: - gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency - is an alternative to #13949, which nukes only one circular dependency ACKs for top commit: ryanofsky: Code review ACK c8dc0e3. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review theStack: Code-review ACK c8dc0e3 glozow: utACK c8dc0e3, agree these changes are an improvement. Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -264,6 +264,7 @@ BITCOIN_CORE_H = \
   torcontrol.h \
   txdb.h \
   txmempool.h \
+  txmempool_entry.h \
   txorphanage.h \
   txrequest.h \
   undo.h \
diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp
@@ -6,6 +6,7 @@
 #include <policy/policy.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 
 
 static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp
@@ -6,6 +6,7 @@
 #include <policy/policy.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <validation.h>
 
 #include <vector>
diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp
@@ -7,6 +7,7 @@
 #include <rpc/mempool.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 
 #include <univalue.h>
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -34,6 +34,7 @@
 #include <timedata.h>
 #include <tinyformat.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <txorphanage.h>
 #include <txrequest.h>
 #include <util/check.h> // For NDEBUG compile time check
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -39,6 +39,7 @@
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <uint256.h>
 #include <univalue.h>
 #include <util/check.h>
diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
@@ -16,7 +16,7 @@
 #include <streams.h>
 #include <sync.h>
 #include <tinyformat.h>
-#include <txmempool.h>
+#include <txmempool_entry.h>
 #include <uint256.h>
 #include <util/serfloat.h>
 #include <util/system.h>
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
@@ -10,6 +10,7 @@
 #include <sync.h>
 #include <tinyformat.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <uint256.h>
 #include <util/moneystr.h>
 #include <util/rbf.h>
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
@@ -18,6 +18,7 @@
 #include <rpc/server_util.h>
 #include <rpc/util.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <univalue.h>
 #include <util/moneystr.h>
 #include <util/time.h>
diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp
@@ -11,6 +11,7 @@
 #include <test/fuzz/util/mempool.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 
 #include <cstdint>
 #include <optional>
diff --git a/src/test/fuzz/util/mempool.cpp b/src/test/fuzz/util/mempool.cpp
@@ -7,7 +7,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/util.h>
 #include <test/fuzz/util/mempool.h>
-#include <txmempool.h>
+#include <txmempool_entry.h>
 
 #include <limits>
 
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -41,6 +41,7 @@
 #include <timedata.h>
 #include <txdb.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/thread.h>
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
@@ -41,42 +41,6 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
     return true;
 }
 
-CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
-                                 int64_t time, unsigned int entry_height,
-                                 bool spends_coinbase, int64_t sigops_cost, LockPoints lp)
-    : tx{tx},
-      nFee{fee},
-      nTxWeight(GetTransactionWeight(*tx)),
-      nUsageSize{RecursiveDynamicUsage(tx)},
-      nTime{time},
-      entryHeight{entry_height},
-      spendsCoinbase{spends_coinbase},
-      sigOpCost{sigops_cost},
-      m_modified_fee{nFee},
-      lockPoints{lp},
-      nSizeWithDescendants{GetTxSize()},
-      nModFeesWithDescendants{nFee},
-      nSizeWithAncestors{GetTxSize()},
-      nModFeesWithAncestors{nFee},
-      nSigOpCostWithAncestors{sigOpCost} {}
-
-void CTxMemPoolEntry::UpdateModifiedFee(CAmount fee_diff)
-{
-    nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, fee_diff);
-    nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, fee_diff);
-    m_modified_fee = SaturatingAdd(m_modified_fee, fee_diff);
-}
-
-void CTxMemPoolEntry::UpdateLockPoints(const LockPoints& lp)
-{
-    lockPoints = lp;
-}
-
-size_t CTxMemPoolEntry::GetTxSize() const
-{
-    return GetVirtualTransactionSize(nTxWeight, sigOpCost, ::nBytesPerSigOp);
-}
-
 void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants,
                                       const std::set<uint256>& setExclude, std::set<uint256>& descendants_to_remove)
 {
diff --git a/src/txmempool.h b/src/txmempool.h
@@ -25,6 +25,7 @@
 #include <primitives/transaction.h>
 #include <random.h>
 #include <sync.h>
+#include <txmempool_entry.h>
 #include <util/epochguard.h>
 #include <util/hasher.h>
 
@@ -41,132 +42,11 @@ extern RecursiveMutex cs_main;
 /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
 static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF;
 
-struct LockPoints {
-    // Will be set to the blockchain height and median time past
-    // values that would be necessary to satisfy all relative locktime
-    // constraints (BIP68) of this tx given our view of block chain history
-    int height{0};
-    int64_t time{0};
-    // As long as the current chain descends from the highest height block
-    // containing one of the inputs used in the calculation, then the cached
-    // values are still valid even after a reorg.
-    CBlockIndex* maxInputBlock{nullptr};
-};
-
 /**
  * Test whether the LockPoints height and time are still valid on the current chain
  */
 bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-struct CompareIteratorByHash {
-    // SFINAE for T where T is either a pointer type (e.g., a txiter) or a reference_wrapper<T>
-    // (e.g. a wrapped CTxMemPoolEntry&)
-    template <typename T>
-    bool operator()(const std::reference_wrapper<T>& a, const std::reference_wrapper<T>& b) const
-    {
-        return a.get().GetTx().GetHash() < b.get().GetTx().GetHash();
-    }
-    template <typename T>
-    bool operator()(const T& a, const T& b) const
-    {
-        return a->GetTx().GetHash() < b->GetTx().GetHash();
-    }
-};
-
-/** \class CTxMemPoolEntry
- *
- * CTxMemPoolEntry stores data about the corresponding transaction, as well
- * as data about all in-mempool transactions that depend on the transaction
- * ("descendant" transactions).
- *
- * When a new entry is added to the mempool, we update the descendant state
- * (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for
- * all ancestors of the newly added transaction.
- *
- */
-
-class CTxMemPoolEntry
-{
-public:
-    typedef std::reference_wrapper<const CTxMemPoolEntry> CTxMemPoolEntryRef;
-    // two aliases, should the types ever diverge
-    typedef std::set<CTxMemPoolEntryRef, CompareIteratorByHash> Parents;
-    typedef std::set<CTxMemPoolEntryRef, CompareIteratorByHash> Children;
-
-private:
-    const CTransactionRef tx;
-    mutable Parents m_parents;
-    mutable Children m_children;
-    const CAmount nFee;             //!< Cached to avoid expensive parent-transaction lookups
-    const size_t nTxWeight;         //!< ... and avoid recomputing tx weight (also used for GetTxSize())
-    const size_t nUsageSize;        //!< ... and total memory usage
-    const int64_t nTime;            //!< Local time when entering the mempool
-    const unsigned int entryHeight; //!< Chain height when entering the mempool
-    const bool spendsCoinbase;      //!< keep track of transactions that spend a coinbase
-    const int64_t sigOpCost;        //!< Total sigop cost
-    CAmount m_modified_fee;         //!< Used for determining the priority of the transaction for mining in a block
-    LockPoints lockPoints;     //!< Track the height and time at which tx was final
-
-    // Information about descendants of this transaction that are in the
-    // mempool; if we remove this transaction we must remove all of these
-    // descendants as well.
-    uint64_t nCountWithDescendants{1}; //!< number of descendant transactions
-    uint64_t nSizeWithDescendants;   //!< ... and size
-    CAmount nModFeesWithDescendants; //!< ... and total fees (all including us)
-
-    // Analogous statistics for ancestor transactions
-    uint64_t nCountWithAncestors{1};
-    uint64_t nSizeWithAncestors;
-    CAmount nModFeesWithAncestors;
-    int64_t nSigOpCostWithAncestors;
-
-public:
-    CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
-                    int64_t time, unsigned int entry_height,
-                    bool spends_coinbase,
-                    int64_t sigops_cost, LockPoints lp);
-
-    const CTransaction& GetTx() const { return *this->tx; }
-    CTransactionRef GetSharedTx() const { return this->tx; }
-    const CAmount& GetFee() const { return nFee; }
-    size_t GetTxSize() const;
-    size_t GetTxWeight() const { return nTxWeight; }
-    std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
-    unsigned int GetHeight() const { return entryHeight; }
-    int64_t GetSigOpCost() const { return sigOpCost; }
-    CAmount GetModifiedFee() const { return m_modified_fee; }
-    size_t DynamicMemoryUsage() const { return nUsageSize; }
-    const LockPoints& GetLockPoints() const { return lockPoints; }
-
-    // Adjusts the descendant state.
-    void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount);
-    // Adjusts the ancestor state
-    void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
-    // Updates the modified fees with descendants/ancestors.
-    void UpdateModifiedFee(CAmount fee_diff);
-    // Update the LockPoints after a reorg
-    void UpdateLockPoints(const LockPoints& lp);
-
-    uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
-    uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
-    CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; }
-
-    bool GetSpendsCoinbase() const { return spendsCoinbase; }
-
-    uint64_t GetCountWithAncestors() const { return nCountWithAncestors; }
-    uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
-    CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
-    int64_t GetSigOpCostWithAncestors() const { return nSigOpCostWithAncestors; }
-
-    const Parents& GetMemPoolParentsConst() const { return m_parents; }
-    const Children& GetMemPoolChildrenConst() const { return m_children; }
-    Parents& GetMemPoolParents() const { return m_parents; }
-    Children& GetMemPoolChildren() const { return m_children; }
-
-    mutable size_t vTxHashesIdx; //!< Index in mempool's vTxHashes
-    mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms
-};
-
 // extracts a transaction hash from CTxMemPoolEntry or CTransactionRef
 struct mempoolentry_txid
 {
diff --git a/src/txmempool_entry.h b/src/txmempool_entry.h
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py
diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan
