From a0e3eb7549d2ba4dd3af12b9ce65e29158f59078 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 11:01:52 +0100 Subject: [PATCH] tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition If the removal reason of a transaction is BLOCK, then the `removeTx` boolean argument should be true. Before this PR, `CBlockPolicyEstimator` have to complete updating the fee stats before the mempool clears that's why having removeTx call outside reason!= `BLOCK` in `addUnchecked` was not a bug. But in a case where the `CBlockPolicyEstimator` update is asynchronous, the mempool might clear before we update the `CBlockPolicyEstimator` fee stats. Transactions that are removed for `BLOCK` reasons will also be incorrectly removed from `CBlockPolicyEstimator` stats as failures. --- src/txmempool.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e057d7ece1..7fd9c1cc25 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -497,12 +497,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); + const auto& hash = it->GetTx().GetHash(); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); + if (minerPolicyEstimator) { + minerPolicyEstimator->removeTx(hash, false); + } } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -512,7 +516,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) std::chrono::duration_cast>(it->GetTime()).count() ); - const uint256 hash = it->GetTx().GetHash(); for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); @@ -535,7 +538,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) cachedInnerUsage -= memusage::DynamicUsage(it->GetMemPoolParentsConst()) + memusage::DynamicUsage(it->GetMemPoolChildrenConst()); mapTx.erase(it); nTransactionsUpdated++; - if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);} } // Calculates descendants of entry that are not already in setDescendants, and adds to