diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 210b94915cc..6852cd52a33 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -62,59 +62,27 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool::setEntries& all_conflicts) { AssertLockHeld(pool.cs); - uint64_t nConflictingCount = 0; - for (const auto& mi : iters_conflicting) { - nConflictingCount += mi->GetCountWithDescendants(); - // Rule #5: don't consider replacing more than MAX_REPLACEMENT_CANDIDATES - // entries from the mempool. This potentially overestimates the number of actual - // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple - // times), but we just want to be conservative to avoid doing too much work. - if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) { - return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)", - tx.GetHash().ToString(), - nConflictingCount, - MAX_REPLACEMENT_CANDIDATES); - } + // Rule #5: don't consider replacements that conflict directly with more + // than MAX_REPLACEMENT_CANDIDATES distinct clusters. This implies a bound + // on how many mempool clusters might need to be re-sorted in order to + // process the replacement (though the actual number of clusters we + // relinearize may be greater than this number, due to cluster splitting). + auto num_clusters = pool.GetUniqueClusterCount(iters_conflicting); + if (num_clusters > MAX_REPLACEMENT_CANDIDATES) { + return strprintf("rejecting replacement %s; too many conflicting clusters (%u > %d)", + tx.GetHash().ToString(), + num_clusters, + MAX_REPLACEMENT_CANDIDATES); } // Calculate the set of all transactions that would have to be evicted. for (CTxMemPool::txiter it : iters_conflicting) { + // The cluster count limit ensures that we won't do too much work on a + // single invocation of this function. pool.CalculateDescendants(it, all_conflicts); } return std::nullopt; } -std::optional HasNoNewUnconfirmed(const CTransaction& tx, - const CTxMemPool& pool, - const CTxMemPool::setEntries& iters_conflicting) -{ - AssertLockHeld(pool.cs); - 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); - } - } - - 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(tx.vin[j].prevout.hash)) { - return strprintf("replacement %s adds unconfirmed input, idx %d", - tx.GetHash().ToString(), j); - } - } - } - return std::nullopt; -} - std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, const std::set& direct_conflicts, const Txid& txid) @@ -130,32 +98,6 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& return std::nullopt; } -std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, - CFeeRate replacement_feerate, - const Txid& txid) -{ - for (const auto& mi : iters_conflicting) { - // 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. - // - // 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 original_feerate(mi->GetModifiedFee(), mi->GetTxSize()); - if (replacement_feerate <= original_feerate) { - return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", - txid.ToString(), - replacement_feerate.ToString(), - original_feerate.ToString()); - } - } - return std::nullopt; -} - std::optional PaysForRBF(CAmount original_fees, CAmount replacement_fees, size_t replacement_vsize, diff --git a/src/policy/rbf.h b/src/policy/rbf.h index cc397b463dc..b135ecf2dc2 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -21,8 +21,8 @@ class CFeeRate; class uint256; -/** Maximum number of transactions that can be replaced by RBF (Rule #5). This includes all - * mempool conflicts and their descendants. */ +/** Maximum number of unique clusters that can be affected by an RBF (Rule #5); + * see GetEntriesForConflicts() */ static constexpr uint32_t MAX_REPLACEMENT_CANDIDATES{100}; /** The rbf state of unconfirmed transactions */ @@ -57,28 +57,20 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) E RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); /** Get all descendants of iters_conflicting. Checks that there are no more than - * MAX_REPLACEMENT_CANDIDATES potential entries. May overestimate if the entries in - * iters_conflicting have overlapping descendants. + * MAX_REPLACEMENT_CANDIDATES distinct clusters affected. + * * @param[in] iters_conflicting The set of iterators to mempool entries. * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, * which includes iters_conflicting and all entries' descendants. * Not cleared at the start; any existing mempool entries will * remain in the set. - * @returns an error message if MAX_REPLACEMENT_CANDIDATES may be exceeded, otherwise a std::nullopt. + * @returns an error message if the number of affected clusters would exceed MAX_REPLACEMENT_CANDIDATES, std::nullopt otherwise */ std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, CTxMemPool::setEntries& all_conflicts) 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. - * @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, - const CTxMemPool::setEntries& iters_conflicting) - EXCLUSIVE_LOCKS_REQUIRED(pool.cs); - /** Check the intersection between two sets of transactions (a set of mempool entries and a set of * txids) to make sure they are disjoint. * @param[in] ancestors Set of mempool entries corresponding to ancestors of the @@ -92,14 +84,6 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& const std::set& direct_conflicts, const Txid& txid); -/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each - * of the transactions in iters_conflicting. - * @param[in] iters_conflicting The set of mempool entries. - * @returns error message if fees insufficient, otherwise std::nullopt. - */ -std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, - CFeeRate replacement_feerate, const Txid& txid); - /** The replacement transaction must pay more fees than the original transactions. The additional * fees must pay for the replacement's bandwidth at or above the incremental relay feerate. * @param[in] original_fees Total modified fees of original transaction(s). diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp index 300f31d4408..2ca57341533 100644 --- a/src/test/fuzz/rbf.cpp +++ b/src/test/fuzz/rbf.cpp @@ -144,9 +144,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf) } assert(!pool.GetIter(parent_entry.GetTx().GetHash())); AddToMempool(pool, parent_entry); - if (fuzzed_data_provider.ConsumeBool()) { - child.vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0}; - } + child.vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0}; mempool_txs.emplace_back(child); const auto child_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back()); running_vsize_total += child_entry.GetTxSize(); diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index 31cf994c875..45dd8bcb120 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -37,37 +37,6 @@ static inline CTransactionRef make_tx(const std::vector& inputs return MakeTransactionRef(tx); } -// Make two child transactions from parent (which must have at least 2 outputs). -// Each tx will have the same outputs, using the amounts specified in output_values. -static inline std::pair make_two_siblings(const CTransactionRef parent, - const std::vector& output_values) -{ - assert(parent->vout.size() >= 2); - - // First tx takes first parent output - CMutableTransaction tx1 = CMutableTransaction(); - tx1.vin.resize(1); - tx1.vout.resize(output_values.size()); - - tx1.vin[0].prevout.hash = parent->GetHash(); - tx1.vin[0].prevout.n = 0; - // Add a witness so wtxid != txid - CScriptWitness witness; - witness.stack.emplace_back(10); - tx1.vin[0].scriptWitness = witness; - - for (size_t i = 0; i < output_values.size(); ++i) { - tx1.vout[i].scriptPubKey = CScript() << OP_11 << OP_EQUAL; - tx1.vout[i].nValue = output_values[i]; - } - - // Second tx takes second parent output - CMutableTransaction tx2 = tx1; - tx2.vin[0].prevout.n = 1; - - return std::make_pair(MakeTransactionRef(tx1), MakeTransactionRef(tx2)); -} - static CTransactionRef add_descendants(const CTransactionRef& tx, int32_t num_descendants, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) { @@ -79,39 +48,13 @@ static CTransactionRef add_descendants(const CTransactionRef& tx, int32_t num_de for (int32_t i{0}; i < num_descendants; ++i) { auto next_tx = make_tx(/*inputs=*/{tx_to_spend}, /*output_values=*/{(50 - i) * CENT}); AddToMempool(pool, entry.FromTx(next_tx)); + BOOST_CHECK(pool.GetIter(next_tx->GetHash()).has_value()); tx_to_spend = next_tx; } // Return last created tx return tx_to_spend; } -static CTransactionRef add_descendant_to_parents(const std::vector& parents, CTxMemPool& pool) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) -{ - AssertLockHeld(::cs_main); - AssertLockHeld(pool.cs); - TestMemPoolEntryHelper entry; - // Assumes this isn't already spent in mempool - auto child_tx = make_tx(/*inputs=*/parents, /*output_values=*/{50 * CENT}); - AddToMempool(pool, entry.FromTx(child_tx)); - // Return last created tx - return child_tx; -} - -// Makes two children for a single parent -static std::pair add_children_to_parent(const CTransactionRef parent, CTxMemPool& pool) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) -{ - AssertLockHeld(::cs_main); - AssertLockHeld(pool.cs); - TestMemPoolEntryHelper entry; - // Assumes this isn't already spent in mempool - auto children_tx = make_two_siblings(/*parent=*/parent, /*output_values=*/{50 * CENT}); - AddToMempool(pool, entry.FromTx(children_tx.first)); - AddToMempool(pool, entry.FromTx(children_tx.second)); - return children_tx; -} - BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) { CTxMemPool& pool = *Assert(m_node.mempool); @@ -149,12 +92,6 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) const auto tx8 = make_tx(/*inputs=*/ {m_coinbase_txns[4]}, /*output_values=*/ {999 * CENT}); AddToMempool(pool, entry.Fee(high_fee).FromTx(tx8)); - // Normal txs, will chain txns right before CheckConflictTopology test - const auto tx9 = make_tx(/*inputs=*/ {m_coinbase_txns[5]}, /*output_values=*/ {995 * CENT}); - AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx9)); - const auto tx10 = make_tx(/*inputs=*/ {m_coinbase_txns[6]}, /*output_values=*/ {995 * CENT}); - AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx10)); - // Will make these two parents of single child const auto tx11 = make_tx(/*inputs=*/ {m_coinbase_txns[7]}, /*output_values=*/ {995 * CENT}); AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx11)); @@ -173,11 +110,6 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) const auto entry6_low_prioritised = pool.GetIter(tx6->GetHash()).value(); const auto entry7_high = pool.GetIter(tx7->GetHash()).value(); const auto entry8_high = pool.GetIter(tx8->GetHash()).value(); - const auto entry9_unchained = pool.GetIter(tx9->GetHash()).value(); - const auto entry10_unchained = pool.GetIter(tx10->GetHash()).value(); - const auto entry11_unchained = pool.GetIter(tx11->GetHash()).value(); - const auto entry12_unchained = pool.GetIter(tx12->GetHash()).value(); - const auto entry13_unchained = pool.GetIter(tx13->GetHash()).value(); BOOST_CHECK_EQUAL(entry1_normal->GetFee(), normal_fee); BOOST_CHECK_EQUAL(entry2_normal->GetFee(), normal_fee); @@ -198,23 +130,6 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) const auto unused_txid = Txid::FromUint256(GetRandHash()); - // Tests for PaysMoreThanConflicts - // These tests use feerate, not absolute fee. - BOOST_CHECK(PaysMoreThanConflicts(/*iters_conflicting=*/set_12_normal, - /*replacement_feerate=*/CFeeRate(entry1_normal->GetModifiedFee() + 1, entry1_normal->GetTxSize() + 2), - /*txid=*/unused_txid).has_value()); - // Replacement must be strictly greater than the originals. - BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1_normal->GetModifiedFee(), entry1_normal->GetTxSize()), unused_txid).has_value()); - BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1_normal->GetModifiedFee() + 1, entry1_normal->GetTxSize()), unused_txid) == std::nullopt); - // These tests use modified fees (including prioritisation), not base fees. - BOOST_CHECK(PaysMoreThanConflicts({entry5_low}, CFeeRate(entry5_low->GetModifiedFee() + 1, entry5_low->GetTxSize()), unused_txid) == std::nullopt); - BOOST_CHECK(PaysMoreThanConflicts({entry6_low_prioritised}, CFeeRate(entry6_low_prioritised->GetFee() + 1, entry6_low_prioritised->GetTxSize()), unused_txid).has_value()); - BOOST_CHECK(PaysMoreThanConflicts({entry6_low_prioritised}, CFeeRate(entry6_low_prioritised->GetModifiedFee() + 1, entry6_low_prioritised->GetTxSize()), unused_txid) == std::nullopt); - // PaysMoreThanConflicts checks individual feerate, not ancestor feerate. This test compares - // replacement_feerate and entry4_high's feerate, which are the same. The replacement_feerate is - // considered too low even though entry4_high has a low ancestor feerate. - BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4_high->GetModifiedFee(), entry4_high->GetTxSize()), unused_txid).has_value()); - // Tests for EntriesAndTxidsDisjoint BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {tx1->GetHash()}, unused_txid) == std::nullopt); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx3->GetHash()}, unused_txid) == std::nullopt); @@ -244,112 +159,91 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) BOOST_CHECK(PaysForRBF(high_fee, high_fee + 4, 20, higher_relay_feerate, unused_txid) == std::nullopt); BOOST_CHECK(PaysForRBF(low_fee, high_fee, 99999999, incremental_relay_feerate, unused_txid).has_value()); BOOST_CHECK(PaysForRBF(low_fee, high_fee + 99999999, 99999999, incremental_relay_feerate, unused_txid) == std::nullopt); +} - // Tests for GetEntriesForConflicts - CTxMemPool::setEntries all_parents{entry1_normal, entry3_low, entry5_low, entry7_high, entry8_high}; - CTxMemPool::setEntries all_children{entry2_normal, entry4_high, entry6_low_prioritised}; - const std::vector parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2], - m_coinbase_txns[3], m_coinbase_txns[4]}); - const auto conflicts_with_parents = make_tx(parent_inputs, {50 * CENT}); - CTxMemPool::setEntries all_conflicts; - BOOST_CHECK(GetEntriesForConflicts(/*tx=*/ *conflicts_with_parents.get(), - /*pool=*/ pool, - /*iters_conflicting=*/ all_parents, - /*all_conflicts=*/ all_conflicts) == std::nullopt); - BOOST_CHECK(all_conflicts == all_entries); - auto conflicts_size = all_conflicts.size(); - all_conflicts.clear(); +BOOST_FIXTURE_TEST_CASE(rbf_conflicts_calculator, TestChain100Setup) +{ + CTxMemPool& pool = *Assert(m_node.mempool); + LOCK2(::cs_main, pool.cs); + TestMemPoolEntryHelper entry; - add_descendants(tx2, 23, pool); - BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts) == std::nullopt); - conflicts_size += 23; - BOOST_CHECK_EQUAL(all_conflicts.size(), conflicts_size); - all_conflicts.clear(); + const CAmount normal_fee{CENT/10}; - add_descendants(tx4, 23, pool); - BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts) == std::nullopt); - conflicts_size += 23; - BOOST_CHECK_EQUAL(all_conflicts.size(), conflicts_size); - all_conflicts.clear(); - - add_descendants(tx6, 23, pool); - BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts) == std::nullopt); - conflicts_size += 23; - BOOST_CHECK_EQUAL(all_conflicts.size(), conflicts_size); - all_conflicts.clear(); - - add_descendants(tx7, 23, pool); - BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts) == std::nullopt); - conflicts_size += 23; - BOOST_CHECK_EQUAL(all_conflicts.size(), conflicts_size); - BOOST_CHECK_EQUAL(all_conflicts.size(), 100); - all_conflicts.clear(); - - // Exceeds maximum number of conflicts. - add_descendants(tx8, 1, pool); - BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts).has_value()); - - // Tests for HasNoNewUnconfirmed - const auto spends_unconfirmed = make_tx({tx1}, {36 * CENT}); - for (const auto& input : spends_unconfirmed->vin) { - // Spends unconfirmed inputs. - BOOST_CHECK(pool.exists(input.prevout.hash)); + // Create two parent transactions with 51 outputs each + const int NUM_OUTPUTS = 51; + std::vector output_values; + output_values.reserve(NUM_OUTPUTS); + for (int i = 0; i < NUM_OUTPUTS; ++i) { + output_values.push_back(1 * COIN); } - BOOST_CHECK(HasNoNewUnconfirmed(/*tx=*/ *spends_unconfirmed.get(), - /*pool=*/ pool, - /*iters_conflicting=*/ all_entries) == std::nullopt); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2_normal}) == std::nullopt); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, empty_set).has_value()); - const auto spends_new_unconfirmed = make_tx({tx1, tx8}, {36 * CENT}); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2_normal}).has_value()); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value()); + const auto parent_tx_1 = make_tx(/*inputs=*/ {m_coinbase_txns[0]}, /*output_values=*/ output_values); + const auto parent_tx_2 = make_tx(/*inputs=*/ {m_coinbase_txns[1]}, /*output_values=*/ output_values); + AddToMempool(pool, entry.Fee(normal_fee).FromTx(parent_tx_1)); + AddToMempool(pool, entry.Fee(normal_fee).FromTx(parent_tx_2)); - const auto spends_conflicting_confirmed = make_tx({m_coinbase_txns[0], m_coinbase_txns[1]}, {45 * CENT}); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_conflicting_confirmed.get(), pool, {entry1_normal, entry3_low}) == std::nullopt); + std::vector direct_children; - // Tests for CheckConflictTopology + // Create individual spends of these outputs + for (const auto& parent_tx : {parent_tx_1, parent_tx_2}) { + for (auto i = 0; i < NUM_OUTPUTS; ++i) { + auto pretx = make_tx(/*inputs=*/ {parent_tx}, /*output_values=*/ {995 * CENT}); + CMutableTransaction tx(*pretx); + tx.vin[0].prevout.n = i; + AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx)); + BOOST_CHECK(pool.GetIter(tx.GetHash()).has_value()); + direct_children.push_back(MakeTransactionRef(tx)); + } + } - // Tx4 has 23 descendants - BOOST_CHECK_EQUAL(pool.CheckConflictTopology(set_34_cpfp).value(), strprintf("%s has 24 descendants, max 1 allowed", entry3_low->GetSharedTx()->GetHash().ToString())); + // At this point, we should have 2 clusters in the mempool, each with 52 + // transactions. - // No descendants yet - BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained}) == std::nullopt); + // parent_tx and all children are in one cluster, so we can have as many + // conflicts within this cluster as we want without violating the RBF conflicts + // limit. + const auto parent_entry_1 = pool.GetIter(parent_tx_1->GetHash()).value(); + const auto parent_entry_2 = pool.GetIter(parent_tx_2->GetHash()).value(); + const auto conflicting_transaction = make_tx({parent_tx_1, parent_tx_2}, {50 * CENT}); + CTxMemPool::setEntries all_conflicts, dummy; + BOOST_CHECK(GetEntriesForConflicts(/*tx=*/ *conflicting_transaction.get(), + /*pool=*/ pool, + /*iters_conflicting=*/ {parent_entry_1, parent_entry_2}, + /*all_conflicts=*/ all_conflicts) == std::nullopt); - // Add 1 descendant, still ok - add_descendants(tx9, 1, pool); - BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained}) == std::nullopt); + dummy.clear(); + // Conflicting directly with all those conflicts doesn't change anything. + BOOST_CHECK(GetEntriesForConflicts(/*tx=*/ *conflicting_transaction.get(), + /*pool=*/ pool, + /*iters_conflicting=*/ all_conflicts, + /*all_conflicts=*/ dummy) == std::nullopt); + BOOST_CHECK_EQUAL(all_conflicts.size(), dummy.size()); + dummy.clear(); - // N direct conflicts; ok - BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained, entry10_unchained, entry11_unchained}) == std::nullopt); + // If we mine the parent_tx's, then the clusters split (102 clusters). + pool.removeForBlock({parent_tx_1, parent_tx_2}, /* dummy */ 1); - // Add 1 descendant, still ok, even if it's considered a direct conflict as well - const auto child_tx = add_descendants(tx10, 1, pool); - const auto entry10_child = pool.GetIter(child_tx->GetHash()).value(); - BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained, entry10_unchained, entry11_unchained}) == std::nullopt); - BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained, entry10_unchained, entry11_unchained, entry10_child}) == std::nullopt); + // Add some descendants now to each of the direct children (we can do this now that the clusters have split). + for (const auto& child : direct_children) { + add_descendants(child, 10, pool); + } - // One more, size 3 cluster too much - const auto grand_child_tx = add_descendants(child_tx, 1, pool); - const auto entry10_grand_child = pool.GetIter(grand_child_tx->GetHash()).value(); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry9_unchained, entry10_unchained, entry11_unchained}).value(), strprintf("%s has 2 descendants, max 1 allowed", entry10_unchained->GetSharedTx()->GetHash().ToString())); - // even if direct conflict is descendent itself - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry9_unchained, entry10_grand_child, entry11_unchained}).value(), strprintf("%s has 2 ancestors, max 1 allowed", entry10_grand_child->GetSharedTx()->GetHash().ToString())); - - // Make a single child from two singleton parents - const auto two_parent_child_tx = add_descendant_to_parents({tx11, tx12}, pool); - const auto entry_two_parent_child = pool.GetIter(two_parent_child_tx->GetHash()).value(); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry11_unchained}).value(), strprintf("%s is not the only parent of child %s", entry11_unchained->GetSharedTx()->GetHash().ToString(), entry_two_parent_child->GetSharedTx()->GetHash().ToString())); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry12_unchained}).value(), strprintf("%s is not the only parent of child %s", entry12_unchained->GetSharedTx()->GetHash().ToString(), entry_two_parent_child->GetSharedTx()->GetHash().ToString())); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry_two_parent_child}).value(), strprintf("%s has 2 ancestors, max 1 allowed", entry_two_parent_child->GetSharedTx()->GetHash().ToString())); - - // Single parent with two children, we will conflict with the siblings directly only - const auto two_siblings = add_children_to_parent(tx13, pool); - const auto entry_sibling_1 = pool.GetIter(two_siblings.first->GetHash()).value(); - const auto entry_sibling_2 = pool.GetIter(two_siblings.second->GetHash()).value(); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry_sibling_1}).value(), strprintf("%s is not the only child of parent %s", entry_sibling_1->GetSharedTx()->GetHash().ToString(), entry13_unchained->GetSharedTx()->GetHash().ToString())); - BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry_sibling_2}).value(), strprintf("%s is not the only child of parent %s", entry_sibling_2->GetSharedTx()->GetHash().ToString(), entry13_unchained->GetSharedTx()->GetHash().ToString())); + // We can conflict with 100 different clusters, even if they have lots of transactions. + CTxMemPool::setEntries conflicts; + for (auto i = 0; i < 100; ++i) { + conflicts.insert(pool.GetIter(direct_children[i]->GetHash()).value()); + } + BOOST_CHECK(GetEntriesForConflicts(/*tx=*/ *conflicting_transaction.get(), + /*pool=*/ pool, + /*iters_conflicting=*/ conflicts, + /*all_conflicts=*/ dummy) == std::nullopt); + // Conflicting with 1 more distinct cluster causes failure, however. + conflicts.insert(pool.GetIter(direct_children[100]->GetHash()).value()); + BOOST_CHECK(GetEntriesForConflicts(/*tx=*/ *conflicting_transaction.get(), + /*pool=*/ pool, + /*iters_conflicting=*/ conflicts, + /*all_conflicts=*/ dummy).has_value()); } BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup) @@ -427,7 +321,7 @@ BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup) BOOST_CHECK(ImprovesFeerateDiagram(*changeset) == std::nullopt); changeset.reset(); - // Adding a grandchild makes the cluster size 3, which is uncalculable + // Adding a grandchild makes the cluster size 3, which is also calculable const auto tx5 = make_tx(/*inputs=*/ {tx2}, /*output_values=*/ {995 * CENT}); AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx5)); const auto entry5 = pool.GetIter(tx5->GetHash()).value(); @@ -439,9 +333,7 @@ BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup) changeset->StageAddition(tx1_conflict, tx1_fee, 0, 1, 0, false, 4, LockPoints()); changeset->StageAddition(entry4.GetSharedTx(), tx2_fee + entry5->GetModifiedFee() + 1, 0, 1, 0, false, 4, LockPoints()); const auto res3 = ImprovesFeerateDiagram(*changeset); - BOOST_CHECK(res3.has_value()); - BOOST_CHECK(res3.value().first == DiagramCheckError::UNCALCULABLE); - BOOST_CHECK_MESSAGE(res3.value().second == strprintf("%s has 2 descendants, max 1 allowed", tx1->GetHash().GetHex()), res3.value().second); + BOOST_CHECK(res3 == std::nullopt); } BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) @@ -451,7 +343,6 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) TestMemPoolEntryHelper entry; const CAmount low_fee{CENT/100}; - const CAmount normal_fee{CENT/10}; const CAmount high_fee{CENT}; // low -> high -> medium fee transactions that would result in two chunks together since they @@ -460,7 +351,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) AddToMempool(pool, entry.Fee(low_fee).FromTx(low_tx)); const auto entry_low = pool.GetIter(low_tx->GetHash()).value(); - const auto low_size = entry_low->GetTxSize(); + const auto low_size = entry_low->GetAdjustedWeight(); const auto replacement_tx = make_tx(/*inputs=*/ {m_coinbase_txns[0]}, /*output_values=*/ {9 * COIN}); auto entry_replacement = entry.FromTx(replacement_tx); @@ -474,7 +365,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_one.has_value()); std::vector expected_old_chunks{{low_fee, low_size}}; BOOST_CHECK(replace_one->first == expected_old_chunks); - std::vector expected_new_chunks{{0, int32_t(entry_replacement.GetTxSize())}}; + std::vector expected_new_chunks{{0, entry_replacement.GetAdjustedWeight()}}; BOOST_CHECK(replace_one->second == expected_new_chunks); } @@ -487,7 +378,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_one_fee.has_value()); std::vector expected_old_diagram{{low_fee, low_size}}; BOOST_CHECK(replace_one_fee->first == expected_old_diagram); - std::vector expected_new_diagram{{high_fee, low_size}}; + std::vector expected_new_diagram{{high_fee, entry_replacement.GetAdjustedWeight()}}; BOOST_CHECK(replace_one_fee->second == expected_new_diagram); } @@ -495,7 +386,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) const auto high_tx = make_tx(/*inputs=*/ {low_tx}, /*output_values=*/ {995 * CENT}); AddToMempool(pool, entry.Fee(high_fee).FromTx(high_tx)); const auto entry_high = pool.GetIter(high_tx->GetHash()).value(); - const auto high_size = entry_high->GetTxSize(); + const auto high_size = entry_high->GetAdjustedWeight(); { auto changeset = pool.GetChangeSet(); @@ -506,7 +397,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_single_chunk.has_value()); std::vector expected_old_chunks{{low_fee + high_fee, low_size + high_size}}; BOOST_CHECK(replace_single_chunk->first == expected_old_chunks); - std::vector expected_new_chunks{{high_fee, low_size}}; + std::vector expected_new_chunks{{high_fee, entry_replacement.GetAdjustedWeight()}}; BOOST_CHECK(replace_single_chunk->second == expected_new_chunks); } @@ -519,36 +410,20 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_cpfp_child.has_value()); std::vector expected_old_chunks{{low_fee + high_fee, low_size + high_size}}; BOOST_CHECK(replace_cpfp_child->first == expected_old_chunks); - std::vector expected_new_chunks{{high_fee, low_size}, {low_fee, low_size}}; + std::vector expected_new_chunks{{high_fee, entry_replacement.GetAdjustedWeight()}, {low_fee, low_size}}; BOOST_CHECK(replace_cpfp_child->second == expected_new_chunks); } - // third transaction causes the topology check to fail - const auto normal_tx = make_tx(/*inputs=*/ {high_tx}, /*output_values=*/ {995 * CENT}); - AddToMempool(pool, entry.Fee(normal_fee).FromTx(normal_tx)); - const auto entry_normal = pool.GetIter(normal_tx->GetHash()).value(); - - { - auto changeset = pool.GetChangeSet(); - changeset->StageRemoval(entry_low); - changeset->StageRemoval(entry_high); - changeset->StageRemoval(entry_normal); - changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints()); - const auto replace_too_large{changeset->CalculateChunksForRBF()}; - BOOST_CHECK(!replace_too_large.has_value()); - BOOST_CHECK_EQUAL(util::ErrorString(replace_too_large).original, strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", high_tx->GetHash().GetHex())); - } - // Make a size 2 cluster that is itself two chunks; evict both txns const auto high_tx_2 = make_tx(/*inputs=*/ {m_coinbase_txns[1]}, /*output_values=*/ {10 * COIN}); AddToMempool(pool, entry.Fee(high_fee).FromTx(high_tx_2)); const auto entry_high_2 = pool.GetIter(high_tx_2->GetHash()).value(); - const auto high_size_2 = entry_high_2->GetTxSize(); + const auto high_size_2 = entry_high_2->GetAdjustedWeight(); const auto low_tx_2 = make_tx(/*inputs=*/ {high_tx_2}, /*output_values=*/ {9 * COIN}); AddToMempool(pool, entry.Fee(low_fee).FromTx(low_tx_2)); const auto entry_low_2 = pool.GetIter(low_tx_2->GetHash()).value(); - const auto low_size_2 = entry_low_2->GetTxSize(); + const auto low_size_2 = entry_low_2->GetAdjustedWeight(); { auto changeset = pool.GetChangeSet(); @@ -563,7 +438,7 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_two_chunks_single_cluster->second == expected_new_chunks); } - // You can have more than two direct conflicts if there are multiple affected clusters, all of size 2 or less + // You can have more than two direct conflicts const auto conflict_1 = make_tx(/*inputs=*/ {m_coinbase_txns[2]}, /*output_values=*/ {10 * COIN}); AddToMempool(pool, entry.Fee(low_fee).FromTx(conflict_1)); const auto conflict_1_entry = pool.GetIter(conflict_1->GetHash()).value(); @@ -606,25 +481,6 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup) BOOST_CHECK(replace_multiple_clusters_2->first.size() == 4); BOOST_CHECK(replace_multiple_clusters_2->second.size() == 1); } - - // Add another descendant to conflict_1, making the cluster size > 2 should fail at this point. - const auto conflict_1_grand_child = make_tx(/*inputs=*/{conflict_1_child}, /*output_values=*/ {995 * CENT}); - AddToMempool(pool, entry.Fee(high_fee).FromTx(conflict_1_grand_child)); - const auto conflict_1_grand_child_entry = pool.GetIter(conflict_1_child->GetHash()).value(); - - { - auto changeset = pool.GetChangeSet(); - changeset->StageRemoval(conflict_1_entry); - changeset->StageRemoval(conflict_2_entry); - changeset->StageRemoval(conflict_3_entry); - changeset->StageRemoval(conflict_1_child_entry); - changeset->StageRemoval(conflict_1_grand_child_entry); - changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints()); - const auto replace_cluster_size_3{changeset->CalculateChunksForRBF()}; - - BOOST_CHECK(!replace_cluster_size_3.has_value()); - BOOST_CHECK_EQUAL(util::ErrorString(replace_cluster_size_3).original, strprintf("%s has 2 descendants, max 1 allowed", conflict_1->GetHash().GetHex())); - } } BOOST_AUTO_TEST_CASE(feerate_chunks_utilities) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 672198ec2ea..15b2508119c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1287,135 +1287,15 @@ std::vector CTxMemPool::GatherClusters(const std::vector CTxMemPool::CheckConflictTopology(const setEntries& direct_conflicts) -{ - for (const auto& direct_conflict : direct_conflicts) { - // Ancestor and descendant counts are inclusive of the tx itself. - const auto ancestor_count{direct_conflict->GetCountWithAncestors()}; - const auto descendant_count{direct_conflict->GetCountWithDescendants()}; - const bool has_ancestor{ancestor_count > 1}; - const bool has_descendant{descendant_count > 1}; - const auto& txid_string{direct_conflict->GetSharedTx()->GetHash().ToString()}; - // The only allowed configurations are: - // 1 ancestor and 0 descendant - // 0 ancestor and 1 descendant - // 0 ancestor and 0 descendant - if (ancestor_count > 2) { - return strprintf("%s has %u ancestors, max 1 allowed", txid_string, ancestor_count - 1); - } else if (descendant_count > 2) { - return strprintf("%s has %u descendants, max 1 allowed", txid_string, descendant_count - 1); - } else if (has_ancestor && has_descendant) { - return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string); - } - // Additionally enforce that: - // If we have a child, we are its only parent. - // If we have a parent, we are its only child. - if (has_descendant) { - const auto& our_child = direct_conflict->GetMemPoolChildrenConst().begin(); - if (our_child->get().GetCountWithAncestors() > 2) { - return strprintf("%s is not the only parent of child %s", - txid_string, our_child->get().GetSharedTx()->GetHash().ToString()); - } - } else if (has_ancestor) { - const auto& our_parent = direct_conflict->GetMemPoolParentsConst().begin(); - if (our_parent->get().GetCountWithDescendants() > 2) { - return strprintf("%s is not the only child of parent %s", - txid_string, our_parent->get().GetSharedTx()->GetHash().ToString()); - } - } - } - return std::nullopt; -} - util::Result, std::vector>> CTxMemPool::ChangeSet::CalculateChunksForRBF() { LOCK(m_pool->cs); - FeeFrac replacement_feerate{0, 0}; - for (auto it : m_entry_vec) { - replacement_feerate += {it->GetModifiedFee(), it->GetTxSize()}; + + if (!CheckMemPoolPolicyLimits()) { + return util::Error{Untranslated("cluster size limit exceeded")}; } - auto err_string{m_pool->CheckConflictTopology(m_to_remove)}; - if (err_string.has_value()) { - // Unsupported topology for calculating a feerate diagram - return util::Error{Untranslated(err_string.value())}; - } - - // new diagram will have chunks that consist of each ancestor of - // direct_conflicts that is at its own fee/size, along with the replacement - // tx/package at its own fee/size - - // old diagram will consist of the ancestors and descendants of each element of - // all_conflicts. every such transaction will either be at its own feerate (followed - // by any descendant at its own feerate), or as a single chunk at the descendant's - // ancestor feerate. - - std::vector old_chunks; - // Step 1: build the old diagram. - - // The above clusters are all trivially linearized; - // they have a strict topology of 1 or two connected transactions. - - // OLD: Compute existing chunks from all affected clusters - for (auto txiter : m_to_remove) { - // Does this transaction have descendants? - if (txiter->GetCountWithDescendants() > 1) { - // Consider this tx when we consider the descendant. - continue; - } - // Does this transaction have ancestors? - FeeFrac individual{txiter->GetModifiedFee(), txiter->GetTxSize()}; - if (txiter->GetCountWithAncestors() > 1) { - // We'll add chunks for either the ancestor by itself and this tx - // by itself, or for a combined package. - FeeFrac package{txiter->GetModFeesWithAncestors(), static_cast(txiter->GetSizeWithAncestors())}; - if (individual >> package) { - // The individual feerate is higher than the package, and - // therefore higher than the parent's fee. Chunk these - // together. - old_chunks.emplace_back(package); - } else { - // Add two points, one for the parent and one for this child. - old_chunks.emplace_back(package - individual); - old_chunks.emplace_back(individual); - } - } else { - old_chunks.emplace_back(individual); - } - } - - // No topology restrictions post-chunking; sort - std::sort(old_chunks.begin(), old_chunks.end(), std::greater()); - - std::vector new_chunks; - - /* Step 2: build the NEW diagram - * CON = Conflicts of proposed chunk - * CNK = Proposed chunk - * NEW = OLD - CON + CNK: New diagram includes all chunks in OLD, minus - * the conflicts, plus the proposed chunk - */ - - // OLD - CON: Add any parents of direct conflicts that are not conflicted themselves - for (auto direct_conflict : m_to_remove) { - // If a direct conflict has an ancestor that is not in all_conflicts, - // it can be affected by the replacement of the child. - if (direct_conflict->GetMemPoolParentsConst().size() > 0) { - // Grab the parent. - const CTxMemPoolEntry& parent = direct_conflict->GetMemPoolParentsConst().begin()->get(); - if (!m_to_remove.contains(m_pool->mapTx.iterator_to(parent))) { - // This transaction would be left over, so add to the NEW - // diagram. - new_chunks.emplace_back(parent.GetModifiedFee(), parent.GetTxSize()); - } - } - } - // + CNK: Add the proposed chunk itself - new_chunks.emplace_back(replacement_feerate); - - // No topology restrictions post-chunking; sort - std::sort(new_chunks.begin(), new_chunks.end(), std::greater()); - return std::make_pair(old_chunks, new_chunks); + return m_pool->m_txgraph->GetMainStagingDiagrams(); } CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) @@ -1438,6 +1318,7 @@ CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTran } m_entry_vec.push_back(newit); + return newit; } diff --git a/src/txmempool.h b/src/txmempool.h index 04f2fdd4f5a..1750596c0cf 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -516,6 +516,16 @@ public: */ void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); + size_t GetUniqueClusterCount(const setEntries& iters_conflicting) const EXCLUSIVE_LOCKS_REQUIRED(cs) { + std::vector entries; + entries.reserve(iters_conflicting.size()); + for (auto it : iters_conflicting) { + entries.emplace_back(&*it); + } + Assume(!m_txgraph->IsOversized(TxGraph::Level::MAIN)); + return m_txgraph->CountDistinctClusters(entries, TxGraph::Level::MAIN); + } + /** * Try to calculate all in-mempool ancestors of entry. * (these are all calculated including the tx itself) @@ -710,11 +720,6 @@ public: return m_sequence_number; } - /* Check that all direct conflicts are in a cluster size of two or less. Each - * direct conflict may be in a separate cluster. - */ - std::optional CheckConflictTopology(const setEntries& direct_conflicts); - private: /** Remove a set of transactions from the mempool. * If a transaction is in this set, then all in-mempool descendants must diff --git a/src/validation.cpp b/src/validation.cpp index 50660ca75b7..3e00958f39d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1070,21 +1070,6 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) TxValidationState& state = ws.m_state; CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - // Enforce Rule #6. The replacement transaction must have a higher feerate than its direct conflicts. - // - The motivation for this check is to ensure that the replacement transaction is preferable for - // block-inclusion, compared to what would be removed from the mempool. - // - This logic predates ancestor feerate-based transaction selection, which is why it doesn't - // consider feerates of descendants. - // - Note: Ancestor feerate-based transaction selection has made this comparison insufficient to - // guarantee that this is incentive-compatible for miners, because it is possible for a - // descendant transaction of a direct conflict to pay a higher feerate than the transaction that - // might replace them, under these rules. - if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { - // This fee-related failure is TX_RECONSIDERABLE because validating in a package may change - // the result. - return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, - strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); - } CTxMemPool::setEntries all_conflicts; @@ -1094,20 +1079,13 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) strprintf("too many potential replacements%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } - // Enforce Rule #2. - if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, all_conflicts)}) { - // Sibling eviction is only done for TRUC transactions, which cannot have multiple ancestors. - Assume(!ws.m_sibling_eviction); - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, - strprintf("replacement-adds-unconfirmed%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); - } - // Check if it's economically rational to mine this transaction rather than the ones it // replaces and pays for its own relay fees. Enforce Rules #3 and #4. for (CTxMemPool::txiter it : all_conflicts) { m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } + if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, m_pool.m_opts.incremental_relay_feerate, hash)}) { // Result may change in a package context @@ -1119,6 +1097,19 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) for (auto it : all_conflicts) { m_subpackage.m_changeset->StageRemoval(it); } + + // Run cluster size limit checks and fail if we exceed them. + if (!m_subpackage.m_changeset->CheckMemPoolPolicyLimits()) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-large-cluster", ""); + } + + if (const auto err_string{ImprovesFeerateDiagram(*m_subpackage.m_changeset)}) { + // We checked above for the cluster size limits being respected, so a + // failure here can only be due to an insufficient fee. + Assume(err_string->first == DiagramCheckError::FAILURE); + return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "replacement-failed", err_string->second); + } + return true; } @@ -1205,9 +1196,14 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn strprintf("package feerate %s <= parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString())); } + // Run cluster size limit checks and fail if we exceed them. + if (!m_subpackage.m_changeset->CheckMemPoolPolicyLimits()) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "too-large-cluster", ""); + } + // Check if it's economically rational to mine this package rather than the ones it replaces. - // This takes the place of ReplacementChecks()'s PaysMoreThanConflicts() in the package RBF setting. if (const auto err_tup{ImprovesFeerateDiagram(*m_subpackage.m_changeset)}) { + Assume(err_tup->first == DiagramCheckError::FAILURE); return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: " + err_tup.value().second, ""); } diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 91a79113d67..81ecef540ef 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -13,6 +13,7 @@ from test_framework.messages import ( from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_greater_than, assert_greater_than_or_equal, assert_raises_rpc_error, get_fee, @@ -39,6 +40,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): def run_test(self): self.wallet = MiniWallet(self.nodes[0]) + self.generate(self.nodes[0], 100) self.log.info("Running test simple doublespend...") self.test_simple_doublespend() @@ -58,12 +60,12 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.log.info("Running test new unconfirmed inputs...") self.test_new_unconfirmed_inputs() + self.log.info("Running test new unconfirmed input from low feerate tx...") + self.test_new_unconfirmed_input_with_low_feerate() + 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() - self.log.info("Running test RPC...") self.test_rpc() @@ -114,7 +116,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception due to insufficient fee reject_reason = "insufficient fee" - reject_details = f"{reject_reason}, rejecting replacement {tx.txid_hex}; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB" + reject_details = f"{reject_reason}, rejecting replacement {tx.txid_hex}, not enough additional fees to relay; 0.00 < 0.00000011" res = self.nodes[0].testmempoolaccept(rawtxs=[tx_hex])[0] assert_equal(res["reject-reason"], reject_reason) assert_equal(res["reject-details"], reject_details) @@ -265,7 +267,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): )["hex"] # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0) + assert_raises_rpc_error(-26, "does not improve feerate diagram", self.nodes[0].sendrawtransaction, tx1b_hex, 0) def test_spends_of_conflicting_outputs(self): """Replacements that spend conflicting tx outputs are rejected""" @@ -315,9 +317,9 @@ class ReplaceByFeeTest(BitcoinTestFramework): assert_raises_rpc_error(-26, "bad-txns-spends-conflicting-tx", self.nodes[0].sendrawtransaction, tx2_hex, 0) def test_new_unconfirmed_inputs(self): - """Replacements that add new unconfirmed inputs are rejected""" + """Replacements that add new unconfirmed inputs may be accepted""" confirmed_utxo = self.make_utxo(self.nodes[0], int(1.1 * COIN)) - unconfirmed_utxo = self.make_utxo(self.nodes[0], int(0.1 * COIN), confirmed=False) + unconfirmed_utxo = self.make_utxo(self.nodes[0], int(0.2 * COIN), confirmed=False) self.wallet.send_self_transfer( from_node=self.nodes[0], @@ -333,19 +335,30 @@ class ReplaceByFeeTest(BitcoinTestFramework): )["tx"] tx2_hex = tx2.serialize().hex() - # This will raise an exception - reject_reason = "replacement-adds-unconfirmed" - reject_details = f"{reject_reason}, replacement {tx2.txid_hex} adds unconfirmed input, idx 1" - res = self.nodes[0].testmempoolaccept(rawtxs=[tx2_hex])[0] - assert_equal(res["reject-reason"], reject_reason) - assert_equal(res["reject-details"], reject_details) - assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx2_hex, 0) + # This will not raise an exception + tx2_id = self.nodes[0].sendrawtransaction(tx2_hex, 0) + assert tx2_id in self.nodes[0].getrawmempool() + + def test_new_unconfirmed_input_with_low_feerate(self): + """Replacements that add new unconfirmed inputs are allowed, but must pass the feerate diagram check""" + confirmed_utxos = [self.make_utxo(self.nodes[0], int(1.1 * COIN)) for _ in range(3)] + large_low_feerate = self.wallet.create_self_transfer(utxo_to_spend=confirmed_utxos[0], target_vsize=10000, fee=Decimal("0.00001000")) + self.nodes[0].sendrawtransaction(large_low_feerate['hex']) + unconfirmed_utxo = large_low_feerate['new_utxo'] + + # These two transactions are approximately the same size. The replacement tx pays twice the fee. + tx_to_replace = self.wallet.create_self_transfer_multi(utxos_to_spend=[confirmed_utxos[1], confirmed_utxos[2]], fee_per_output=2000) + tx_replacement = self.wallet.create_self_transfer_multi(utxos_to_spend=[confirmed_utxos[1], unconfirmed_utxo], fee_per_output=4000) + assert_greater_than(tx_replacement['fee']*tx_to_replace['tx'].get_vsize(), tx_to_replace['fee']*tx_replacement['tx'].get_vsize()) + + self.nodes[0].sendrawtransaction(tx_to_replace['hex']) + assert_raises_rpc_error(-26, "insufficient feerate: does not improve feerate diagram", self.nodes[0].sendrawtransaction, tx_replacement['hex']) def test_too_many_replacements(self): - """Replacements that evict too many transactions are rejected""" - # Try directly replacing more than MAX_REPLACEMENT_LIMIT - # transactions + """Replacements that conflict with too many clusters are rejected""" + # Try directly replacing transactions in more than MAX_REPLACEMENT_LIMIT + # distinct clusters # Start by creating a single transaction with many outputs initial_nValue = 10 * COIN @@ -385,7 +398,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception reject_reason = "too many potential replacements" - reject_details = f"{reject_reason}, rejecting replacement {double_tx.txid_hex}; too many potential replacements ({MAX_REPLACEMENT_LIMIT + 1} > {MAX_REPLACEMENT_LIMIT})" + reject_details = f"{reject_reason}, rejecting replacement {double_tx.txid_hex}; too many conflicting clusters ({MAX_REPLACEMENT_LIMIT + 1} > {MAX_REPLACEMENT_LIMIT})" res = self.nodes[0].testmempoolaccept(rawtxs=[double_tx_hex])[0] assert_equal(res["reject-reason"], reject_reason) assert_equal(res["reject-details"], reject_details) @@ -397,89 +410,6 @@ class ReplaceByFeeTest(BitcoinTestFramework): double_tx_hex = double_tx.serialize().hex() self.nodes[0].sendrawtransaction(double_tx_hex, 0) - def test_too_many_replacements_with_default_mempool_params(self): - """ - Test rule 5 (do not allow replacements that cause more than 100 - evictions) without having to rely on non-default mempool parameters. - - In order to do this, create a number of "root" UTXOs, and then hang - enough transactions off of each root UTXO to exceed the MAX_REPLACEMENT_LIMIT. - Then create a conflicting RBF replacement transaction. - """ - # Clear mempools to avoid cross-node sync failure. - for node in self.nodes: - self.generate(node, 1) - normal_node = self.nodes[1] - wallet = MiniWallet(normal_node) - - # This has to be chosen so that the total number of transactions can exceed - # MAX_REPLACEMENT_LIMIT without having any one tx graph run into the descendant - # limit; 10 works. - num_tx_graphs = 10 - - # (Number of transactions per graph, rule 5 failure expected) - cases = [ - # Test the base case of evicting fewer than MAX_REPLACEMENT_LIMIT - # transactions. - ((MAX_REPLACEMENT_LIMIT // num_tx_graphs) - 1, False), - - # Test hitting the rule 5 eviction limit. - (MAX_REPLACEMENT_LIMIT // num_tx_graphs, True), - ] - - for (txs_per_graph, failure_expected) in cases: - self.log.debug(f"txs_per_graph: {txs_per_graph}, failure: {failure_expected}") - # "Root" utxos of each txn graph that we will attempt to double-spend with - # an RBF replacement. - root_utxos = [] - - # For each root UTXO, create a package that contains the spend of that - # UTXO and `txs_per_graph` children tx. - for graph_num in range(num_tx_graphs): - root_utxos.append(wallet.get_utxo()) - - parent_tx = wallet.send_self_transfer_multi( - from_node=normal_node, - utxos_to_spend=[root_utxos[graph_num]], - num_outputs=txs_per_graph, - ) - new_utxos = parent_tx['new_utxos'] - - for utxo in new_utxos: - # Create spends for each output from the "root" of this graph. - child_tx = wallet.send_self_transfer( - from_node=normal_node, - utxo_to_spend=utxo, - ) - - assert normal_node.getmempoolentry(child_tx['txid']) - - num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph) - - if failure_expected: - assert num_txs_invalidated > MAX_REPLACEMENT_LIMIT - else: - assert num_txs_invalidated <= MAX_REPLACEMENT_LIMIT - - # Now attempt to submit a tx that double-spends all the root tx inputs, which - # would invalidate `num_txs_invalidated` transactions. - tx_hex = wallet.create_self_transfer_multi( - utxos_to_spend=root_utxos, - fee_per_output=10_000_000, # absurdly high feerate - )["hex"] - - if failure_expected: - assert_raises_rpc_error( - -26, "too many potential replacements", normal_node.sendrawtransaction, tx_hex, 0) - else: - txid = normal_node.sendrawtransaction(tx_hex, 0) - assert normal_node.getmempoolentry(txid) - - # Clear the mempool once finished, and rescan the other nodes' wallet - # to account for the spends we've made on `normal_node`. - self.generate(normal_node, 1) - self.wallet.rescan_utxos() - def test_prioritised_transactions(self): # Ensure that fee deltas used via prioritisetransaction are # correctly used by replacement logic @@ -503,7 +433,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): )["hex"] # Verify tx1b cannot replace tx1a. - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0) + assert_raises_rpc_error(-26, "does not improve feerate diagram", self.nodes[0].sendrawtransaction, tx1b_hex, 0) # Use prioritisetransaction to set tx1a's fee to 0. self.nodes[0].prioritisetransaction(txid=tx1a_txid, fee_delta=int(-0.1 * COIN)) diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py index 712176e9348..a0308da72ac 100755 --- a/test/functional/mempool_ephemeral_dust.py +++ b/test/functional/mempool_ephemeral_dust.py @@ -419,8 +419,7 @@ class EphemeralDustTest(BitcoinTestFramework): res = self.nodes[0].submitpackage([dusty_tx["hex"] for dusty_tx in dusty_txs] + [insufficient_sweep_tx["hex"]]) assert_equal(res['package_msg'], "transaction failed") - #assert_equal(res['tx-results'][insufficient_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {insufficient_sweep_tx['txid']} (wtxid={insufficient_sweep_tx['wtxid']}) did not spend parent's ephemeral dust") - assert_equal(res['tx-results'][insufficient_sweep_tx["wtxid"]]["error"], f"replacement-adds-unconfirmed, replacement {insufficient_sweep_tx['txid']} adds unconfirmed input, idx 46") + assert_equal(res['tx-results'][insufficient_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {insufficient_sweep_tx['txid']} (wtxid={insufficient_sweep_tx['wtxid']}) did not spend parent's ephemeral dust") assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [sweep_all_but_one_tx["tx"]]) # Cycle out the partial sweep to avoid triggering package RBF behavior which limits package to no in-mempool ancestors diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py index 3eb0abd37e3..759e3cb07d3 100755 --- a/test/functional/mempool_package_rbf.py +++ b/test/functional/mempool_package_rbf.py @@ -89,9 +89,6 @@ class PackageRBFTest(BitcoinTestFramework): self.test_too_numerous_ancestors() self.test_package_rbf_with_wrong_pkg_size() self.test_insufficient_feerate() - self.test_wrong_conflict_cluster_size_linear() - self.test_wrong_conflict_cluster_size_parents_child() - self.test_wrong_conflict_cluster_size_parent_children() self.test_0fee_package_rbf() self.test_child_conflicts_parent_mempool_ancestor() @@ -193,12 +190,12 @@ class PackageRBFTest(BitcoinTestFramework): def test_package_rbf_max_conflicts(self): node = self.nodes[0] - self.log.info("Check Package RBF cannot replace more than MAX_REPLACEMENT_CANDIDATES transactions") - num_coins = 51 + self.log.info("Check Package RBF cannot conflict with more than MAX_REPLACEMENT_CANDIDATES clusters") + num_coins = 101 parent_coins = self.coins[:num_coins] del self.coins[:num_coins] - # Original transactions: 51 transactions with 1 descendants each -> 102 total transactions + # Original transactions: 101 transactions with 1 descendants each -> 202 total transactions, 101 clusters size_two_clusters = [] for coin in parent_coins: size_two_clusters.append(self.wallet.send_self_transfer_chain(from_node=node, chain_length=2, utxo_to_spend=coin)) @@ -213,12 +210,13 @@ class PackageRBFTest(BitcoinTestFramework): parent_fee_per_conflict = 10000 child_feerate = 10000 * DEFAULT_FEE - # Conflict against all transactions by double-spending each parent, causing 102 evictions + # Conflict against all transactions by double-spending each parent, causing 101 cluster conflicts package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_coins, fee_per_output=parent_fee_per_conflict) package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].txid_hex}; too many potential replacements (102 > 100)", pkg_results["package_msg"]) + assert_equal("transaction failed", pkg_results["package_msg"]) + assert_equal(f"too many potential replacements, rejecting replacement {package_parent['txid']}; too many conflicting clusters (101 > 100)", pkg_results["tx-results"][package_parent["wtxid"]]["error"]) self.assert_mempool_contents(expected=expected_txns) # Make singleton tx to conflict with in next batch @@ -227,16 +225,17 @@ class PackageRBFTest(BitcoinTestFramework): node.sendrawtransaction(singleton_tx["hex"]) expected_txns.append(singleton_tx["tx"]) - # Double-spend same set minus last, and double-spend singleton. This hits 101 evictions; should still fail. + # Double-spend same set minus last, and double-spend singleton. This is still too many conflicted clusters. # N.B. we can't RBF just a child tx in the clusters, as that would make resulting cluster of size 3. double_spending_coins = parent_coins[:-1] + [singleton_coin] package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=double_spending_coins, fee_per_output=parent_fee_per_conflict) package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].txid_hex}; too many potential replacements (101 > 100)", pkg_results["package_msg"]) + assert_equal("transaction failed", pkg_results["package_msg"]) + assert_equal(f"too many potential replacements, rejecting replacement {package_parent['txid']}; too many conflicting clusters (101 > 100)", pkg_results["tx-results"][package_parent["wtxid"]]["error"]) self.assert_mempool_contents(expected=expected_txns) - # Finally, evict MAX_REPLACEMENT_CANDIDATES + # Finally, conflict with MAX_REPLACEMENT_CANDIDATES clusters package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_coins[:-1], fee_per_output=parent_fee_per_conflict) package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) @@ -290,167 +289,6 @@ class PackageRBFTest(BitcoinTestFramework): self.assert_mempool_contents(expected=package_txns1 + package_txns2_succeed) self.generate(node, 1) - def test_wrong_conflict_cluster_size_linear(self): - self.log.info("Test that conflicting with a cluster not sized two is rejected: linear chain") - node = self.nodes[0] - - # Coins we will conflict with - coin1 = self.coins.pop() - coin2 = self.coins.pop() - coin3 = self.coins.pop() - - # Three transactions chained; package RBF against any of these - # should be rejected - self.ctr += 1 - parent_result = self.wallet.create_self_transfer( - fee=DEFAULT_FEE, - utxo_to_spend=coin1, - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - child_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[parent_result["new_utxo"], coin2], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - grandchild_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[child_result["new_utxos"][0], coin3], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - expected_txns = [parent_result["tx"], child_result["tx"], grandchild_result["tx"]] - for tx in expected_txns: - node.sendrawtransaction(tx.serialize().hex()) - self.assert_mempool_contents(expected=expected_txns) - - # Now make conflicting packages for each coin - package_hex1, _package_txns1 = self.create_simple_package(coin1, DEFAULT_FEE, DEFAULT_CHILD_FEE) - - package_result = node.submitpackage(package_hex1) - assert_equal(f"package RBF failed: {parent_result['tx'].txid_hex} has 2 descendants, max 1 allowed", package_result["package_msg"]) - - package_hex2, _package_txns2 = self.create_simple_package(coin2, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex2) - assert_equal(f"package RBF failed: {child_result['tx'].txid_hex} has both ancestor and descendant, exceeding cluster limit of 2", package_result["package_msg"]) - - package_hex3, _package_txns3 = self.create_simple_package(coin3, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex3) - assert_equal(f"package RBF failed: {grandchild_result['tx'].txid_hex} has 2 ancestors, max 1 allowed", package_result["package_msg"]) - - # Check that replacements were actually rejected - self.assert_mempool_contents(expected=expected_txns) - self.generate(node, 1) - - def test_wrong_conflict_cluster_size_parents_child(self): - self.log.info("Test that conflicting with a cluster not sized two is rejected: two parents one child") - node = self.nodes[0] - - # Coins we will conflict with - coin1 = self.coins.pop() - coin2 = self.coins.pop() - coin3 = self.coins.pop() - - self.ctr += 1 - parent1_result = self.wallet.create_self_transfer( - fee=DEFAULT_FEE, - utxo_to_spend=coin1, - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - parent2_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[coin2], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - child_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[parent1_result["new_utxo"], parent2_result["new_utxos"][0], coin3], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - expected_txns = [parent1_result["tx"], parent2_result["tx"], child_result["tx"]] - for tx in expected_txns: - node.sendrawtransaction(tx.serialize().hex()) - self.assert_mempool_contents(expected=expected_txns) - - # Now make conflicting packages for each coin - package_hex1, _package_txns1 = self.create_simple_package(coin1, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex1) - assert_equal(f"package RBF failed: {parent1_result['tx'].txid_hex} is not the only parent of child {child_result['tx'].txid_hex}", package_result["package_msg"]) - - package_hex2, _package_txns2 = self.create_simple_package(coin2, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex2) - assert_equal(f"package RBF failed: {child_result['tx'].txid_hex} has 2 ancestors, max 1 allowed", package_result["package_msg"]) - - package_hex3, _package_txns3 = self.create_simple_package(coin3, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex3) - assert_equal(f"package RBF failed: {child_result['tx'].txid_hex} has 2 ancestors, max 1 allowed", package_result["package_msg"]) - - # Check that replacements were actually rejected - self.assert_mempool_contents(expected=expected_txns) - self.generate(node, 1) - - def test_wrong_conflict_cluster_size_parent_children(self): - self.log.info("Test that conflicting with a cluster not sized two is rejected: one parent two children") - node = self.nodes[0] - - # Coins we will conflict with - coin1 = self.coins.pop() - coin2 = self.coins.pop() - coin3 = self.coins.pop() - - self.ctr += 1 - parent_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - num_outputs=2, - utxos_to_spend=[coin1], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - child1_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[parent_result["new_utxos"][0], coin2], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - self.ctr += 1 - child2_result = self.wallet.create_self_transfer_multi( - fee_per_output=int(DEFAULT_FEE * COIN), - utxos_to_spend=[parent_result["new_utxos"][1], coin3], - sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr, - ) - - # Submit them to mempool - expected_txns = [parent_result["tx"], child1_result["tx"], child2_result["tx"]] - for tx in expected_txns: - node.sendrawtransaction(tx.serialize().hex()) - self.assert_mempool_contents(expected=expected_txns) - - # Now make conflicting packages for each coin - package_hex1, _package_txns1 = self.create_simple_package(coin1, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex1) - assert_equal(f"package RBF failed: {child1_result['tx'].txid_hex} is not the only child of parent {parent_result['tx'].txid_hex}", package_result["package_msg"]) - - package_hex2, _package_txns2 = self.create_simple_package(coin2, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex2) - assert_equal(f"package RBF failed: {child1_result['tx'].txid_hex} is not the only child of parent {parent_result['tx'].txid_hex}", package_result["package_msg"]) - - package_hex3, _package_txns3 = self.create_simple_package(coin3, DEFAULT_FEE, DEFAULT_CHILD_FEE) - package_result = node.submitpackage(package_hex3) - assert_equal(f"package RBF failed: {child2_result['tx'].txid_hex} is not the only child of parent {parent_result['tx'].txid_hex}", package_result["package_msg"]) - - # Check that replacements were actually rejected - self.assert_mempool_contents(expected=expected_txns) - self.generate(node, 1) - def test_package_rbf_with_wrong_pkg_size(self): self.log.info("Test that package RBF doesn't work with packages larger than 2 due to pkg size") node = self.nodes[0] diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py index 098631b5c41..23726330cc7 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -513,7 +513,7 @@ class MempoolTRUC(BitcoinTestFramework): tx_v3_child_2_rule6 = self.wallet.create_self_transfer( utxo_to_spend=tx_v3_parent["new_utxos"][1], fee_rate=DEFAULT_FEE, version=3 ) - rule6_str = f"insufficient fee (including sibling eviction), rejecting replacement {tx_v3_child_2_rule6['txid']}; new feerate" + rule6_str = f"insufficient fee (including sibling eviction), rejecting replacement {tx_v3_child_2_rule6['txid']}" assert_raises_rpc_error(-26, rule6_str, node.sendrawtransaction, tx_v3_child_2_rule6["hex"]) self.check_mempool([tx_v3_parent['txid'], tx_v3_child_1['txid']]) @@ -544,11 +544,6 @@ class MempoolTRUC(BitcoinTestFramework): tx_v3_replacement_only = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_for_conflict, fee_per_output=4000000) # Override maxfeerate - it costs a lot to replace these 100 transactions. assert node.testmempoolaccept([tx_v3_replacement_only["hex"]], maxfeerate=0)[0]["allowed"] - # Adding another one exceeds the limit. - utxos_for_conflict.append(tx_v3_parent["new_utxos"][1]) - tx_v3_child_2_rule5 = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_for_conflict, fee_per_output=4000000, version=3) - rule5_str = f"too many potential replacements (including sibling eviction), rejecting replacement {tx_v3_child_2_rule5['txid']}; too many potential replacements (101 > 100)" - assert_raises_rpc_error(-26, rule5_str, node.sendrawtransaction, tx_v3_child_2_rule5["hex"]) self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_1["txid"]]) self.log.info("Test sibling eviction is successful if it meets all RBF rules") @@ -566,7 +561,7 @@ class MempoolTRUC(BitcoinTestFramework): fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"]*COIN)) tx_v3_child_3 = self.wallet.create_self_transfer_multi( - utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*2, version=3 + utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*4, version=3 ) node.sendrawtransaction(tx_v3_child_3["hex"]) self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]])