Prevent low feerate txs from (directly) replacing high feerate txs

Previously all conflicting transactions were evaluated as a whole to
determine if the feerate was being increased. This meant that low
feerate children pulled the feerate down, potentially allowing a high
transaction with a high feerate to be replaced by one with a lower
feerate.

Author: petertodd

qa/replace-by-fee/rbf-tests.py

@@ -219,7 +219,7 @@ def branch(prevout, initial_value, max_txs, *, tree_width=5, fee=0.0001*COIN, _t
                 self.proxy.getrawtransaction(tx.GetHash())
 
     def test_replacement_feeperkb(self):
-        """Replacement requires overall fee-per-KB to be higher"""
+        """Replacement requires fee-per-KB to be higher"""
         tx0_outpoint = self.make_txout(1.1*COIN)
 
         tx1a = CTransaction([CTxIn(tx0_outpoint, nSequence=0)],

src/main.cpp

@@ -1008,32 +1008,60 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
         {
             LOCK(pool.cs);
 
-            // For efficiency we simply sum up the pre-calculated
-            // fees/size-with-descendants values from the mempool package
-            // tracking; this does mean the pathological case of diamond tx
-            // graphs will be overcounted.
+            CFeeRate newFeeRate(nFees, nSize);
             BOOST_FOREACH(const uint256 hashConflicting, setConflicts)
             {
                 CTxMemPool::txiter mi = pool.mapTx.find(hashConflicting);
                 if (mi == pool.mapTx.end())
                     continue;
+
+                // 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.
+                //
+                // The mining code doesn't (currently) take children into
+                // account (CPFP) so 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 oldFeeRate(mi->GetFee(), mi->GetTxSize());
+                if (newFeeRate <= oldFeeRate)
+                {
+                    return state.DoS(0,
+                            error("AcceptToMemoryPool: rejecting replacement %s; new feerate %s <= old feerate %s",
+                                  hash.ToString(),
+                                  newFeeRate.ToString(),
+                                  oldFeeRate.ToString()),
+                            REJECT_INSUFFICIENTFEE, "insufficient fee");
+                }
+
+                // For efficiency we simply sum up the pre-calculated
+                // fees/size-with-descendants values from the mempool package
+                // tracking; this does mean the pathological case of diamond tx
+                // graphs will be overcounted.
                 nConflictingFees += mi->GetFeesWithDescendants();
                 nConflictingSize += mi->GetSizeWithDescendants();
             }
 
-            // First of all we can't allow a replacement unless it pays greater
-            // fees than the transactions it conflicts with - if we did the
-            // bandwidth used by those conflicting transactions would not be
-            // paid for
+            // 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.
             if (nFees < nConflictingFees)
             {
                 return state.DoS(0, error("AcceptToMemoryPool: rejecting replacement %s, less fees than conflicting txs; %s < %s",
                                           hash.ToString(), FormatMoney(nFees), FormatMoney(nConflictingFees)),
                                  REJECT_INSUFFICIENTFEE, "insufficient fee");
             }
 
-            // Secondly in addition to paying more fees than the conflicts the
-            // new transaction must additionally pay for its own bandwidth.
+            // Finally in addition to paying more fees than the conflicts the
+            // new transaction must pay for its own bandwidth.
             CAmount nDeltaFees = nFees - nConflictingFees;
             if (nDeltaFees < ::minRelayTxFee.GetFee(nSize))
             {
@@ -1044,20 +1072,6 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
                               FormatMoney(::minRelayTxFee.GetFee(nSize))),
                         REJECT_INSUFFICIENTFEE, "insufficient fee");
             }
-
-            // Finally replace only if we end up with a larger fees-per-kb than
-            // the replacements.
-            CFeeRate oldFeeRate(nConflictingFees, nConflictingSize);
-            CFeeRate newFeeRate(nFees, nSize);
-            if (newFeeRate <= oldFeeRate)
-            {
-                return state.DoS(0,
-                        error("AcceptToMemoryPool: rejecting replacement %s; new feerate %s <= old feerate %s",
-                              hash.ToString(),
-                              newFeeRate.ToString(),
-                              oldFeeRate.ToString()),
-                        REJECT_INSUFFICIENTFEE, "insufficient fee");
-            }
         }
 
         // Check against previous transactions