refactor: Make m_cs_fee_estimator non-recursive #22014

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:210521-fees changing 2 files +37 −14

hebasto commented at 8:21 AM on May 21, 2021: member

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

Related to #19303.
Add thread safety annotations to CBlockPolicyEstimator public functions 5c3033d45e
hebasto cross-referenced this on May 21, 2021 from issue Replace all of the RecursiveMutex instances with the Mutex ones by hebasto

in src/policy/fees.h:284 in 5efec94926 outdated

 278 | @@ -267,6 +279,10 @@ class CBlockPolicyEstimator
 279 |      unsigned int HistoricalBlockSpan() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 280 |      /** Calculation of highest target that reasonable estimate can be provided for */
 281 |      unsigned int MaxUsableEstimate() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 282 | +
 283 | +    /** A non-thread-safe helper for the removeTx function */
 284 | +    bool removeTxNonThreadSafeHelper(const uint256& hash, bool inBlock)

MarcoFalke commented at 8:27 AM on May 21, 2021:

    bool _removeTx(const uint256& hash, bool inBlock)

According to similar methods:

    void _RelayTransaction(const uint256& txid, const uint256& wtxid)
        EXCLUSIVE_LOCKS_REQUIRED(cs_main);

hebasto commented at 8:40 AM on May 21, 2021:

Thanks! It's great to have a common approach for such a naming case. Updated.

MarcoFalke commented at 8:28 AM on May 21, 2021: member

haven't reviewed, but I left a style comment
MarcoFalke renamed this:
~~Make m_cs_fee_estimator non-recursive~~
refactor: Make m_cs_fee_estimator non-recursive
on May 21, 2021
refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper
This changes removes recursion in the m_cs_fee_estimator locks.
5ee5b696b5
refactor: Make m_cs_fee_estimator non-recursive 8c277b19c8
hebasto force-pushed on May 21, 2021
hebasto commented at 8:39 AM on May 21, 2021: member
Updated 5efec949265fd2a04f131a2a454ec984b033eb66 -> 8c277b19c8f262e550cffe263e6d910b687ac882 (pr22014.01 -> pr22014.02, diff):
- addressed @MarcoFalke's comment
DrahtBot added the label Refactoring on May 21, 2021
DrahtBot added the label TX fees and policy on May 21, 2021
in src/policy/fees.cpp:565 in 5ee5b696b5 outdated
```
 561 | @@ -556,7 +562,8 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo
 562 |  
 563 |  bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry)
 564 |  {
 565 | -    if (!removeTx(entry->GetTx().GetHash(), true)) {
 566 | +    AssertLockHeld(m_cs_fee_estimator);
```
amadeuszpawlik commented at 1:30 PM on May 21, 2021:

Is this assert needed? The same assertion is done in _removeTx on the line below
hebasto commented at 2:42 PM on May 21, 2021:

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, the AssertLockHeld(m_cs_fee_estimator) will emit the -Wthread-safety-analysis warning

the code below could be changed in the future, but AssertLockHeld in the first line will guard thread safety during both compile-time and run-time

Actually, I just followed a pattern from our Developer Notes.
DrahtBot commented at 6:05 AM on May 26, 2021: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on May 26, 2021 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
DrahtBot cross-referenced this on Sep 23, 2021 from issue Package-aware fee estimation by darosior
theStack approved
theStack commented at 4:58 PM on October 14, 2021: contributor

Code-review ACK 8c277b19c8f262e550cffe263e6d910b687ac882

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.
in src/policy/fees.h:283 in 8c277b19c8
```
 278 | @@ -267,6 +279,10 @@ class CBlockPolicyEstimator
 279 |      unsigned int HistoricalBlockSpan() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 280 |      /** Calculation of highest target that reasonable estimate can be provided for */
 281 |      unsigned int MaxUsableEstimate() const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 282 | +
 283 | +    /** A non-thread-safe helper for the removeTx function */
```
MarcoFalke commented at 4:07 PM on October 26, 2021:

why is it not thread safe when it requires the caller to acquire the lock?

edit: Just a nit, so feel free to ignore.

hebasto commented at 5:50 AM on October 27, 2021:

... 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?
amadeuszpawlik approved
amadeuszpawlik commented at 4:45 PM on December 2, 2021: contributor

ACK 8c277b19c8f262e550cffe263e6d910b687ac882 reviewed, built and ran tests
MarcoFalke merged this on Dec 2, 2021
MarcoFalke closed this on Dec 2, 2021
hebasto deleted the branch on Dec 2, 2021
sidhujag referenced this in commit c6c63e40d8 on Dec 3, 2021
RandyMcMillan referenced this in commit 4f92fcea0c on Dec 23, 2021
bitcoin locked this on Dec 2, 2022

Contributors

Labels

Refactoring TX fees and policy

Linked (view graph)

#19303 Replace all of the RecursiveMutex instances with the Mutex ones #34745 refactor: replace `ArgsManager::cs_args RecursiveMutex` with `Mutex`

refactor: Make m_cs_fee_estimator non-recursive #22014

Conflicts