Prevent consecutive ::cs_main locks #18639

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:200414-locks changing 5 files +15 −12
  1. hebasto commented at 7:47 PM on April 14, 2020: member

    This PR prevents consecutive ::cs_main locks in the following functions:

    • FlushStateToDisk() that could happen when it is called from:
      • AcceptToMemoryPoolWithTime()
      • CChainState::DisconnectTip()
      • CChainState::ConnectTip()
      • CChainState::AcceptBlock()
      • CChainState::RewindBlockIndex()
    • UnloadBlockIndex()
    • CChainState::ReplayBlocks()
  2. DrahtBot added the label RPC/REST/ZMQ on Apr 14, 2020
  3. DrahtBot added the label Validation on Apr 14, 2020
  4. MarcoFalke removed the label RPC/REST/ZMQ on Apr 14, 2020
  5. in src/validation.cpp:2237 in 9b4b3c9442 outdated 
    2233 | @@ -2234,7 +2234,6 @@ bool CChainState::FlushStateToDisk(
2234 |      FlushStateMode mode,
2235 |      int nManualPruneHeight)
2236 |  {
2237 | -    LOCK(cs_main);
    MarcoFalke commented at 9:29 PM on April 14, 2020:

    Removed locks are generally replaced with AssertLockHeld(cs_main); and the corresponding compiler annotations

    hebasto commented at 10:17 PM on April 14, 2020:

    Updated.

    MarcoFalke commented at 11:31 PM on April 14, 2020:

    Please also add the annotation in the header

    EXCLUSIVE_LOCKS_REQUIRED(cs_main)

    hebasto commented at 12:04 AM on April 15, 2020:

    Please also add the annotation in the header

    EXCLUSIVE_LOCKS_REQUIRED(cs_main)

    It is already done: https://github.com/bitcoin/bitcoin/blob/fe187562ef9765942d7c7d801e04d70d294d5aef/src/validation.h#L671-L675

  6. hebasto force-pushed on Apr 14, 2020
  7. hebasto commented at 10:16 PM on April 14, 2020: member

    Updated 9b4b3c9442f5b2ccdce8d70161a355aec5402cef -> fe187562ef9765942d7c7d801e04d70d294d5aef (pr18639.01 -> pr18639.02, diff):

    Removed locks are generally replaced with AssertLockHeld(cs_main); and the corresponding compiler annotations

  8. hebasto cross-referenced this on Apr 15, 2020 from issue Thread Safety Analysis warnings by hebasto
  9. Prevent consecutive cs_main locks in FlushStateToDisk f01d286fb9
  10. hebasto force-pushed on Apr 17, 2020
  11. hebasto renamed this:
    Prevent consecutive ::cs_main locks in FlushStateToDisk()
    Prevent consecutive ::cs_main locks
    on Apr 17, 2020
  12. hebasto commented at 12:21 AM on April 17, 2020: member

    Updated fe187562ef9765942d7c7d801e04d70d294d5aef -> 18f0cb80c0cea7c9bc0b69a0f1fe0ca6cae8cde1 (pr18639.02 -> pr18639.03, diff):

    analogous changes added for:

    • UnloadBlockIndex()
    • CChainState::ReplayBlocks()
  13. Prevent consecutive cs_main locks in UnloadBlockIndex 31a92b2d4b
  14. Prevent consecutive cs_main locks in CChainState::ReplayBlocks e09b98605f
  15. hebasto force-pushed on Apr 17, 2020
  16. MarcoFalke commented at 10:47 AM on April 17, 2020: member

    What problem is this solving?

  17. hebasto commented at 11:07 AM on April 17, 2020: member

    @MarcoFalke

    What problem is this solving?

    Currently, ::cs_main has type RecursiveMutex which derives from std::recursive_mutex.

    But I'm agree with the following TODO statement: https://github.com/bitcoin/bitcoin/blob/4a71c469058b824ed6c5132ab9901e194fa8aae1/src/sync.h#L106-L110

  18. DrahtBot cross-referenced this on Apr 18, 2020 from issue Make g_chainman internal to validation by MarcoFalke
  19. DrahtBot commented at 5:03 PM on April 18, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18731 ([WIP] refactor: Make CCheckQueue RAII-styled by hebasto)
    • #18710 (Add local thread pool to CCheckQueue by hebasto)
    • #18698 (Make g_chainman internal to validation by MarcoFalke)

    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.

  20. MarcoFalke commented at 5:20 PM on April 18, 2020: member

    Ok, Concept ACK based on that motivation, but OP should probably mention that. Some general notes, though:

    • I am not sure if we can ever make cs_main not recursive. There might be circular dependencies in similar ways to how csPathCached can not be made a Mutex right now.
    • We currently have no mechanism to prevent double locking a Mutex. This is undefined behavior and currently happens on some of the Mutexes we have. Someone should probably fix that.
  21. hebasto commented at 5:25 PM on April 18, 2020: member
    • We currently have no mechanism to prevent double locking a Mutex. This is undefined behavior and currently happens on some of the Mutexes we have. Someone should probably fix that.

    Thanks for reminding :)

  22. MarcoFalke commented at 5:35 PM on April 18, 2020: member

    tsan should detect those: https://github.com/llvm/llvm-project/blame/master/compiler-rt/test/tsan/mutex_double_lock.cpp

  23. DrahtBot cross-referenced this on Apr 20, 2020 from issue Add local thread pool to CCheckQueue by hebasto
  24. DrahtBot cross-referenced this on Apr 22, 2020 from issue refactor: Make CCheckQueue RAII-styled by hebasto
  25. promag commented at 10:05 PM on April 26, 2020: member

    Code review ACK e09b98605f6fdd2ad3bcdf4c68d229905f7a29cd.

    I don't see much value in this change especially because a) it's touching validation b) it's not fixing a problem.

  26. hebasto closed this on Apr 30, 2020
  27. bitcoin locked this on Feb 15, 2022
Contributors
hebastoMarcoFalkeDrahtBotpromag
Labels
Validation
Linked (view graph)
#16979 rpc: Worker threads still run after timeout error for two gettxoutsetinfo RPC calls#18636 Thread Safety Analysis warnings
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-19 06:54 UTC