validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235

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

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?

I tested with clang 14.

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

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.

Restarted some CI jobs that failed due to connectivity issues.

Concept ACK.

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.

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

1. The following testing patch:

diff --git a/src/validation.cpp b/src/validation.cpp
index c12dc9e8b..2f8e8c0c9 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2845,6 +2845,12 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
     }
 }
 
+void CChainState::foo()
+{
+    BlockValidationState state;
+    InvalidateBlock(state, nullptr);
+}
+
 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 {
     AssertLockNotHeld(m_chainstate_mutex);
diff --git a/src/validation.h b/src/validation.h
index fdfd29d1f..7bdc8aae3 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -755,6 +755,8 @@ private:
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     friend ChainstateManager;
+
+    void foo();
 };
 
 /**

results in:

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

2. And the following testing patch:

diff --git a/src/validation.cpp b/src/validation.cpp
index c12dc9e8b..84f99312b 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2845,6 +2845,12 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
     }
 }
 
+void CChainState::foo()
+{
+    BlockValidationState state;
+    InvalidateBlock(state, nullptr);
+}
+
 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 {
     AssertLockNotHeld(m_chainstate_mutex);
@@ -2861,6 +2867,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
     // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time
     LOCK(m_chainstate_mutex);
 
+    foo();
+
     CBlockIndex *pindexMostWork = nullptr;
     CBlockIndex *pindexNewTip = nullptr;
     int nStopAtHeight = gArgs.GetIntArg("-stopatheight", DEFAULT_STOPATHEIGHT);
diff --git a/src/validation.h b/src/validation.h
index fdfd29d1f..39102ba43 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -755,6 +755,8 @@ private:
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     friend ChainstateManager;
+
+    void foo() EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex);
 };
 
 /**

causes the following warning:

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

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

ACK 99de8068cd08ecc2ad5dfe603bf3c2cc5b8b33aa.

Some note for the future:

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

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

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?

<details><summary>suggested run-time assertions</summary><p>

diff --git a/src/validation.cpp b/src/validation.cpp
index c12dc9e8b6..109b1f95ce 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2848,6 +2848,7 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 {
     AssertLockNotHeld(m_chainstate_mutex);
+    AssertLockNotHeld(::cs_main);
 
@@ -2949,6 +2950,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
 
 bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
 {
+    AssertLockNotHeld(m_chainstate_mutex);
+    AssertLockNotHeld(::cs_main);
     {
         LOCK(cs_main);
@@ -2979,6 +2982,7 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex
 bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
 {
     AssertLockNotHeld(m_chainstate_mutex);
+    AssertLockNotHeld(::cs_main);
 
@@ -4089,6 +4093,8 @@ bool CChainState::LoadGenesisBlock()
 
 void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
 {
+    AssertLockNotHeld(m_chainstate_mutex);

</p></details>

(edited OP and review comments)

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