From fec7965277c2287d3eaba59fdc5b75729bd4838a Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Wed, 31 Jan 2024 06:32:04 +0000 Subject: [PATCH] Reject multiple-tx RBF with unconfirmed inputs Rule #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 | 36 ++++++++++++++++++++++++---------- src/policy/rbf.h | 2 +- src/test/rbf_tests.cpp | 2 +- test/functional/feature_rbf.py | 4 ++++ 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 074c314fdd9e3..99fdc3d382fad 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -94,19 +94,35 @@ std::optional 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); } } } diff --git a/src/policy/rbf.h b/src/policy/rbf.h index c952e2792c965..1bca40edba925 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -63,7 +63,7 @@ std::optional 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 HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool, diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index fb6a3614c068d..d47e48a9167ad 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -216,7 +216,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()); diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 8b747f6a9bdcd..3eb54050c9202 100755 --- a/test/functional/feature_rbf.py +++ b/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(