From 5adb2ab084333c7a968895848123302b88cb4183 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 12 Jan 2026 10:19:19 +0000 Subject: [PATCH] refactor: test block policy estimator directly The purpose of the test was to exercise CBlockPolicyEstimator behavior, but it previously used a real CTxMemPool plus validation signals to track the txs. Since TryAddToMempool does not emit TransactionAddedToMempool callbacks, the test also had to fire those callbacks manually and sync the validation interface queue around estimate checks. Call processTransaction() and processBlock() directly instead. This removes the mempool and validation-signal plumbing from the test, makes event ordering explicit and synchronous, and avoids coupling the test to the validation interface notifications. This is useful because subsequent commits moved CBlockPolicyEstimator from being validation interface client to FeeRateEstimatorManager. --- src/test/blockpolicyestimator_tests.cpp | 164 ++++++++---------------- 1 file changed, 57 insertions(+), 107 deletions(-) diff --git a/src/test/blockpolicyestimator_tests.cpp b/src/test/blockpolicyestimator_tests.cpp index 294f3a32da1..1c1aa0dfd6b 100644 --- a/src/test/blockpolicyestimator_tests.cpp +++ b/src/test/blockpolicyestimator_tests.cpp @@ -20,8 +20,6 @@ BOOST_FIXTURE_TEST_SUITE(blockpolicyestimator_tests, ChainTestingSetup) BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { CBlockPolicyEstimator feeEst{FeeestPath(*m_node.args), DEFAULT_ACCEPT_STALE_FEE_ESTIMATES}; - CTxMemPool& mpool = *Assert(m_node.mempool); - m_node.validation_signals->RegisterValidationInterface(&feeEst); TestMemPoolEntryHelper entry; CAmount basefee(2000); CAmount deltaFee(100); @@ -35,9 +33,9 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Store the hashes of transactions that have been // added to the mempool by their associate fee - // txHashes[j] is populated with transactions either of + // mempool_txs[j] is populated with transactions either of // fee = basefee * (j+1) - std::vector txHashes[10]; + std::list mempool_txs[10]; // Create a transaction template CScript garbage; @@ -51,7 +49,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) CFeeRate baseRate(basefee, GetVirtualTransactionSize(CTransaction(tx))); // Create a fake block - std::vector block; + std::vector block_txs; int blocknum = 0; // Loop through 200 blocks @@ -61,23 +59,18 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; // make transaction unique - { - LOCK2(cs_main, mpool.cs); - TryAddToMempool(mpool, entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); - // Since TransactionAddedToMempool callbacks are generated in ATMP, - // not TryAddToMempool, we cheat and create one manually here - const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); - const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), - feeV[j], - virtual_size, - entry.nHeight, - /*mempool_limit_bypassed=*/false, - /*submitted_in_package=*/false, - /*chainstate_is_current=*/true, - /*has_no_mempool_parents=*/true)}; - m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); - } - txHashes[j].push_back(tx.GetHash()); + // Simulate the tx being added to the mempool by calling processTransaction(tx_info) + mempool_txs[j].emplace_back(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; + feeEst.processTransaction(tx_info); } } //Create blocks where higher fee txs are included more often @@ -85,24 +78,17 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // 10/10 blocks add highest fee transactions // 9/10 blocks add 2nd highest and so on until ... // 1/10 blocks add lowest fee transactions - while (txHashes[9-h].size()) { - CTransactionRef ptx = mpool.get(txHashes[9-h].back()); - if (ptx) - block.push_back(ptx); - txHashes[9-h].pop_back(); + while (mempool_txs[9 - h].size()) { + auto& tx_entry = mempool_txs[9 - h].back(); + block_txs.emplace_back(tx_entry); + mempool_txs[9 - h].pop_back(); } } - { - LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); - } - - block.clear(); + feeEst.processBlock(block_txs, ++blocknum); + block_txs.clear(); // Check after just a few txs that combining buckets works as expected if (blocknum == 3) { - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); // At this point we should need to combine 3 buckets to get enough data points // So estimateFee(1) should fail and estimateFee(2) should return somewhere around // 9*baserate. estimateFee(2) %'s are 100,100,90 = average 97% @@ -112,9 +98,6 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) } } - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); - std::vector origFeeEst; // Highest feerate is 10*baseRate and gets in all blocks, // second highest feerate is 9*baseRate and gets in 9/10 blocks = 90%, @@ -141,13 +124,9 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Mine 50 more blocks with no transactions happening, estimates shouldn't change // We haven't decayed the moving average enough so we still have enough data points in every bucket while (blocknum < 250) { - LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); + feeEst.processBlock(block_txs, ++blocknum); } - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); - BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] + deltaFee); @@ -161,34 +140,23 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; - { - LOCK2(cs_main, mpool.cs); - TryAddToMempool(mpool, entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); - // Since TransactionAddedToMempool callbacks are generated in ATMP, - // not TryAddToMempool, we cheat and create one manually here - const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); - const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), - feeV[j], - virtual_size, - entry.nHeight, - /*mempool_limit_bypassed=*/false, - /*submitted_in_package=*/false, - /*chainstate_is_current=*/true, - /*has_no_mempool_parents=*/true)}; - m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); - } - txHashes[j].push_back(tx.GetHash()); + // Simulate the tx being added to the mempool by calling processTransaction(tx_info) + mempool_txs[j].emplace_back(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; + feeEst.processTransaction(tx_info); } } - { - LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); - } + feeEst.processBlock(block_txs, ++blocknum); } - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); - for (int i = 1; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -196,22 +164,15 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Mine all those transactions // Estimates should still not be below original for (int j = 0; j < 10; j++) { - while(txHashes[j].size()) { - CTransactionRef ptx = mpool.get(txHashes[j].back()); - if (ptx) - block.push_back(ptx); - txHashes[j].pop_back(); + while (mempool_txs[j].size()) { + auto& tx_entry = mempool_txs[j].back(); + block_txs.emplace_back(tx_entry); + mempool_txs[j].pop_back(); } } - { - LOCK(mpool.cs); - mpool.removeForBlock(block, 266); - } - block.clear(); - - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); + feeEst.processBlock(block_txs, ++blocknum); + block_txs.clear(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { @@ -224,38 +185,27 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; - { - LOCK2(cs_main, mpool.cs); - TryAddToMempool(mpool, entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); - // Since TransactionAddedToMempool callbacks are generated in ATMP, - // not TryAddToMempool, we cheat and create one manually here - const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); - const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), - feeV[j], - virtual_size, - entry.nHeight, - /*mempool_limit_bypassed=*/false, - /*submitted_in_package=*/false, - /*chainstate_is_current=*/true, - /*has_no_mempool_parents=*/true)}; - m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); - } - CTransactionRef ptx = mpool.get(tx.GetHash()); - if (ptx) - block.push_back(ptx); + // These txs are mined in the same block, so there is no need to + // retain them in mempool_txs; use a local entry to build block_txs. + const CTxMemPoolEntry tx_entry{entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)}; + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /*mempool_limit_bypassed=*/false, + /*submitted_in_package=*/false, + /*chainstate_is_current=*/true, + /*has_no_mempool_parents=*/true)}; + feeEst.processTransaction(tx_info); + block_txs.emplace_back(tx_entry); } } - { - LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); - } - - block.clear(); + feeEst.processBlock(block_txs, ++blocknum); + block_txs.clear(); } - // Wait for fee estimator to catch up - m_node.validation_signals->SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 9; i++) { // At 9, the original estimate was already at the bottom (b/c scale = 2) BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee);