refactor: Make m_cs_fee_estimator non-recursive

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

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:

0    bool _removeTx(const uint256& hash, bool inBlock)

According to similar methods:

0    void _RelayTransaction(const uint256& txid, const uint256& wtxid)
1        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: member

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

Conflicts

No conflicts as of last run.

theStack approved

theStack commented at 4:58 pm on October 14, 2021: member

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

DrahtBot locked this on Dec 2, 2022

refactor: Make m_cs_fee_estimator non-recursive #22014

Conflicts