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

8c277b19c8 refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov)
5ee5b696b5 refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov)
5c3033d45e 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 8c277b19c8
  amadeuszpawlik:
    ACK 8c277b19c8 reviewed, built and ran tests

Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1
This commit is contained in:
MarcoFalke 2021-12-02 19:54:14 +01:00
commit 0b30bdd519
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
2 changed files with 37 additions and 14 deletions

View file

@ -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);

View file

@ -186,47 +186,59 @@ public:
/** Process all the transactions that have been included in a block */
void processBlock(unsigned int nBlockHeight,
std::vector<const CTxMemPoolEntry*>& entries);
std::vector<const CTxMemPoolEntry*>& 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);
bool removeTx(uint256 hash, bool inBlock)
EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
/** 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;
mutable Mutex m_cs_fee_estimator;
unsigned int nBestSeenHeight GUARDED_BY(m_cs_fee_estimator);
unsigned int firstRecordedHeight GUARDED_BY(m_cs_fee_estimator);
@ -267,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