[refactor] interfaces: Add missing LockAnnotation for cs_main #15855

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1904-interfacesLock changing 11 files +55 −20
  1. MarcoFalke commented at 4:40 PM on April 19, 2019: member

    This adds missing LockAnnotation lock(::cs_main); to src/interfaces/chain.cpp (as well as tests and benchmarks)

  2. MarcoFalke added the label Refactoring on Apr 19, 2019
  3. MarcoFalke force-pushed on Apr 19, 2019
  4. DrahtBot commented at 5:43 PM on April 19, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
    • #15921 (Tidy up ValidationState interface by jnewbery)

    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.

  5. MarcoFalke force-pushed on Apr 19, 2019
  6. in src/wallet/test/wallet_tests.cpp:250 in fa90b1a2e1 outdated 
     245 | @@ -245,9 +246,11 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 246 |  BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
 247 |  {
 248 |      auto chain = interfaces::MakeChain();
 249 | +    LockAnnotation lock(::cs_main);
 250 | +    auto locked_chain = chain->lock();
    promag commented at 10:50 PM on April 21, 2019:

    Why was this moved?

  7. in src/test/txvalidationcache_tests.cpp:80 in fa90b1a2e1 outdated 
      78 | -    BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash());
  79 | -    mempool.clear();
  80 | +    {
  81 | +        LOCK(cs_main);
  82 | +        BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash());
  83 | +        mempool.clear();
    promag commented at 10:52 PM on April 21, 2019:

    This gives the bad impression cs_main is necessary.

  8. promag commented at 10:53 PM on April 21, 2019: member

    Concept ACK.

  9. MarcoFalke force-pushed on Apr 22, 2019
  10. MarcoFalke commented at 1:08 PM on April 22, 2019: member

    Addressed feedback by @promag

  11. MarcoFalke force-pushed on May 1, 2019
  12. MarcoFalke force-pushed on May 1, 2019
  13. DrahtBot added the label Needs rebase on May 7, 2019
  14. MarcoFalke force-pushed on May 7, 2019
  15. practicalswift commented at 4:30 PM on May 7, 2019: contributor

    Concept ACK

  16. MarcoFalke force-pushed on May 7, 2019
  17. DrahtBot removed the label Needs rebase on May 7, 2019
  18. MarcoFalke force-pushed on May 10, 2019
  19. MarcoFalke force-pushed on May 10, 2019
  20. promag commented at 3:04 PM on May 13, 2019: member

    utACK face05a080c081fc426ff34ecb39ac072710034b.

  21. in src/wallet/test/wallet_tests.cpp:40 in face05a080 outdated 
      36 | @@ -37,14 +37,14 @@ static void AddKey(CWallet& wallet, const CKey& key)
  37 |  BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
  38 |  {
  39 |      auto chain = interfaces::MakeChain();
  40 | +    LockAnnotation lock(::cs_main);
    practicalswift commented at 4:47 PM on May 13, 2019:

    I might be missing something but shouldn't LockAnnotation be used only when we are holding the mutex but the compiler thread analysis doesn't understand it? In this case we aren't holding cs_main here, right?

    If it is used simply to opt-out of thread-safety analysis (when we are not holding the mutex) then NO_THREAD_SAFETY_ANALYSIS should be used, no?

    MarcoFalke commented at 4:51 PM on May 13, 2019:

    The tests are single threaded, so taking a lock is equivalent to pretending it is taken.

    Not sure how I can use NO_THREAD_SAFETY_ANALYSIS to achieve that.

    practicalswift commented at 5:04 PM on May 13, 2019:

    Yes, I know the tests are single-threaded but I think it might be confusing for newcomers if we use LockAnnotation to both a.) guarantee to the compiler that the lock is taken (like in src/interfaces/chain.cpp), and b.) to opt-out of thread safety analysis when the lock is not taken (like in this file).

    Just to make it crystal clear to readers what we are doing we could use different names for these two use-cases? (If using NO_THREAD_SAFETY_ANALYSIS is not an option.)

    Something like struct SCOPED_LOCKABLE DisableThreadSafetyAnalysis (like LockAnnotation) or #define DISABLE_THREAD_SAFETY_ANALYSIS(cs, code) [&] NO_THREAD_SAFETY_ANALYSIS { code; }() (like WITH_LOCK)?

    MarcoFalke commented at 5:33 PM on May 13, 2019:

    Added a comment instead. Is that fine?

  22. MarcoFalke force-pushed on May 13, 2019
  23. practicalswift commented at 6:30 PM on May 13, 2019: contributor

    @MarcoFalke The added comment makes it easier to understand for human reviewers. That is good, but I still think we would be better off if we used different annotations for the "opt-out of analysis" and "give the compiler a locking guarantee when it fails to understand our code" use cases:

    When analysing locks using static analysis etc I want to be able to let LockAnnotation imply the stronger "AssertLockHeld(…)" (instead of the weaker NO_THREAD_SAFETY_ANALYSIS).

    This change breaks that implication.

    Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time? The opt-out variant shouldn't have that (obviously) :-)

  24. [refactor] interfaces: Add missing LockAnnotation for cs_main fa3c651143
  25. MarcoFalke force-pushed on May 13, 2019
  26. MarcoFalke commented at 6:47 PM on May 13, 2019: member

    Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time

    Sounds good. I have addressed your concerns, but this suggestion should go into a follow-up pull request?

  27. ryanofsky approved
  28. ryanofsky commented at 8:15 PM on May 13, 2019: member

    utACK fa3c6511435149782545ac0d09d4722dc115d709

  29. practicalswift commented at 5:25 AM on May 14, 2019: contributor

    utACK fa3c6511435149782545ac0d09d4722dc115d709

  30. MarcoFalke merged this on May 14, 2019
  31. MarcoFalke closed this on May 14, 2019
  32. MarcoFalke referenced this in commit 40c66bb3d1 on May 14, 2019
  33. MarcoFalke deleted the branch on May 14, 2019
  34. deadalnix referenced this in commit 0ae0f30d6b on May 27, 2020
  35. kittywhiskers referenced this in commit a723fa65fb on Oct 21, 2021
  36. kittywhiskers referenced this in commit 02c1f4324e on Oct 21, 2021
  37. kittywhiskers referenced this in commit 004979b67e on Oct 21, 2021
  38. kittywhiskers referenced this in commit f5fbcdc2f7 on Oct 21, 2021
  39. kittywhiskers referenced this in commit 9d091af9db on Oct 21, 2021
  40. kittywhiskers referenced this in commit 07ee416e5e on Oct 24, 2021
  41. UdjinM6 referenced this in commit f1a2cdb680 on Oct 25, 2021
  42. PastaPastaPasta referenced this in commit 6c681a3685 on Oct 25, 2021
  43. PastaPastaPasta referenced this in commit 462e9b826c on Nov 1, 2021
  44. PastaPastaPasta referenced this in commit e7d6a8fb93 on Nov 1, 2021
  45. PastaPastaPasta referenced this in commit 38b2bfa29c on Nov 1, 2021
  46. PastaPastaPasta referenced this in commit 0bc782dfcb on Nov 3, 2021
  47. pravblockc referenced this in commit aaee20b769 on Nov 18, 2021
  48. pravblockc referenced this in commit ff1be08588 on Nov 18, 2021
  49. DrahtBot locked this on Dec 16, 2021
Contributors
MarcoFalkeDrahtBotpromagpracticalswiftryanofsky
Labels
Refactoring
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: 2026-04-17 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me