[primitives, consensus] Make fChecked an atomic<bool> #14072
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CheckBlock can be invoked from multiple threads in parallel. To avoid a data race, fChecked should be atomic. In practice, this would have had limited effect currently since these paths are effectively single-threaded in practice, except to maybe occasionally cause a slight bit of redundant work to be performed in CheckBlock.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
maflcko
reviewed
Aug 27, 2018
| :CBlock(static_cast<CBlockHeader>(block)) | ||
| { | ||
| vtx = block.vtx; | ||
| fChecked = false; |
There was a problem hiding this comment.
Any reason to set this to false when copying?
nit: Also, could rename the member to m_checked, since it is only used in one function on master.
| CBlock(const CBlock &block) | ||
| :CBlock(static_cast<CBlockHeader>(block)) | ||
| { | ||
| vtx = block.vtx; |
There was a problem hiding this comment.
Perform initialization in initialization list instead :-)
| :CBlock(static_cast<CBlockHeader>(block)) | ||
| { | ||
| vtx = block.vtx; | ||
| fChecked = false; |
There was a problem hiding this comment.
Perform initialization in initialization list instead :-)
|
Concept ACK |
|
Could remove the tsan suppression for |
|
Closing in favour of #14803. |
laanwj
added a commit
that referenced
this pull request
Dec 1, 2018
c5ed6e7 Move CheckBlock() call to critical section (Hennadii Stepanov) Pull request description: This is an alternative to #14803. Refs: - #14058 - #14072 - #14803 (comment) by @gmaxwell > It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here? - #14803 (comment) by @MarcoFalke Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 17, 2021
…ction c5ed6e7 Move CheckBlock() call to critical section (Hennadii Stepanov) Pull request description: This is an alternative to bitcoin#14803. Refs: - bitcoin#14058 - bitcoin#14072 - bitcoin#14803 (comment) by @gmaxwell > It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here? - bitcoin#14803 (comment) by @MarcoFalke Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb
vijaydasmp
pushed a commit
to vijaydasmp/dash
that referenced
this pull request
Sep 5, 2021
…ction c5ed6e7 Move CheckBlock() call to critical section (Hennadii Stepanov) Pull request description: This is an alternative to bitcoin#14803. Refs: - bitcoin#14058 - bitcoin#14072 - bitcoin#14803 (comment) by @gmaxwell > It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here? - bitcoin#14803 (comment) by @MarcoFalke Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb
vijaydasmp
pushed a commit
to vijaydasmp/dash
that referenced
this pull request
Sep 6, 2021
…ction c5ed6e7 Move CheckBlock() call to critical section (Hennadii Stepanov) Pull request description: This is an alternative to bitcoin#14803. Refs: - bitcoin#14058 - bitcoin#14072 - bitcoin#14803 (comment) by @gmaxwell > It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here? - bitcoin#14803 (comment) by @MarcoFalke Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a data race / thread sanitizer issue uncovered in #14058
If there are alternate suggestions about how to effectuate this change in a more compact way without having to manually declare the copy constructor and copy assignment operator (because it is deleted for
std::atomic<bool>that feedback would be welcome).CheckBlock can be invoked from multiple threads in parallel. To avoid
a data race, fChecked should be atomic. In practice, this would have
had limited effect currently since these paths are effectively
single-threaded in practice, except to maybe occasionally cause a
slight bit of redundant work to be performed in CheckBlock.