diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index e4b5f7b7cbb4a..074c314fdd9e3 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -86,58 +86,29 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool::setEntries& iters_conflicting) { AssertLockHeld(pool.cs); - - // 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. - // - // Furthermore, to avoid replacing an existing transaction with a worse - // transaction, we also require all unconfirmed inputs to come from a - // *single* replaced transaction. Otherwise you could replace a desirable - // transaction, and an undesirable transaction, with a single undesirable - // transaction. - // - // 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. - - // First, identify all unconfirmed inputs that the replacement transaction - // has. - std::set unconfirmed_prevouts; - for (unsigned int j = 0; j < tx.vin.size(); j++) { - // 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))) { - unconfirmed_prevouts.insert(tx.vin[j].prevout); + std::set parents_of_conflicts; + for (const auto& mi : iters_conflicting) { + for (const CTxIn& txin : mi->GetTx().vin) { + parents_of_conflicts.insert(txin.prevout.hash); } } - if (!unconfirmed_prevouts.empty()) { - // The replacement has unconfirmed inputs. Check if they all came from - // a single replaced transaction. - for (const auto& mi : iters_conflicting) { - bool found_unconfirmed_prevout = false; - for (const CTxIn& txin : mi->GetTx().vin) { - if (unconfirmed_prevouts.erase(txin.prevout)) { - found_unconfirmed_prevout = true; - } - } - - // We've fully processed *a* replaced transaction that had - // unconfirmed prevouts. We can quit now, as any further - // unconfirmed outputs are either from another replaced - // transaction - not allowed - or entirely new. - if (found_unconfirmed_prevout) { - break; + 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); } } - - // By putting this check here, we handle the case where - // iters_conflicting is empty. - if (!unconfirmed_prevouts.empty()) { - return strprintf("replacement %s adds unconfirmed input(s)", - tx.GetHash().ToString()); - } } return std::nullopt; } diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 0a2acc76d6bb0..c952e2792c965 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -63,8 +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, and only one of the original transactions - * may have unconfirmed inputs. + * one of the original transactions. * @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/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 919ff2403ef32..8b747f6a9bdcd 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -70,11 +70,8 @@ def run_test(self): self.log.info("Running test new unconfirmed inputs...") self.test_new_unconfirmed_inputs() - self.log.info("Running test new unconfirmed inputs...") - self.test_new_unconfirmed_inputs() - - self.log.info("Running test unconfirmed inputs from multiple replacements...") - self.test_unconfirmed_inputs_from_multiple_replacements() + self.log.info("Running test too many replacements...") + self.test_too_many_replacements() self.log.info("Running test too many replacements using default mempool params...") self.test_too_many_replacements_with_default_mempool_params() @@ -367,41 +364,6 @@ def test_new_unconfirmed_inputs(self): # This will raise an exception assert_raises_rpc_error(-26, "replacement-adds-unconfirmed", self.nodes[0].sendrawtransaction, tx2_hex, 0) - def test_unconfirmed_inputs_from_multiple_replacements(self): - """Replacements that with unconfirmed inputs from multiple replacements are rejected""" - - # Start by creating a single transaction with 2 outputs - initial_nValue = 10 * COIN - utxo = self.make_utxo(self.nodes[0], initial_nValue) - - splitting_tx_utxos = self.wallet.send_self_transfer_multi( - from_node=self.nodes[0], - utxos_to_spend=[utxo], - sequence=0, - num_outputs=2, - amount_per_output=1 * COIN, - )["new_utxos"] - - # Now spend each of those outputs individually - for utxo in splitting_tx_utxos: - self.wallet.send_self_transfer( - from_node=self.nodes[0], - utxo_to_spend=utxo, - sequence=0, - fee=Decimal("0.1"), - ) - - # Now create doublespend of the whole lot; should fail. - double_tx = self.wallet.create_self_transfer_multi( - utxos_to_spend=splitting_tx_utxos, - sequence=0, - amount_per_output=1 * COIN, - )["tx"] - double_tx_hex = double_tx.serialize().hex() - - # This will raise an exception - assert_raises_rpc_error(-26, "replacement-adds-unconfirmed", self.nodes[0].sendrawtransaction, double_tx_hex, 0) - def test_too_many_replacements(self): """Replacements that evict too many transactions are rejected""" # Try directly replacing more than MAX_REPLACEMENT_LIMIT @@ -421,10 +383,6 @@ def test_too_many_replacements(self): amount_per_output=split_value, )["new_utxos"] - # Mine the split transaction so that 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(