Skip to content

Guard fHavePruned to avoid potential data race #22212

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Guard fHavePruned to avoid potential data race #22212

wants to merge 3 commits into from

Conversation

amadeuszpawlik
Copy link
Contributor

@amadeuszpawlik amadeuszpawlik commented Jun 10, 2021 •
edited
Loading

As detailed in #21627, there is a potential data race on fHavePruned as one thread could be reading it while another one is writing to it.

Guard fHavePruned, lock in IsBlockPruned (FlushStateToDisk is holding cs_main while writing to the variable, so this ensures that the data race cannot occur).

@amadeuszpawlik amadeuszpawlik force-pushed the fix_guard_fhavepruned branch 2 times, most recently from 9591794 to b20925d Compare June 10, 2021 15:08
@amadeuszpawlik
Guard fHavePruned to avoid potential data race
2402883 
A potential data race has been detected on `fHavePruned` where one
thread is writing to it via `FlushStateToDisk` as another thread is
reading it via `IsBlockPruned`.
@amadeuszpawlik amadeuszpawlik force-pushed the fix_guard_fhavepruned branch from b20925d to 2402883 Compare June 10, 2021 15:32
@amadeuszpawlik amadeuszpawlik marked this pull request as ready for review June 10, 2021 19:17
@DrahtBot DrahtBot mentioned this pull request Jun 14, 2021
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 14, 2021 •
edited
Loading

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

Conflicts

Reviewers, 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
Copy link
Member

maflcko commented Jun 15, 2021

Looks ok, just two notes:

  • Would be nice to move blockstorage away from cs_main to it's own lock, so that blockstorage related RPC don't block the validation thread, which might be busy accepting txs to the mempool.
  • The fix is a good first step, but likely incomplete. See CBlockIndex::nStatus race when pruning #17161 (comment)

@amadeuszpawlik
Guard fHavePruned with its own lock
008b1aa 
Go from using `cs_main` to `cs_HavePruned` as a mutex for accessing
`fHavePruned`. This so as to not block the validation thread with
blockstorage related RPCs.
@amadeuszpawlik
Fix double cs_HavePruned lock in init
b0aa15f
@amadeuszpawlik
Copy link
Contributor Author

Thanks for the feedback @MarcoFalke
I changed cs_main to cs_HavePruned; good point.
As for your second point, I'll have to investigate more.

amadeuszpawlik
amadeuszpawlik commented Jun 15, 2021
View reviewed changes
src/init.cpp
@@ -1476,6 +1477,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

try {
LOCK(cs_main);
LOCK(cs_HavePruned);
Copy link
Contributor Author

@amadeuszpawlik amadeuszpawlik Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure whether to lock the whole block or line 1485 specifically
My reasoning here was that verifying blocks ought to have higher priority than answering RPC requests

@DrahtBot DrahtBot mentioned this pull request Jun 30, 2021
Merged
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@bitcoin bitcoin locked and limited conversation to collaborators Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Block storage Needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amadeuszpawlik @DrahtBot @maflcko @fanquake