Reject multiple-tx RBF with unconfirmed inputs

Rule bitcoin#6 prevents fee-rates from being decreased for direct conflicts, and thus
transactions spending confirmed inputs. But it does not prevent fee-rates from
being decreased when unconfirmed inputs are involved as a transaction spending
confirmed inputs can be merged with a transaction spending unconfirmed inputs.
Requiring replacements with unconfirmed inputs to be done one at a time ensures
that fee-rates always increase, while still allowing for RBF of CPFP
transactions.

This is necessary because relace-by-fee-rate assumes that fee-rates can't be
decreased; if they can be you can get infinite cycles of replacements.

src/policy/rbf.cpp

@@ -95,19 +95,35 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
     }
 
     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.
+        // Does this input spend an unconfirmed output?
         //
-        // 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))) {
+        // Rather than check the UTXO set - potentially expensive - it's
+        // cheaper to just check if the input refers to a tx that's in the
+        // mempool.
+        if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
+            // 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)) {
                 return strprintf("replacement %s adds unconfirmed input, idx %d",
                                  tx.GetHash().ToString(), j);
+
+            // Allow a CPFP transaction spending an unconfirmed input to be
+            // replaced. But only if it is the only conflict, and thus all new
+            // inputs are confirmed.
+            //
+            // The effect of this check is to prevent replacements from
+            // reducing the fee-rate of transactions. Rule #6 already prevents
+            // this for replacements spending confirmed inputs. Replacements
+            // involving unconfirmed spends however aren't caught by rule #6,
+            // so this eliminates the other case where this can happen.
+            } else if (iters_conflicting.size() > 1) {
+                return strprintf("replacement %s with unconfirmed input, idx %d, has multiple conflicts",
+                                 tx.GetHash().ToString(), j);
             }
         }
     }

src/policy/rbf.h

@@ -63,7 +63,7 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMem
     EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
 
 /** The replacement transaction may only include an unconfirmed input if that input was included in
- * one of the original transactions.
+ * one of the original transactions, and that is the only transaction being replaced.
  * @returns error message if tx spends unconfirmed inputs not also spent by iters_conflicting,
  * otherwise std::nullopt. */
 std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,

src/test/rbf_tests.cpp

@@ -214,7 +214,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
     }
     BOOST_CHECK(HasNoNewUnconfirmed(/*tx=*/ *spends_unconfirmed.get(),
                                     /*pool=*/ pool,
-                                    /*iters_conflicting=*/ all_entries) == std::nullopt);
+                                    /*iters_conflicting=*/ all_entries).has_value());
     BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2}) == std::nullopt);
     BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, empty_set).has_value());
 

test/functional/feature_rbf.py

@@ -383,6 +383,10 @@ def test_too_many_replacements(self):
             amount_per_output=split_value,
         )["new_utxos"]
 
+        # Mine the split transaction so we're spending confirmed inputs in the
+        # next step.
+        self.generate(self.nodes[0], 1)
+
         # Now spend each of those outputs individually
         for utxo in splitting_tx_utxos:
             self.wallet.send_self_transfer(

test/functional/p2p_permissions.py

@@ -119,7 +119,7 @@ def check_tx_relay(self):
             [tx],
             self.nodes[1],
             success=False,
-            reject_reason='{} (wtxid={}) from peer=0 was not accepted: txn-mempool-conflict'.format(txid, tx.getwtxid())
+            reject_reason='{} (wtxid={}) from peer=0 was not accepted: insufficient fee, rejecting replacement'.format(txid, tx.getwtxid())
         )
 
         p2p_rebroadcast_wallet.send_txs_and_test(