Skip to content

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Jan 19, 2022

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.

-BEGIN VERIFY SCRIPT-
s() { sed -i 's/m_cs_chainstate/m_chainstate_mutex/g' $1; }
s src/validation.cpp
s src/validation.h
-END VERIFY SCRIPT-
@maflcko
Copy link
Member

maflcko commented Jan 19, 2022

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

@hebasto
Copy link
Member

hebasto commented Jan 19, 2022

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.

@hebasto
Copy link
Member

hebasto commented Jan 19, 2022

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

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hebasto
Copy link
Member

hebasto commented Jan 20, 2022

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 95975dd by @vasild which works with DEBUG_LOCKORDER.

@hebasto
Copy link
Member

hebasto commented Jan 24, 2022

@w0xlt

May I ask you to reorder last two commits for the same reason as #24108 (review)?

@w0xlt w0xlt force-pushed the m_cs_chainstate_mutex branch from 320890c to 020acea Compare January 24, 2022 16:29
@w0xlt
Copy link
Contributor Author

w0xlt commented Jan 24, 2022

Done. Commits reordered.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +537 to +538
* A lock that must be held when modifying this ChainState - held in ActivateBestChain() and
* InvalidateBlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (please ignore)

With new annotations some parts of the comment look redundant:

Suggested change
* A lock that must be held when modifying this ChainState - held in ActivateBestChain() and
* InvalidateBlock()
* A lock that must be held when modifying this ChainState

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK 020acea 🌴

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 020acea

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.

@maflcko maflcko merged commit ad05e68 into bitcoin:master Jan 31, 2022
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 020acea

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:

diff
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

@hebasto
Copy link
Member

hebasto commented Jan 31, 2022

@vasild

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

You might want EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex).

@w0xlt w0xlt deleted the m_cs_chainstate_mutex branch January 31, 2022 12:00
@vasild
Copy link
Contributor

vasild commented Jan 31, 2022

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 1, 2022
@maflcko
Copy link
Member

maflcko commented Feb 2, 2022

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

@vasild
Copy link
Contributor

vasild commented Feb 2, 2022

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.

@hebasto
Copy link
Member

hebasto commented Feb 2, 2022

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.

vasild added a commit to vasild/bitcoin that referenced this pull request Feb 2, 2022
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.
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Feb 8, 2022
…_REQUIRED()

99de806 validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() (Vasil Dimov)

Pull request description:

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

ACKs for top commit:
  hebasto:
    ACK 99de806.
  jonatack:
    ACK 99de806. Tested with Debian clang version 13.0.1.  Reproduced hebasto's results. Verified that  `LoadExternalBlockFile()` needs the annotation added here.

Tree-SHA512: 59640d9ad472cdb5066ecde89cc0aff8632a351fc030f39bb43800d2c856fb1aed3576e4134212d32be161b18780f06dc5066ac71df7f7cd69e3f21f886e1542
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jul 17, 2022
bitcoin/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.
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 12, 2022
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
backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Jan 18, 2023
bitcoin/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.
@bitcoin bitcoin locked and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants