Test each RBF rule more thoroughly and in isolation so we’re not relying on things like overall mempool acceptance logic, ordering of mempool checks, RPC results, etc.
RBF was pretty recently refactored out, so there isn’t much unit test coverage. From https://marcofalke.github.io/btc_cov/test_bitcoin.coverage/src/policy/rbf.cpp.gcov.html:
Concept ACK and code looks good on first read.
0test/rbf_tests.cpp:47:14: error: calling function 'addUnchecked' requires holding mutex 'pool.cs' exclusively [-Werror,-Wthread-safety-analysis]
1 pool.addUnchecked(entry.FromTx(next_tx));
2 ^
3test/rbf_tests.cpp:47:14: error: calling function 'addUnchecked' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
83+ // Two independent high-feerate transactions, tx7 and tx8
84+ CTransactionRef tx7 = make_tx(/*output_values=*/ {999 * CENT}, /*inputs=*/ {m_coinbase_txns[3]});
85+ pool.addUnchecked(entry.Fee(high_fee).FromTx(tx7));
86+ CTransactionRef tx8 = make_tx(/*output_values=*/ {999 * CENT}, /*inputs=*/ {m_coinbase_txns[4]});
87+ pool.addUnchecked(entry.Fee(high_fee).FromTx(tx8));
88+ pool.PrioritiseTransaction(tx6->GetHash(), 1 * COIN);
130+
131+ // Tests for EntriesAndTxidsDisjoint
132+ BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {tx1->GetHash()}, unused_txid) == std::nullopt);
133+ BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx3->GetHash()}, unused_txid) == std::nullopt);
134+ // EntriesAndTxidsDisjoint uses txids, not wtxids.
135+ BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt);
0 BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt);
1 BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetHash()}, unused_txid).has_value());
rules out the case where tx2 has no witness data(same wtxid) but some other bug.,.
139+ BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value());
140+ BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx1->GetHash()}, unused_txid) == std::nullopt);
141+
142+ // Tests for PaysForRBF
143+ const auto incremental_relay_feerate{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE)};
144+ const auto higher_relay_feerate{CFeeRate(2, 1)};
0 const auto higher_relay_feerate{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE * 2};
in case the incremental rate ever changes out from underneath the test
144+ const auto higher_relay_feerate{CFeeRate(2, 1)};
145+ // Must pay at least as much as the original.
146+ BOOST_CHECK(PaysForRBF(/*original_fees=*/high_fee,
147+ /*replacement_fees=*/high_fee,
148+ /*replacement_vsize=*/1,
149+ /*relay_fee=*/CFeeRate(0),
Not really a part of this test, but I find the name relay_fee and the header description very misleading.
0* [@param](/bitcoin-bitcoin/contributor/param/)[in] relay_fee The node's minimum feerate for transaction relay.
This is whatever ::incrementalRelayFee is during runtime, which is explicitly not that? i.e. mempool minfee could be much higher, yes?
Yeah looking back I should have named it incremental_relay_feerate, and that description is not helpful… oops
This should always be called with the incremental relay feerate. The reason it’s parameterizable instead of having the function use ::incrementalRelayFee is to avoid using globals within this module (see #22675 (review)).
170+ /*iters_conflicting=*/ all_parents,
171+ /*all_conflicts=*/ all_conflicts) == std::nullopt);
172+ BOOST_CHECK(all_conflicts == all_entries);
173+ all_conflicts.clear();
174+
175+ add_descendants(tx2, pool, 23);
crACK 645c593a6027e7939c00d70f185ccbd0f381b7b5
Just a few improvements suggested
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
119+ // These tests use feerate, not absolute fee.
120+ BOOST_CHECK(PaysMoreThanConflicts(/*iters_conflicting=*/set_12_normal,
121+ /*replacement_feerate=*/CFeeRate(entry1->GetModifiedFee() + 1, entry1->GetTxSize() + 2),
122+ /*txid=*/unused_txid).has_value());
123+ // Replacement must be strictly greater than the originals.
124+ BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1->GetModifiedFee(), entry1->GetTxSize()), unused_txid).has_value());
CFeeRate(entry1->GetModifiedFee() - 1, entry1->GetTxSize()). In case of accidental refactoring of PaysMoreThanConflitcts and the <= is dropped to = it should catch it.
Changing the check in PaysMoreThanConflict to anything else than <= (ie <, >, >=, ==) would fail the test already.
Changing to = would not compile.
126+ // These tests use modified fees (including prioritisation), not base fees.
127+ BOOST_CHECK(PaysMoreThanConflicts({entry5}, CFeeRate(entry5->GetModifiedFee() + 1, entry5->GetTxSize()), unused_txid) == std::nullopt);
128+ BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetFee() + 1, entry6->GetTxSize()), unused_txid).has_value());
129+ BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetModifiedFee() + 1, entry6->GetTxSize()), unused_txid) == std::nullopt);
130+ // These tests only check individual feerate. Ancestor feerate does not matter.
131+ BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4->GetModifiedFee(), entry4->GetTxSize()), unused_txid).has_value());
entry8 ?
The opposite, actually. We’re showing that, if individual feerate is the same, the original is preferred even though it has a lower ancestor feerate. I don’t think a high-feerate parent is particularly interesting, since they’d be mined without the child (which is why we usually use min(ancestor, individual) feerate).
Also note that entry8 doesn’t have any parents.
138+ BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetHash()}, unused_txid).has_value());
139+ // If entry2 is an ancestor of a tx, that tx cannot replace entry1. However,
140+ // EntriesAndTxidsDisjoint uses the ancestors directly. It does not calculate descendants.
141+ BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx1->GetHash()}, unused_txid).has_value());
142+ BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value());
143+ BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx1->GetHash()}, unused_txid) == std::nullopt);
entry1 should be part of the replacement transaction’s ancestors so this topology should fail in a normal evaluation in PreChecks ? I’m correct can be nice to document.
157+ // Additional fees must cover the replacement's vsize at incremental relay fee
158+ BOOST_CHECK(PaysForRBF(high_fee, high_fee + 1, 2, incremental_relay_feerate, unused_txid).has_value());
159+ BOOST_CHECK(PaysForRBF(high_fee, high_fee + 2, 2, incremental_relay_feerate, unused_txid) == std::nullopt);
160+ BOOST_CHECK(PaysForRBF(high_fee, high_fee + 2, 2, higher_relay_feerate, unused_txid).has_value());
161+ BOOST_CHECK(PaysForRBF(high_fee, high_fee + 4, 2, higher_relay_feerate, unused_txid) == std::nullopt);
162+ BOOST_CHECK(PaysForRBF(low_fee, high_fee, 99999999, incremental_relay_feerate, unused_txid).has_value());
additional_fees are so high, even with a big transaction that it can succeed PaysForRBF, to verify that huge size in itself isn’t a source of error ?
201+ BOOST_CHECK_EQUAL(all_conflicts.size(), 100);
202+ all_conflicts.clear();
203+
204+ // Exceeds maximum number of conflicts.
205+ add_descendants(tx8, pool, 1);
206+ BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts).has_value());
FetchConflictingEntriesAndCheckMaxReplacement ? (can be in other commit/PR).
217+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2}) == std::nullopt);
218+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, empty_set).has_value());
219+
220+ CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
221+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2}).has_value());
222+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value());
IsRBFOptIn remaining in rbf.cpp deserving unit test coverage.
203+
204+ // Exceeds maximum number of conflicts.
205+ add_descendants(tx8, pool, 1);
206+ BOOST_CHECK(GetEntriesForConflicts(*conflicts_with_parents.get(), pool, all_parents, all_conflicts).has_value());
207+
208+ // Tests for HasNoNewUnconfirmed
Tests pass even if the check for mempool existence in HasNoNewUnconfirmed removed:
0diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
1index e25f5c7c5b..f1d8301ab6 100644
2--- a/src/policy/rbf.cpp
3+++ b/src/policy/rbf.cpp
4@@ -106,10 +106,10 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
5 if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
6 // Rather than check the UTXO set - potentially expensive - it's cheaper to just check
7 // if the new input refers to a tx that's in the mempool.
8- if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
9+ //if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
10 return strprintf("replacement %s adds unconfirmed input, idx %d",
11 tx.GetHash().ToString(), j);
12- }
13+ //}
14 }
15 }
16 return std::nullopt;
116@@ -117,6 +117,7 @@ BITCOIN_TESTS =\
117 test/prevector_tests.cpp \
118 test/raii_event_tests.cpp \
119 test/random_tests.cpp \
120+ test/rbf_tests.cpp \
ci/test/06_script_b.sh and update the headers with any relevant suggestions in the tidy CI output.
35+ tx.vout[i].nValue = output_values[i];
36+ }
37+ return MakeTransactionRef(tx);
38+}
39+
40+void add_descendants(const CTransactionRef& tx, CTxMemPool& pool, int32_t num_descendants)
0static void add_descendants(const CTransactionRef& tx, CTxMemPool& pool, int32_t num_descendants)
params order; from our developer notes:
When ordering function parameters, place input parameters first, then any in-out parameters, followed by any output parameters.
36+ }
37+ return MakeTransactionRef(tx);
38+}
39+
40+void add_descendants(const CTransactionRef& tx, CTxMemPool& pool, int32_t num_descendants)
41+ EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
In 3 places s/cs_main/::cs_main/ (the absolute reference to cs_main is preferred for new code)
0 EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs)
14+#include <optional>
15+#include <vector>
16+
17+BOOST_FIXTURE_TEST_SUITE(rbf_tests, TestingSetup)
18+
19+inline CTransactionRef make_tx(const std::vector<CAmount>& output_values,
0static inline CTransactionRef make_tx(const std::vector<CAmount>& output_values,
inputs, then outputs (e.g. also per the members order in struct CMutableTransaction)output_values to outputs for simplicity
can be static
done
maybe order the params as
inputs, thenoutputs(e.g. also per the members order instruct CMutableTransaction)perhaps rename
output_valuestooutputsfor simplicity
note output_values is a vector of CAmounts, not CTxOuts
44+ AssertLockHeld(pool.cs);
45+ TestMemPoolEntryHelper entry;
46+ // Assumes this isn't already spent in mempool
47+ auto tx_to_spend = tx;
48+ for (int32_t i{0}; i < num_descendants; ++i) {
49+ auto next_tx = make_tx(/*output_values=*/ {(50 - i) * CENT}, /*inputs=*/ {tx_to_spend});
maybe remove unneeded spaces throughout this new code per our current usual named arg style
0 auto next_tx = make_tx(/*output_values=*/{(50 - i) * CENT}, /*inputs=*/{tx_to_spend});
58+ LOCK2(cs_main, pool.cs);
59+ TestMemPoolEntryHelper entry;
60+
61+ const CAmount low_fee{100};
62+ const CAmount normal_fee{10000};
63+ const CAmount high_fee{1 * COIN};
0 const CAmount high_fee{COIN};
1 *
144+ // the caller passed in. As such, no error is returned even though entry2 is a descendant of tx1.
145+ BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx1->GetHash()}, unused_txid) == std::nullopt);
146+
147+ // Tests for PaysForRBF
148+ const auto incremental_relay_feerate{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE)};
149+ const auto higher_relay_feerate{CFeeRate(2 * DEFAULT_INCREMENTAL_RELAY_FEE)};
Use braced initialization for type safety where you can (some in these tests with num_bytes passed won’t compile without a cast due to narrowing)
0- const auto incremental_relay_feerate{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE)};
1- const auto higher_relay_feerate{CFeeRate(2 * DEFAULT_INCREMENTAL_RELAY_FEE)};
2+ const auto incremental_relay_feerate{CFeeRate{DEFAULT_INCREMENTAL_RELAY_FEE}};
3+ const auto higher_relay_feerate{CFeeRate{2 * DEFAULT_INCREMENTAL_RELAY_FEE}};
const CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};, which I think incorporates your suggestion and is less verbose than before
222+
223+ CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
224+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2}).has_value());
225+ BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value());
226+
227+ CTransactionRef spends_conflicting_confirmed = make_tx({45 * CENT}, {m_coinbase_txns[0], m_coinbase_txns[1]});
These CTransactionRefs can be const if you like.
0@@ -167,9 +168,9 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
1 // Tests for GetEntriesForConflicts
2 CTxMemPool::setEntries all_parents{entry1, entry3, entry5, entry7, entry8};
3 CTxMemPool::setEntries all_children{entry2, entry4, entry6};
4- std::vector<CTransactionRef> parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2],
5+ const std::vector<CTransactionRef> parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2],
6 m_coinbase_txns[3], m_coinbase_txns[4]});
7- CTransactionRef conflicts_with_parents = make_tx({50 * CENT}, parent_inputs);
8+ const CTransactionRef conflicts_with_parents = make_tx({50 * CENT}, parent_inputs);
9@@ -209,7 +210,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
10 // Tests for HasNoNewUnconfirmed
11- CTransactionRef spends_unconfirmed = make_tx({36 * CENT}, {tx1});
12+ const CTransactionRef spends_unconfirmed = make_tx({36 * CENT}, {tx1});
13@@ -220,11 +221,11 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
14- CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
15+ const CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
16 BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2}).has_value());
17 BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value());
18
19- CTransactionRef spends_conflicting_confirmed = make_tx({45 * CENT}, {m_coinbase_txns[0], m_coinbase_txns[1]});
20+ const CTransactionRef spends_conflicting_confirmed = make_tx({45 * CENT}, {m_coinbase_txns[0], m_coinbase_txns[1]});
21 }
make_tx to const auto
added const std::vector<CTransactionRef> parent_inputs
Still looking at this but seeing tests green with very different values for low_fee and normal_fee and high_fee. Only when low_fee > high_fee do I start to see some red (which may make sense given the RBF rules, but then maybe use test values that highlight the minimum necessary value deltas). The tests pass with these values, for instance:
0- const CAmount low_fee{100};
1- const CAmount normal_fee{10000};
2- const CAmount high_fee{1 * COIN};
3+ const CAmount low_fee{1000000};
4+ const CAmount normal_fee{100};
5+ const CAmount high_fee{1000000};
Various minor comments follow, feel free to pick/choose/ignore.
Still looking at this but seeing tests green with very different values for low_fee and normal_fee and high_fee.
The fees should now be 10x apart each, so hopefully less arbitrary.
Also, apologies for the confusion, I should have mentioned that I pulled this out of #25038 which includes a new check for ancestor feerate. The low-high fees are essentially CPFPs and make a significant difference there, but are largely ignored in the current rules (hence all the “notice how ancestor feerate is not checked here” test cases in this PR). There’s probably still room to make the values tighter, but hopefully this background helps.
Test each component of the RBF policy in isolation. Unlike the RBF
functional tests, these do not rely on things like RPC results, mempool
submission, etc.
This PR is just adding test coverage and doesn’t conflict with anything other than my draft PR it’s pulled out from. As such, it seems there is sufficient review to merge even though I am the author.
It seems review was insufficient. Adding wrong or low quality test coverage could lead to things not being tested but assumed to be tested.