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.
test/rbf_tests.cpp:47:14: error: calling function 'addUnchecked' requires holding mutex 'pool.cs' exclusively [-Werror,-Wthread-safety-analysis]
pool.addUnchecked(entry.FromTx(next_tx));
^
test/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);
should note what this is doing ahead of time, or do it JIT before usage
Added comment, hopefully clearer now
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);
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt);
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.,.
Done
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)};
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.
* [@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);
QoL improvement for future readers/editors would be accumulating these constants
Done
crACK 645c593a6027e7939c00d70f185ccbd0f381b7b5
Just a few improvements suggested
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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());
I think you can test the boundary value just under 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.
Yeah, I think the off-by-one error is accounted for on these lines. i.e. same fee fails, same fee + 1 succeeds.
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());
I'm wondering what you're aiming to demonstrate with this test ? If you would like to show that a replacement candidate with a higher ancestor feerate won't replace conflicts with higher individual feerate, I believe the replacement topology should have a parent with a higher feerate, namely here with 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.
I've added a comment to clarify. Hopefully resolves this, but feel free to unresolve if not!
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);
I think in practice 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.
Antoine: It's precisely what's being tested the 2 lines just above?
Yeah, that's the idea for the comment "EntriesAndTxidsDisjoint uses the ancestors directly. It does not calculate descendants." I've edited the comment and moved it down 2 lines to make it more clear what's being tested.
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());
I think here you can the variant where the 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 ?
Good idea, done.
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());
Bikeshedding of the day. This function name could actually reflect the fact that BIP125 is enforced here for codebase clarity, something like FetchConflictingEntriesAndCheckMaxReplacement ? (can be in other commit/PR).
Agree the naming could be changed to reflect that the function checks max replacements. But maybe for another PR :sweat_smile:
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());
If you're bored, I think there is IsRBFOptIn remaining in rbf.cpp deserving unit test coverage.
Will save for later PR. It's fuzzed in src/test/fuzz/rbf.cpp so I'm less worried atm.
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:
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index e25f5c7c5b..f1d8301ab6 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -106,10 +106,10 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
// Rather than check the UTXO set - potentially expensive - it's cheaper to just check
// if the new input refers to a tx that's in the mempool.
- if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
+ //if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
return strprintf("replacement %s adds unconfirmed input, idx %d",
tx.GetHash().ToString(), j);
- }
+ //}
}
}
return std::nullopt;
Good point! Added a test for conflicting confirmed inputs.
Concept ACK
Concept ACK, will review soon
ACK 520657a5620a11bd0426b14d6bb8053667a4f20d
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 \
Optional, feel free to ignore, maybe add this file to the RUN_TIDY include-what-you-use list in 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)
static 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.
both done
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)
EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs)
Done, thanks
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,
static 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
auto next_tx = make_tx(/*output_values=*/{(50 - i) * CENT}, /*inputs=*/{tx_to_spend});
done
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};
const CAmount high_fee{COIN};
Changed these to be 1 order of magnitude apart each (hopefully less arbitrary than before) and removed the 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)
- const auto incremental_relay_feerate{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE)};
- const auto higher_relay_feerate{CFeeRate(2 * DEFAULT_INCREMENTAL_RELAY_FEE)};
+ const auto incremental_relay_feerate{CFeeRate{DEFAULT_INCREMENTAL_RELAY_FEE}};
+ const auto higher_relay_feerate{CFeeRate{2 * DEFAULT_INCREMENTAL_RELAY_FEE}};
changed to const CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};, which I think incorporates your suggestion and is less verbose than before
Even better indeed :+1:
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.
@@ -167,9 +168,9 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
// Tests for GetEntriesForConflicts
CTxMemPool::setEntries all_parents{entry1, entry3, entry5, entry7, entry8};
CTxMemPool::setEntries all_children{entry2, entry4, entry6};
- std::vector<CTransactionRef> parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2],
+ 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]});
- CTransactionRef conflicts_with_parents = make_tx({50 * CENT}, parent_inputs);
+ const CTransactionRef conflicts_with_parents = make_tx({50 * CENT}, parent_inputs);
@@ -209,7 +210,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
// Tests for HasNoNewUnconfirmed
- CTransactionRef spends_unconfirmed = make_tx({36 * CENT}, {tx1});
+ const CTransactionRef spends_unconfirmed = make_tx({36 * CENT}, {tx1});
@@ -220,11 +221,11 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
- CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
+ const CTransactionRef spends_new_unconfirmed = make_tx({36 * CENT}, {tx1, tx8});
BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2}).has_value());
BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value());
- CTransactionRef spends_conflicting_confirmed = make_tx({45 * CENT}, {m_coinbase_txns[0], m_coinbase_txns[1]});
+ const CTransactionRef spends_conflicting_confirmed = make_tx({45 * CENT}, {m_coinbase_txns[0], m_coinbase_txns[1]});
}
changed everything holding return values from 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:
- const CAmount low_fee{100};
- const CAmount normal_fee{10000};
- const CAmount high_fee{1 * COIN};
+ const CAmount low_fee{1000000};
+ const CAmount normal_fee{100};
+ 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 has 3 ACKs (thanks @w0xlt, @instagibbs, @jonatack) and the previous state had 2 other ACKs (thanks @darosior, @t-bast). 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.
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.