refactor: Make m_cs_fee_estimator non-recursive #22014
Conversation
src/policy/fees.h
Outdated
| @@ -267,6 +279,10 @@ class CBlockPolicyEstimator | |||
| 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 removeTxNonThreadSafeHelper(const uint256& hash, bool inBlock) | |||
There was a problem hiding this comment.
| bool removeTxNonThreadSafeHelper(const uint256& hash, bool inBlock) | |
| bool _removeTx(const uint256& hash, bool inBlock) |
According to similar methods:
void _RelayTransaction(const uint256& txid, const uint256& wtxid)
EXCLUSIVE_LOCKS_REQUIRED(cs_main);There was a problem hiding this comment.
Thanks! It's great to have a common approach for such a naming case. Updated.
This changes removes recursion in the m_cs_fee_estimator locks.
|
Updated 5efec94 -> 8c277b1 (pr22014.01 -> pr22014.02, diff):
|
| @@ -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); | |||
There was a problem hiding this comment.
Is this assert needed? The same assertion is done in _removeTx on the line below
There was a problem hiding this comment.
Technically, it is no needed.
There are two reasons for that:
- if somehow annotation
EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator)will disappear (e.g., human fault) from the function declaration, theAssertLockHeld(m_cs_fee_estimator)will emit the-Wthread-safety-analysiswarning - the code below could be changed in the future, but
AssertLockHeldin the first line will guard thread safety during both compile-time and run-time
Actually, I just followed a pattern from our Developer Notes.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
Code-review ACK 8c277b1
Carefully reviewed the m_cs_fee_estimator lock acquires in src/policy/fees.cpp (on master first), concluded that allowing recursion was needed due to the CBlockPolicyEstimator::removeTx(...) method being called in CBlockPolicyEstimator::FlushUnconfirmed(...) and CBlockPolicyEstimator::processBlockTx(...) (in turn called by CBlockPolicyEstimator::processBlock(...)) while holding the lock already. The PR solves this by introducing a private method _removeTx which doesn't take the lock, and replacing the internal calls with this. Also checked that the introduced thread safety annotations make sense.
| @@ -267,6 +279,10 @@ class CBlockPolicyEstimator | |||
| 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 */ | |||
There was a problem hiding this comment.
why is it not thread safe when it requires the caller to acquire the lock?
edit: Just a nit, so feel free to ignore.
There was a problem hiding this comment.
... it requires the caller to acquire the lock
EXCLUSIVE_LOCKS_REQUIRED just produces a warning with clang only. Or do you mean an AssertLockHeld macro in the function body?
From my understanding, every function which requires external locking is not thread safe internally.
Anyway, what are you suggesting: to drop the comment or improve it? If the latter, how?
8c277b1 refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov) 5ee5b69 refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov) 5c3033d 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 bitcoin#19303. ACKs for top commit: theStack: Code-review ACK 8c277b1 amadeuszpawlik: ACK 8c277b1 reviewed, built and ran tests Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1
…cursive 0feeb1f refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov) adeff42 refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov) f74a61b 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 0feeb1f amadeuszpawlik: ACK 0feeb1f reviewed, built and ran tests Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1
This PR eliminates the only place that
m_cs_fee_estimatoris recursively locked by refactoring out_removeTxmember function.Related to #19303.