Skip to content

Commit d7dc9fd

Browse files
sdaftuarsdaftuar
committed
Move CalculateChunksForRBF() to the mempool changeset
1 parent 284a1d3 commit d7dc9fd

File tree

7 files changed

+148
-74
lines changed

7 files changed

+148
-74
lines changed

‎src/policy/rbf.cpp‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,14 +184,10 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
184184
return std::nullopt;
185185
}
186186

187-
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
188-
const CTxMemPool::setEntries& direct_conflicts,
189-
const CTxMemPool::setEntries& all_conflicts,
190-
CAmount replacement_fees,
191-
int64_t replacement_vsize)
187+
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool::ChangeSet& changeset)
192188
{
193189
// Require that the replacement strictly improves the mempool's feerate diagram.
194-
const auto chunk_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, all_conflicts, all_conflicts)};
190+
const auto chunk_results{changeset.CalculateChunksForRBF()};
195191

196192
if (!chunk_results.has_value()) {
197193
return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(chunk_results).original);

‎src/policy/rbf.h‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -117,19 +117,9 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
117117

118118
/**
119119
* The replacement transaction must improve the feerate diagram of the mempool.
120-
* @param[in] pool The mempool.
121-
* @param[in] direct_conflicts Set of in-mempool txids corresponding to the direct conflicts i.e.
122-
* input double-spends with the proposed transaction
123-
* @param[in] all_conflicts Set of mempool entries corresponding to all transactions to be evicted
124-
* @param[in] replacement_fees Fees of proposed replacement package
125-
* @param[in] replacement_vsize Size of proposed replacement package
120+
* @param[in] changeset The changeset containing proposed additions/removals
126121
* @returns error type and string if mempool diagram doesn't improve, otherwise std::nullopt.
127122
*/
128-
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
129-
const CTxMemPool::setEntries& direct_conflicts,
130-
const CTxMemPool::setEntries& all_conflicts,
131-
CAmount replacement_fees,
132-
int64_t replacement_vsize)
133-
EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
123+
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool::ChangeSet& changeset);
134124

135125
#endif // BITCOIN_POLICY_RBF_H

‎src/test/fuzz/rbf.cpp‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,18 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
104104
std::vector<CTransaction> mempool_txs;
105105
size_t iter{0};
106106

107-
int32_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000);
108-
109107
// Keep track of the total vsize of CTxMemPoolEntry's being added to the mempool to avoid overflow
110108
// Add replacement_vsize since this is added to new diagram during RBF check
109+
std::optional<CMutableTransaction> replacement_tx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
110+
if (!replacement_tx) {
111+
return;
112+
}
113+
assert(iter <= g_outpoints.size());
114+
replacement_tx->vin.resize(1);
115+
replacement_tx->vin[0].prevout = g_outpoints[iter++];
116+
CTransaction replacement_tx_final{*replacement_tx};
117+
auto replacement_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, replacement_tx_final);
118+
int32_t replacement_vsize = replacement_entry.GetTxSize();
111119
int64_t running_vsize_total{replacement_vsize};
112120

113121
LOCK2(cs_main, pool.cs);
@@ -162,9 +170,17 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
162170
pool.CalculateDescendants(txiter, all_conflicts);
163171
}
164172

165-
// Calculate the chunks for a replacement.
166173
CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
167-
auto calc_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
174+
auto changeset = pool.GetChangeSet();
175+
for (auto& txiter : all_conflicts) {
176+
changeset->StageRemoval(txiter);
177+
}
178+
changeset->StageAddition(replacement_entry.GetSharedTx(), replacement_fees,
179+
replacement_entry.GetTime().count(), replacement_entry.GetHeight(),
180+
replacement_entry.GetSequence(), replacement_entry.GetSpendsCoinbase(),
181+
replacement_entry.GetSigOpCost(), replacement_entry.GetLockPoints());
182+
// Calculate the chunks for a replacement.
183+
auto calc_results{changeset->CalculateChunksForRBF()};
168184

169185
if (calc_results.has_value()) {
170186
// Sanity checks on the chunks.
@@ -192,7 +208,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
192208
}
193209

194210
// If internals report error, wrapper should too
195-
auto err_tuple{ImprovesFeerateDiagram(pool, direct_conflicts, all_conflicts, replacement_fees, replacement_vsize)};
211+
auto err_tuple{ImprovesFeerateDiagram(*changeset)};
196212
if (!calc_results.has_value()) {
197213
assert(err_tuple.value().first == DiagramCheckError::UNCALCULABLE);
198214
} else {

‎src/test/rbf_tests.cpp‎

Lines changed: 103 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -369,44 +369,79 @@ BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup)
369369

370370
const auto entry1 = pool.GetIter(tx1->GetHash()).value();
371371
const auto tx1_fee = entry1->GetModifiedFee();
372-
const auto tx1_size = entry1->GetTxSize();
373372
const auto entry2 = pool.GetIter(tx2->GetHash()).value();
374373
const auto tx2_fee = entry2->GetModifiedFee();
375-
const auto tx2_size = entry2->GetTxSize();
376374

377375
// conflicting transactions
378376
const auto tx1_conflict = make_tx(/*inputs=*/ {m_coinbase_txns[0], m_coinbase_txns[2]}, /*output_values=*/ {10 * COIN});
377+
const auto tx3 = make_tx(/*inputs=*/ {tx1_conflict}, /*output_values=*/ {995 * CENT});
378+
auto entry3 = entry.FromTx(tx3);
379379

380380
// Now test ImprovesFeerateDiagram with various levels of "package rbf" feerates
381381

382382
// It doesn't improve itself
383-
const auto res1 = ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee, tx1_size + tx2_size);
383+
auto changeset = pool.GetChangeSet();
384+
changeset->StageRemoval(entry1);
385+
changeset->StageRemoval(entry2);
386+
changeset->StageAddition(tx1_conflict, tx1_fee, 0, 1, 0, false, 4, LockPoints());
387+
changeset->StageAddition(tx3, tx2_fee, 0, 1, 0, false, 4, LockPoints());
388+
const auto res1 = ImprovesFeerateDiagram(*changeset);
384389
BOOST_CHECK(res1.has_value());
385390
BOOST_CHECK(res1.value().first == DiagramCheckError::FAILURE);
386391
BOOST_CHECK(res1.value().second == "insufficient feerate: does not improve feerate diagram");
387392

388393
// With one more satoshi it does
389-
BOOST_CHECK(ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee + 1, tx1_size + tx2_size) == std::nullopt);
390-
394+
changeset.reset();
395+
changeset = pool.GetChangeSet();
396+
changeset->StageRemoval(entry1);
397+
changeset->StageRemoval(entry2);
398+
changeset->StageAddition(tx1_conflict, tx1_fee+1, 0, 1, 0, false, 4, LockPoints());
399+
changeset->StageAddition(tx3, tx2_fee, 0, 1, 0, false, 4, LockPoints());
400+
BOOST_CHECK(ImprovesFeerateDiagram(*changeset) == std::nullopt);
401+
402+
changeset.reset();
391403
// With prioritisation of in-mempool conflicts, it affects the results of the comparison using the same args as just above
392404
pool.PrioritiseTransaction(entry1->GetSharedTx()->GetHash(), /*nFeeDelta=*/1);
393-
const auto res2 = ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee + 1, tx1_size + tx2_size);
405+
changeset = pool.GetChangeSet();
406+
changeset->StageRemoval(entry1);
407+
changeset->StageRemoval(entry2);
408+
changeset->StageAddition(tx1_conflict, tx1_fee+1, 0, 1, 0, false, 4, LockPoints());
409+
changeset->StageAddition(tx3, tx2_fee, 0, 1, 0, false, 4, LockPoints());
410+
const auto res2 = ImprovesFeerateDiagram(*changeset);
394411
BOOST_CHECK(res2.has_value());
395412
BOOST_CHECK(res2.value().first == DiagramCheckError::FAILURE);
396413
BOOST_CHECK(res2.value().second == "insufficient feerate: does not improve feerate diagram");
414+
changeset.reset();
415+
397416
pool.PrioritiseTransaction(entry1->GetSharedTx()->GetHash(), /*nFeeDelta=*/-1);
398417

399-
// With one less vB it does
400-
BOOST_CHECK(ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee, tx1_size + tx2_size - 1) == std::nullopt);
418+
// With fewer vbytes it does
419+
CMutableTransaction tx4{entry3.GetTx()};
420+
tx4.vin[0].scriptWitness = CScriptWitness(); // Clear out the witness, to reduce size
421+
auto entry4 = entry.FromTx(MakeTransactionRef(tx4));
422+
changeset = pool.GetChangeSet();
423+
changeset->StageRemoval(entry1);
424+
changeset->StageRemoval(entry2);
425+
changeset->StageAddition(tx1_conflict, tx1_fee, 0, 1, 0, false, 4, LockPoints());
426+
changeset->StageAddition(entry4.GetSharedTx(), tx2_fee, 0, 1, 0, false, 4, LockPoints());
427+
BOOST_CHECK(ImprovesFeerateDiagram(*changeset) == std::nullopt);
428+
changeset.reset();
401429

402430
// Adding a grandchild makes the cluster size 3, which is uncalculable
403-
const auto tx3 = make_tx(/*inputs=*/ {tx2}, /*output_values=*/ {995 * CENT});
404-
AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx3));
405-
const auto res3 = ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee + 1, tx1_size + tx2_size);
431+
const auto tx5 = make_tx(/*inputs=*/ {tx2}, /*output_values=*/ {995 * CENT});
432+
AddToMempool(pool, entry.Fee(normal_fee).FromTx(tx5));
433+
const auto entry5 = pool.GetIter(tx5->GetHash()).value();
434+
435+
changeset = pool.GetChangeSet();
436+
changeset->StageRemoval(entry1);
437+
changeset->StageRemoval(entry2);
438+
changeset->StageRemoval(entry5);
439+
changeset->StageAddition(tx1_conflict, tx1_fee, 0, 1, 0, false, 4, LockPoints());
440+
changeset->StageAddition(entry4.GetSharedTx(), tx2_fee + entry5->GetModifiedFee() + 1, 0, 1, 0, false, 4, LockPoints());
441+
const auto res3 = ImprovesFeerateDiagram(*changeset);
406442
BOOST_CHECK(res3.has_value());
407443
BOOST_CHECK(res3.value().first == DiagramCheckError::UNCALCULABLE);
408-
BOOST_CHECK(res3.value().second == strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", tx2->GetHash().GetHex()));
409-
444+
BOOST_CHECK(res3.value().second == strprintf("%s has 2 ancestors, max 1 allowed", tx5->GetHash().GetHex()));
410445
}
411446

412447
BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
@@ -427,19 +462,28 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
427462
const auto entry_low = pool.GetIter(low_tx->GetHash()).value();
428463
const auto low_size = entry_low->GetTxSize();
429464

465+
const auto replacement_tx = make_tx(/*inputs=*/ {m_coinbase_txns[0]}, /*output_values=*/ {9 * COIN});
466+
auto entry_replacement = entry.FromTx(replacement_tx);
467+
430468
// Replacement of size 1
431469
{
432-
const auto replace_one{pool.CalculateChunksForRBF(/*replacement_fees=*/0, /*replacement_vsize=*/1, {entry_low}, {entry_low})};
470+
auto changeset = pool.GetChangeSet();
471+
changeset->StageRemoval(entry_low);
472+
changeset->StageAddition(replacement_tx, 0, 0, 1, 0, false, 4, LockPoints());
473+
const auto replace_one{changeset->CalculateChunksForRBF()};
433474
BOOST_CHECK(replace_one.has_value());
434475
std::vector<FeeFrac> expected_old_chunks{{low_fee, low_size}};
435476
BOOST_CHECK(replace_one->first == expected_old_chunks);
436-
std::vector<FeeFrac> expected_new_chunks{{0, 1}};
477+
std::vector<FeeFrac> expected_new_chunks{{0, int32_t(entry_replacement.GetTxSize())}};
437478
BOOST_CHECK(replace_one->second == expected_new_chunks);
438479
}
439480

440481
// Non-zero replacement fee/size
441482
{
442-
const auto replace_one_fee{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low})};
483+
auto changeset = pool.GetChangeSet();
484+
changeset->StageRemoval(entry_low);
485+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
486+
const auto replace_one_fee{changeset->CalculateChunksForRBF()};
443487
BOOST_CHECK(replace_one_fee.has_value());
444488
std::vector<FeeFrac> expected_old_diagram{{low_fee, low_size}};
445489
BOOST_CHECK(replace_one_fee->first == expected_old_diagram);
@@ -454,7 +498,11 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
454498
const auto high_size = entry_high->GetTxSize();
455499

456500
{
457-
const auto replace_single_chunk{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_low}, {entry_low, entry_high})};
501+
auto changeset = pool.GetChangeSet();
502+
changeset->StageRemoval(entry_low);
503+
changeset->StageRemoval(entry_high);
504+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
505+
const auto replace_single_chunk{changeset->CalculateChunksForRBF()};
458506
BOOST_CHECK(replace_single_chunk.has_value());
459507
std::vector<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}};
460508
BOOST_CHECK(replace_single_chunk->first == expected_old_chunks);
@@ -464,7 +512,10 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
464512

465513
// Conflict with the 2nd tx, resulting in new diagram with three entries
466514
{
467-
const auto replace_cpfp_child{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high}, {entry_high})};
515+
auto changeset = pool.GetChangeSet();
516+
changeset->StageRemoval(entry_high);
517+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
518+
const auto replace_cpfp_child{changeset->CalculateChunksForRBF()};
468519
BOOST_CHECK(replace_cpfp_child.has_value());
469520
std::vector<FeeFrac> expected_old_chunks{{low_fee + high_fee, low_size + high_size}};
470521
BOOST_CHECK(replace_cpfp_child->first == expected_old_chunks);
@@ -476,12 +527,16 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
476527
const auto normal_tx = make_tx(/*inputs=*/ {high_tx}, /*output_values=*/ {995 * CENT});
477528
AddToMempool(pool, entry.Fee(normal_fee).FromTx(normal_tx));
478529
const auto entry_normal = pool.GetIter(normal_tx->GetHash()).value();
479-
const auto normal_size = entry_normal->GetTxSize();
480530

481531
{
482-
const auto replace_too_large{pool.CalculateChunksForRBF(/*replacement_fees=*/normal_fee, /*replacement_vsize=*/normal_size, {entry_low}, {entry_low, entry_high, entry_normal})};
532+
auto changeset = pool.GetChangeSet();
533+
changeset->StageRemoval(entry_low);
534+
changeset->StageRemoval(entry_high);
535+
changeset->StageRemoval(entry_normal);
536+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
537+
const auto replace_too_large{changeset->CalculateChunksForRBF()};
483538
BOOST_CHECK(!replace_too_large.has_value());
484-
BOOST_CHECK_EQUAL(util::ErrorString(replace_too_large).original, strprintf("%s has 2 descendants, max 1 allowed", low_tx->GetHash().GetHex()));
539+
BOOST_CHECK_EQUAL(util::ErrorString(replace_too_large).original, strprintf("%s has 2 ancestors, max 1 allowed", normal_tx->GetHash().GetHex()));
485540
}
486541

487542
// Make a size 2 cluster that is itself two chunks; evict both txns
@@ -496,7 +551,11 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
496551
const auto low_size_2 = entry_low_2->GetTxSize();
497552

498553
{
499-
const auto replace_two_chunks_single_cluster{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {entry_high_2}, {entry_high_2, entry_low_2})};
554+
auto changeset = pool.GetChangeSet();
555+
changeset->StageRemoval(entry_high_2);
556+
changeset->StageRemoval(entry_low_2);
557+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
558+
const auto replace_two_chunks_single_cluster{changeset->CalculateChunksForRBF()};
500559
BOOST_CHECK(replace_two_chunks_single_cluster.has_value());
501560
std::vector<FeeFrac> expected_old_chunks{{high_fee, high_size_2}, {low_fee, low_size_2}};
502561
BOOST_CHECK(replace_two_chunks_single_cluster->first == expected_old_chunks);
@@ -518,7 +577,12 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
518577
const auto conflict_3_entry = pool.GetIter(conflict_3->GetHash()).value();
519578

520579
{
521-
const auto replace_multiple_clusters{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry})};
580+
auto changeset = pool.GetChangeSet();
581+
changeset->StageRemoval(conflict_1_entry);
582+
changeset->StageRemoval(conflict_2_entry);
583+
changeset->StageRemoval(conflict_3_entry);
584+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
585+
const auto replace_multiple_clusters{changeset->CalculateChunksForRBF()};
522586
BOOST_CHECK(replace_multiple_clusters.has_value());
523587
BOOST_CHECK(replace_multiple_clusters->first.size() == 3);
524588
BOOST_CHECK(replace_multiple_clusters->second.size() == 1);
@@ -530,7 +594,13 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
530594
const auto conflict_1_child_entry = pool.GetIter(conflict_1_child->GetHash()).value();
531595

532596
{
533-
const auto replace_multiple_clusters_2{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry})};
597+
auto changeset = pool.GetChangeSet();
598+
changeset->StageRemoval(conflict_1_entry);
599+
changeset->StageRemoval(conflict_2_entry);
600+
changeset->StageRemoval(conflict_3_entry);
601+
changeset->StageRemoval(conflict_1_child_entry);
602+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
603+
const auto replace_multiple_clusters_2{changeset->CalculateChunksForRBF()};
534604

535605
BOOST_CHECK(replace_multiple_clusters_2.has_value());
536606
BOOST_CHECK(replace_multiple_clusters_2->first.size() == 4);
@@ -543,10 +613,17 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
543613
const auto conflict_1_grand_child_entry = pool.GetIter(conflict_1_child->GetHash()).value();
544614

545615
{
546-
const auto replace_cluster_size_3{pool.CalculateChunksForRBF(/*replacement_fees=*/high_fee, /*replacement_vsize=*/low_size, {conflict_1_entry, conflict_2_entry, conflict_3_entry}, {conflict_1_entry, conflict_2_entry, conflict_3_entry, conflict_1_child_entry, conflict_1_grand_child_entry})};
616+
auto changeset = pool.GetChangeSet();
617+
changeset->StageRemoval(conflict_1_entry);
618+
changeset->StageRemoval(conflict_2_entry);
619+
changeset->StageRemoval(conflict_3_entry);
620+
changeset->StageRemoval(conflict_1_child_entry);
621+
changeset->StageRemoval(conflict_1_grand_child_entry);
622+
changeset->StageAddition(replacement_tx, high_fee, 0, 1, 0, false, 4, LockPoints());
623+
const auto replace_cluster_size_3{changeset->CalculateChunksForRBF()};
547624

548625
BOOST_CHECK(!replace_cluster_size_3.has_value());
549-
BOOST_CHECK_EQUAL(util::ErrorString(replace_cluster_size_3).original, strprintf("%s has 2 descendants, max 1 allowed", conflict_1->GetHash().GetHex()));
626+
BOOST_CHECK_EQUAL(util::ErrorString(replace_cluster_size_3).original, strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", conflict_1_child->GetHash().GetHex()));
550627
}
551628
}
552629

0 commit comments

Comments
 (0)