consensus: don’t call GetBlockPos in ReadBlockFromDisk without cs_main lock #22895

1. 
   jonatack commented at 5:43 pm on September 5, 2021: member

   Commit ccd8ef65 “Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock” in #11281 moved the cs_main lock from caller to ReadBlockFromDisk() for calling CBlockIndex::GetBlockPos(), but the second invocation doesn’t have the lock, and IIUC there is no guarantee the compiler can know if state has changed.

   Use the blockPos local variable instead, rename it to block_pos, and make it const.

2. consensus: don't call GetBlockPos in ReadBlockFromDisk without lock 350e034e64
3. DrahtBot added the label Block storage on Sep 5, 2021
4. DrahtBot added the label Consensus on Sep 5, 2021
5. MarcoFalke removed the label Consensus on Sep 6, 2021
6. theStack approved
7. 
   theStack commented at 4:53 pm on September 6, 2021: member

   Code-review ACK 350e034e64d175f3db4c85ddca42e76e279912f6

   Good catch!

8. 
   promag commented at 5:02 pm on September 6, 2021: member

   Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6.
9. 
   MarcoFalke commented at 6:47 am on September 7, 2021: member

   Shouldn’t there be a lock annotation to prevent this in the future?
10. 
    jonatack commented at 4:01 pm on September 9, 2021: member

    Shouldn’t there be a lock annotation to prevent this in the future?

    Proposed in #22932.

11. 
    DrahtBot commented at 4:23 pm on September 12, 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:

    • #9245 (Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr)

    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.

12. 
    laanwj commented at 2:54 pm on September 16, 2021: member

    Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6
13. laanwj merged this on Sep 16, 2021
14. laanwj closed this on Sep 16, 2021

    ---

15. jonatack deleted the branch on Sep 16, 2021
16. sidhujag referenced this in commit 7649ab6b04 on Sep 19, 2021
17. luke-jr referenced this in commit 94c04681ed on Oct 10, 2021
18. fanquake referenced this in commit 344f55d0bc on Oct 20, 2021
19. fanquake referenced this in commit 0c35038426 on Oct 21, 2021
20. luke-jr referenced this in commit d4d7848120 on Dec 14, 2021
21. laanwj referenced this in commit cf5bb048e8 on Jan 27, 2022
22. fanquake referenced this in commit 60c48fb634 on Feb 14, 2022
23. fanquake referenced this in commit 7febe4f3c7 on Feb 15, 2022
24. fanquake referenced this in commit 9b5f674abb on Mar 1, 2022
25. DrahtBot locked this on Oct 30, 2022

Contributors
jonatack theStack promag MarcoFalke DrahtBot laanwj

Labels
Block storage