-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it
#24103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
-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.
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_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. |
hebasto
left a comment
There was a problem hiding this 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.
| * A lock that must be held when modifying this ChainState - held in ActivateBestChain() and | ||
| * InvalidateBlock() |
There was a problem hiding this comment.
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:
| * 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 |
theStack
left a comment
There was a problem hiding this 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 🌴
shaavan
left a comment
There was a problem hiding this 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.
There was a problem hiding this 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:
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.