Guard fHavePruned to avoid potential data race #22212

1. 
   amadeuszpawlik commented at 1:16 pm on June 10, 2021: contributor

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

2. fanquake added the label Block storage on Jun 10, 2021
3. amadeuszpawlik force-pushed on Jun 10, 2021
4. amadeuszpawlik force-pushed on Jun 10, 2021
5. Guard `fHavePruned` to avoid potential data race

   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`.

   24028831b3
6. amadeuszpawlik force-pushed on Jun 10, 2021
7. amadeuszpawlik marked this as ready for review on Jun 10, 2021
8. 
   DrahtBot commented at 9:13 pm on June 14, 2021: 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:

   • #22371 by MarcoFalke

   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.

9. 
   MarcoFalke commented at 6:03 am on June 15, 2021: member

   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 #17161 (comment)
10. Guard `fHavePruned` with its own lock

    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.

    008b1aa6c2
11. Fix double cs_HavePruned lock in init b0aa15fde0
12. 
    amadeuszpawlik commented at 7:20 pm on June 15, 2021: contributor

    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.
13. in src/init.cpp:1480 in 008b1aa6c2 outdated

    1476@@ -1476,6 +1477,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    1477 
    1478             try {
    1479                 LOCK(cs_main);
    1480+                LOCK(cs_HavePruned);
    

    ---

    ---

    

    amadeuszpawlik commented at 7:27 pm on June 15, 2021:

    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
14. DrahtBot added the label Needs rebase on Jul 20, 2021
15. 
    DrahtBot commented at 5:11 pm on July 20, 2021: member

    🐙 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”.

16. 
    DrahtBot commented at 12:28 pm on December 22, 2021: member

    • 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.
17. amadeuszpawlik closed this on Dec 28, 2021

    ---

18. DrahtBot locked this on Dec 28, 2022

Contributors
amadeuszpawlik DrahtBot MarcoFalke

Labels
Block storage Needs rebase