From cfe585df25c482c157e239fcd6c81b7afe3bab00 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 19 Nov 2025 16:05:25 +0000 Subject: [PATCH] validation: emit block mempool removal signal from ConnectTip Return the removed mempool transaction info from CTxMemPool::removeForBlock instead of dispatching the MempoolTransactionsRemovedForBlock notification from the mempool. Emit it from ConnectTip after mempool removal and before BlockConnected, passing the connected block, the removed mempool transactions, and the block height to the callback. Because the signal now originates from ConnectTip, where the IBD state is known, gate it on !IsInitialBlockDownload(): the notification is no longer fired for blocks connected during initial block download or reindex, while the mempool removal in removeForBlock still runs unconditionally. This keeps fee rate estimators from recording blocks connected before the node is synced. --- src/bench/mempool_stress.cpp | 2 +- src/policy/fees/estimator_man.cpp | 4 ++-- src/policy/fees/estimator_man.h | 2 +- src/test/fuzz/tx_pool.cpp | 2 +- src/test/mempool_tests.cpp | 2 +- src/test/rbf_tests.cpp | 2 +- src/txmempool.cpp | 6 ++---- src/txmempool.h | 2 +- src/validation.cpp | 9 +++++++-- src/validationinterface.cpp | 15 +++++++++------ src/validationinterface.h | 7 +++++-- 11 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index ab1146e1dc9..1d22707075a 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -156,7 +156,7 @@ static void ComplexMemPool(benchmark::Bench& bench) // in the same state at the end of the function, so we benchmark both // mining a block and reorging the block's contents back into the mempool. bench.run([&]() NO_THREAD_SAFETY_ANALYSIS { - pool.removeForBlock(tx_remove_for_block, /*nBlockHeight=*/100); + pool.removeForBlock(tx_remove_for_block); for (auto& tx: tx_remove_for_block) { AddTx(tx, pool, det_rand); } diff --git a/src/policy/fees/estimator_man.cpp b/src/policy/fees/estimator_man.cpp index 472367e8fa3..9b5c5093c46 100644 --- a/src/policy/fees/estimator_man.cpp +++ b/src/policy/fees/estimator_man.cpp @@ -72,9 +72,9 @@ void FeeRateEstimatorManager::TransactionRemovedFromMempool(const CTransactionRe m_block_policy_estimator->removeTx(tx->GetHash()); } -void FeeRateEstimatorManager::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +void FeeRateEstimatorManager::MempoolTransactionsRemovedForBlock(const std::shared_ptr& /*block*/, const std::vector& txs_removed_for_block, unsigned int block_height) { - m_block_policy_estimator->processBlock(txs_removed_for_block, nBlockHeight); + m_block_policy_estimator->processBlock(txs_removed_for_block, block_height); } CFeeRate FeeRateEstimatorManager::BlockPolicyEstimateRawFee(unsigned int target, double threshold, FeeEstimateHorizon horizon, EstimationResult* buckets) const diff --git a/src/policy/fees/estimator_man.h b/src/policy/fees/estimator_man.h index bbcb8aa1b28..0e8c36b8cc0 100644 --- a/src/policy/fees/estimator_man.h +++ b/src/policy/fees/estimator_man.h @@ -81,7 +81,7 @@ protected: /** Overridden from CValidationInterface. */ void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) override; - void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) override; + void MempoolTransactionsRemovedForBlock(const std::shared_ptr& block, const std::vector& txs_removed_for_block, unsigned int block_height) override; private: std::unique_ptr m_block_policy_estimator; diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index c14586f446a..ce59be4270a 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -156,7 +156,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha // Try updating the mempool for this block, as though it were mined. LOCK2(::cs_main, tx_pool.cs); - tx_pool.removeForBlock(block_template->block.vtx, chainstate.m_chain.Height() + 1); + tx_pool.removeForBlock(block_template->block.vtx); // Now try to add those transactions back, as though a reorg happened. std::vector hashes_to_update; diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 0d1e6b779bb..80810f387a8 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -283,7 +283,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) clock += HALFLIFE; BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + DEFAULT_INCREMENTAL_RELAY_FEE); // ... we should keep the same min fee until we get a block - pool.removeForBlock(vtx, 1); + pool.removeForBlock(vtx); clock += HALFLIFE; BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + DEFAULT_INCREMENTAL_RELAY_FEE)/2.0)); // ... then feerate should drop 1/2 each halflife diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index 15134791eb5..8b41f6090a8 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -216,7 +216,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_conflicts_calculator, TestChain100Setup) dummy.clear(); // If we mine the parent_tx's, then the clusters split (102 clusters). - pool.removeForBlock({parent_tx_1, parent_tx_2}, /*nBlockHeight=*/ 1); + pool.removeForBlock({parent_tx_1, parent_tx_2}); // Add some descendants now to each of the direct children (we can do this now that the clusters have split). for (const auto& child : direct_children) { diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e5ec3b32cab..ed758a3ceb8 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -402,7 +402,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) } } -void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) +std::vector CTxMemPool::removeForBlock(const std::vector& vtx) { // Remove confirmed txs and conflicts when a new block is connected, updating the fee logic AssertLockHeld(cs); @@ -420,14 +420,12 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne ClearPrioritisation(tx->GetHash()); } } - if (m_opts.signals) { - m_opts.signals->MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); - } lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; if (!m_txgraph->DoWork(/*max_cost=*/POST_CHANGE_COST)) { LogDebug(BCLog::MEMPOOL, "Mempool in non-optimal ordering after block."); } + return txs_removed_for_block; } void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const diff --git a/src/txmempool.h b/src/txmempool.h index 35339d3f252..7d7aafde781 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -329,7 +329,7 @@ public: * and updates an entry's LockPoints. * */ void removeForReorg(CChain& chain, std::function filter_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector removeForBlock(const std::vector& vtx) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Look up wtxids in the mempool and (partially) sort by mining score. * diff --git a/src/validation.cpp b/src/validation.cpp index 09cb93c998d..27e72df6082 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3087,14 +3087,19 @@ bool Chainstate::ConnectTip( Ticks(time_5 - time_4), Ticks(m_chainman.time_chainstate), Ticks(m_chainman.time_chainstate) / m_chainman.num_blocks_total); - // Remove conflicting transactions from the mempool.; + // Remove conflicting transactions from the mempool. + std::vector txs_removed_for_block; if (m_mempool) { - m_mempool->removeForBlock(block_to_connect->vtx, pindexNew->nHeight); + txs_removed_for_block = m_mempool->removeForBlock(block_to_connect->vtx); disconnectpool.removeForBlock(block_to_connect->vtx); } // Update m_chain & related variables. m_chain.SetTip(*pindexNew); m_chainman.UpdateIBDStatus(); + // Not fired while IBD is active. removeForBlock() above still runs. + if (m_mempool && m_chainman.m_options.signals && !m_chainman.IsInitialBlockDownload()) { + m_chainman.m_options.signals->MempoolTransactionsRemovedForBlock(block_to_connect, std::move(txs_removed_for_block), pindexNew->nHeight); + } UpdateTip(pindexNew); const auto time_6{SteadyClock::now()}; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index ec2f2047da6..128f14a6d54 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -230,13 +230,16 @@ void ValidationSignals::BlockConnected(const ChainstateRole& role, std::shared_p ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } -void ValidationSignals::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +void ValidationSignals::MempoolTransactionsRemovedForBlock(std::shared_ptr block, std::vector txs_removed_for_block, unsigned int block_height) { - auto log_msg = LOG_MSG("%s: block height=%s txs removed=%s", __func__, - nBlockHeight, - txs_removed_for_block.size()); - auto event = [txs_removed_for_block, nBlockHeight, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); + Assume(block); + auto log_msg = LOG_MSG("%s: block hash=%s block height=%s txs removed=%s block txs=%s", __func__, + block->GetHash().ToString(), + block_height, + txs_removed_for_block.size(), + block->vtx.size()); + auto event = [block = std::move(block), txs_removed_for_block = std::move(txs_removed_for_block), block_height, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(block, txs_removed_for_block, block_height); }); }; ENQUEUE_AND_LOG_EVENT(std::move(event), std::move(log_msg)); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 8cfe41380ec..ec70cc09569 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -112,9 +113,11 @@ protected: * as a result of new block being connected. * MempoolTransactionsRemovedForBlock will be fired before BlockConnected. * + * Not fired while initial block download is active. + * * Called on a background thread. */ - virtual void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) {} + virtual void MempoolTransactionsRemovedForBlock(const std::shared_ptr& block, const std::vector& txs_removed_for_block, unsigned int block_height) {} /** * Notifies listeners of a block being connected. * @@ -222,7 +225,7 @@ public: void ActiveTipChange(const CBlockIndex&, bool); void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); - void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); + void MempoolTransactionsRemovedForBlock(std::shared_ptr, std::vector, unsigned int block_height); void BlockConnected(const kernel::ChainstateRole&, std::shared_ptr, const CBlockIndex* pindex); void BlockDisconnected(std::shared_ptr, const CBlockIndex* pindex); void ChainStateFlushed(const kernel::ChainstateRole&, const CBlockLocator&);