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

pull practicalswift wants to merge 4 commits into bitcoin:master from practicalswift:validation-cs_main changing 4 files +22 −10
  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


practicalswift DrahtBot MarcoFalke

Labels
RPC/REST/ZMQ Validation

Milestone
0.19.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me