tx fees, policy: update CBlockPolicyEstimator's processBlock para…

…meter

Update `processBlock` parameter to reference to a vector of `CTransactionRef`
instead of reference to a vector of `CTxMempoolEntry`.

This will enable fee estimator to process txs removed from new block from
`CValidationInterface` `MempoolTransactionsRemovedForConnectedBlock` callback notifications.

Co-authored-by: Matt Corallo <git@bluematt.me>

src/policy/fees.cpp

@@ -11,7 +11,6 @@
 #include <kernel/mempool_entry.h>
 #include <logging.h>
 #include <policy/feerate.h>
-#include <primitives/transaction.h>
 #include <random.h>
 #include <serialize.h>
 #include <streams.h>
@@ -604,36 +603,39 @@ void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo&
     assert(bucketIndex == bucketIndex3);
 }
 
-bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry)
+bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTransactionRef& tx)
 {
     AssertLockHeld(m_cs_fee_estimator);
-    if (!_removeTx(entry->GetTx().GetHash(), true)) {
+    std::map<uint256, TxStatsInfo>::iterator pos = mapMemPoolTxs.find(tx->GetHash());
+    if (pos == mapMemPoolTxs.end()) {
         // This transaction wasn't being tracked for fee estimation
         return false;
     }
 
     // How many blocks did it take for miners to include this transaction?
     // blocksToConfirm is 1-based, so a transaction included in the earliest
     // possible block has confirmation count of 1
-    int blocksToConfirm = nBlockHeight - entry->GetHeight();
+    int blocksToConfirm = nBlockHeight - pos->second.blockHeight;
+
+    // Feerates are stored and reported as BTC-per-kb:
+    CAmount feerate = pos->second.m_fee_per_k;
+    _removeTx(tx->GetHash(), true);
+
     if (blocksToConfirm <= 0) {
         // This can't happen because we don't process transactions from a block with a height
         // lower than our greatest seen height
         LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy error Transaction had negative blocksToConfirm\n");
         return false;
     }
 
-    // Feerates are stored and reported as BTC-per-kb:
-    CFeeRate feeRate(entry->GetFee(), entry->GetTxSize());
-
-    feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
-    shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
-    longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
+    feeStats->Record(blocksToConfirm, (double)feerate);
+    shortStats->Record(blocksToConfirm, (double)feerate);
+    longStats->Record(blocksToConfirm, (double)feerate);
     return true;
 }
 
 void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
-                                         std::vector<const CTxMemPoolEntry*>& entries)
+                                         std::vector<CTransactionRef>& txs_removed_for_block)
 {
     LOCK(m_cs_fee_estimator);
     if (nBlockHeight <= nBestSeenHeight) {
@@ -662,8 +664,8 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
 
     unsigned int countedTxs = 0;
     // Update averages with data points from current block
-    for (const auto& entry : entries) {
-        if (processBlockTx(nBlockHeight, entry))
+    for (const auto& tx : txs_removed_for_block) {
+        if (processBlockTx(nBlockHeight, tx))
             countedTxs++;
     }
 
@@ -674,7 +676,7 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
 
 
     LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy estimates updated by %u of %u block txs, since last block %u of %u tracked, mempool map size %u, max target %u from %s\n",
-             countedTxs, entries.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(),
+             countedTxs, txs_removed_for_block.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(),
              MaxUsableEstimate(), HistoricalBlockSpan() > BlockSpan() ? "historical" : "current");
 
     trackedTxs = 0;

src/policy/fees.h

@@ -7,6 +7,7 @@
 
 #include <consensus/amount.h>
 #include <policy/feerate.h>
+#include <primitives/transaction.h>
 #include <random.h>
 #include <sync.h>
 #include <threadsafety.h>
@@ -35,7 +36,6 @@ static constexpr std::chrono::hours MAX_FILE_AGE{60};
 static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false};
 
 class AutoFile;
-class CTxMemPoolEntry;
 class TxConfirmStats;
 
 struct NewMempoolTransactionInfo;
@@ -204,7 +204,7 @@ class CBlockPolicyEstimator
 
     /** Process all the transactions that have been included in a block */
     void processBlock(unsigned int nBlockHeight,
-                      std::vector<const CTxMemPoolEntry*>& entries)
+                      std::vector<CTransactionRef>& txs_removed_for_block)
         EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
 
     /** Process a transaction accepted to the mempool*/
@@ -293,7 +293,7 @@ class CBlockPolicyEstimator
     std::map<double, unsigned int> bucketMap GUARDED_BY(m_cs_fee_estimator); // Map of bucket upper-bound to index into all vectors by bucket
 
     /** Process a transaction confirmed in a block*/
-    bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
+    bool processBlockTx(unsigned int nBlockHeight, const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 
     /** Helper for estimateSmartFee */
     double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon, EstimationResult *result) const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);

src/test/fuzz/policy_estimator.cpp

@@ -58,12 +58,12 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
                     const CTransaction tx{*mtx};
                     mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
                 }
-                std::vector<const CTxMemPoolEntry*> ptrs;
-                ptrs.reserve(mempool_entries.size());
+                std::vector<CTransactionRef> txs;
+                txs.reserve(mempool_entries.size());
                 for (const CTxMemPoolEntry& mempool_entry : mempool_entries) {
-                    ptrs.push_back(&mempool_entry);
+                    txs.push_back(mempool_entry.GetSharedTx());
                 }
-                block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral<unsigned int>(), ptrs);
+                block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral<unsigned int>(), txs);
             },
             [&] {
                 (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool());

src/txmempool.cpp

@@ -496,13 +496,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
     // We increment mempool sequence value no matter removal reason
     // even if not directly reported below.
     uint64_t mempool_sequence = GetAndIncrementSequence();
-
+    const uint256 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(),
@@ -511,8 +514,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
         it->GetFee(),
         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);
 
@@ -533,7 +534,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
@@ -639,17 +639,6 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
 void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight)
 {
     AssertLockHeld(cs);
-    std::vector<const CTxMemPoolEntry*> entries;
-    for (const auto& tx : vtx)
-    {
-        uint256 hash = tx->GetHash();
-
-        indexed_transaction_set::iterator i = mapTx.find(hash);
-        if (i != mapTx.end())
-            entries.push_back(&*i);
-    }
-    // Before the txs in the new block have been removed from the mempool, update policy estimates
-    if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);}
     std::vector<CTransactionRef> txs_removed_for_block;
     txs_removed_for_block.reserve(vtx.size());
     for (const auto& tx : vtx)
@@ -664,6 +653,10 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
         removeConflicts(*tx);
         ClearPrioritisation(tx->GetHash());
     }
+    // Before the txs in the new block have been removed from the mempool, update policy estimates
+    if (minerPolicyEstimator) {
+        minerPolicyEstimator->processBlock(nBlockHeight, txs_removed_for_block);
+    }
     GetMainSignals().MempoolTransactionsRemovedForConnectedBlock(txs_removed_for_block, nBlockHeight);
     lastRollingFeeUpdate = GetTime();
     blockSinceLastRollingFeeBump = true;