[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

pull practicalswift wants to merge 124 commits into bitcoin:master from practicalswift:guarded-by-trivial changing 58 files +746 −648
  1. practicalswift commented at 4:51 pm on September 3, 2017: contributor

    Add additional Clang thread safety analysis annotations as discussed with @sipa (https://github.com/bitcoin/bitcoin/pull/10866#issuecomment-316238630) and others.

    This is a follow-up to #10866 which 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:

    1. Add missing locks (based on the GUARDED_BY(...) analysis below). Please review thoroughly.
    2. Add GUARDED_BY(...) annotations to allow for Clang’s thread safety analysis at compile-time. Does not change run-time behaviour.
    3. 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.

  2. practicalswift force-pushed on Sep 3, 2017
  3. practicalswift force-pushed on Sep 3, 2017
  4. sipa commented at 0:21 am on September 4, 2017: member
    Awesome, I’m very happy to see someone work on this again.
  5. fanquake added the label Refactoring on Sep 4, 2017
  6. practicalswift force-pushed on Sep 4, 2017
  7. practicalswift force-pushed on Sep 4, 2017
  8. sipa commented at 9:00 am on September 4, 2017: member
    You must have introduced a lock inversion, based on the Travis output.
  9. 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.
  10. practicalswift force-pushed on Sep 4, 2017
  11. practicalswift force-pushed on Sep 4, 2017
  12. practicalswift force-pushed on Sep 4, 2017
  13. 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?
  14. 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.

  15. practicalswift force-pushed on Sep 4, 2017
  16. TheBlueMatt commented at 8:17 pm on September 4, 2017: member
    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.
  17. 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?
  18. practicalswift force-pushed on Sep 11, 2017
  19. 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 :-)

  20. 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?
  21. 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.

  22. practicalswift force-pushed on Sep 13, 2017
  23. 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 :-)

    Bitcoin locks hall of fame

    Locks introduced by currently active core devs:

    • @TheBlueMatt (10 locks): cs_filter, cs_args, cs_vSend, cs_most_recent_block, cs_callbacks_pending (previously m_cs_callbacks_pending), cs_vAddedNodes, cs_SubVer, cs_addrName, cs_addrLocal, cs_sendProcessing
    • @sipa (8 locks): cs_KeyStore, cs_addrMan (previously cs), cs_mapLocalHost, cs_LastBlockFile, cs_vOneShots, cs_pathCached (previously csPathCached), cs_nTimeOffset, cs_nBlockSequenceId
    • @theuni (3 locks): cs_hSocket, cs_vRecv, cs_vProcessMsg
    • @morcos (2 locks): cs_feeEstimator, cs_feeFilter
    • @gmaxwell (1 lock): cs_warnings

    Previous lock introducers:

    • Locks included in the first commit in the repo (Satoshi locks!): cs_main, cs_vNodes, cs_db, cs_inventory, cs_Shutdown
    • gavinandresen: cs_wallet, cs_txMemPool (previously cs), cs_setBanned
    • sje397: cs_totalBytesSent, cs_totalBytesRecv
    • Clark Gaebel: cs_test (previously cs)
    • domob1812: cs_rpcWarmup
    • Philip Kaufmann: cs_proxyInfos
  24. 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.

    These are the four cases I need help with:

    For each of the above cases I see two alternatives:

    1. A GUARDED_BY(…) for some variable accessed directly or indirectly is missing.
    2. The lock is not technically necessary.
  25. 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.

  26. TheBlueMatt commented at 6:25 pm on September 15, 2017: member

    @practicalswift

    • 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.
  27. practicalswift commented at 6:48 pm on September 15, 2017: contributor
    @TheBlueMatt Thanks! I’ll look into the suggested fixes.
  28. practicalswift referenced this in commit bf3c3b5414 on Sep 16, 2017
  29. 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? :-)
  30. 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? :-)
  31. practicalswift referenced this in commit 5f4e12df29 on Sep 18, 2017
  32. fanquake added this to the milestone 0.16.0 on Sep 25, 2017
  33. practicalswift force-pushed on Oct 2, 2017
  34. practicalswift referenced this in commit 22a36759ef on Oct 2, 2017
  35. practicalswift commented at 2:33 pm on October 2, 2017: contributor
    Rebased!
  36. practicalswift force-pushed on Oct 4, 2017
  37. practicalswift referenced this in commit d516f7e2e7 on Oct 4, 2017
  38. practicalswift force-pushed on Oct 5, 2017
  39. practicalswift referenced this in commit ee1c0ef933 on Oct 5, 2017
  40. practicalswift force-pushed on Oct 17, 2017
  41. practicalswift referenced this in commit 1d1c41f315 on Oct 17, 2017
  42. practicalswift force-pushed on Oct 30, 2017
  43. practicalswift referenced this in commit 8ae79649ca on Oct 30, 2017
  44. practicalswift force-pushed on Oct 30, 2017
  45. practicalswift referenced this in commit 5d0f0aea8b on Oct 30, 2017
  46. practicalswift force-pushed on Oct 30, 2017
  47. practicalswift force-pushed on Oct 30, 2017
  48. practicalswift force-pushed on Oct 30, 2017
  49. practicalswift force-pushed on Oct 31, 2017
  50. practicalswift referenced this in commit 750582cb31 on Oct 31, 2017
  51. practicalswift referenced this in commit 967e06ad91 on Oct 31, 2017
  52. ryanofsky commented at 3:20 pm on November 2, 2017: member
    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.
  53. practicalswift commented at 4:47 pm on November 2, 2017: contributor
    @ryanofsky Very good point! I’ll do that! Thanks.
  54. 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 :-)
  55. ryanofsky commented at 5:24 pm on November 2, 2017: member
    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.
  56. practicalswift commented at 5:42 pm on November 2, 2017: contributor
    @ryanofsky Excellent! That’ll be our plan!
  57. practicalswift force-pushed on Nov 2, 2017
  58. practicalswift referenced this in commit 3224ea8944 on Nov 2, 2017
  59. practicalswift force-pushed on Nov 3, 2017
  60. practicalswift force-pushed on Nov 3, 2017
  61. practicalswift referenced this in commit f556fa051c on Nov 3, 2017
  62. practicalswift referenced this in commit ce15a30b9d on Nov 3, 2017
  63. practicalswift force-pushed on Nov 6, 2017
  64. practicalswift force-pushed on Nov 6, 2017
  65. practicalswift referenced this in commit a24ffba485 on Nov 6, 2017
  66. practicalswift referenced this in commit 9f1bff31d2 on Nov 6, 2017
  67. practicalswift force-pushed on Nov 6, 2017
  68. practicalswift force-pushed on Nov 8, 2017
  69. practicalswift referenced this in commit bd31ed786f on Nov 8, 2017
  70. practicalswift force-pushed on Nov 9, 2017
  71. practicalswift referenced this in commit 3388e05887 on Nov 9, 2017
  72. TheBlueMatt commented at 1:00 am on November 10, 2017: member
    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 :).
  73. ryanofsky commented at 1:06 am on November 10, 2017: member
    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.
  74. TheBlueMatt commented at 3:13 am on November 10, 2017: member

    @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.

    On November 9, 2017 8:06:51 PM EST, Russell Yanofsky notifications@github.com wrote:

    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.

    – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11226#issuecomment-343343387

  75. ryanofsky commented at 3:39 am on November 10, 2017: member

    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.

  76. TheBlueMatt commented at 4:06 am on November 10, 2017: member

    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.

    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 https://github.com/bitcoin/bitcoin/pull/11634, #11623 https://github.com/bitcoin/bitcoin/pull/11623, #11596 https://github.com/bitcoin/bitcoin/pull/11596, etc, and I’m assuming there will be more.

    @practicalswift https://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.

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11226#issuecomment-343366242, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHpnNC6mocaDz3cXFXZ-P61DDJuR_ks5s08WNgaJpZM4PLPK1.

  77. MarcoFalke referenced this in commit ee92243e66 on Nov 10, 2017
  78. practicalswift force-pushed on Nov 10, 2017
  79. practicalswift referenced this in commit 601883bb5b on Nov 10, 2017
  80. practicalswift force-pushed on Nov 10, 2017
  81. practicalswift referenced this in commit 25f5555d64 on Nov 10, 2017
  82. 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.)?

  83. fanquake renamed this:
    Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock)
    [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock)
    on Nov 12, 2017
  84. practicalswift force-pushed on Nov 14, 2017
  85. practicalswift referenced this in commit 00be878e90 on Nov 14, 2017
  86. practicalswift force-pushed on Nov 14, 2017
  87. practicalswift referenced this in commit 2f56e176b0 on Nov 14, 2017
  88. practicalswift force-pushed on Nov 14, 2017
  89. practicalswift referenced this in commit 553865f8c2 on Nov 14, 2017
  90. practicalswift force-pushed on Nov 16, 2017
  91. practicalswift referenced this in commit 814268ce9d on Nov 16, 2017
  92. practicalswift force-pushed on Nov 16, 2017
  93. practicalswift referenced this in commit 1946460c38 on Nov 16, 2017
  94. practicalswift force-pushed on Nov 20, 2017
  95. practicalswift referenced this in commit 01878d3ac2 on Nov 20, 2017
  96. practicalswift force-pushed on Nov 22, 2017
  97. practicalswift force-pushed on Nov 29, 2017
  98. practicalswift referenced this in commit b9c6396132 on Nov 29, 2017
  99. Add missing locks to init.cpp (in AppInitMain + ThreadImport) and validation.cpp 
    Locks added to init.cpp due to the following locking requirements:
* reading the value pointed to by 'pblocktree' requires holding mutex 'cs_main'
* reading the value pointed to by 'pblocktree' requires holding mutex 'cs_main'
* reading variable 'mapBlockIndex' requires holding mutex 'cs_main'
* reading the value pointed to by 'pcoinsdbview' requires holding mutex 'cs_main'
* reading the value pointed to by 'pcoinsTip' requires holding mutex 'cs_main'
* reading variable 'chainActive' requires holding mutex 'cs_main'
* reading variable 'chainActive' requires holding mutex 'cs_main'
* reading variable 'chainActive' requires holding mutex 'cs_main'
* reading variables 'mapBlockIndex' and 'chainActive' require holding mutex 'cs_main'

Locks added to validation.cpp due to the following locking requirements:
* reading variable 'scriptExecutionCache' requires holding mutex 'cs_main'
* writing variable 'fCheckForPruning' requires holding mutex 'cs_LastBlockFile' exclusively
* reading variable 'mapBlockIndex' requires holding mutex 'cs_main'
* reading variable 'nLastBlockFile' requires holding mutex 'cs_LastBlockFile'
* reading variables 'mapBlockIndex' and 'chainActive' require holding mutex 'cs_main'
* writing variable 'nLastBlockFile' requires holding mutex 'cs_LastBlockFile' exclusively
* writing variable 'nBlockSequenceId' requires holding mutex 'cs_nBlockSequenceId' exclusively
    8a1a11b77f
  100. Add LOCK(cs). The variables 'mapTx' and 'mapNextTx' are guarded by that mutex. aeafe946af
  101. Add LOCK(cs_main). The variable 'mapNodeState' (accessed via ProcessBlockAvailability(...)/State(...)) is guarded by that mutex. 37fad6a003
  102. Add LOCK(node->cs_filter). The variable 'fRelayTxes' is guarded by that mutex. 2e7344abdb
  103. Add LOCK(walletInstance->cs_wallet). The variable 'mapWallet' is guarded by that mutex. 1543ad0f59
  104. Add GUARDED_BY(cs) [CAddrMan] annotation 5f8ab503b0
  105. Add GUARDED_BY(cs) [CTxMemPool] annotation aedabc6677
  106. Add GUARDED_BY(cs_addrLocal) annotation 0fec14b4bc
  107. Add GUARDED_BY(cs_addrName) annotation 3c7f4c135e
  108. Add GUARDED_BY(cs_args) annotation e0a8d2a172
  109. Add GUARDED_BY(cs_db) annotation 41534f7242
  110. Add GUARDED_BY(cs_feeEstimator) annotation 4a7ae55108
  111. Add GUARDED_BY(cs_feeFilter) annotation 94c7702dfa
  112. Add GUARDED_BY(cs_filter) annotation faffd219bd
  113. Add GUARDED_BY(cs_hSocket) annotation 4724e2ba99
  114. Add GUARDED_BY(cs_inventory) annotation 0cd42f04a4
  115. Add GUARDED_BY(cs_KeyStore) annotation 8d30aa314b
  116. Add GUARDED_BY(cs_main) annotation 742c48998c
  117. Add GUARDED_BY(cs_mapLocalHost) annotation 7649019575
  118. Add GUARDED_BY(cs_most_recent_block) annotation 51e756f0e8
  119. Add GUARDED_BY(cs_proxyInfos) annotation 1ed45d11e4
  120. Add GUARDED_BY(cs_sendProcessing) annotation 8d7f2ad125
  121. Add GUARDED_BY(cs_vSend) annotation ae800092d6
  122. Add GUARDED_BY(cs_setBanned) annotation 38dd5f03be
  123. Add GUARDED_BY(cs_SubVer) annotation 96fcd0f0c8
  124. Add GUARDED_BY(cs_vAddedNodes) annotation fca5c87ca8
  125. Add GUARDED_BY(cs_vNodes) annotation 778635e83b
  126. Add GUARDED_BY(cs_vOneShots) annotation d294f95fa8
  127. Add GUARDED_BY(cs_vProcessMsg) annotation 8821c19f64
  128. Add GUARDED_BY(cs_vRecv) annotation 738f9b333b
  129. Add GUARDED_BY(cs_wallet) annotation 2632c859b4
  130. Add GUARDED_BY(cs_warnings) annotation e05f0d434c
  131. Add GUARDED_BY(csPathCached) annotation e30b8b6b39
  132. Add GUARDED_BY(m_cs_callbacks_pending) annotation e9bb3dd0ff
  133. Add EXCLUSIVE_LOCKS_REQUIRED(...) annotations 66411edf67
  134. Add EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile). cs_LastBlockFile is guarding nLastBlockFile. 1a3dbf3cb7
  135. Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding nWalletVersion and nWalletMaxVersion. 9c9d4d6f83
  136. Rename CAddrMan.cs to CAddrMan.cs_addrMan. Rename CTxMemPool.cs to CTxMemPool.cs_txMemPool. 855c071fcb
  137. Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding nOrderPosNext. 00f5c213a2
  138. Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapKeyMetadata. 468ad0148d
  139. Add EXCLUSIVE_LOCKS_REQUIRED(cs_txMemPool). cs_txMemPool is guarding minerPolicyEstimator, cachedInnerUsage, lastRollingFeeUpdate, blockSinceLastRollingFeeBump and rollingMinimumFeeRate. cb8a8d19ba
  140. Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapTxSpends and nTimeFirstKey. a2d7d5693a
  141. Add EXCLUSIVE_LOCKS_REQUIRED(mempool.cs_txMemPool). mempool.cs_txMemPool is guarding mempool.vTxHashes. 1aa043c306
  142. 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
  143. Add comment about fRPCInWarmup/rpcWarmupStatus being GUARDED_BY(cs_rpcWarmup) 45c436be3d
  144. Add GUARDED_BY(cs_nTimeOffset) annotation. 64f012bb01
  145. Add GUARDED_BY(cs_LastBlockFile) annotation. Add EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile) annotation. 3a5cf6c452
  146. Add GUARDED_BY(cs_nBlockSequenceId) annotation. Add EXCLUSIVE_LOCKS_REQUIRED(cs_nBlockSequenceId) annotation. 01ab138b80
  147. Add GUARDED_BY(cs_rpcWarmup) annotation. 5b39f27ac7
  148. Add GUARDED_BY(cs_wallet) annotation for nWalletVersion/nWalletMaxVersion fe7be0bc13
  149. Add GUARDED_BY(cs_main) annotation 0e1f2cb133
  150. Add PT_GUARDED_BY(cs_main) annotation 2e421e2314
  151. Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding *pcoinsTip. b852213040
  152. Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding *pblocktree. 0845e5da22
  153. Add LOCK(cs_main) where accessing chainActive e1d589b4f8
  154. Add PT_GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding chainActive. 9f95edba43
  155. Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding scriptExecutionCache. 3a622a2b3f
  156. CWallet::CreateWalletFromFile(...): Change from NO_THREAD_SAFETY_ANALYSIS to EXCLUSIVE_LOCKS_REQUIRED(cs_main) cbb6033f02
  157. Document all NO_THREAD_SAFETY_ANALYSIS annotations f2b4aa9d5f
  158. Add LOCK(cs_main) when accessing mapBlockIndex 2997281331
  159. Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding mapBlockIndex. 147f44930b
  160. 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
  161. Add LOCK(cs_addrMan) when accessing vRandom be28bf1b96
  162. Use GUARDED_BY(...) instead of PT_GUARDED_BY(...) for guarded std::shared_ptr<T>:s 08dc5d679a
  163. Add LOCK(pwallet->cs_wallet) when accessing mapWallet (via CWallet::IsSpent(...)) a1c20893c6
  164. Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding versionbitscache. aedf8de368
  165. Operate directly on pool->vTxHashes to resolve Clang thread-safety analysis aliasing confusion 32ba72a021
  166. [tentative/experimental change, needs review before merge] Lock env->cs_db instead of bitdb.cs_db 0c470a1611
  167. Make sure the cs_main lock covers all chainActive accesses while limiting the lock scope. Avoid keeping cs_main locked during wait_until(...). 3d61525a9b
  168. Add GUARDED_BY(cs_main). Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding mapBlockIndex. 33f2089264
  169. Add GUARDED_BY(cs_filter). Add EXCLUSIVE_LOCKS_REQUIRED(cs_filter). cs_main is guarding fRelayTxes. ed86da25ea
  170. Add GUARDED_BY(cs_main). cs_main is guarding versionbitscache. cc900ec4c4
  171. Add GUARDED_BY(cs_main). cs_main is guarding mapOrphanTransactions. 56a201b153
  172. Add GUARDED_BY(cs_warnings). cs_warnings is guarding fLargeWorkForkFound/fLargeWorkInvalidChainFound. 288630a83c
  173. Add LOCK(cs_KeyStore) when accessing mapKeys/mapWatchKeys/mapScripts/setWatchOnly 195196332c
  174. Add GUARDED_BY(cs_KeyStore). cs_KeyStore is guarding mapKeys/mapWatchKeys/mapScripts/setWatchOnly. 00d046fe68
  175. Add NO_THREAD_SAFETY_ANALYSIS annotation for Init(const Options& connOptions) 1752e15b7b
  176. Add GUARDED_BY(cs_mapLocalHost). cs_mapLocalHost is guarding mapLocalHost/vfLimited. 01097f7c47
  177. Add PT_GUARDED_BY(cs_main). cs_main is guarding *pindexBestHeader. 4b003e8b08
  178. Add GUARDED_BY(cs_wallet). Add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet). cs_wallet is guarding mapAddressBook. c42ed7da9b
  179. Add GUARDED_BY(cs_callbacks_pending). cs_callbacks_pending is guarding m_are_callbacks_running. b7dceee2ce
  180. Add GUARDED_BY(cs_LastBlockFile). Add EXCLUSIVE_LOCKS_REQUIRED(cs_LastBlockFile). cs_LastBlockFile is guarding fCheckForPruning. f7560f7e33
  181. Add GUARDED_BY(cs_KeyStore). cs_KeyStore is guarding mapCryptedKeys. df76e327ae
  182. Add SendMessages(...) lock requirement (pto->cs_sendProcessing) also to the header file c534d2b3a8
  183. Add EXCLUSIVE_LOCKS_REQUIRED(cs_addrMan) - needed when building with DEBUG_ADDRMAN. cs_addrMan is guarding vRandom. e442df06e2
  184. Revert "Add LOCK(node->cs_filter). The variable 'fRelayTxes' is guarded by that mutex." 
    This reverts commit c17eedee26c492f4b6abd1a4baead30afc4e742c.
    d6995395fa
  185. Revert "Add GUARDED_BY(cs_filter). Add EXCLUSIVE_LOCKS_REQUIRED(cs_filter). cs_main is guarding fRelayTxes." 
    This reverts commit edd8005e1a60d2652544c6287677f19a47a7c63a.
    3d05a8f5bd
  186. Revert "Add GUARDED_BY(cs_filter) annotation" 
    This reverts commit 1d8b7fa0a47b49e538606363db71ad5d1407c866.
    8917d74ed0
  187. Add EXCLUSIVE_LOCKS_REQUIRED(cs_main) to StaleBlockRequestAllowed(...) 54d4facd15
  188. Add EXCLUSIVE_LOCKS_REQUIRED(env.cs_db). env.cs_db is guarding mapDb. 49b8c3adad
  189. Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding chainActive and mapNodeState. 1f6ff489e2
  190. Remove EXCLUSIVE_LOCKS_REQUIRED(cs_filter) from PeerLogicValidation::InitializeNode(CNode *pnode) bdca80cffa
  191. Add GUARDED_BY(cs_addrMan). Add EXCLUSIVE_LOCKS_REQUIRED(cs_addrMan). Change lock scope. 6622f35d7f
  192. Add EXCLUSIVE_LOCKS_REQUIRED(cs_main) to ReceivedBlockTransactions. Remove redundant lock. 9267aa0bed
  193. Remove invalid assert and corresponding lock. chainActive height can go down. f67c107cab
  194. Add GetDepthInMainChain lock requirement to header file. Add locking requirements that follow. e376f75038
  195. Add EXCLUSIVE_LOCKS_REQUIRED(cs_main). cs_main is guarding g_last_tip_update. 7bf10ac5d6
  196. Repeat already held lock inside lambda to please clang -Wthread-safety e8641cd848
  197. Mirror locking requirements to header files. ac75e0aee4
  198. Add missing locks to tests a93483f870
  199. Remove incorrect lock requirement annotation from ProcessNewBlock 86b35047eb
  200. Add missing cs_LastBlockFile lock when accessing fCheckForPruning 
    The variable fCheckForPruning is guarded by the mutex cs_LastBlockFile.
    89cfabfefa
  201. Add locking to SetNull(). Remove NO_THREAD_SAFETY_ANALYSIS. f1996dc599
  202. Perform locking inside CConnman::Start f82c0f577f
  203. Clarify NO_THREAD_SAFETY_ANALYSIS comment 963317b78e
  204. Add locking to Init() e0eadcf7e5
  205. Remove NO_THREAD_SAFETY_ANALYSIS annotation from validateaddress(...) 
    Use locking logic that does not confuse the Clang thread-safety analyzer.
    5b8b4499e1
  206. Remove incorrect NO_THREAD_SAFETY_ANALYSIS comment 6d768ed4de
  207. Remove NO_THREAD_SAFETY_ANALYSIS from AppInitMain. Add locking to AppInitMain, ThreadImport and validation.cpp. b305c66b5d
  208. Use correct locking in TestSequenceLocks. 529aa91df9
  209. Add temporary documentation with references to PR:s adding LOCK(...):s 5b031f281f
  210. Avoid new lock by moving LogPrint statement de9907cf35
  211. [wip] Document why newly introduced locks are needed 6a2cacd571
  212. Revert locking changes in getblocktemplate(...) 825061717b
  213. Add GUARDED_BY(cs_txMemPool) to nCheckFrequency. Add corresponding LOCK(...). 4c01a4507c
  214. Fix incorrect PT_GUARDED_BY locks. Add GUARDED_BY(cs_filter) fRelayTxes. Add corresponding LOCK(...):s. 789a315013
  215. Fix variable name typo 5d8e34545a
  216. Add GUARDED_BY(cs_wallet) to nRelockTime. Add corresponding LOCK(...). 2909d8a4ae
  217. Add GUARDED_BY(cs_LastBlockFile) to vinfoBlockFile. Add corresponding LOCK(...). e7b27cddbe
  218. WIP: Fix conditional locking in validateaddress(...) f131976e16
  219. Add refereces to tracking GitHub issues 91f8024d02
  220. Remove comment for already merged LOCK(...) 358aa923f9
  221. Set m_last_block_processed to nullptr in SetNull(). Acquire mutex cs_wallet when accessing m_last_block_processed. 192a550fc8
  222. Avoid locking mutexes that are already held by the same thread baf7c1ff9e
  223. practicalswift force-pushed on Dec 4, 2017
  224. 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.
  225. laanwj removed this from the milestone 0.16.0 on Jan 11, 2018
  226. laanwj added this to the milestone 0.17.0 on Jan 11, 2018
  227. 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?

  228. 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.
  229. practicalswift commented at 9:46 pm on February 22, 2018: contributor
    Closing. Will split this into subsets and submit as separate PR:s.
  230. practicalswift closed this on Feb 22, 2018
  231. 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! :-)

  232. PastaPastaPasta referenced this in commit a3cd9783af on Dec 27, 2019
  233. PastaPastaPasta referenced this in commit ba8f6a31a2 on Jan 2, 2020
  234. PastaPastaPasta referenced this in commit 675f655722 on Jan 4, 2020
  235. PastaPastaPasta referenced this in commit 76c527d49d on Jan 12, 2020
  236. PastaPastaPasta referenced this in commit a36d0ef602 on Jan 12, 2020
  237. PastaPastaPasta referenced this in commit be7ff9c186 on Jan 12, 2020
  238. PastaPastaPasta referenced this in commit 6db34655f5 on Jan 12, 2020
  239. PastaPastaPasta referenced this in commit 523338304e on Jan 12, 2020
  240. PastaPastaPasta referenced this in commit 2377ac6533 on Jan 12, 2020
  241. PastaPastaPasta referenced this in commit 58adb1c28d on Jan 16, 2020
  242. PastaPastaPasta referenced this in commit e5d9dba3e9 on Jan 22, 2020
  243. PastaPastaPasta referenced this in commit efea72890c on Jan 22, 2020
  244. PastaPastaPasta referenced this in commit 0c4c5ff939 on Jan 29, 2020
  245. PastaPastaPasta referenced this in commit a3ad3120bf on Jan 29, 2020
  246. ckti referenced this in commit 2c0e302702 on Mar 28, 2021
  247. practicalswift deleted the branch on Apr 10, 2021
  248. gades referenced this in commit 386e85aae4 on Jun 30, 2021
  249. gades referenced this in commit 2d10e92600 on Feb 10, 2022
  250. DrahtBot locked this on Aug 18, 2022


practicalswift sipa promag TheBlueMatt laanwj ryanofsky MarcoFalke

Labels
Refactoring

Milestone
0.17.0

github-metadata-mirror

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: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me