From 5c3033d45e5ec15499ce7a0222ffa0210a0f66bc Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 21 May 2021 10:15:52 +0300 Subject: [PATCH 1/3] Add thread safety annotations to CBlockPolicyEstimator public functions --- src/policy/fees.h | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/policy/fees.h b/src/policy/fees.h index c444d71a313..a1fddc95625 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -186,44 +186,55 @@ public: /** Process all the transactions that have been included in a block */ void processBlock(unsigned int nBlockHeight, - std::vector& entries); + std::vector& entries) + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ - void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); + void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Remove a transaction from the mempool tracking stats*/ bool removeTx(uint256 hash, bool inBlock); /** DEPRECATED. Return a feerate estimate */ - CFeeRate estimateFee(int confTarget) const; + CFeeRate estimateFee(int confTarget) const + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Estimate feerate needed to get be included in a block within confTarget * blocks. If no answer can be given at confTarget, return an estimate at * the closest target where one can be given. 'conservative' estimates are * valid over longer time horizons also. */ - CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const; + CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Return a specific fee estimate calculation with a given success * threshold and time horizon, and optionally return detailed data about * calculation */ - CFeeRate estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, EstimationResult *result = nullptr) const; + CFeeRate estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, + EstimationResult* result = nullptr) const + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Write estimation data to a file */ - bool Write(CAutoFile& fileout) const; + bool Write(CAutoFile& fileout) const + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Read estimation data from a file */ - bool Read(CAutoFile& filein); + bool Read(CAutoFile& filein) + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */ - void FlushUnconfirmed(); + void FlushUnconfirmed() + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Calculation of highest target that estimates are tracked for */ - unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const; + unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Drop still unconfirmed transactions and record current estimations, if the fee estimation file is present. */ - void Flush(); + void Flush() + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); private: mutable RecursiveMutex m_cs_fee_estimator; From 5ee5b696b588695ff78aaac08d5d85154f1953cf Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 21 May 2021 10:47:37 +0300 Subject: [PATCH 2/3] refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper This changes removes recursion in the m_cs_fee_estimator locks. --- src/policy/fees.cpp | 11 +++++++++-- src/policy/fees.h | 7 ++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 7da171d2e1c..2b70c18b7ef 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -473,6 +473,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::iterator pos = mapMemPoolTxs.find(hash); if (pos != mapMemPoolTxs.end()) { feeStats->removeTx(pos->second.blockHeight, nBestSeenHeight, pos->second.bucketIndex, inBlock); @@ -556,7 +562,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; } @@ -965,7 +972,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); diff --git a/src/policy/fees.h b/src/policy/fees.h index a1fddc95625..fdbf9569306 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -194,7 +194,8 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Remove a transaction from the mempool tracking stats*/ - bool removeTx(uint256 hash, bool inBlock); + bool removeTx(uint256 hash, bool inBlock) + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** DEPRECATED. Return a feerate estimate */ CFeeRate estimateFee(int confTarget) const @@ -278,6 +279,10 @@ private: unsigned int HistoricalBlockSpan() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); /** Calculation of highest target that reasonable estimate can be provided for */ unsigned int MaxUsableEstimate() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); + + /** A non-thread-safe helper for the removeTx function */ + bool _removeTx(const uint256& hash, bool inBlock) + EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); }; class FeeFilterRounder From 8c277b19c8f262e550cffe263e6d910b687ac882 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 21 May 2021 10:53:05 +0300 Subject: [PATCH 3/3] refactor: Make m_cs_fee_estimator non-recursive --- src/policy/fees.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/policy/fees.h b/src/policy/fees.h index fdbf9569306..c05cabadf94 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -238,7 +238,7 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); private: - mutable RecursiveMutex m_cs_fee_estimator; + mutable Mutex m_cs_fee_estimator; unsigned int nBestSeenHeight GUARDED_BY(m_cs_fee_estimator); unsigned int firstRecordedHeight GUARDED_BY(m_cs_fee_estimator);