validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235

vasild · 2022-02-02T10:44:50Z

#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 · 2022-02-02T11:16:29Z

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 · 2022-02-02T11:20:42Z

I tested with clang 14.

maflcko · 2022-02-02T11:27:24Z

src/validation.h

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


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?

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.

Can you elaborate?

See the commit that you ACKed: 9ac8f6d

It is wasted effort, since cs_main is recursive

It is impossible to properly implement: Add missing thread safety annotations #20272 (comment)

It is too verbose for globals, as they propagate outside the scope of "their" class

hebasto · 2022-02-02T11:28:13Z

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

hebasto · 2022-02-02T11:52:44Z

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 · 2022-02-02T12:21:13Z

Restarted some CI jobs that failed due to connectivity issues.

bitcoin#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 · 2022-02-02T13:06:28Z

Concept ACK.

vasild · 2022-02-02T13:07:19Z

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 (5730f73) 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.

hebasto

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

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

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.

hebasto

ACK 99de806.

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 · 2022-02-02T14:11:21Z

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

ACK 99de806. 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?

suggested run-time assertions

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

maflcko · 2022-02-08T14:53:41Z

(edited OP and review comments)

jonatack · 2022-02-08T19:23:41Z

(edited OP and review comments)

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

Added the thread safety assertions in my comment in #24235 (review) to #24177.

Summary: ``` So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex. ``` Backport of [[bitcoin/bitcoin#24103 | core#24103]]. Backport of [[bitcoin/bitcoin#24235 | core#24235]]. Test Plan: With Debug and Clang: ninja all check Run the TSAN build. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12227

vasild mentioned this pull request Feb 2, 2022

Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it #24103

Merged

maflcko reviewed Feb 2, 2022

View reviewed changes

DrahtBot added Block storage P2P Validation labels Feb 2, 2022

vasild force-pushed the followup_to_24103 branch from 5730f73 to 99de806 Compare February 2, 2022 13:04

maflcko added Refactoring and removed P2P Validation Block storage labels Feb 2, 2022

hebasto reviewed Feb 2, 2022

View reviewed changes

hebasto approved these changes Feb 2, 2022

View reviewed changes

hebasto mentioned this pull request Feb 2, 2022

p2p: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it #24157

Merged

maflcko mentioned this pull request Feb 4, 2022

validation: don't re-acquire cs_main during IBD in CChainState::IsInitialBlockDownload() #24220

Closed

jonatack reviewed Feb 4, 2022

View reviewed changes

maflcko merged commit 280a777 into bitcoin:master Feb 8, 2022

jonatack mentioned this pull request Feb 8, 2022

validation, refactor: add missing thread safety lock assertions #24177

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 9, 2022

Merge bitcoin#24235: validation: use stronger EXCLUSIVE_LOCKS_REQUIRED()

4a4b4fe

bitcoin deleted a comment from jr32182 Feb 9, 2022

vasild deleted the followup_to_24103 branch February 9, 2022 11:20

bitcoin locked and limited conversation to collaborators Feb 9, 2023

validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235

validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235

Uh oh!

Conversation

vasild commented Feb 2, 2022 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

vasild commented Feb 2, 2022

Uh oh!

maflcko Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

vasild commented Feb 2, 2022

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 2, 2022

Uh oh!

jonatack left a comment • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 8, 2022

Uh oh!

jonatack commented Feb 8, 2022

Uh oh!

Uh oh!

vasild commented Feb 2, 2022 •

edited by maflcko

Loading

hebasto left a comment •

edited

Loading

jonatack left a comment •

edited by maflcko

Loading