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.
-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-
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)?
0$ git diff
1diff --git a/src/validation.cpp b/src/validation.cpp
2index 7c0654c2c..bc69bc423 100644
3--- a/src/validation.cpp
4+++ b/src/validation.cpp
5@@ -2821,6 +2821,8 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
6
7 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
8 {
9+ AssertLockNotHeld(m_chainstate_mutex);
10+
11 // Note that while we're often called here from ProcessNewBlock, this is
12 // far from a guarantee. Things in the P2P/RPC will often end up calling
13 // us in the middle of ProcessNewBlock - do not assume pblock is set
14@@ -2950,6 +2952,8 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex
15
16 bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
17 {
18+ AssertLockNotHeld(m_chainstate_mutex);
19+
20 // Genesis block can't be invalidated
21 assert(pindex);
22 if (pindex->nHeight == 0) return false;
23diff --git a/src/validation.h b/src/validation.h
24index 299dc66e4..21b8f9579 100644
25--- a/src/validation.h
26+++ b/src/validation.h
27@@ -701,7 +701,7 @@ public:
28 */
29 bool ActivateBestChain(
30 BlockValidationState& state,
31- std::shared_ptr<const CBlock> pblock = nullptr) LOCKS_EXCLUDED(cs_main);
32+ std::shared_ptr<const CBlock> pblock = nullptr) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main);
33
34 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);
35
36@@ -721,7 +721,7 @@ public:
37 */
38 bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);
39 /** Mark a block as invalid. */
40- bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);
41+ bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main);
42 /** Remove invalidity status from a block and its descendants. */
43 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
44
ACK 320890c715a03603be9188a313a3f0b4f5924c5b
m_cs_chainstate -> m_chainstate_mutex is an improvement, IMO.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.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
532@@ -533,10 +533,11 @@ class CChainState
533 arith_uint256 nLastPreciousChainwork = 0;
534
535 /**
536- * the ChainState CriticalSection
537- * A lock that must be held when modifying this ChainState - held in ActivateBestChain()
538+ * The ChainState Mutex
539+ * A lock that must be held when modifying this ChainState - held in ActivateBestChain() and
540+ * InvalidateBlock()
nit (please ignore)
With new annotations some parts of the comment look redundant:
0 * A lock that must be held when modifying this ChainState
reACK 020acea99b605c9b5ee7939a6acef131db84ad4a
Changes since my last review:
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:
0diff --git i/src/validation.cpp w/src/validation.cpp
1index c12dc9e8b6..84f99312b0 100644
2--- i/src/validation.cpp
3+++ w/src/validation.cpp
4@@ -2842,12 +2842,18 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) {
5
6 if (GetMainSignals().CallbacksPending() > 10) {
7 SyncWithValidationInterfaceQueue();
8 }
9 }
10
11+void CChainState::foo()
12+{
13+ BlockValidationState state;
14+ InvalidateBlock(state, nullptr);
15+}
16+
17 bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr<const CBlock> pblock)
18 {
19 AssertLockNotHeld(m_chainstate_mutex);
20
21 // Note that while we're often called here from ProcessNewBlock, this is
22 // far from a guarantee. Things in the P2P/RPC will often end up calling
23@@ -2858,12 +2864,14 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
24 // ABC maintains a fair degree of expensive-to-calculate internal state
25 // because this function periodically releases cs_main so that it does not lock up other threads for too long
26 // during large connects - and to allow for e.g. the callback queue to drain
27 // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time
28 LOCK(m_chainstate_mutex);
29
30+ foo();
31+
32 CBlockIndex *pindexMostWork = nullptr;
33 CBlockIndex *pindexNewTip = nullptr;
34 int nStopAtHeight = gArgs.GetIntArg("-stopatheight", DEFAULT_STOPATHEIGHT);
35 do {
36 // Block until the validation queue drains. This should largely
37 // never happen in normal operation, however may happen during
38diff --git i/src/validation.h w/src/validation.h
39index fb258005f1..ec2c3b5f00 100644
40--- i/src/validation.h
41+++ w/src/validation.h
42@@ -743,12 +743,14 @@ private:
43
44 /** Check warning conditions and do some notifications on new chain tip set. */
45 void UpdateTip(const CBlockIndex* pindexNew)
46 EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
47
48 friend ChainstateManager;
49+
50+ void foo();
51 };
52
53 /**
54 * Provides an interface for creating and interacting with one or two
55 * chainstates: an IBD chainstate generated by downloading blocks, and
56 * an optional snapshot chainstate loaded from a UTXO snapshot. Managed
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.