Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it#24103
Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it#24103maflcko merged 3 commits intobitcoin:masterfrom
m_cs_chainstate with Mutex, and rename it#24103Conversation
-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-
What guarantees that in the future no such recursion is introduced (in a rare code-path)? |
Negative TS annotations could help. |
$ 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);
|
shaavan
left a comment
There was a problem hiding this comment.
ACK 320890c715a03603be9188a313a3f0b4f5924c5b
- The name changing from
m_cs_chainstate->m_chainstate_mutexis an improvement, IMO. - Using
AssertLockNotHeld()will avoid any risk of recursive locking in future PRs. Som_chainstate_mutexcould be safely converted from RecursiveMutex to Mutex.
|
May I ask you to reorder last two commits for the same reason as #24108 (review)? |
Co-authored-by: Hennadii Stepanov <[email protected]>
320890c to
020acea
Compare
|
Done. Commits reordered. |
| * A lock that must be held when modifying this ChainState - held in ActivateBestChain() and | ||
| * InvalidateBlock() |
There was a problem hiding this comment.
nit (please ignore)
With new annotations some parts of the comment look redundant:
| * 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 |
There was a problem hiding this comment.
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
You might want |
|
@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 Here is a followup that changes to the stronger |
Agree. |
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.
…_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
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.
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
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.
This PR is related to #19303 and gets rid of the
RecursiveMutex m_cs_chainstate.m_cs_chainstateis only held inActivateBestChain()andInvalidateBlock().So apparently there is no recursion involved, so the
m_cs_chainstatecan be a non-recursive mutex.