validation, refactor: add missing thread safety lock assertions

jonatack commented at 11:56 am on January 27, 2022: member

A number of functions in validation.{h,cpp} have a thread safety lock annotation in the declaration but are missing the corresponding run-time lock assertion in the definition.

jonatack commented at 12:05 pm on January 27, 2022: member

How to review: verify that a Clang debug build is clean (no -Wthread-safety-analysis warnings) and that each lock assertion (AssertLockHeld) has a corresponding lock annotation (EXCLUSIVE_LOCKS_REQUIRED) in the declaration.

For more information about Clang thread safety analysis, see - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

DrahtBot added the label Validation on Jan 27, 2022

in src/validation.cpp:2660 in fe73d8fad1 outdated

2657@@ -2650,6 +2658,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
2658  * known to be invalid (it's however far from certain to be valid).
2659  */
2660 CBlockIndex* CChainState::FindMostWorkChain() {

shaavan commented at 2:29 pm on January 27, 2022:

nit:

0CBlockIndex* CChainState::FindMostWorkChain()
1{

jonatack commented at 2:52 pm on January 27, 2022:

done

in src/validation.cpp:1530 in fe73d8fad1 outdated

1525@@ -1524,6 +1526,8 @@ void CChainState::CheckForkWarningConditions()
1526 // Called both upon regular invalid block discovery *and* InvalidateBlock
1527 void CChainState::InvalidChainFound(CBlockIndex* pindexNew)
1528 {
1529+    AssertLockHeld(cs_main);
1530+

shaavan commented at 2:30 pm on January 27, 2022:

nit:

jonatack commented at 2:52 pm on January 27, 2022:

done

in src/validation.cpp:5035 in fe73d8fad1 outdated

5030@@ -5014,6 +5031,8 @@ void ChainstateManager::Reset()
5031 
5032 void ChainstateManager::MaybeRebalanceCaches()
5033 {
5034+    AssertLockHeld(::cs_main);
5035+

shaavan commented at 2:35 pm on January 27, 2022:

nit:

jonatack commented at 2:52 pm on January 27, 2022:

done

in src/validation.h:531 in fe73d8fad1 outdated

528@@ -529,6 +529,7 @@ class CChainState
529     //! @returns whether or not the CoinsViews object has been fully initialized and we can
530     //!          safely flush this object to disk.
531     bool CanFlushToDisk() const EXCLUSIVE_LOCKS_REQUIRED(cs_main) {

shaavan commented at 2:35 pm on January 27, 2022:

nit:

0    bool CanFlushToDisk() const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1    {

jonatack commented at 2:52 pm on January 27, 2022:

done

shaavan commented at 2:36 pm on January 27, 2022: contributor

Concept ACK

I think it makes sense to add the missing thread-safety lock assertions, which prevents the risk of a race condition.

I shall take a thorough look at this PR before ACKing it. Meanwhile, I found some nit suggestions that should be addressed.

jonatack force-pushed on Jan 27, 2022

jonatack commented at 2:53 pm on January 27, 2022: member

Thanks @shaavan, updated with your feedback.

hebasto commented at 9:29 pm on January 28, 2022: member

Concept ACK, it follows https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization.

shaavan commented at 12:23 pm on January 30, 2022: contributor

I looked into both src/validation.cpp and src/validation.h, and I found a list of functions that are declared using EXCLUSIVE_LOCK_REQUIRED() but does not have AssertLockHeld() in their definition, and are also not yet addressed in this PR.

Declared in src/validation.cpp:

LimitMempoolSize [EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main)]
CheckFeeRate [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)]
AcceptToMemoryPool [EXCLUSIVE_LOCKS_REQUIRED(cs_main)]
ContextualCheckBlockHeader [EXCLUSIVE_LOCKS_REQUIRED(cs_main)]
PreChecks [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)]
PolicyScriptChecks [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)]
ConsensusScriptChecks [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)]
Finalize [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)]

Declared in src/validation.h:

~DisconnectTip [EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs)] but only AssertLockHeld(cs_main)~
ProcessTransaction [EXCLUSIVE_LOCKS_REQUIRED(cs_main)]

I think all of these functions come in the scope of this PR, and should be addressed in it.

Edit: DisconnectTip() has AssertLockHeld() statement for both the mutex. So no need to make any changes to it. Thanks @jonatack for catching it.

Done, except for touching the DisconnectTip() conditional lock assertion on m_mempool->cs

jonatack force-pushed on Jan 30, 2022

jonatack commented at 2:51 pm on January 30, 2022: member

@shaavan thanks! Done, except for touching the DisconnectTip() conditional lock assertion on m_mempool->cs (same in ActivateBestChainStep() and ConnectTip()) added in 617661703ac.

shaavan approved

shaavan commented at 10:12 am on January 31, 2022: contributor

ACK 1c0737e9ccc8f633b4fe06b17d04f83e6d35f4df

Changes since my last review:

Added missing thread-safety lock for these functions.

I was able to successfully compile this PR on Ubuntu 20.04. I agree that this PR can be merged.

shaavan commented at 10:16 am on January 31, 2022: contributor

Btw I was curious about one thing. Why some functions have AssertLockHeld(cs_main) and some AssertLockHeld(::cs_main)?

jonatack commented at 10:19 am on January 31, 2022: member

Btw I was curious about one thing. Why some functions have AssertLockHeld(cs_main) and some AssertLockHeld(::cs_main)?

Good question. See #22932 (review)

MarcoFalke added the label Refactoring on Jan 31, 2022

shaavan commented at 11:04 am on January 31, 2022: contributor

So what I understood from #22932 (comment) is that ::cs_main is preferred over cs_main because the former one explicitly indicates that it’s a global mutex. And each time a function is updated, cs_main should be converted to ::cs_main if not already done. If my analysis is erroneous, please do correct me. And thanks, @jonatack, for helping with clarification.

shaavan commented at 11:09 am on January 31, 2022: contributor

I found a minor nit that I didn’t address in my previous review. Feel free to ignore, in case not updating the PR for another reason.

in src/validation.cpp:293 in 1c0737e9cc outdated

287@@ -288,6 +288,9 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
288 static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age)
289     EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main)
290 {
291+    AssertLockHeld(pool.cs);
292+    AssertLockHeld(::cs_main);
293+

shaavan commented at 11:10 am on January 31, 2022:

nit:

jonatack commented at 1:02 pm on January 31, 2022:

Ok. Will do if need to retouch.

jonatack commented at 7:35 pm on February 8, 2022:

Done

jonatack commented at 1:05 pm on January 31, 2022: member

each time a function is updated, cs_main should be converted to ::cs_main if not already done

For new code, yes; for updated code, I suppose it depends on the PR author and on reviewer feedback.

(Thanks for reviewing here!)

DrahtBot commented at 5:43 am on February 2, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24232 (assumeutxo: add init and completion logic by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack force-pushed on Feb 8, 2022

jonatack commented at 8:07 pm on February 8, 2022: member

Rebased and updated per git range-diff 280a777 1c0737e 686d35d to add missing assertions seen in #24235#pullrequestreview-873446100 and to replace a lock in ChainstateManager::Reset() with thread safety analysis.

jonatack force-pushed on Feb 8, 2022

vasild approved

vasild commented at 12:06 pm on February 9, 2022: member

ACK 686d35de6bd769efba35177cd9c91f431f46e5a5

ChainstateManager::Reset() is called just from the destructor ChainstateManager::~ChainstateManager() and from a test, but from the test the destructor is called shortly after. So, maybe the Reset() method can be removed, its body copied to the destructor and the explicit call to Reset() from the test removed? The only difference is that now, from the test, Reset() is called first and UnloadBlockIndex() is called afterwards (via the destructor). That order will be swapped, a cursory look shows that should be ok.

jonatack commented at 1:46 pm on February 9, 2022: member

Thanks @vasild for having a look.

maybe the Reset() method can be removed, its body copied to the destructor and the explicit call to Reset() from the test removed

Done. It looks like Reset() was added in 89cdf4d5692 when the requirements probably weren’t finalized. Also saw that we can replace the cs_main lock in UnloadBlockIndex() with a thread safety annotation. Updated per git diff 686d35d 9a18bdb.

jonatack force-pushed on Feb 9, 2022

MarcoFalke commented at 1:55 pm on February 9, 2022: member

Mind splitting the inline refactor from the annotations refactor into another pull? If not, that is fine, too.

jonatack commented at 3:00 pm on February 9, 2022: member

Mind splitting the inline refactor from the annotations refactor into another pull? If not, that is fine, too.

Done (#24299). This is now the first 2 commits of the 3 that were ACKed by @vasild above (thanks!)

jonatack force-pushed on Feb 9, 2022

in src/validation.cpp:291 in acdead8a26 outdated

287@@ -288,6 +288,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
288 static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age)
289     EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main)
290 {
291+    AssertLockHeld(pool.cs);
292+    AssertLockHeld(::cs_main);

hebasto commented at 5:21 pm on February 9, 2022:

nit: Reorder here and in EXCLUSIVE_LOCKS_REQUIRED?

jonatack commented at 6:24 pm on February 9, 2022:

Done (thanks!)

hebasto approved

hebasto commented at 5:22 pm on February 9, 2022: member

ACK acdead8a26e92991987efb346566b5474aff8e74, I have reviewed the code (it was a bit tedious :smile:) and it looks OK, I agree it can be merged.

Add missing thread safety lock assertions in validation.cpp

Co-authored-by: Shashwat <svangani239@gmail.com>

37af8a20cf

Add missing thread safety lock assertions in validation.h f485a07454

jonatack force-pushed on Feb 9, 2022

jonatack commented at 6:26 pm on February 9, 2022: member

Updated per git diff acdead8 f485a07. @shaavan, @vasild, @hebasto: thank you for reviewing, mind having a final look?

hebasto approved

hebasto commented at 6:44 pm on February 9, 2022: member

re-ACK f485a0745455b46390f1c14260643ad69c8fe2ad, only suggested change since my previous review.

vasild approved

vasild commented at 2:36 pm on February 16, 2022: member

ACK f485a0745455b46390f1c14260643ad69c8fe2ad

MarcoFalke merged this on Feb 17, 2022

MarcoFalke closed this on Feb 17, 2022

jonatack deleted the branch on Feb 17, 2022

laanwj referenced this in commit cba41db327 on Mar 7, 2022

Fabcien referenced this in commit 7c8d98584d on Dec 19, 2022

DrahtBot locked this on Feb 17, 2023

validation, refactor: add missing thread safety lock assertions #24177

Conflicts