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

src/Makefile.am

@@ -372,6 +372,7 @@ BITCOIN_CORE_H = \
   torcontrol.h \
   txdb.h \
   txmempool.h \
+  txmempool_entry.h \
   txorphanage.h \
   undo.h \
   unordered_lru_cache.h \

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)

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>

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>
 

src/net_processing.cpp

@@ -32,6 +32,7 @@
 #include <timedata.h>
 #include <tinyformat.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <txorphanage.h>
 #include <util/check.h>
 #include <util/strencodings.h>

src/node/interfaces.cpp

@@ -49,6 +49,7 @@
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 #include <uint256.h>
 #include <util/check.h>
 #include <util/system.h>

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>

src/policy/rbf.cpp

@@ -0,0 +1,184 @@
+// Copyright (c) 2016-2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <policy/rbf.h>
+
+#include <consensus/amount.h>
+#include <policy/feerate.h>
+#include <primitives/transaction.h>
+#include <sync.h>
+#include <tinyformat.h>
+#include <txmempool.h>
+#include <txmempool_entry.h>
+#include <uint256.h>
+#include <util/moneystr.h>
+#include <util/rbf.h>
+
+#include <limits>
+#include <vector>
+
+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
+{
+    AssertLockHeld(pool.cs);
+
+    CTxMemPool::setEntries ancestors;
+
+    // First check the transaction itself.
+    if (SignalsOptInRBF(tx)) {
+        return RBFTransactionState::REPLACEABLE_BIP125;
+    }
+
+    // If this transaction is not in our mempool, then we can't be sure
+    // we will know about all its inputs.
+    if (!pool.exists(GenTxid::Txid(tx.GetHash()))) {
+        return RBFTransactionState::UNKNOWN;
+    }
+
+    // If all the inputs have nSequence >= maxint-1, it still might be
+    // signaled for RBF if any unconfirmed parents have signaled.
+    std::string dummy;
+    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
+    pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
+
+    for (CTxMemPool::txiter it : ancestors) {
+        if (SignalsOptInRBF(it->GetTx())) {
+            return RBFTransactionState::REPLACEABLE_BIP125;
+        }
+    }
+    return RBFTransactionState::FINAL;
+}
+
+RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
+{
+    // If we don't have a local mempool we can only check the transaction itself.
+    return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
+}
+
+std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
+                                                  CTxMemPool& pool,
+                                                  const CTxMemPool::setEntries& iters_conflicting,
+                                                  CTxMemPool::setEntries& all_conflicts)
+{
+    AssertLockHeld(pool.cs);
+    const uint256 txid = tx.GetHash();
+    uint64_t nConflictingCount = 0;
+    for (const auto& mi : iters_conflicting) {
+        nConflictingCount += mi->GetCountWithDescendants();
+        // Rule #5: don't consider replacing more than MAX_REPLACEMENT_CANDIDATES
+        // entries from the mempool. This potentially overestimates the number of actual
+        // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple
+        // times), but we just want to be conservative to avoid doing too much work.
+        if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) {
+            return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
+                             txid.ToString(),
+                             nConflictingCount,
+                             MAX_REPLACEMENT_CANDIDATES);
+        }
+    }
+    // Calculate the set of all transactions that would have to be evicted.
+    for (CTxMemPool::txiter it : iters_conflicting) {
+        pool.CalculateDescendants(it, all_conflicts);
+    }
+    return std::nullopt;
+}
+
+std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
+                                               const CTxMemPool& pool,
+                                               const CTxMemPool::setEntries& iters_conflicting)
+{
+    AssertLockHeld(pool.cs);
+    std::set<uint256> parents_of_conflicts;
+    for (const auto& mi : iters_conflicting) {
+        for (const CTxIn& txin : mi->GetTx().vin) {
+            parents_of_conflicts.insert(txin.prevout.hash);
+        }
+    }
+
+    for (unsigned int j = 0; j < tx.vin.size(); j++) {
+        // Rule #2: We don't want to accept replacements that require low feerate junk to be
+        // mined first.  Ideally we'd keep track of the ancestor feerates and make the decision
+        // based on that, but for now requiring all new inputs to be confirmed works.
+        //
+        // Note that if you relax this to make RBF a little more useful, this may break the
+        // CalculateMempoolAncestors RBF relaxation which subtracts the conflict count/size from the
+        // descendant limit.
+        if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
+            // Rather than check the UTXO set - potentially expensive - it's cheaper to just check
+            // if the new input refers to a tx that's in the mempool.
+            if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
+                return strprintf("replacement %s adds unconfirmed input, idx %d",
+                                 tx.GetHash().ToString(), j);
+            }
+        }
+    }
+    return std::nullopt;
+}
+
+std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
+                                                   const std::set<uint256>& direct_conflicts,
+                                                   const uint256& txid)
+{
+    for (CTxMemPool::txiter ancestorIt : ancestors) {
+        const uint256& hashAncestor = ancestorIt->GetTx().GetHash();
+        if (direct_conflicts.count(hashAncestor)) {
+            return strprintf("%s spends conflicting transaction %s",
+                             txid.ToString(),
+                             hashAncestor.ToString());
+        }
+    }
+    return std::nullopt;
+}
+
+std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting,
+                                                 CFeeRate replacement_feerate,
+                                                 const uint256& txid)
+{
+    for (const auto& mi : iters_conflicting) {
+        // Don't allow the replacement to reduce the feerate of the mempool.
+        //
+        // We usually don't want to accept replacements with lower feerates than what they replaced
+        // as that would lower the feerate of the next block. Requiring that the feerate always be
+        // increased is also an easy-to-reason about way to prevent DoS attacks via replacements.
+        //
+        // We only consider the feerates of transactions being directly replaced, not their indirect
+        // descendants. While that does mean high feerate children are ignored when deciding whether
+        // or not to replace, we do require the replacement to pay more overall fees too, mitigating
+        // most cases.
+        CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize());
+        if (replacement_feerate <= original_feerate) {
+            return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
+                             txid.ToString(),
+                             replacement_feerate.ToString(),
+                             original_feerate.ToString());
+        }
+    }
+    return std::nullopt;
+}
+
+std::optional<std::string> PaysForRBF(CAmount original_fees,
+                                      CAmount replacement_fees,
+                                      size_t replacement_vsize,
+                                      CFeeRate relay_fee,
+                                      const uint256& txid)
+{
+    // Rule #3: The replacement fees must be greater than or equal to fees of the
+    // transactions it replaces, otherwise the bandwidth used by those conflicting transactions
+    // would not be paid for.
+    if (replacement_fees < original_fees) {
+        return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
+                         txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
+    }
+
+    // Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS
+    // vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by
+    // increasing the fee by tiny amounts.
+    CAmount additional_fees = replacement_fees - original_fees;
+    if (additional_fees < relay_fee.GetFee(replacement_vsize)) {
+        return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
+                         txid.ToString(),
+                         FormatMoney(additional_fees),
+                         FormatMoney(relay_fee.GetFee(replacement_vsize)));
+    }
+    return std::nullopt;
+}

src/rpc/mempool.cpp

@@ -13,6 +13,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 <validation.h>

src/test/fuzz/policy_estimator.cpp

@@ -9,6 +9,7 @@
 #include <test/fuzz/util.h>
 #include <test/util/setup_common.h>
 #include <txmempool.h>
+#include <txmempool_entry.h>
 
 #include <cstdint>
 #include <optional>

src/test/fuzz/util/mempool.cpp

@@ -0,0 +1,27 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <consensus/amount.h>
+#include <primitives/transaction.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/util.h>
+#include <test/fuzz/util/mempool.h>
+#include <txmempool_entry.h>
+
+#include <limits>
+
+CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept
+{
+    // Avoid:
+    // policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long'
+    //
+    // Reproduce using CFeeRate(348732081484775, 10).GetFeePerK()
+    const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/std::numeric_limits<CAmount>::max() / CAmount{100'000})};
+    assert(MoneyRange(fee));
+    const int64_t time = fuzzed_data_provider.ConsumeIntegral<int64_t>();
+    const unsigned int entry_height = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
+    const bool spends_coinbase = fuzzed_data_provider.ConsumeBool();
+    const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, MAX_BLOCK_SIGOPS_COST);
+    return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, spends_coinbase, sig_op_cost, {}};
+}

src/test/util/setup_common.cpp

@@ -36,6 +36,7 @@
 #include <test/util/net.h>
 #include <test/util/txmempool.h>
 #include <txdb.h>
+#include <txmempool_entry.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/thread.h>

src/txmempool.cpp

@@ -48,42 +48,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_count, LockPoints lp)
-    : tx{tx},
-      nFee{fee},
-      nTxSize(tx->GetTotalSize()),
-      nUsageSize{RecursiveDynamicUsage(tx)},
-      nTime{time},
-      entryHeight{entry_height},
-      spendsCoinbase{spends_coinbase},
-      sigOpCount{sigops_count},
-      m_modified_fee{nFee},
-      lockPoints{lp},
-      nSizeWithDescendants{GetTxSize()},
-      nModFeesWithDescendants{nFee},
-      nSizeWithAncestors{GetTxSize()},
-      nModFeesWithAncestors{nFee},
-      nSigOpCountWithAncestors{sigOpCount} {}
-
-void CTxMemPoolEntry::UpdateModifiedFee(CAmount newFeeDelta)
-{
-    nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, newFeeDelta);
-    nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, newFeeDelta);
-    m_modified_fee = SaturatingAdd(m_modified_fee, newFeeDelta);
-}
-
-void CTxMemPoolEntry::UpdateLockPoints(const LockPoints& lp)
-{
-    lockPoints = lp;
-}
-
-size_t CTxMemPoolEntry::GetTxSize() const
-{
-    return GetVirtualTransactionSize(nTxSize, sigOpCount);
-}
-
 void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants,
                                       const std::set<uint256>& setExclude, std::set<uint256>& descendants_to_remove,
                                       uint64_t ancestor_size_limit, uint64_t ancestor_count_limit)