validation: use stronger EXCLUSIVE_LOCKS

vasild commented at 10:44 am on February 2, 2022: member

#24103 added annotations to denote that the callers of CChainState::ActivateBestChain() and CChainState::InvalidateBlock() must not own m_chainstate_mutex at the time of the call.

Replace the added LOCKS_EXCLUDED() with a stronger EXCLUSIVE_LOCKS_REQUIRED(), see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative for the difference between both.

hebasto commented at 11:16 am on February 2, 2022: member

I’m a bit vague about using negated annotation for global mutexes, in particular EXCLUSIVE_LOCKS_REQUIRED(!::cs_main).

Btw, which clang versions were tested?

vasild commented at 11:20 am on February 2, 2022: member

I tested with clang 14.

in src/validation.h:644 in 5730f7327f outdated

639@@ -639,7 +640,8 @@ class CChainState
640      */
641     bool ActivateBestChain(
642         BlockValidationState& state,
643-        std::shared_ptr<const CBlock> pblock = nullptr) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main);
644+        std::shared_ptr<const CBlock> pblock = nullptr)
645+        EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !m_chainstate_mutex);

MarcoFalke commented at 11:27 am on February 2, 2022:

Tend toward NACK using negative annotations with global mutexes, especially cs_main. I presume LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) doesn’t work?

vasild commented at 1:09 pm on February 2, 2022:

What’s wrong with using negative annotations with global mutexes? I don’t see how the scope of the mutex is relevant, but maybe I miss something since @hebasto also wasn’t excited about it. Can you elaborate?

I presume LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) doesn’t work?

It works. Updated the PR.

MarcoFalke commented at 1:21 pm on February 2, 2022:

Can you elaborate?

See the commit that you ACKed: 9ac8f6d7ddc4c949d84de5584baa813144c7fb26

It is wasted effort, since cs_main is recursive
It is impossible to properly implement: #20272 (review)
It is too verbose for globals, as they propagate outside the scope of “their” class

hebasto commented at 11:28 am on February 2, 2022: member

Shouldn’t -Wthread-safety-negative be added to configure.am and/or CI jobs?

hebasto commented at 11:52 am on February 2, 2022: member

Shouldn’t -Wthread-safety-negative be added to configure.am and/or CI jobs?

No. It shouldn’t. It gets too noisy with warnings about cs_main.

Sorry.

hebasto commented at 12:21 pm on February 2, 2022: member

Restarted some CI jobs that failed due to connectivity issues.

DrahtBot added the label Block storage on Feb 2, 2022

DrahtBot added the label P2P on Feb 2, 2022

DrahtBot added the label Validation on Feb 2, 2022

validation: use stronger EXCLUSIVE_LOCKS_REQUIRED()

https://github.com/bitcoin/bitcoin/pull/24103 added annotations to
denote that the callers of `CChainState::ActivateBestChain()` and
`CChainState::InvalidateBlock()` must not own `m_chainstate_mutex` at
the time of the call.

Replace the added `LOCKS_EXCLUDED()` with a stronger
`EXCLUSIVE_LOCKS_REQUIRED()`, see
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative for the
difference between both.

99de8068cd

vasild force-pushed on Feb 2, 2022

hebasto commented at 1:06 pm on February 2, 2022: member

Concept ACK.

vasild commented at 1:07 pm on February 2, 2022: member

5730f7327f...99de8068cd: lower the scope to just m_chainstate_mutex and don’t change anything about cs_main which was a kind of scope creep.

I think the previous version was better (5730f7327f) as it provided stronger guarantees, but there is some frowning against using EXCLUSIVE_LOCKS_REQUIRED(!cs_main) (see comments above). Since the purpose of this PR is to do m_chainstate_mutex, leave cs_main alone.

MarcoFalke removed the label P2P on Feb 2, 2022

MarcoFalke removed the label Validation on Feb 2, 2022

MarcoFalke removed the label Block storage on Feb 2, 2022

MarcoFalke added the label Refactoring on Feb 2, 2022

hebasto commented at 1:29 pm on February 2, 2022: member

Tested 99de8068cd08ecc2ad5dfe603bf3c2cc5b8b33aa with Ubuntu clang version 12.0.0-3ubuntu1~20.04.4.

The following testing patch:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index c12dc9e8b..2f8e8c0c9 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -2845,6 +2845,12 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 5     }
 6 }
 7 
 8+void CChainState::foo()
 9+{
10+    BlockValidationState state;
11+    InvalidateBlock(state, nullptr);
12+}
13+
14 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
15 {
16     AssertLockNotHeld(m_chainstate_mutex);
17diff --git a/src/validation.h b/src/validation.h
18index fdfd29d1f..7bdc8aae3 100644
19--- a/src/validation.h
20+++ b/src/validation.h
21@@ -755,6 +755,8 @@ private:
22         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
23 
24     friend ChainstateManager;
25+
26+    void foo();
27 };
28 
29 /**

results in:

0$ make
1...
2validation.cpp:2851:5: warning: calling function 'InvalidateBlock' requires negative capability '!m_chainstate_mutex' [-Wthread-safety-analysis]
3    InvalidateBlock(state, nullptr);
4    ^
51 warning generated
6...

And the following testing patch:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index c12dc9e8b..84f99312b 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -2845,6 +2845,12 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 5     }
 6 }
 7 
 8+void CChainState::foo()
 9+{
10+    BlockValidationState state;
11+    InvalidateBlock(state, nullptr);
12+}
13+
14 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
15 {
16     AssertLockNotHeld(m_chainstate_mutex);
17@@ -2861,6 +2867,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
18     // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time
19     LOCK(m_chainstate_mutex);
20 
21+    foo();
22+
23     CBlockIndex *pindexMostWork = nullptr;
24     CBlockIndex *pindexNewTip = nullptr;
25     int nStopAtHeight = gArgs.GetIntArg("-stopatheight", DEFAULT_STOPATHEIGHT);
26diff --git a/src/validation.h b/src/validation.h
27index fdfd29d1f..39102ba43 100644
28--- a/src/validation.h
29+++ b/src/validation.h
30@@ -755,6 +755,8 @@ private:
31         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
32 
33     friend ChainstateManager;
34+
35+    void foo() EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex);
36 };
37 
38 /**

causes the following warning:

0$ make
1...
2validation.cpp:2870:5: warning: cannot call function 'foo' while mutex 'm_chainstate_mutex' is held [-Wthread-safety-analysis]
3    foo();
4    ^
51 warning generated.
6...

The similar patch being applied to the master branch does not fire any thread-safety warnings.

hebasto approved

hebasto commented at 1:40 pm on February 2, 2022: member

ACK 99de8068cd08ecc2ad5dfe603bf3c2cc5b8b33aa.

Some note for the future:

clang-format-diff.py fails to handle multiple multi-line TS annotations
Should we recommend to use negative capability forms of TS annotations for member mutexes in new code?

vasild commented at 2:11 pm on February 2, 2022: member

2. Should we recommend to use negative capability forms of TS annotations for member mutexes in new code?

If a method does LOCK(m) I think it is a good practice to include EXCLUSIVE_LOCKS_REQUIRED(!m) in its declaration (assuming non-recursive mutex).

jonatack commented at 6:39 pm on February 4, 2022: member

ACK 99de8068cd08ecc2ad5dfe603bf3c2cc5b8b33aa. Tested with Debian clang version 13.0.1. Reproduced hebasto’s results. Verified that LoadExternalBlockFile() needs the annotation added here.

Should the following run-time thread safety assertions be added?

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index c12dc9e8b6..109b1f95ce 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -2848,6 +2848,7 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 5 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 6 {
 7     AssertLockNotHeld(m_chainstate_mutex);
 8+    AssertLockNotHeld(::cs_main);
 9 
10@@ -2949,6 +2950,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
11 
12 bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
13 {
14+    AssertLockNotHeld(m_chainstate_mutex);
15+    AssertLockNotHeld(::cs_main);
16     {
17         LOCK(cs_main);
18@@ -2979,6 +2982,7 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex
19 bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
20 {
21     AssertLockNotHeld(m_chainstate_mutex);
22+    AssertLockNotHeld(::cs_main);
23 
24@@ -4089,6 +4093,8 @@ bool CChainState::LoadGenesisBlock()
25 
26 void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
27 {
28+    AssertLockNotHeld(m_chainstate_mutex);

MarcoFalke commented at 2:53 pm on February 8, 2022: member

(edited OP and review comments)

MarcoFalke merged this on Feb 8, 2022

MarcoFalke closed this on Feb 8, 2022

jonatack commented at 7:23 pm on February 8, 2022: member

(edited OP and review comments)

Thanks @MarcoFalke, left an @-reference in my ACK.

Added the thread safety assertions in my comment in #24235#pullrequestreview-873446100 to #24177.

sidhujag referenced this in commit 4a4b4fe68d on Feb 9, 2022

fanquake deleted a comment on Feb 9, 2022

vasild deleted the branch on Feb 9, 2022

Fabcien referenced this in commit 5c662d2831 on Oct 12, 2022

DrahtBot locked this on Feb 9, 2023

validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235