policy: Add Clang thread safety annotations for variables guarded by cs

practicalswift commented at 1:48 pm on April 30, 2018: contributor

Add Clang thread safety annotations for variables guarded by cs_feeEstimator
~~Add missing cs_feeEstimator locks~~

laanwj added the label Refactoring on May 1, 2018

in src/policy/fees.h:231 in 827c86802e outdated

229@@ -230,10 +230,12 @@ class CBlockPolicyEstimator
230     unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const;
231 
232 private:
233-    unsigned int nBestSeenHeight;
234-    unsigned int firstRecordedHeight;
235-    unsigned int historicalFirst;
236-    unsigned int historicalBest;
237+    mutable CCriticalSection cs_feeEstimator;

MarcoFalke commented at 2:52 pm on May 14, 2018:

Might as well rename to m_cs_fee_estimator according to developer notes.

practicalswift force-pushed on May 14, 2018

practicalswift commented at 3:07 pm on May 14, 2018: contributor

@MarcoFalke Good idea. Renamed from cs_feeEstimator to m_cs_fee_estimator. Please re-review :-)

practicalswift force-pushed on May 14, 2018

DrahtBot commented at 11:49 pm on July 22, 2018: member

DrahtBot closed this on Jul 22, 2018

DrahtBot reopened this on Jul 22, 2018

practicalswift commented at 4:22 pm on August 27, 2018: contributor

@MarcoFalke Would you mind re-review this locking PR you looked at before? :-)

in src/policy/fees.cpp:713 in a4d815d5d1 outdated

710@@ -710,6 +711,7 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
711 
712 unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon horizon) const
713 {
714+    LOCK(m_cs_fee_estimator);

Empact commented at 7:07 am on September 2, 2018:

I don’t think we need exclusive locking here, as GetMaxConfirms is a read so concurrent access wouldn’t result in incoherent data.

in src/policy/fees.cpp:675 in a4d815d5d1 outdated

671@@ -672,6 +672,8 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) const
672 
673 CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, EstimationResult* result) const
674 {
675+    LOCK(m_cs_fee_estimator);

Empact commented at 7:07 am on September 2, 2018:

get is a read of the ptr value, which is only updated in the constructor, so this lock is unnecessary AFAICT. PT_GUARDED_BY could be more appropriate.

DrahtBot commented at 10:37 pm on September 20, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift force-pushed on Oct 8, 2018

practicalswift commented at 1:20 pm on October 8, 2018: contributor

@Empact @MarcoFalke Updated. Would you mind re-reviewing? The current version is annotations only, so no change in locking. In other words: this PR changes only compile-time checking and no runtime behaviour. Should hence hopefully be trivial to review :-)

ken2812221 commented at 7:11 am on December 2, 2018: contributor

utACK 1a01e1c120809b19ddc9c9c6ee5d6686a90f55b3

policy: Add Clang thread safety annotations for variables guarded by cs_feeEstimator 9a789d4dc6

scripted-diff: Rename from cs_feeEstimator to m_cs_fee_estimator

-BEGIN VERIFY SCRIPT-
sed -i 's/cs_feeEstimator/m_cs_fee_estimator/' src/policy/fees.cpp src/policy/fees.h
-END VERIFY SCRIPT-

764e42fee2

in src/policy/fees.h:251 in 1a01e1c120 outdated

247+    std::map<uint256, TxStatsInfo> mapMemPoolTxs GUARDED_BY(m_cs_fee_estimator);
248 
249     /** Classes to track historical data on transaction confirmations */
250     std::unique_ptr<TxConfirmStats> feeStats;
251     std::unique_ptr<TxConfirmStats> shortStats;
252     std::unique_ptr<TxConfirmStats> longStats;

Empact commented at 8:01 am on December 2, 2018:

nit: did you try marking these PT_GUARDED_BY? Seems it would be appropriate, based on the lock in estimateRawFee.

practicalswift force-pushed on Dec 2, 2018

Add locking annotations to feeStats, shortStats and longStats dae1423e5a

practicalswift force-pushed on Dec 2, 2018

practicalswift commented at 3:29 pm on December 19, 2018: contributor

Any chance of getting this reviewed? It is a compile time check only :-)

MarcoFalke merged this on Dec 22, 2018

MarcoFalke closed this on Dec 22, 2018

MarcoFalke referenced this in commit 1ac7d599f9 on Dec 22, 2018

ryanofsky commented at 6:26 pm on January 7, 2019: member

Adding EXCLUSIVE_LOCKS_REQUIRED(cs_feeEstimator) to processBlockTx caused a bunch of problems in #10443 due to cs_feeEstimator being a private member. In pr/fee.30, CTxMemPool::removeForBlock would call CBlockPolicyEstimator::processBlock, which would lock cs_feeEstimator, and then call a lambda passed from the caller to loop through transactions, calling CBlockPolicyEstimator::processTx which would call CBlockPolicyEstimator::processBlockTx. The new annotation to added processBlockTx caused the compiler to error on these calls.

The errors were spurious, since cs_feeEstimator was locked the entire time, but because cs_feeEstimator was a private member of CBlockPolicyEstimator, there was no way to annotate the lambda asserting that cs_feeEstimator was locked. Making the mutex public would have solved the problem, but instead in pr/fee.31 I opted to fix the problem by hiding the processBlockTx call in another callback to be less invasive and simplify some other things.

Just wanted to note this because some people were curious about it. I thought it was interesting because:

There can sometimes be a tension between wanting to make mutexes private, and wanting to make assertions about them in code outside the class.
There doesn’t seem to be any way to annotate a std::function variable asserting that a mutex must always be locked before the function is called, and requiring the compiler to check this. It’s possible to annotate a lambda assigned to a std::function variable, asserting that various mutexes are locked, but these assertions are completely unchecked and uncheckable without a way to annotate std::functions.

practicalswift deleted the branch on Apr 10, 2021

christiancfifi referenced this in commit 008d7ea717 on Aug 24, 2021

christiancfifi referenced this in commit 1a173df3ae on Aug 24, 2021

christiancfifi referenced this in commit 58b6a0cc7a on Aug 25, 2021

vijaydasmp referenced this in commit 98494b24ba on Sep 13, 2021

vijaydasmp referenced this in commit aa6a7a20c2 on Sep 13, 2021

vijaydasmp referenced this in commit ba2baa00ed on Sep 13, 2021

vijaydasmp referenced this in commit 42b169fa42 on Sep 14, 2021

vijaydasmp referenced this in commit d7bf09d67b on Sep 14, 2021

vijaydasmp referenced this in commit 3dbd958d7c on Sep 16, 2021

PastaPastaPasta referenced this in commit 78e04b92f2 on Sep 16, 2021

DrahtBot locked this on Aug 18, 2022

policy: Add Clang thread safety annotations for variables guarded by cs_feeEstimator #13128

Conflicts