Add missing cs_main locks in ThreadImport(…)/Shutdown(…)/gettxoutsetinfo(…)/InitScriptExecutionCache(). Add missing locking annotations. #15192

1. 
   practicalswift commented at 9:29 pm on January 17, 2019: contributor

   • Add missing cs_main lock required when accessing pblocktree. Add annotation.
   • Add missing cs_main lock required when accessing pcoinsTip. Add annotation.
   • Add missing cs_main lock required when accessing pcoinsdbview. Add annotation.
   • Add missing cs_main lock required when accessing scriptExecutionCache. Add annotation.
2. practicalswift renamed this:
   Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add annotations.
   Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add missing locking annotations.
   on Jan 17, 2019
3. 
   DrahtBot commented at 11:33 pm on January 17, 2019: 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:

   • #16443 (refactor: have CCoins* data managed under CChainState by jamesob)
   • #15997 (refactor: GuessVerificationProgress requires cs_main lock by promag)
   • #13204 (Faster sigcache nonce by JeremyRubin)

   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.

4. in src/bench/block_assemble.cpp:76 in 01b4b0bc2c outdated

   72@@ -73,9 +73,12 @@ static void AssembleBlock(benchmark::State& state)
   73     boost::thread_group thread_group;
   74     CScheduler scheduler;
   75     {
   76-        ::pblocktree.reset(new CBlockTreeDB(1 << 20, true));
   77-        ::pcoinsdbview.reset(new CCoinsViewDB(1 << 23, true));
   78-        ::pcoinsTip.reset(new CCoinsViewCache(pcoinsdbview.get()));
   79+        {
   

   ---

   ---

   

   MarcoFalke commented at 4:01 am on January 25, 2019:

   style-nit: I created the above scope to prevent “leaking” of variable names further down, so no need to nest scopes here, just close/open the existing one.

   ---

   

   practicalswift commented at 9:57 am on January 25, 2019:

   Fixed!
5. practicalswift force-pushed on Jan 25, 2019
6. 
   practicalswift commented at 9:57 am on January 25, 2019: contributor

   @MarcoFalke Thanks for reviewing. Feedback addressed! Please re-review :-)
7. 
   practicalswift commented at 4:48 pm on January 31, 2019: contributor

   @MarcoFalke Would it be possible to get a release milestone for this PR? :-)
8. MarcoFalke added this to the milestone 0.18.0 on Jan 31, 2019
9. in src/validation.cpp:3620 in fbb548ff38 outdated

   3625@@ -3625,7 +3626,7 @@ uint64_t CalculateCurrentUsage()
   3626 }
   3627 
   3628 /* Prune a block file (modify associated database entries)*/
   3629-void PruneOneBlockFile(const int fileNumber)
   3630+void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
   

   ---

   ---

   

   MarcoFalke commented at 6:29 pm on February 6, 2019:

   Needs this commit removed
10. practicalswift force-pushed on Feb 6, 2019
11. practicalswift force-pushed on Feb 7, 2019
12. 
    practicalswift commented at 9:53 pm on February 7, 2019: contributor

    @MarcoFalke Fixed! Please re-review :-)
13. MarcoFalke removed this from the milestone 0.18.0 on Feb 11, 2019
14. 
    practicalswift commented at 6:55 am on February 12, 2019: contributor

    @MarcoFalke What is the reason for the removal of the 0.18.0 milestone?
15. 
    MarcoFalke commented at 4:21 pm on February 12, 2019: member

    It couldn’t be merged without review, and that seems not trivial in some of the hunks
16. 
    MarcoFalke commented at 4:23 pm on February 12, 2019: member

    anything in src/test and src/bench looks good and can be merged
17. MarcoFalke referenced this in commit 4d2767c228 on Feb 15, 2019
18. practicalswift force-pushed on Feb 15, 2019
19. 
    practicalswift commented at 12:55 pm on February 15, 2019: contributor

    @MarcoFalke Thanks for the quick turnaround with regards to the test/bench split-up PR. I’ve now rebased this one on top of master.

    Regarding the remaining changes: perhaps I should exclude any non-trivial changes (by opting out) so that we get proper annotations (GUARDED_BY). That will help guard against future missed locks which is one step forward :-)

20. 
    practicalswift commented at 1:10 pm on February 15, 2019: contributor

    @MarcoFalke Is it the added lock InitScriptExecutionCache you feel could be non-trivial to review? If so I can just skip it :-)
21. practicalswift renamed this:
    Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add missing locking annotations.
    Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache(). Add missing locking annotations.
    on Mar 4, 2019
22. DrahtBot added the label RPC/REST/ZMQ on Mar 19, 2019
23. DrahtBot added the label Validation on Mar 19, 2019
24. DrahtBot added the label Needs rebase on Jun 5, 2019
25. practicalswift force-pushed on Jun 5, 2019
26. 
    practicalswift commented at 11:22 pm on June 5, 2019: contributor

    Rebased!
27. DrahtBot removed the label Needs rebase on Jun 5, 2019
28. practicalswift force-pushed on Jun 5, 2019
29. 
    practicalswift commented at 5:55 pm on June 26, 2019: contributor

    @MarcoFalke Should this locking PR be closed? Please advice.
30. 
    MarcoFalke commented at 6:15 pm on June 26, 2019: member

    Concept ACK
31. MarcoFalke added this to the milestone 0.19.0 on Jun 26, 2019
32. practicalswift force-pushed on Jun 26, 2019
33. DrahtBot added the label Needs rebase on Jul 3, 2019
34. Add missing cs_main lock required when accessing scriptExecutionCache. Add annotation. b093101fcc
35. Add missing cs_main lock required when accessing pcoinsdbview. Add annotation. 35cb3a5915
36. Add missing cs_main lock required when accessing pcoinsTip. Add annotation. 0171a1b2cf
37. Add missing cs_main lock required when accessing pblocktree. Add annotation. b6ba1832d8
38. practicalswift force-pushed on Jul 3, 2019
39. DrahtBot removed the label Needs rebase on Jul 4, 2019
40. practicalswift closed this on Aug 10, 2019

    ---

41. practicalswift deleted the branch on Apr 10, 2021
42. Munkybooty referenced this in commit dc8cd6897b on Sep 5, 2021
43. Munkybooty referenced this in commit 8398a6750c on Sep 7, 2021
44. Munkybooty referenced this in commit 9b00ccea66 on Sep 7, 2021
45. Munkybooty referenced this in commit 9a157896f1 on Sep 9, 2021
46. Munkybooty referenced this in commit 4f390c3fb4 on Sep 10, 2021
47. Munkybooty referenced this in commit 9ec6b6e64a on Sep 13, 2021
48. Munkybooty referenced this in commit 06925a1464 on Sep 13, 2021
49. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift DrahtBot MarcoFalke

Labels
RPC/REST/ZMQ Validation

Milestone
0.19.0