This is a follow-up to #10866which this PR is rebased upon (awaiting merge of #10866). The first three commits (8a1dc09, e022990 and cbae151) can be reviewed in #10866.
This PR contains three types of changes:
Add missing locks (based on the GUARDED_BY(...) analysis below). Please review thoroughly.
Add GUARDED_BY(...) annotations to allow for Clang's thread safety analysis at compile-time. Does not change run-time behaviour.
Add EXCLUSIVE_LOCKS_REQUIRED(...) annotations to allow for Clang's thread safety analysis at compile-time. Does not change run-time behaviour.
Background reading: The "C/C++ Thread Safety Analysis" paper (Hutchins, Ballman & Sutherland, 2014) paper describing Clang thread safety analysis and how it is used for the Google C++ codebase:
They essentially provide a static type system for threads, and can detect potential race conditions and deadlocks. This paper describes Clang Thread Safety Analysis, a tool which uses annotations to declare and enforce thread safety policies in C and C++ programs.
[…]
It has been deployed on a large scale at Google; all C++ code at Google is now compiled with thread safety analysis enabled by default.
[…]
The GUARDED_BY attribute declares that a thread must lock mu before it can read or write to balance, thus ensuring that the increment and decrement operations are atomic. Similarly, REQUIRES declares that the calling thread must lock mu before calling withdrawImpl. Because the caller is assumed to have locked mu, it is safe to modify balance within the body of the method.
sipa
commented at 12:21 AM on September 4, 2017:
member
Awesome, I'm very happy to see someone work on this again.
fanquake added the label Refactoring on Sep 4, 2017
practicalswift force-pushed on Sep 4, 2017
practicalswift force-pushed on Sep 4, 2017
sipa
commented at 9:00 AM on September 4, 2017:
member
You must have introduced a lock inversion, based on the Travis output.
promag
commented at 9:24 AM on September 4, 2017:
member
Squash commits Add GUARDED_BY(cs_KeyStore) and also Add GUARDED_BY(cs) annotation.
practicalswift force-pushed on Sep 4, 2017
practicalswift force-pushed on Sep 4, 2017
practicalswift force-pushed on Sep 4, 2017
practicalswift
commented at 3:25 PM on September 4, 2017:
contributor
@sipa Thanks for letting me know! I've now removed the commit with the incorrectly added LOCK(walletInstance->cs_KeyStore) in wallet.cpp that caused the lock inversion. Do you which variables cs_KeyStore is meant to guard?
practicalswift
commented at 3:28 PM on September 4, 2017:
contributor
@promag Fixed! :-) Regarding the repeated GUARDED_BY(cs) commit messages - these actually refer to different cs:es. To make things less confusing I've now disambiguated them by adding the class names in the commit message.
Furthermore I've added an commit which renames renames CAddrMan.cs to CAddrMan.cs_addrMan, and CTxMemPool.cs to CTxMemPool.cs_txMemPool.
practicalswift force-pushed on Sep 4, 2017
TheBlueMatt
commented at 8:17 PM on September 4, 2017:
contributor
Concept ACK. This will generate some turbulence in terms of needing to rebase a ton of existing PRs, so maybe it makes sense to break this into several PRs which only touch given modules? In any case, excited to see this stuff happen.
laanwj
commented at 10:28 PM on September 6, 2017:
member
Concept ACK. Though I'm not sure what a good time to merge a huge, spread-out (though low-risk) change like this is. Maybe just before the 0.16 split?
practicalswift force-pushed on Sep 11, 2017
practicalswift
commented at 8:34 AM on September 11, 2017:
contributor
@laanwj Good point! When is the 0.16 split expected to happen?
The smaller PR #10866 lays the groundwork for the subsequent thread-safety-analysis annotation work. Having that PR merged a bit earlier would help a lot :-)
practicalswift
commented at 8:40 AM on September 11, 2017:
contributor
@TheBlueMatt Sure, I can split this PR into multiple parts to ease reviewing! Just let me know which parts/modules to split it on :-) To avoid duplicating the commits in #10866 we should probably wait until #10866 is merged before splitting this PR up. Sounds reasonable?
practicalswift
commented at 9:13 AM on September 11, 2017:
contributor
@sipa@TheBlueMatt@laanwj Thanks for reviewing and thanks for the encouragement! Do you happen to know which core developers that would be appropriate reviewers for this work (on a deeper GUARDED_BY(…) level for each module)?
The types of errors I might need some help from reviewers to avoid:
False positives: Too restrictive lock requirements. Requiring a lock where a lock is not needed. More concretely: unnecessary GUARDED_BY(…) annotations. (The EXCLUSIVE_LOCKS_REQUIRED(…) annotations follow more or less mechanically for the GUARDED_BY(…).)
False negatives: Missing lock requirements. Not requiring a lock where a lock is needed. More concretely: missing GUARDED_BY(…) annotations.
And perhaps most importantly: help reviewing that the added locks in this PR looks reasonable. In constrast to the annotations they alter run-time behaviour.
There are a couple of AssertLockHeld(…):s that I've been unable to map to variables being guarded. I might need some help to track those down to add the corresponding GUARDED_BY(…):s (and hence EXCLUSIVE_LOCKS_REQUIRED(…)). I'm a bit hesitant to just add EXCLUSIVE_LOCKS_REQUIRED(…) to match the AssertLockHeld(…) (which would be trivial) since that methodology hides the fact that the guard required hasn't been explicitly stated using an GUARDED_BY(…) annotation yet.
practicalswift force-pushed on Sep 13, 2017
practicalswift
commented at 8:39 PM on September 13, 2017:
contributor
The code base contains 37 locks and in the process of documenting them I compiled this list of the initial creators of each lock. These lock introducers would likely be the best possible reviewers for the GUARDED_BY(…):s in this PR for "their" respective locks :-)
practicalswift
commented at 8:27 PM on September 14, 2017:
contributor
I'm now down to only four remaining AssertLockHeld(…):s for which I'm unable to identify which specific variable the lock in question is being a guard for.
For each of the above cases I see two alternatives:
A GUARDED_BY(…) for some variable accessed directly or indirectly is missing.
The lock is not technically necessary.
practicalswift
commented at 9:17 AM on September 15, 2017:
contributor
I've now documented the five places where I'm opting out of thread-safety analysis for various reasons (using the NO_THREAD_SAFETY_ANALYSIS annotation): b72ad1f43d96b6f63e7a4046f31af085da4e86db
Please let me know if there is a way to remove any of these opt-outs.
TheBlueMatt
commented at 6:25 PM on September 15, 2017:
contributor
GetBlockScriptFlags() needs cs_main for versionbitscache (maybe others, but probably just that?)
AcceptBlockHeader is mostly for mapBlockIndex it looks like
RemoveWatchOnly looks like it may need to be an atomic operation, but I haven't dug into it too much. It may very well be the case that you'd miss a NotifyWatchonlyChanged fire if you have an AddWatchOnly and a RemoveWatchOnly call at the same time (though I don't know if there are any negative effects from doing so).
RescanFromTime indeed does not appear to require cs_wallet (obviously ScanForWalletTransactions does, but it takes it itself). It obviously needs cs_main, though.
practicalswift
commented at 6:48 PM on September 15, 2017:
contributor
@TheBlueMatt Thanks! I'll look into the suggested fixes.
practicalswift referenced this in commit bf3c3b5414 on Sep 16, 2017
practicalswift
commented at 2:22 PM on September 16, 2017:
contributor
@jonasschnelli@TheBlueMatt In CDB::PeriodicFlush(CWalletDBWrapper& dbw) (code) we are currently trying to lock bitdb.cs_db – shouldn't that lock be on env->cs_db instead (CDBEnv *env = dbw.env)? Or am I reading it wrong? :-)
practicalswift
commented at 6:12 PM on September 16, 2017:
contributor
@laanwj, after some digging I found your bitdb reference cleanup commit be9e1a968debbb7ede8ed50e9288a62ff15d1e1e, perhaps you know the answer to the question about the bitdb.cs_db lock above? :-)
practicalswift referenced this in commit 5f4e12df29 on Sep 18, 2017
ryanofsky
commented at 3:20 PM on November 2, 2017:
contributor
This change is great, but I might suggest breaking off and grouping the ADD_LOCK commits into a few separate PRs. That way the runtime changes can be more easily and carefully vetted, and this PR can exclusively focus on adding compiler annotations and also be more easily reviewed.
practicalswift
commented at 4:47 PM on November 2, 2017:
contributor
practicalswift
commented at 5:16 PM on November 2, 2017:
contributor
@ryanofsky One slight problem is that this PR won't compile cleanly unless the locks are added. But you thinking was that the individual "add locks"-PR should be merged before this PR gets merged (and hence no need for add locks in this PR when it is time to merge it)? That works for me :-)
ryanofsky
commented at 5:24 PM on November 2, 2017:
contributor
Yeah, I was thinking there would be a handful of other prs which this would depend on since the LOCKs are in different parts of the code, and different people might want to review them. Those PRs would all have their own branches, and the branch for this PR could just git merge in those branches until they get merged to master.
practicalswift
commented at 5:42 PM on November 2, 2017:
contributor
practicalswift referenced this in commit 3388e05887 on Nov 9, 2017
TheBlueMatt
commented at 1:00 AM on November 10, 2017:
contributor
Alright, with the depends, it seems like time to start breaking this up, no? There is no way this is going to get merged in one go, though its been great to see this keep getting rebased since it has caught a few errors already :).
ryanofsky
commented at 1:06 AM on November 10, 2017:
contributor
Pretty sure this is already happening. There have been a bunch of new prs adding locking. As they are merged, the "ADD LOCK" commits here can go away and this will shrink, only adding annotations without changing behaviour.
TheBlueMatt
commented at 3:13 AM on November 10, 2017:
contributor
@ryanofsky I was referring to adding the annotations - as far as I've seen the only @practicalswift PRs in the locking space have been adding missing locks detected by the annotations, not actually adding the annotations.
Pretty sure this is already happening. There have been a bunch of new
prs adding locking. As they are merged, the "ADD LOCK" commits here can
go away and this will shrink, only adding annotations without changing
behaviour.
ryanofsky
commented at 3:39 AM on November 10, 2017:
contributor
Since annotations don't change behaviour, it seems to me that having one PR that just adds annotations is fine and probably easier to review than lots of different PRs adding annotations.
Adding new locks is much more dicey, and changes that add new locks should be smaller and get more careful review. This is happening now with #11634, #11623, #11596, etc, and I'm assuming there will be more.
@practicalswift, you may want to add WIP / Work in progress to the description here to indicate that this is currently a work in progress, and that this PR will eventually only add annotations and not change locking or other runtime behavior, since right now it is doing a little of everything.
TheBlueMatt
commented at 4:06 AM on November 10, 2017:
contributor
I tend to disagree - if we add annotations and get them wrong we're
likely to fool ourselves in the future on locking changes - making sure
that a function which doesn't access variables which need locks but
which needs results from two other function calls to be consistent has
the appropriate annotation may end up pretty important. Even better, we
should take this as an opportunity to fix obviously bad locking and
reduce the coverage of locks when there are easy places to do so!
On 11/09/17 22:39, Russell Yanofsky wrote:
Since annotations don't change behaviour, it seems to me that having one
PR that /just/ adds annotations is fine and probably easier to review
than lots of different PRs adding annotations.
@practicalswifthttps://github.com/practicalswift, you may want to add
WIP / Work in progress to the description here to indicate that this is
currently a work in progress, and that this PR will eventually only add
annotations and not change locking or other runtime behavior, since
right now it is doing a little of everything.
MarcoFalke referenced this in commit ee92243e66 on Nov 10, 2017
practicalswift force-pushed on Nov 10, 2017
practicalswift referenced this in commit 601883bb5b on Nov 10, 2017
practicalswift force-pushed on Nov 10, 2017
practicalswift referenced this in commit 25f5555d64 on Nov 10, 2017
practicalswift
commented at 7:16 PM on November 10, 2017:
contributor
@ryanofsky@TheBlueMatt What about doing both: I start by submitting the LOCK(…):s as separate PR:s and when these are merged I continue by submitting the annotations (GUARDED_BY(…)/EXCLUSIVE_LOCKS_REQUIRED(…)) in separate PR:s. All while keeping this PR updated with both types of changes. Sounds good?
Regarding the latter, should I submit the annotation PR:s on a per-lock basis (i.e. one PR with all cs_wallet annotations, etc.)?
Add EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile). cs_LastBlockFile is guarding nLastBlockFile.1a3dbf3cb7
Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding nWalletVersion and nWalletMaxVersion.9c9d4d6f83
Rename CAddrMan.cs to CAddrMan.cs_addrMan. Rename CTxMemPool.cs to CTxMemPool.cs_txMemPool.855c071fcb
Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding nOrderPosNext.00f5c213a2
Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapKeyMetadata.468ad0148d
Add EXCLUSIVE_LOCKS_REQUIRED(cs_txMemPool). cs_txMemPool is guarding minerPolicyEstimator, cachedInnerUsage, lastRollingFeeUpdate, blockSinceLastRollingFeeBump and rollingMinimumFeeRate.cb8a8d19ba
Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapTxSpends and nTimeFirstKey.a2d7d5693a
Add EXCLUSIVE_LOCKS_REQUIRED(mempool.cs_txMemPool). mempool.cs_txMemPool is guarding mempool.vTxHashes.1aa043c306
Rename locks to get consistent and unique names
* Rename cs to cs_test
* Rename csPathCached to cs_pathCached
* Rename SingleThreadedSchedulerClient.m_cs_callbacks_pending to SingleThreadedSchedulerClient.cs_callbacks_pending
f3a9665abb
Add comment about fRPCInWarmup/rpcWarmupStatus being GUARDED_BY(cs_rpcWarmup)45c436be3d
Add GUARDED_BY(cs_wallet) annotation for nWalletVersion/nWalletMaxVersionfe7be0bc13
Add GUARDED_BY(cs_main) annotation0e1f2cb133
Add PT_GUARDED_BY(cs_main) annotation2e421e2314
Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding *pcoinsTip.b852213040
Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding *pblocktree.0845e5da22
Add LOCK(cs_main) where accessing chainActivee1d589b4f8
Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding chainActive.9f95edba43
Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding scriptExecutionCache.3a622a2b3f
CWallet::CreateWalletFromFile(...): Change from NO_THREAD_SAFETY_ANALYSIS to EXCLUSIVE_LOCKS_REQUIRED(cs_main)cbb6033f02
Document all NO_THREAD_SAFETY_ANALYSIS annotationsf2b4aa9d5f
Add LOCK(cs_main) when accessing mapBlockIndex2997281331
Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding mapBlockIndex.147f44930b
Remove AssertLockHeld(cs_wallet) from CWallet::RescanFromTime(...)
cs_wallet not needed. ScanForWalletTransactions(...) takes it itself.
See https://github.com/bitcoin/bitcoin/pull/11226#issuecomment-329863375
c145fe8bad
Add LOCK(cs_addrMan) when accessing vRandombe28bf1b96
Use GUARDED_BY(...) instead of PT_GUARDED_BY(...) for guarded std::shared_ptr<T>:s08dc5d679a
Add LOCK(pwallet->cs_wallet) when accessing mapWallet (via CWallet::IsSpent(...))a1c20893c6
Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding versionbitscache.aedf8de368
Operate directly on pool->vTxHashes to resolve Clang thread-safety analysis aliasing confusion32ba72a021
[tentative/experimental change, needs review before merge] Lock env->cs_db instead of bitdb.cs_db0c470a1611
Make sure the cs_main lock covers all chainActive accesses while limiting the lock scope. Avoid keeping cs_main locked during wait_until(...).3d61525a9b
Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding mapBlockIndex.33f2089264
Add GUARDED_BY(cs_filter). Add EXCLUSIVE_LOCKS_REQUIRED(cs_filter). cs_main is guarding fRelayTxes.ed86da25ea
Add GUARDED_BY(cs_main). cs_main is guarding versionbitscache.cc900ec4c4
Add GUARDED_BY(cs_main). cs_main is guarding mapOrphanTransactions.56a201b153
Add GUARDED_BY(cs_warnings). cs_warnings is guarding fLargeWorkForkFound/fLargeWorkInvalidChainFound.288630a83c
Add LOCK(cs_KeyStore) when accessing mapKeys/mapWatchKeys/mapScripts/setWatchOnly195196332c
Add GUARDED_BY(cs_KeyStore). cs_KeyStore is guarding mapKeys/mapWatchKeys/mapScripts/setWatchOnly.00d046fe68
Add NO_THREAD_SAFETY_ANALYSIS annotation for Init(const Options& connOptions)1752e15b7b
Add GUARDED_BY(cs_mapLocalHost). cs_mapLocalHost is guarding mapLocalHost/vfLimited.01097f7c47
Add PT_GUARDED_BY(cs_main). cs_main is guarding *pindexBestHeader.4b003e8b08
Add GUARDED_BY(cs_wallet). Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapAddressBook.c42ed7da9b
Add GUARDED_BY(cs_callbacks_pending). cs_callbacks_pending is guarding m_are_callbacks_running.b7dceee2ce
Add GUARDED_BY(cs_LastBlockFile). Add EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile). cs_LastBlockFile is guarding fCheckForPruning.f7560f7e33
Add GUARDED_BY(cs_KeyStore). cs_KeyStore is guarding mapCryptedKeys.df76e327ae
Add SendMessages(...) lock requirement (pto->cs_sendProcessing) also to the header filec534d2b3a8
Add EXCLUSIVE_LOCKS_REQUIRED(cs_addrMan) - needed when building with DEBUG_ADDRMAN. cs_addrMan is guarding vRandom.e442df06e2
Revert "Add LOCK(node->cs_filter). The variable 'fRelayTxes' is guarded by that mutex."
This reverts commit c17eedee26c492f4b6abd1a4baead30afc4e742c.
d6995395fa
Revert "Add GUARDED_BY(cs_filter). Add EXCLUSIVE_LOCKS_REQUIRED(cs_filter). cs_main is guarding fRelayTxes."
This reverts commit edd8005e1a60d2652544c6287677f19a47a7c63a.
3d05a8f5bd
Revert "Add GUARDED_BY(cs_filter) annotation"
This reverts commit 1d8b7fa0a47b49e538606363db71ad5d1407c866.
8917d74ed0
Add EXCLUSIVE_LOCKS_REQUIRED(cs_main) to StaleBlockRequestAllowed(...)54d4facd15
Add EXCLUSIVE_LOCKS_REQUIRED(env.cs_db). env.cs_db is guarding mapDb.49b8c3adad
Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding chainActive and mapNodeState.1f6ff489e2
Remove EXCLUSIVE_LOCKS_REQUIRED(cs_filter) from PeerLogicValidation::InitializeNode(CNode *pnode)bdca80cffa
Add GUARDED_BY(cs_wallet) to nRelockTime. Add corresponding LOCK(...).2909d8a4ae
Add GUARDED_BY(cs_LastBlockFile) to vinfoBlockFile. Add corresponding LOCK(...).e7b27cddbe
WIP: Fix conditional locking in validateaddress(...)f131976e16
Add refereces to tracking GitHub issues91f8024d02
Remove comment for already merged LOCK(...)358aa923f9
Set m_last_block_processed to nullptr in SetNull(). Acquire mutex cs_wallet when accessing m_last_block_processed.192a550fc8
Avoid locking mutexes that are already held by the same threadbaf7c1ff9e
practicalswift force-pushed on Dec 4, 2017
MarcoFalke
commented at 2:24 PM on January 10, 2018:
member
Needs rebase.
For the purpose of easing review and speeding up merge: What about submitting a subset of this that only renames the comments // protected by ${cs} to GUARDED_BY(${cs}). Assuming that the existing comments are reviewed such a subset should be refactoring-only and easier to merge.
laanwj removed this from the milestone 0.16.0 on Jan 11, 2018
laanwj added this to the milestone 0.17.0 on Jan 11, 2018
practicalswift
commented at 4:22 PM on January 31, 2018:
contributor
@MarcoFalke Do you mean a new PR with the GUARDED_BY(…):s in this PR added instead as comments // GUARDED_BY(…)? Sure, confirm and I'll do that.
Regarding rebasing – I suggest we merge (or close in the case of NACK) the locking related PR:s I currently have open and I'll rebase this PR on top of master after that. Makes sense?
MarcoFalke
commented at 11:38 PM on February 2, 2018:
member
Something like e7f4866d499c4f1a5c17fa140be090da7c940c86, but at this point I am not sure if it is going to help review a lot.
practicalswift
commented at 9:46 PM on February 22, 2018:
contributor
Closing. Will split this into subsets and submit as separate PR:s.
practicalswift closed this on Feb 22, 2018
practicalswift
commented at 10:37 PM on March 12, 2018:
contributor
To facilitate reviewing the most important parts of this thread-safety PR (milestone 0.17.0) have been submitted as individual PR:s –
#12665 – Add compile time checking for all run time locking assertions
#11795 – Avoid locking cs_vNodes twice when calling FindNode(...)
#11762 – Avoid locking mutexes that are already held by the same thread
#11694 – Add missing cs_main lock in getblocktemplate(...)
#11689 – Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…)
#11652 – Add missing locks to init.cpp (in AppInitMain + ThreadImport) and validation.cpp
#11634 – Add missing cs_wallet/cs_KeyStore locks to wallet
#11617 – Call FlushStateToDisk(...) regardless of fCheckForPruning
#11596 – Add missing cs_main locks when accessing chainActive
Reviews of these PR:s would be really appreciated! :-)
I'll tackle the remaining parts once the above PR:s have been merged or closed. Complete thread safety annotation coverage is not far away now! :-)
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC
