← Watch feed
Implement new RBF logic for cluster mempool
Public commit record
80/100 · Strong
What the developer wrote
Authored by Suhas Daftuar
Implement new RBF logic for cluster mempool
With a total ordering on mempool transactions, we are now able to calculate a
transaction's mining score at all times. Use this to improve the RBF logic:
- we no longer enforce a "no new unconfirmed parents" rule
- we now require that the mempool's feerate diagram must improve in order
to accept a replacement
- the topology restrictions for conflicts in the package rbf setting have been
eliminated
Revert the temporary change to mempool_ephemeral_dust.py that were previously
made due to RBF validation checks being reordered.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>, glozow <gloriajzhao@gmail.com>
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit rewrites how Bitcoin Core decides whether a transaction can replace existing mempool transactions via RBF (Replace-By-Fee). It removes two old rules—'no new unconfirmed inputs' and 'replacement must pay a higher per-transaction feerate than each conflict'—and replaces them with a stricter economic test: the replacement must improve the mempool's overall feerate diagram. It also changes the limit on how many transactions can be replaced from a raw count of transactions to a count of distinct transaction clusters. The change is a deliberate policy redesign, not a bug fix, and the tests are updated to match the new behavior.
Recommended action
Treat as a planned consensus-adjacent mempool policy change. Review the new feerate-diagram comparison and cluster-count limit for edge cases such as oversized clusters, prioritization deltas, and package RBF with ephemeral dust. Run the updated functional and fuzz tests. No immediate emergency response is indicated, but node operators and wallet developers should be aware that RBF acceptance semantics have changed.
Security signals we found
01
RBF policy rules relaxed: new unconfirmed inputs now allowed
02
RBF policy rules tightened: replacement must improve mempool feerate diagram
03
Replacement candidate limit changed from transaction count to cluster count
04
Removal of per-conflict feerate comparison (Rule #6)
05
Removal of topology restrictions for package RBF conflicts
06
New dependency on cluster mempool total ordering and TxGraph diagram calculation
Evidence from the diff
The patch implements cluster-mempool-based RBF logic. Key code changes: (1) Removes PaysMoreThanConflicts() and HasNoNewUnconfirmed() helpers from src/policy/rbf.cpp/h and their callers in src/validation.cpp. (2) Replaces the old transaction-count replacement cap with a cluster-count cap via new CTxMemPool::GetUniqueClusterCount() using TxGraph::CountDistinctClusters. (3) Replaces the hand-rolled CheckConflictTopology()/CalculateChunksForRBF() in CTxMemPool::ChangeSet with a call to m_txgraph->GetMainStagingDiagrams() after policy-limit checks. (4) Adds ImprovesFeerateDiagram() checks in both single-tx ReplacementChecks() and package RBF PackageMempoolChecks(). (5) Updates extensive unit and functional tests to expect the new error strings and acceptance behavior.
Changed components
src/policy/rbf.cppsrc/policy/rbf.hsrc/txmempool.cppsrc/txmempool.hsrc/validation.cppsrc/test/fuzz/rbf.cppsrc/test/rbf_tests.cpptest/functional/feature_rbf.pytest/functional/mempool_ephemeral_dust.pytest/functional/mempool_package_rbf.pytest/functional/mempool_truc.pyInspect captured patch +190 / −766
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index 210b9491..6852cd52 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -62,59 +62,27 @@ std::optional<std::string> 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<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
- const CTxMemPool& pool,
- const CTxMemPool::setEntries& iters_conflicting)
-{
- AssertLockHeld(pool.cs);
- std::set<Txid> 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<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
const std::set<Txid>& direct_conflicts,
const Txid& txid)
@@ -130,32 +98,6 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
return std::nullopt;
}
-std::optional<std::string> 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<std::string> PaysForRBF(CAmount original_fees,
CAmount replacement_fees,
size_t replacement_vsize,
diff --git a/src/policy/rbf.h b/src/policy/rbf.h
index cc397b46..b135ecf2 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<std::string> 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<std::string> 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<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
const std::set<Txid>& 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<std::string> 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 300f31d4..2ca57341 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 31cf994c..45dd8bcb 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<CTransactionRef>& 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<CTransactionRef, CTransactionRef> make_two_siblings(const CTransactionRef parent,
- const std::vector<CAmount>& 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<CTransactionRef>& 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<CTransactionRef, CTransactionRef> 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);
+}
+
+BOOST_FIXTURE_TEST_CASE(rbf_conflicts_calculator, TestChain100Setup)
+{
+ CTxMemPool& pool = *Assert(m_node.mempool);
+ LOCK2(::cs_main, pool.cs);
+ TestMemPoolEntryHelper entry;
+
+ const CAmount normal_fee{CENT/10};
+
+ // Create two parent transactions with 51 outputs each
+ const int NUM_OUTPUTS = 51;
+ std::vector<CAmount> output_values;
+ output_values.reserve(NUM_OUTPUTS);
+ for (int i = 0; i < NUM_OUTPUTS; ++i) {
+ output_values.push_back(1 * COIN);
+ }
- // 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<CTransactionRef> 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(),
+ 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));
+
+ std::vector<CTransactionRef> direct_children;
+
+ // 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));
+ }
+ }
+
+ // At this point, we should have 2 clusters in the mempool, each with 52
+ // transactions.
+
+ // 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=*/ all_parents,
+ /*iters_conflicting=*/ {parent_entry_1, parent_entry_2},
/*all_conflicts=*/ all_conflicts) == std::nullopt);
- BOOST_CHECK(all_conflicts == all_entries);
- auto conflicts_size = all_conflicts.size();
- all_conflicts.clear();
-
- 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();
-
- 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));
+
+ 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();
+
+ // If we mine the parent_tx's, then the clusters split (102 clusters).
+ pool.removeForBlock({parent_tx_1, parent_tx_2}, /* dummy */ 1);
+
+ // 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);
}
- 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 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);
-
- // Tests for CheckConflictTopology
-
- // 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()));
-
- // No descendants yet
- BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained}) == std::nullopt);
-
- // Add 1 descendant, still ok
- add_descendants(tx9, 1, pool);
- BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained}) == std::nullopt);
-
- // N direct conflicts; ok
- BOOST_CHECK(pool.CheckConflictTopology({entry9_unchained, entry10_unchained, entry11_unchained}) == std::nullopt);
-
- // 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);
-
- // 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<FeeFrac> expected_old_chunks{{low_fee, low_size}};
BOOST_CHECK(replace_one->first == expected_old_chunks);
- std::vector<FeeFrac> expected_new_chunks{{0, int32_t(entry_replacement.GetTxSize())}};
+ std::vector<FeeFrac> 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<FeeFrac> expected_old_diagram{{low_fee, low_size}};
BOOST_CHECK(replace_one_fee->first == expected_old_diagram);
- std::vector<FeeFrac> expected_new_diagram{{high_fee, low_size}};
+ std::vector<FeeFrac> 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<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}};
BOOST_CHECK(replace_single_chunk->first == expected_old_chunks);
- std::vector<FeeFrac> expected_new_chunks{{high_fee, low_size}};
+ std::vector<FeeFrac> 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<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}};
BOOST_CHECK(replace_cpfp_child->first == expected_old_chunks);
- std::vector<FeeFrac> expected_new_chunks{{high_fee, low_size}, {low_fee, low_size}};
+ std::vector<FeeFrac> 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 672198ec..15b25081 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -1287,135 +1287,15 @@ std::vector<CTxMemPool::txiter> CTxMemPool::GatherClusters(const std::vector<Txi
return clustered_txs;
}
-std::optional<std::string> 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::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool::ChangeSet::CalculateChunksForRBF()
{
LOCK(m_pool->cs);
- FeeFrac replacement_feerate{0, 0};
- for (auto it : m_entry_vec) {
- replacement_feerate += {it->GetModifiedFee(), it->GetTxSize()};
- }
- 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())};
+ if (!CheckMemPoolPolicyLimits()) {
+ return util::Error{Untranslated("cluster size limit exceeded")};
}
- // 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<FeeFrac> 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<int32_t>(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<FeeFrac> 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 04f2fdd4..1750596c 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -516,6 +516,16 @@ public:
*/
void UpdateTransactionsFromBlock(const std::vector<Txid>& 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<const TxGraph::Ref *> 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<std::string> 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 50660ca7..3e00958f 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<CTransactionRef>& 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 91a79113..81ecef54 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 712176e9..a0308da7 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 3eb0abd3..759e3cb0 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 098631b5..23726330 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"]])
Risk score
Our methodology →Why this scored 35/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.