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.

1 files 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<std::chrono::duration<std::uint64_t>>(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