Skip to content

Commit

Permalink
Revert "Require unconfirmed inputs to come from a single conflict"
Browse files Browse the repository at this point in the history 
This reverts commit 5e21934.

Turns out this isn't actually needed, and it didn't work anyway:

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2024-January/022314.html
  • Loading branch information
@petertodd
petertodd committed Jan 28, 2024
1 parent d7b1e21 commit 05bbbe1
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 93 deletions.
65 changes: 18 additions & 47 deletions src/policy/rbf.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,58 +86,29 @@ std::optional<std::string> 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<COutPoint> 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<uint256> 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;
}
Expand Down
3 changes: 1 addition & 2 deletions src/policy/rbf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +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, 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<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
Expand Down
46 changes: 2 additions & 44 deletions test/functional/feature_rbf.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down

0 comments on commit 05bbbe1

Please sign in to comment.