Skip to content

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Apr 30, 2018

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

@practicalswift practicalswift force-pushed the guarded-by-cs_feeEstimator branch from 827c868 to 6911fad Compare May 14, 2018 15:06
@practicalswift
Copy link
Contributor Author

practicalswift commented May 14, 2018

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

@practicalswift practicalswift force-pushed the guarded-by-cs_feeEstimator branch from 6911fad to a4d815d Compare May 14, 2018 15:11
@DrahtBot
Copy link
Contributor

The last travis run for this pull request was 69 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Jul 22, 2018
@DrahtBot DrahtBot reopened this Jul 22, 2018
@practicalswift
Copy link
Contributor Author

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 20, 2018

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 practicalswift force-pushed the guarded-by-cs_feeEstimator branch 2 times, most recently from 82c3218 to a3909b6 Compare October 8, 2018 13:17
@practicalswift
Copy link
Contributor Author

@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
Copy link
Contributor

utACK 1a01e1c120809b19ddc9c9c6ee5d6686a90f55b3

-BEGIN VERIFY SCRIPT-
sed -i 's/cs_feeEstimator/m_cs_fee_estimator/' src/policy/fees.cpp src/policy/fees.h
-END VERIFY SCRIPT-
@practicalswift practicalswift force-pushed the guarded-by-cs_feeEstimator branch 2 times, most recently from fed8d5e to 764e42f Compare December 2, 2018 20:27
@practicalswift practicalswift force-pushed the guarded-by-cs_feeEstimator branch from 8b40977 to dae1423 Compare December 2, 2018 23:14
@practicalswift
Copy link
Contributor Author

practicalswift commented Dec 19, 2018

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

@maflcko maflcko merged commit dae1423 into bitcoin:master Dec 22, 2018
maflcko pushed a commit that referenced this pull request Dec 22, 2018
…es guarded by cs_feeEstimator

dae1423 Add locking annotations to feeStats, shortStats and longStats (practicalswift)
764e42f scripted-diff: Rename from cs_feeEstimator to m_cs_fee_estimator (practicalswift)
9a789d4 policy: Add Clang thread safety annotations for variables guarded by cs_feeEstimator (practicalswift)

Pull request description:

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

Tree-SHA512: 24b1d876ad53524ee8989b9658ac1a1b2766ebb3b27a1f84601d207e74d090e33738b814afac2a1f5bcd37565abcb361c6e5adae212840ff1ca32c3c42953391
@ryanofsky
Copy link
Contributor

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 practicalswift deleted the guarded-by-cs_feeEstimator branch April 10, 2021 19:36
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Sep 16, 2021
…variables guarded by cs_feeEstimator

dae1423 Add locking annotations to feeStats, shortStats and longStats (practicalswift)
764e42f scripted-diff: Rename from cs_feeEstimator to m_cs_fee_estimator (practicalswift)
9a789d4 policy: Add Clang thread safety annotations for variables guarded by cs_feeEstimator (practicalswift)

Pull request description:

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

Tree-SHA512: 24b1d876ad53524ee8989b9658ac1a1b2766ebb3b27a1f84601d207e74d090e33738b814afac2a1f5bcd37565abcb361c6e5adae212840ff1ca32c3c42953391
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants