Add compile time checking for run time locking assertions #12665

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:compile-time-checking-of-runtime-assertions changing 24 files +132 −119
  1. practicalswift commented at 5:17 PM on March 10, 2018: contributor

    Add compile time checking for all run time locking assertions.

    The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s added in this PR should follow either directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

    Consider the case where function A(…) contains AssertLockHeld(cs_foo) (without first locking cs_foo in A), and that B(…) calls A(…) (without first locking cs_main):

    • It directly follows that: A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
    • It indirectly follows that: B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
  2. fanquake added the label Refactoring on Mar 10, 2018
  3. practicalswift renamed this:
    Add compile time checking for all run time locking assertions
    Add compile time checking for all run time locking assertions [wip]
    on Mar 11, 2018
  4. practicalswift force-pushed on Mar 11, 2018
  5. practicalswift force-pushed on Mar 11, 2018
  6. practicalswift force-pushed on Mar 11, 2018
  7. practicalswift force-pushed on Mar 11, 2018
  8. in src/txmempool.cpp:508 in aa224a20d6 outdated 
     501 | @@ -502,10 +502,10 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
 502 |      }
 503 |  }
 504 |  
 505 | -void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
 506 | +void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs, mempool.cs)
 507 |  {
 508 |      // Remove transactions spending a coinbase which are now immature and no-longer-final transactions
 509 | -    LOCK(cs);
 510 | +    AssertLockHeld(cs);
    MarcoFalke commented at 9:26 PM on March 11, 2018:

    This 1 line diff should be a separate pull request

    practicalswift commented at 10:31 PM on March 11, 2018:

    That assertion does not seem to hold, so I think it should be removed. I'll investigate.

  9. MarcoFalke commented at 9:27 PM on March 11, 2018: member

    Concept ACK

  10. practicalswift cross-referenced this on Mar 11, 2018 from issue mempool: Avoid locking mutex that is already held by the same thread by practicalswift
  11. practicalswift force-pushed on Mar 11, 2018
  12. practicalswift force-pushed on Mar 11, 2018
  13. practicalswift force-pushed on Mar 11, 2018
  14. practicalswift force-pushed on Mar 11, 2018
  15. practicalswift force-pushed on Mar 11, 2018
  16. practicalswift renamed this:
    Add compile time checking for all run time locking assertions [wip]
    Add compile time checking for all run time locking assertions
    on Mar 11, 2018
  17. practicalswift commented at 6:59 AM on March 12, 2018: contributor

    @MarcoFalke Updated! Please review :-)

  18. practicalswift cross-referenced this on Mar 12, 2018 from issue [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) by practicalswift
  19. laanwj commented at 11:49 AM on March 13, 2018: member

    Concept ACK - Unlike the title implies, not all the added run time checking requirements have an associated AssertLockHeld/AssertLockNotHeld (e.g. CoinSelection) is this on purpose?

  20. practicalswift renamed this:
    Add compile time checking for all run time locking assertions
    Add compile time checking for run time locking assertions
    on Mar 13, 2018
  21. practicalswift force-pushed on Mar 13, 2018
  22. practicalswift commented at 1:52 PM on March 13, 2018: contributor

    @laanwj Thanks for reporting. The annotations for CoinSelection and two other functions were incorrect. That is now fixed.

    The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s added in this PR should follow either directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

    Consider the case where function A(…) contains AssertLockHeld(cs_foo) (without first locking cs_foo in A), and that B(…) calls A(…) (without first locking cs_main):

    • It directly follows that: A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
    • It indirectly follows that: B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.

    I'll run through all annotations again to verify that they all follow directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s.

    These are the annotations added – let me know if something sticks out/looks weird:

    [Edit: List moved to PR description. See above!]
  23. Sjors commented at 8:57 PM on March 13, 2018: member

    Sadly that still didn't catch my mistake here, but I assume this is a step in that direction.

    Lightly tested on macOS (no change, as expected). @practicalswift maybe you can put that overview and additional explanation ("directly follows that...") in the commit message or PR description?

    I didn't see a EXCLUSIVE_LOCKS_REQUIRED in LoadChainTip; I assume that's done in a some dependent function? Maybe that can be made explicit in your overview if it's not too tedious?

    The EXLCUSIVE_LOCKS... stuff in validation.h is not repeated in validation.cpp, that's intentional?

    Should violations merely throw warnings?

  24. meshcollider commented at 9:11 PM on March 13, 2018: contributor

    Concept ACK

  25. practicalswift commented at 9:19 AM on March 14, 2018: contributor

    @Sjors No intention to repeat annotations :-) Which specific annotations did you see repeated in validation.cpp? Did you check them in the files or just in the summary?

    Violations are checked by Travis and cause a Travis build failure if found – see configure.ac.

  26. practicalswift force-pushed on Mar 14, 2018
  27. practicalswift force-pushed on Mar 14, 2018
  28. practicalswift commented at 9:22 AM on March 14, 2018: contributor

    @Sjors Did you mean that you didn't see any AssertLockHeld(…) in LoadChainTip? :-)

  29. practicalswift force-pushed on Mar 14, 2018
  30. Sjors commented at 2:10 PM on March 14, 2018: member

    Which specific annotations did you see repeated in validation.cpp

    None, so that's all good. I just found it slightly confusing that some annotations are in .h files and others in .cpp files.

    Did you mean that you didn't see any AssertLockHeld(…) in LoadChainTip?

    Yes, sorry. To more specific, no AssertLockHeld(mempool.cs). But if you're happy and the compiler is happy, I'm happy :-)

  31. practicalswift force-pushed on Mar 14, 2018
  32. practicalswift commented at 7:59 PM on March 14, 2018: contributor

    @Sjors The annotations should be in the header files, but not all functions are exported so that's why some annotations end up in the .h files.

    LoadChainTip is annotated with EXCLUSIVE_LOCKS_REQUIRED(mempool.cs) since it (indirectly) calls CheckSequenceLocks (which contains AssertLockHeld(mempool.cs)) without first locking mempool.cs.

    The call stack looks like this:

    LoadChainTip
  ActivateBestChain
    ActivateBestChainStep
      UpdateMempoolForReorg
        AcceptToMemoryPool
          AcceptToMemoryPoolWithTime
            AcceptToMemoryPoolWorker
              CheckSequenceLocks
  33. sipa commented at 1:07 AM on March 15, 2018: member

    Concept ACK

  34. Sjors cross-referenced this on Mar 16, 2018 from issue Potential deadlock between main and net by fanquake
  35. practicalswift force-pushed on Mar 23, 2018
  36. practicalswift force-pushed on Apr 3, 2018
  37. practicalswift commented at 11:35 AM on April 3, 2018: contributor

    Rebased!

  38. practicalswift force-pushed on Apr 6, 2018
  39. practicalswift force-pushed on Apr 6, 2018
  40. practicalswift force-pushed on Apr 6, 2018
  41. practicalswift force-pushed on Apr 6, 2018
  42. practicalswift force-pushed on Apr 6, 2018
  43. practicalswift force-pushed on Apr 9, 2018
  44. practicalswift commented at 8:33 AM on April 9, 2018: contributor

    Rebased again!

  45. practicalswift commented at 8:47 AM on April 9, 2018: contributor

    @laanwj This PR is a subset of #11226 (closed in favour of smaller locking PR:s). PR #11226 had a 0.17.0 milestone (originally a 0.16.0 milestone actually). Would it be possible to get a 0.17.0 milestone for this PR?

    Of the locking PR:s this should be the best candidate for early merge. It contains only locking annotations for which we are already asserting using AssertLockHeld(…). Since this PR doesn't change runtime behaviour (compile-time checks only) it should be low risk.

    Having this PR merged would make the other locking assertions PR:s much smaller and easier to review since this PR contains the bulk of the assertions which the other PR:s contain too.

    Keeping this PR rebased is getting a bit time consuming, so let me know if I can do anything to facilitate merging :-)

  46. practicalswift force-pushed on Apr 9, 2018
  47. practicalswift commented at 10:43 PM on April 9, 2018: contributor

    Rebase number 3 performed :-)

  48. practicalswift force-pushed on Apr 9, 2018
  49. practicalswift force-pushed on Apr 10, 2018
  50. Sjors commented at 11:18 AM on April 11, 2018: member

    Can confirm 9d3e5374e30cd2b1c04c0bb45da24236eb3b2b37 still compiles on macOS. @TheBlueMatt and @promag recently worked on concurrency related bugs in init.cpp, so they might be able to review.

  51. practicalswift force-pushed on Apr 22, 2018
  52. practicalswift commented at 9:44 AM on April 22, 2018: contributor

    Rebase number 4 performed :-) Please review :-)

  53. practicalswift commented at 9:53 AM on April 22, 2018: contributor

    @laanwj This PR is getting a bit heavy to keep up-to-date. Do you think this PR has a chance of getting a 0.17.0 milestone (like #11226 had before getting closed in favour of this and other smaller PR:s)? :-)

  54. in src/rpc/rawtransaction.cpp:1136 in 9a17c13c04 outdated 
    1132 | @@ -1133,7 +1133,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
1133 |      return hashTx.GetHex();
1134 |  }
1135 |  
1136 | -UniValue testmempoolaccept(const JSONRPCRequest& request)
1137 | +UniValue testmempoolaccept(const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)
    MarcoFalke commented at 12:57 PM on April 22, 2018:

    I fail to see how this could compile, since we never LOCK(mempool.cs) before calling into this, no?

  55. in src/test/miner_tests.cpp:205 in 9a17c13c04 outdated 
     201 | @@ -202,7 +202,7 @@ void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey,
 202 |  }
 203 |  
 204 |  // NOTE: These tests rely on CreateNewBlock doing its own self-validation!
 205 | -BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 206 | +BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)
    MarcoFalke commented at 12:58 PM on April 22, 2018:

    Same here

  56. in src/txmempool.cpp:508 in 9a17c13c04 outdated 
     501 | @@ -502,7 +502,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
 502 |      }
 503 |  }
 504 |  
 505 | -void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
 506 | +void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
 507 |  {
 508 |      // Remove transactions spending a coinbase which are now immature and no-longer-final transactions
 509 |      LOCK(cs);
    MarcoFalke commented at 12:59 PM on April 22, 2018:

    It seem the lock is taken here, but you require EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)

  57. in src/validation.cpp:479 in 9a17c13c04 outdated 
     466 | @@ -468,7 +467,7 @@ static bool IsCurrentForFeeEstimation()
 467 |   * and instead just erase from the mempool as needed.
 468 |   */
 469 |  
 470 | -void UpdateMempoolForReorg(DisconnectedBlockTransactions &disconnectpool, bool fAddToMempool)
 471 | +void UpdateMempoolForReorg(DisconnectedBlockTransactions &disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
    MarcoFalke commented at 1:00 PM on April 22, 2018:

    What about adding static to functions that have their lock annotations added in the cpp file and not in the header?

  58. practicalswift force-pushed on Apr 24, 2018
  59. practicalswift force-pushed on Apr 24, 2018
  60. practicalswift force-pushed on Apr 24, 2018
  61. practicalswift force-pushed on Apr 24, 2018
  62. practicalswift force-pushed on Apr 24, 2018
  63. practicalswift force-pushed on Apr 24, 2018
  64. practicalswift force-pushed on Apr 24, 2018
  65. Add compile time checking for all run time locking assertions
    The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s
added in this PR should follow either directly or indirectly from
AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

Consider the case where function A(…) contains AssertLockHeld(cs_foo)
(without first locking cs_foo in A), and that B(…) calls A(…) (without
first locking cs_main):

It directly follows that:
* A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.

It indirectly follows that:
* B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
    0d3b75d59e
  66. practicalswift force-pushed on Apr 24, 2018
  67. practicalswift commented at 10:06 PM on April 24, 2018: contributor

    @MarcoFalke Updated. I've now tried to address your feedback. Turns out that I was a bit too strict with the locking requirements for mempool.cs, so the updated diff is significantly smaller and hopefully easier to review. Please re-review :-)

  68. MarcoFalke commented at 6:56 PM on April 25, 2018: member

    utACK (could use ::mempool.cs instead of mempool.cs, since it is referring to the global, I guess)

    Also, you could split up the cs_wallet and ::mempool.cs into two separate prs. I have the feeling it is easier to get stuff in that only touches a single "module".

  69. Use ::mempool.cs instead of mempool.cs to clarify that we are using the global 2e24b53266
  70. practicalswift cross-referenced this on Apr 25, 2018 from issue Add compile time checking for all cs_KeyStore runtime locking assertions by practicalswift
  71. practicalswift cross-referenced this on Apr 25, 2018 from issue mempool: Add compile time checking for ::mempool.cs runtime locking assertions by practicalswift
  72. practicalswift cross-referenced this on Apr 25, 2018 from issue wallet: Add compile time checking for cs_wallet runtime locking assertions by practicalswift
  73. practicalswift cross-referenced this on Apr 25, 2018 from issue Add compile time checking for cs_main runtime locking assertions by practicalswift
  74. practicalswift commented at 9:41 PM on April 25, 2018: contributor

    @MarcoFalke Thanks for the utACK!

    Added a commit which changed from mempool.cs to ::mempool.cs.

    I've now split up this PR into four separate PRs – one per lock:

    Please review these four :-)

  75. MarcoFalke referenced this in commit 3315007e03 on Apr 28, 2018
  76. practicalswift closed this on May 2, 2018
  77. MarcoFalke referenced this in commit 66cc47be98 on May 5, 2018
  78. MarcoFalke referenced this in commit 0dec5b5af4 on May 14, 2018
  79. MarcoFalke referenced this in commit 91186e5984 on Aug 25, 2018
  80. PastaPastaPasta referenced this in commit 8784e4b82a on Jun 17, 2020
  81. PastaPastaPasta referenced this in commit f671d8e2c0 on Jun 17, 2020
  82. PastaPastaPasta referenced this in commit 1ca8c77592 on Jun 27, 2020
  83. PastaPastaPasta referenced this in commit 5aa65d0e68 on Jun 27, 2020
  84. PastaPastaPasta referenced this in commit 2ba0fb5ad8 on Jun 28, 2020
  85. PastaPastaPasta referenced this in commit 3cfa14c5af on Jun 28, 2020
  86. PastaPastaPasta referenced this in commit 4d49ad803c on Jun 29, 2020
  87. PastaPastaPasta referenced this in commit 795076168b on Jul 1, 2020
  88. practicalswift deleted the branch on Apr 10, 2021
  89. Munkybooty referenced this in commit c69df3bc97 on Jun 30, 2021
  90. Munkybooty referenced this in commit c09e919c67 on Jun 30, 2021
  91. Munkybooty referenced this in commit 8a6e8d7a28 on Jul 1, 2021
  92. Munkybooty referenced this in commit a898756320 on Jul 2, 2021
  93. Munkybooty referenced this in commit 5c9d57b8b8 on Jul 4, 2021
  94. UdjinM6 referenced this in commit 0f26a0df06 on Jul 7, 2021
  95. Munkybooty referenced this in commit 23f332711e on Jul 7, 2021
  96. Munkybooty referenced this in commit eae3b9abc4 on Jul 7, 2021
  97. Munkybooty referenced this in commit 06e467d037 on Jul 8, 2021
  98. gades referenced this in commit cb0f86cd7b on Mar 10, 2022
  99. gades referenced this in commit e0e5cea096 on Mar 16, 2022
  100. gades referenced this in commit 956217b1c7 on May 1, 2022
  101. bitcoin locked this on Aug 16, 2022
Contributors
practicalswiftMarcoFalkelaanwjSjorsmeshcollidersipa
Labels
Refactoring
Linked (view graph)
#9215 Potential deadlock between main and net#11226 [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock)#12671 mempool: Avoid locking mutex that is already held by the same thread#13077 Add compile time checking for all cs_KeyStore runtime locking assertions#13080 mempool: Add compile time checking for ::mempool.cs runtime locking assertions#13081 wallet: Add compile time checking for cs_wallet runtime locking assertions#13083 Add compile time checking for cs_main runtime locking assertions
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: 2026-05-20 06:55 UTC