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

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()
DrahtBot added the label RPC/REST/ZMQ on Apr 14, 2020
DrahtBot added the label Validation on Apr 14, 2020
MarcoFalke removed the label RPC/REST/ZMQ on Apr 14, 2020
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
hebasto force-pushed on Apr 14, 2020
hebasto commented at 10:16 PM on April 14, 2020: member
Updated 9b4b3c9442f5b2ccdce8d70161a355aec5402cef -> fe187562ef9765942d7c7d801e04d70d294d5aef (pr18639.01 -> pr18639.02, diff):
- addressed @MarcoFalke's comment:
Removed locks are generally replaced with AssertLockHeld(cs_main); and the corresponding compiler annotations
Prevent consecutive cs_main locks in FlushStateToDisk f01d286fb9
hebasto force-pushed on Apr 17, 2020
hebasto renamed this:
~~Prevent consecutive ::cs_main locks in FlushStateToDisk()~~
Prevent consecutive ::cs_main locks
on Apr 17, 2020
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()
Prevent consecutive cs_main locks in UnloadBlockIndex 31a92b2d4b
Prevent consecutive cs_main locks in CChainState::ReplayBlocks e09b98605f
hebasto force-pushed on Apr 17, 2020
MarcoFalke commented at 10:47 AM on April 17, 2020: member

What problem is this solving?
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
DrahtBot commented at 5:03 PM on April 18, 2020: member


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



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.
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.
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 :)
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
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.
hebasto closed this on Apr 30, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Prevent consecutive ::cs_main locks #18639

Conflicts