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

jonatack · 2022-01-27T11:56:48Z

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 · 2022-01-27T12:05:12Z

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

shaavan

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.

src/validation.cpp

src/validation.h

jonatack · 2022-01-27T14:53:35Z

Thanks @shaavan, updated with your feedback.

hebasto

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

shaavan

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 · 2022-01-30T14:51:47Z

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

shaavan

ACK 1c0737e

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 · 2022-01-31T10:16:43Z

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

jonatack · 2022-01-31T10:19:34Z

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

Good question. See #22932 (comment)

shaavan · 2022-01-31T11:04:33Z

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

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.

src/validation.cpp

jonatack · 2022-01-31T13:05:34Z

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 · 2022-02-02T05:43:59Z

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 · 2022-02-08T20:07:56Z

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

vasild

ACK 686d35d

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 · 2022-02-09T13:46:06Z

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 89cdf4d 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.

maflcko · 2022-02-09T13:55:29Z

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

jonatack · 2022-02-09T15:00:48Z

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

hebasto

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

src/validation.cpp

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

jonatack · 2022-02-09T18:26:19Z

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

hebasto

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

vasild

ACK f485a07

…nager::Reset thread safety cleanups ae9ceed validation, refactoring: remove ChainstateManager::Reset() (Jon Atack) daad009 validation: replace lock with annotation in UnloadBlockIndex() (Jon Atack) Pull request description: Thread safety refactoring seen in #24177: - replace re-acquiring lock cs_main with a thread safety annotation in UnloadBlockIndex() - remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing) ACKs for top commit: laanwj: Code review ACK ae9ceed vasild: ACK ae9ceed klementtan: crACK ae9ceed Tree-SHA512: cebb782572997cc2dda01590d6bb6c5e479e8202324d8b6ff459b814ce09e818b996c881736bfebd1b8bf4b6d7a0f79faf3ffea176a4699dd7d7429de2db2d13

Summary: Co-authored-by: Shashwat <svangani239@gmail.com> This is a backport of [[bitcoin/bitcoin#24177 | core#24177]] Some of this work was already done in D4581. Also add the lock annotation for CoinsDB which was missed in D7582 Test Plan: With DEBUG and TSAN: `ninja all check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12915

DrahtBot added the Validation label Jan 27, 2022

shaavan reviewed Jan 27, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from fe73d8f to 8b5f841 Compare January 27, 2022 14:53

hebasto reviewed Jan 28, 2022

View reviewed changes

shaavan reviewed Jan 30, 2022

View reviewed changes

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from 8b5f841 to 1c0737e Compare January 30, 2022 14:51

shaavan approved these changes Jan 31, 2022

View reviewed changes

maflcko added the Refactoring label Jan 31, 2022

shaavan reviewed Jan 31, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 2, 2022

assumeutxo: add init and completion logic #24232

Closed

1 task

DrahtBot mentioned this pull request Feb 2, 2022

assumeutxo #15606

Closed

18 tasks

jonatack mentioned this pull request Feb 8, 2022

validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() #24235

Merged

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from 1c0737e to 7ac8b46 Compare February 8, 2022 20:04

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from 7ac8b46 to 686d35d Compare February 8, 2022 20:37

vasild approved these changes Feb 9, 2022

View reviewed changes

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from 686d35d to 9a18bdb Compare February 9, 2022 13:46

jonatack mentioned this pull request Feb 9, 2022

validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups #24299

Merged

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from 9a18bdb to acdead8 Compare February 9, 2022 15:01

hebasto approved these changes Feb 9, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jonatack and others added 2 commits February 9, 2022 19:13

Add missing thread safety lock assertions in validation.cpp

37af8a2

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

Add missing thread safety lock assertions in validation.h

f485a07

jonatack force-pushed the add-missing-lock-assertions-in-validation branch from acdead8 to f485a07 Compare February 9, 2022 18:24

hebasto approved these changes Feb 9, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 10, 2022

validation: Remove useless call to mempool->clear() #20030

Closed

vasild approved these changes Feb 16, 2022

View reviewed changes

maflcko merged commit 03c8c69 into bitcoin:master Feb 17, 2022

jonatack deleted the add-missing-lock-assertions-in-validation branch February 17, 2022 08:36

bitcoin locked and limited conversation to collaborators Feb 17, 2023

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

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

Uh oh!

Conversation

jonatack commented Jan 27, 2022

Uh oh!

jonatack commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jan 27, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jan 30, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Jan 31, 2022

Uh oh!

jonatack commented Jan 31, 2022

Uh oh!

shaavan commented Jan 31, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jan 31, 2022

Uh oh!

DrahtBot commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Feb 9, 2022

Uh oh!

maflcko commented Feb 9, 2022

Uh oh!

jonatack commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Feb 9, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jan 27, 2022 •

edited

Loading

shaavan left a comment •

edited

Loading

DrahtBot commented Feb 2, 2022 •

edited

Loading

jonatack commented Feb 8, 2022 •

edited

Loading

jonatack commented Feb 9, 2022 •

edited

Loading