Merge bitcoin/bitcoin#22855: RBF move 3/3: move followups + improve RBF documentation

0ef08f8 add missing includes in policy/rbf (glozow)
c6abeb7 make MAX_BIP125_RBF_SEQUENCE constexpr (glozow)
3cf46f6 [doc] improve RBF documentation (glozow)
c78eb86 [policy/refactor] pass in relay fee instead of using global (glozow)

Pull request description:

  Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.

ACKs for top commit:
  jnewbery:
    utACK 0ef08f8
  fanquake:
    ACK 0ef08f8

Tree-SHA512: 6797ae758beca0c9673cb00ce85da48e9a4ac5cb5100074ca93e004cdb31d24d91a1a7721b57fc2f619addfeb4950d8caf45fee0f5b7528defbbd121eb4d271f

Author: fanquake

src/policy/rbf.cpp

@@ -48,26 +48,27 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
 }
 
 std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
-                            CTxMemPool& pool,
-                            const CTxMemPool::setEntries& iters_conflicting,
-                            CTxMemPool::setEntries& all_conflicts)
+                                                  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();
-        // This potentially overestimates the number of actual descendants but we just want to be
-        // conservative to avoid doing too much work.
+        // BIP125 Rule #5: don't consider replacing more than MAX_BIP125_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_BIP125_REPLACEMENT_CANDIDATES) {
             return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
                              txid.ToString(),
                              nConflictingCount,
                              MAX_BIP125_REPLACEMENT_CANDIDATES);
         }
     }
-    // If not too many to replace, then calculate the set of
-    // transactions that would have to be evicted
+    // Calculate the set of all transactions that would have to be evicted.
     for (CTxMemPool::txiter it : iters_conflicting) {
         pool.CalculateDescendants(it, all_conflicts);
     }
@@ -81,19 +82,19 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
     AssertLockHeld(pool.cs);
     std::set<uint256> parents_of_conflicts;
     for (const auto& mi : iters_conflicting) {
-        for (const CTxIn &txin : mi->GetTx().vin) {
+        for (const CTxIn& txin : mi->GetTx().vin) {
             parents_of_conflicts.insert(txin.prevout.hash);
         }
     }
 
     for (unsigned int j = 0; j < tx.vin.size(); j++) {
-        // 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.
+        // BIP125 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, above. See the comment above the first
-        // CalculateMempoolAncestors call for more info.
+        // 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.
@@ -111,7 +112,7 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
                                                    const uint256& txid)
 {
     for (CTxMemPool::txiter ancestorIt : ancestors) {
-        const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
+        const uint256& hashAncestor = ancestorIt->GetTx().GetHash();
         if (direct_conflicts.count(hashAncestor)) {
             return strprintf("%s spends conflicting transaction %s",
                              txid.ToString(),
@@ -150,24 +151,26 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
 std::optional<std::string> PaysForRBF(CAmount original_fees,
                                       CAmount replacement_fees,
                                       size_t replacement_vsize,
+                                      CFeeRate relay_fee,
                                       const uint256& txid)
 {
-    // The replacement must pay greater fees than the transactions it
-    // replaces - if we did the bandwidth used by those conflicting
-    // transactions would not be paid for.
+    // BIP125 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));
     }
 
-    // Finally in addition to paying more fees than the conflicts the
-    // new transaction must pay for its own bandwidth.
+    // BIP125 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 < ::incrementalRelayFee.GetFee(replacement_vsize)) {
+    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(::incrementalRelayFee.GetFee(replacement_vsize)));
+                         FormatMoney(relay_fee.GetFee(replacement_vsize)));
     }
     return std::nullopt;
 }

src/policy/rbf.h

@@ -5,7 +5,12 @@
 #ifndef BITCOIN_POLICY_RBF_H
 #define BITCOIN_POLICY_RBF_H
 
+#include <primitives/transaction.h>
 #include <txmempool.h>
+#include <uint256.h>
+
+#include <optional>
+#include <string>
 
 /** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all
  * mempool conflicts and their descendants. */
@@ -48,14 +53,14 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
 std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool,
                                                   const CTxMemPool::setEntries& iters_conflicting,
                                                   CTxMemPool::setEntries& all_conflicts)
-                                                  EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
+    EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
 
 /** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input
  * was included in one of the original transactions."
  * @returns error message if Rule #2 is broken, otherwise std::nullopt. */
 std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
                                                const CTxMemPool::setEntries& iters_conflicting)
-                                               EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
+    EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
 
 /** Check the intersection between two sets of transactions (a set of mempool entries and a set of
  * txids) to make sure they are disjoint.
@@ -84,12 +89,14 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
  * @param[in]   original_fees       Total modified fees of original transaction(s).
  * @param[in]   replacement_fees    Total modified fees of replacement transaction(s).
  * @param[in]   replacement_vsize   Total virtual size of replacement transaction(s).
+ * @param[in]   relay_fee           The node's minimum feerate for transaction relay.
  * @param[in]   txid                Transaction ID, included in the error message if violation occurs.
  * @returns error string if fees are insufficient, otherwise std::nullopt.
  */
 std::optional<std::string> PaysForRBF(CAmount original_fees,
                                       CAmount replacement_fees,
                                       size_t replacement_vsize,
+                                      CFeeRate relay_fee,
                                       const uint256& txid);
 
 #endif // BITCOIN_POLICY_RBF_H

src/util/rbf.h

@@ -9,15 +9,15 @@
 
 class CTransaction;
 
-static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
+static constexpr uint32_t MAX_BIP125_RBF_SEQUENCE{0xfffffffd};
 
 /** Check whether the sequence numbers on this transaction are signaling opt-in to replace-by-fee,
  * according to BIP 125.  Allow opt-out of transaction replacement by setting nSequence >
  * MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs.
 *
 * SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All
 * inputs rather than just one is for the sake of multi-party protocols, where we don't want a single
-* party to be able to disable replacement. */
-bool SignalsOptInRBF(const CTransaction &tx);
+* party to be able to disable replacement by opting out in their own input. */
+bool SignalsOptInRBF(const CTransaction& tx);
 
 #endif // BITCOIN_UTIL_RBF_H

src/validation.cpp

@@ -772,39 +772,43 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // pathological case by making sure setConflicts and setAncestors don't
     // intersect.
     if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) {
+        // We classify this as a consensus error because a transaction depending on something it
+        // conflicts with would be inconsistent.
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string);
     }
 
 
-    // If we don't hold the lock allConflicting might be incomplete; the
-    // subsequent RemoveStaged() and addUnchecked() calls don't guarantee
-    // mempool consistency for us.
     fReplacementTransaction = setConflicts.size();
-    if (fReplacementTransaction)
-    {
+    if (fReplacementTransaction) {
         CFeeRate newFeeRate(nModifiedFees, nSize);
+        // It's possible that the replacement pays more fees than its direct conflicts but not more
+        // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
+        // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not
+        // more economically rational to mine. Before we go digging through the mempool for all
+        // transactions that would need to be removed (direct conflicts and all descendants), check
+        // that the replacement transaction pays more than its direct conflicts.
         if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
         }
 
-        // Calculate all conflicting entries and enforce Rule #5.
+        // Calculate all conflicting entries and enforce BIP125 Rule #5.
         if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
                                  "too many potential replacements", *err_string);
         }
-        // Enforce Rule #2.
+        // Enforce BIP125 Rule #2.
         if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
                                  "replacement-adds-unconfirmed", *err_string);
         }
 
-        // Check if it's economically rational to mine this transaction rather
-        // than the ones it replaces. Enforce Rules #3 and #4.
+        // Check if it's economically rational to mine this transaction rather than the ones it
+        // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4.
         for (CTxMemPool::txiter it : allConflicting) {
             nConflictingFees += it->GetModifiedFee();
             nConflictingSize += it->GetTxSize();
         }
-        if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, hash)}) {
+        if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, ::incrementalRelayFee, hash)}) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
         }
     }