From e9c5aeb11d641b8cae373452339760809625021d Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Fri, 12 Jan 2024 11:08:06 -0500 Subject: [PATCH] test: Add tests for CompareFeerateDiagram and CheckConflictTopology --- src/test/rbf_tests.cpp | 213 +++++++++++++++++++++++++++++++++-------- 1 file changed, 175 insertions(+), 38 deletions(-) diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index e6c135eed98..995c570484e 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -37,7 +37,7 @@ static inline CTransactionRef make_tx(const std::vector& inputs return MakeTransactionRef(tx); } -static void add_descendants(const CTransactionRef& tx, int32_t num_descendants, CTxMemPool& pool) +static CTransactionRef add_descendants(const CTransactionRef& tx, int32_t num_descendants, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) { AssertLockHeld(::cs_main); @@ -50,6 +50,21 @@ static void add_descendants(const CTransactionRef& tx, int32_t num_descendants, pool.addUnchecked(entry.FromTx(next_tx)); tx_to_spend = next_tx; } + // Return last created tx + return tx_to_spend; +} + +static CTransactionRef add_descendant_to_parents(const std::vector& parents, CTxMemPool& pool) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(pool.cs); + TestMemPoolEntryHelper entry; + // Assumes this isn't already spent in mempool + auto child_tx = make_tx(/*inputs=*/parents, /*output_values=*/{50 * CENT}); + pool.addUnchecked(entry.FromTx(child_tx)); + // Return last created tx + return child_tx; } BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) @@ -89,28 +104,46 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) const auto tx8 = make_tx(/*inputs=*/ {m_coinbase_txns[4]}, /*output_values=*/ {999 * CENT}); pool.addUnchecked(entry.Fee(high_fee).FromTx(tx8)); - const auto entry1 = pool.GetIter(tx1->GetHash()).value(); - const auto entry2 = pool.GetIter(tx2->GetHash()).value(); - const auto entry3 = pool.GetIter(tx3->GetHash()).value(); - const auto entry4 = pool.GetIter(tx4->GetHash()).value(); - const auto entry5 = pool.GetIter(tx5->GetHash()).value(); - const auto entry6 = pool.GetIter(tx6->GetHash()).value(); - const auto entry7 = pool.GetIter(tx7->GetHash()).value(); - const auto entry8 = pool.GetIter(tx8->GetHash()).value(); + // Normal txs, will chain txns right before CheckConflictTopology test + const auto tx9 = make_tx(/*inputs=*/ {m_coinbase_txns[5]}, /*output_values=*/ {995 * CENT}); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx9)); + const auto tx10 = make_tx(/*inputs=*/ {m_coinbase_txns[6]}, /*output_values=*/ {995 * CENT}); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx10)); - BOOST_CHECK_EQUAL(entry1->GetFee(), normal_fee); - BOOST_CHECK_EQUAL(entry2->GetFee(), normal_fee); - BOOST_CHECK_EQUAL(entry3->GetFee(), low_fee); - BOOST_CHECK_EQUAL(entry4->GetFee(), high_fee); - BOOST_CHECK_EQUAL(entry5->GetFee(), low_fee); - BOOST_CHECK_EQUAL(entry6->GetFee(), low_fee); - BOOST_CHECK_EQUAL(entry7->GetFee(), high_fee); - BOOST_CHECK_EQUAL(entry8->GetFee(), high_fee); + // Will make these two parents of single child + const auto tx11 = make_tx(/*inputs=*/ {m_coinbase_txns[7]}, /*output_values=*/ {995 * CENT}); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx11)); + const auto tx12 = make_tx(/*inputs=*/ {m_coinbase_txns[8]}, /*output_values=*/ {995 * CENT}); + pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx12)); - CTxMemPool::setEntries set_12_normal{entry1, entry2}; - CTxMemPool::setEntries set_34_cpfp{entry3, entry4}; - CTxMemPool::setEntries set_56_low{entry5, entry6}; - CTxMemPool::setEntries all_entries{entry1, entry2, entry3, entry4, entry5, entry6, entry7, entry8}; + const auto entry1_normal = pool.GetIter(tx1->GetHash()).value(); + const auto entry2_normal = pool.GetIter(tx2->GetHash()).value(); + const auto entry3_low = pool.GetIter(tx3->GetHash()).value(); + const auto entry4_high = pool.GetIter(tx4->GetHash()).value(); + const auto entry5_low = pool.GetIter(tx5->GetHash()).value(); + 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(); + + BOOST_CHECK_EQUAL(entry1_normal->GetFee(), normal_fee); + BOOST_CHECK_EQUAL(entry2_normal->GetFee(), normal_fee); + BOOST_CHECK_EQUAL(entry3_low->GetFee(), low_fee); + BOOST_CHECK_EQUAL(entry4_high->GetFee(), high_fee); + BOOST_CHECK_EQUAL(entry5_low->GetFee(), low_fee); + BOOST_CHECK_EQUAL(entry6_low_prioritised->GetFee(), low_fee); + BOOST_CHECK_EQUAL(entry7_high->GetFee(), high_fee); + BOOST_CHECK_EQUAL(entry8_high->GetFee(), high_fee); + + CTxMemPool::setEntries set_12_normal{entry1_normal, entry2_normal}; + CTxMemPool::setEntries set_34_cpfp{entry3_low, entry4_high}; + CTxMemPool::setEntries set_56_low{entry5_low, entry6_low_prioritised}; + CTxMemPool::setEntries set_78_high{entry7_high, entry8_high}; + CTxMemPool::setEntries all_entries{entry1_normal, entry2_normal, entry3_low, entry4_high, + entry5_low, entry6_low_prioritised, entry7_high, entry8_high}; CTxMemPool::setEntries empty_set; const auto unused_txid{GetRandHash()}; @@ -118,29 +151,29 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) // Tests for PaysMoreThanConflicts // These tests use feerate, not absolute fee. BOOST_CHECK(PaysMoreThanConflicts(/*iters_conflicting=*/set_12_normal, - /*replacement_feerate=*/CFeeRate(entry1->GetModifiedFee() + 1, entry1->GetTxSize() + 2), + /*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->GetModifiedFee(), entry1->GetTxSize()), unused_txid).has_value()); - BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1->GetModifiedFee() + 1, entry1->GetTxSize()), unused_txid) == std::nullopt); + 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}, CFeeRate(entry5->GetModifiedFee() + 1, entry5->GetTxSize()), unused_txid) == std::nullopt); - BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetFee() + 1, entry6->GetTxSize()), unused_txid).has_value()); - BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetModifiedFee() + 1, entry6->GetTxSize()), unused_txid) == std::nullopt); + 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's feerate, which are the same. The replacement_feerate is - // considered too low even though entry4 has a low ancestor feerate. - BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4->GetModifiedFee(), entry4->GetTxSize()), unused_txid).has_value()); + // 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); - BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetHash()}, unused_txid).has_value()); + BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx2->GetHash()}, unused_txid).has_value()); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx1->GetHash()}, unused_txid).has_value()); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value()); // EntriesAndTxidsDisjoint does not calculate descendants of iters_conflicting; it uses whatever - // the caller passed in. As such, no error is returned even though entry2 is a descendant of tx1. - BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx1->GetHash()}, unused_txid) == std::nullopt); + // the caller passed in. As such, no error is returned even though entry2_normal is a descendant of tx1. + BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx1->GetHash()}, unused_txid) == std::nullopt); // Tests for PaysForRBF const CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE}; @@ -163,8 +196,8 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) BOOST_CHECK(PaysForRBF(low_fee, high_fee + 99999999, 99999999, incremental_relay_feerate, unused_txid) == std::nullopt); // Tests for GetEntriesForConflicts - CTxMemPool::setEntries all_parents{entry1, entry3, entry5, entry7, entry8}; - CTxMemPool::setEntries all_children{entry2, entry4, entry6}; + CTxMemPool::setEntries all_parents{entry1_normal, entry3_low, entry5_low, entry7_high, entry8_high}; + CTxMemPool::setEntries all_children{entry2_normal, entry4_high, entry6_low_prioritised}; const std::vector parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2], m_coinbase_txns[3], m_coinbase_txns[4]}); const auto conflicts_with_parents = make_tx(parent_inputs, {50 * CENT}); @@ -215,15 +248,119 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) BOOST_CHECK(HasNoNewUnconfirmed(/*tx=*/ *spends_unconfirmed.get(), /*pool=*/ pool, /*iters_conflicting=*/ all_entries) == std::nullopt); - BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2}) == 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}).has_value()); + 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, entry3}) == std::nullopt); + BOOST_CHECK(HasNoNewUnconfirmed(*spends_conflicting_confirmed.get(), pool, {entry1_normal, entry3_low}) == std::nullopt); + + // Tests for CheckConflictTopology + + // Tx4 has 23 descendants + BOOST_CHECK(pool.CheckConflictTopology(set_34_cpfp).has_value()); + + // 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())); +} + +BOOST_AUTO_TEST_CASE(feerate_diagram_utilities) +{ + // Sanity check the correctness of the feerate diagram comparison. + + // A strictly better case. + std::vector old_diagram{{FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}}; + std::vector new_diagram{{FeeFrac{0, 0}, FeeFrac{1000, 300}, FeeFrac{1050, 400}}}; + + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); + + // Incomparable diagrams + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{1000, 300}, FeeFrac{1000, 400}}; + + BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); + + // Strictly better but smaller size. + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{1100, 300}}; + + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); + + // New diagram is strictly better due to the first chunk, even though + // second chunk contributes no fees + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{1100, 100}, FeeFrac{1100, 200}}; + + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); + + // Feerate of first new chunk is better with, but second chunk is worse + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{750, 100}, FeeFrac{999, 350}, FeeFrac{1150, 1000}}; + + BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); + + // If we make the second chunk slightly better, the new diagram now wins. + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{750, 100}, FeeFrac{1000, 350}, FeeFrac{1150, 500}}; + + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(old_diagram, new_diagram))); + + // Identical diagrams, cannot be strictly better + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + + BOOST_CHECK(std::is_eq(CompareFeerateDiagram(old_diagram, new_diagram))); + + // Same aggregate fee, but different total size (trigger single tail fee check step) + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 399}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}}; + + // No change in evaluation when tail check needed. + BOOST_CHECK(std::is_gt(CompareFeerateDiagram(old_diagram, new_diagram))); + + // Padding works on either argument + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(new_diagram, old_diagram))); + + // Trigger multiple tail fee check steps + old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 399}}; + new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}, FeeFrac{1050, 401}, FeeFrac{1050, 402}}; + + BOOST_CHECK(std::is_gt(CompareFeerateDiagram(old_diagram, new_diagram))); + BOOST_CHECK(std::is_lt(CompareFeerateDiagram(new_diagram, old_diagram))); + + // Multiple tail fee check steps, unordered result + new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}, FeeFrac{1050, 401}, FeeFrac{1050, 402}, FeeFrac{1051, 403}}; + BOOST_CHECK(CompareFeerateDiagram(old_diagram, new_diagram) == std::partial_ordering::unordered); } BOOST_AUTO_TEST_SUITE_END()