Merge bitcoin/bitcoin#22014: refactor: Make m_cs_fee_estimator non-recursive

8c277b19c8f262e550cffe263e6d910b687ac882 refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov)
5ee5b696b588695ff78aaac08d5d85154f1953cf refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov)
5c3033d45e5ec15499ce7a0222ffa0210a0f66bc Add thread safety annotations to CBlockPolicyEstimator public functions (Hennadii Stepanov)

Pull request description:

  This PR eliminates the only place that `m_cs_fee_estimator` is recursively locked by refactoring out `_removeTx` member function.

  Related to #19303.

ACKs for top commit:
  theStack:
    Code-review ACK 8c277b19c8f262e550cffe263e6d910b687ac882
  amadeuszpawlik:
    ACK 8c277b19c8f262e550cffe263e6d910b687ac882 reviewed, built and ran tests

Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1

1 files changed, 9 insertions, 2 deletions

diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index d8c21bd833..36cf786bd5 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -493,6 +493,12 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe
 bool CBlockPolicyEstimator::removeTx(uint256 hash, bool inBlock)
 {
     LOCK(m_cs_fee_estimator);
+    return _removeTx(hash, inBlock);
+}
+
+bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock)
+{
+    AssertLockHeld(m_cs_fee_estimator);
     std::map<uint256, TxStatsInfo>::iterator pos = mapMemPoolTxs.find(hash);
     if (pos != mapMemPoolTxs.end()) {
         feeStats->removeTx(pos->second.blockHeight, nBestSeenHeight, pos->second.bucketIndex, inBlock);
@@ -576,7 +582,8 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo
 
 bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry)
 {
-    if (!removeTx(entry->GetTx().GetHash(), true)) {
+    AssertLockHeld(m_cs_fee_estimator);
+    if (!_removeTx(entry->GetTx().GetHash(), true)) {
         // This transaction wasn't being tracked for fee estimation
         return false;
     }
@@ -985,7 +992,7 @@ void CBlockPolicyEstimator::FlushUnconfirmed() {
     // Remove every entry in mapMemPoolTxs
     while (!mapMemPoolTxs.empty()) {
         auto mi = mapMemPoolTxs.begin();
-        removeTx(mi->first, false); // this calls erase() on mapMemPoolTxs
+        _removeTx(mi->first, false); // this calls erase() on mapMemPoolTxs
     }
     int64_t endclear = GetTimeMicros();
     LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, (endclear - startclear)*0.000001);