Replace RecursiveMutex `m_cs_chainstate` with Mutex, and rename it #24103

This PR is related to #19303 and gets rid of the RecursiveMutex m_cs_chainstate.

m_cs_chainstate is only held in ActivateBestChain() and InvalidateBlock(). So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex.

So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex.

What guarantees that in the future no such recursion is introduced (in a rare code-path)?

So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex.

What guarantees that in the future no such recursion is introduced (in a rare code-path)?

Negative TS annotations could help.

So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex.

What guarantees that in the future no such recursion is introduced (in a rare code-path)?

$ git diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7c0654c2c..bc69bc423 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2821,6 +2821,8 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 
 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 {
+    AssertLockNotHeld(m_chainstate_mutex);
+
     // Note that while we're often called here from ProcessNewBlock, this is
     // far from a guarantee. Things in the P2P/RPC will often end up calling
     // us in the middle of ProcessNewBlock - do not assume pblock is set
@@ -2950,6 +2952,8 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex
 
 bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
 {
+    AssertLockNotHeld(m_chainstate_mutex);
+
     // Genesis block can't be invalidated
     assert(pindex);
     if (pindex->nHeight == 0) return false;
diff --git a/src/validation.h b/src/validation.h
index 299dc66e4..21b8f9579 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -701,7 +701,7 @@ public:
      */
     bool ActivateBestChain(
         BlockValidationState& state,
-        std::shared_ptr<const CBlock> pblock = nullptr) LOCKS_EXCLUDED(cs_main);
+        std::shared_ptr<const CBlock> pblock = nullptr) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main);
 
     bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -721,7 +721,7 @@ public:
      */
     bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);
     /** Mark a block as invalid. */
-    bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);
+    bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main);
     /** Remove invalidity status from a block and its descendants. */
     void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 

ACK 320890c715a03603be9188a313a3f0b4f5924c5b

• The name changing from m_cs_chainstate -> m_chainstate_mutex is an improvement, IMO.
• Using AssertLockNotHeld() will avoid any risk of recursive locking in future PRs. So m_chainstate_mutex could be safely converted from RecursiveMutex to Mutex.

So apparently there is no recursion involved, so the m_cs_chainstate can be a non-recursive mutex.

What guarantees that in the future no such recursion is introduced (in a rare code-path)?

Btw, we have a nice 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 by @vasild which works with DEBUG_LOCKORDER.

@w0xlt

May I ask you to reorder last two commits for the same reason as #24108#pullrequestreview-858412054?

Done. Commits reordered.

ACK 020acea99b605c9b5ee7939a6acef131db84ad4a, I have reviewed the code and it looks OK, I agree it can be merged.

Code-review ACK 020acea99b605c9b5ee7939a6acef131db84ad4a 🌴

reACK 020acea99b605c9b5ee7939a6acef131db84ad4a

Changes since my last review:

• Commits have been reordered. This is done so that each commit is individually compilable and to keep the commit history safe.

ACK 020acea99b605c9b5ee7939a6acef131db84ad4a

The call graphs of CChainState::ActivateBestChain() and CChainState::InvalidateBlock() are bit involved:

https://doxygen.bitcoincore.org/class_c_chain_state.html#a1d10196aeadf2e5c76b94123635b7c7b

https://doxygen.bitcoincore.org/class_c_chain_state.html#ae8ab9222f3ad9a6aab8dba5ffa5eb530

I checked manually that recursion is not happening, but I imagine it would be easy to miss it, so I am wondering if there is an easier and more error proof way to check that something like this is not happening: ActivateBestChain() -> func1() -> func2() -> func3() -> InvalidateBlock()? Is the added annotation LOCKS_EXCLUDED(m_chainstate_mutex) doing exactly that?

Edit (to answer the last question): not necessary, here is a counter example where I added a recursive call and it compiles just fine:

<details> <summary>diff</summary>

diff --git i/src/validation.cpp w/src/validation.cpp
index c12dc9e8b6..84f99312b0 100644
--- i/src/validation.cpp
+++ w/src/validation.cpp
@@ -2842,12 +2842,18 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
 
     if (GetMainSignals().CallbacksPending() > 10) {
         SyncWithValidationInterfaceQueue();
     }
 }
 
+void CChainState::foo()
+{
+    BlockValidationState state;
+    InvalidateBlock(state, nullptr);
+}
+
 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
 {
     AssertLockNotHeld(m_chainstate_mutex);
 
     // Note that while we're often called here from ProcessNewBlock, this is
     // far from a guarantee. Things in the P2P/RPC will often end up calling
@@ -2858,12 +2864,14 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
     // ABC maintains a fair degree of expensive-to-calculate internal state
     // because this function periodically releases cs_main so that it does not lock up other threads for too long
     // during large connects - and to allow for e.g. the callback queue to drain
     // 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);
     do {
         // Block until the validation queue drains. This should largely
         // never happen in normal operation, however may happen during
diff --git i/src/validation.h w/src/validation.h
index fb258005f1..ec2c3b5f00 100644
--- i/src/validation.h
+++ w/src/validation.h
@@ -743,12 +743,14 @@ private:
 
     /** Check warning conditions and do some notifications on new chain tip set. */
     void UpdateTip(const CBlockIndex* pindexNew)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     friend ChainstateManager;
+
+    void foo();
 };
 
 /**
  * Provides an interface for creating and interacting with one or two
  * chainstates: an IBD chainstate generated by downloading blocks, and
  * an optional snapshot chainstate loaded from a UTXO snapshot. Managed

</details>

@vasild

Is the added annotation LOCKS_EXCLUDED(m_chainstate_mutex) doing exactly that?

You might want EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex).

@hebasto, yeah, that provides a stronger guarantee as per https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative, but if both are missing then we may still get recursion that is not detected at compile time. I guess that, in order to be sure, one has to manually check no recursion is happening which is tedious and error prone.

I was under the assumption that the strong annotations had been added here. Maybe this should be reverted for now?

I think it is ok, no need to revert it. After all, other code also uses the weaker LOCKS_EXCLUDED() and it is better than no annotations at all.

Here is a followup that changes to the stronger EXCLUSIVE_LOCKS_REQUIRED(!...): #24235. It caused some avalanche effect, requiring some extra annotations which I think is good.

I think it is ok, no need to revert it. After all, other code also uses the weaker LOCKS_EXCLUDED() and it is better than no annotations at all.

Agree.