threading: remove ancient CRITICAL_SECTION macros #32592

1. 
   theuni commented at 6:12 pm on May 22, 2025: member

   Now that #32467 is merged, the only remaining usage of our old CRITICAL_SECTION macros (other than tests) is in getblocktemplate() and it can safely be replaced with a REVERSE_LOCK.

   This PR makes that replacement, replaces the old CRITICAL_SECTION macro usage in tests, then deletes the macros themselves.

   While testing this a few weeks ago, I noticed that REVERSE_LOCK does not currently work properly with our thread-safety annotations as after the REVERSE_LOCK is acquired, clang still believes that the mutex is locked. #32465 fixes this problem. Without that fix, this PR would potentially allow a false-negative if code were added in the future to this chunk of getblocktemplate which required cs_main to be locked.

   I added a test for the reverse lock here in the form of a compiler warning in reverselock_tests.cpp to simulate that possibility. This PR will therefore cause a new warning (and should fail a warnings-as-errors ci check) until #32465 is merged and this is rebased on top of it.

2. mining: use a reverse lock rather than manual critsect macros

   No functional change.

   9276a75c74
3. tests: get rid of remaining manual critsect usage c77e8bad0c
4. 
   DrahtBot commented at 6:12 pm on May 22, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32592.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	hebasto, fjahr
   Stale ACK	furszy

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32465 (thread-safety: fix annotations with REVERSE_LOCK by theuni)

   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. threading: remove obsolete critsect macros f63cfe89c1
6. tests: Add Assertions in reverse_lock tests to exercise thread-safety annotations ee98ad2e9d
7. DrahtBot added the label CI failed on May 22, 2025
8. 
   DrahtBot commented at 7:04 pm on May 22, 2025: contributor

   🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/42732951890 LLM reason (✨ experimental): The CI failure is due to a thread safety error in reverselock_tests.cpp.

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

9. theuni force-pushed on May 22, 2025
10. 
    theuni commented at 7:22 pm on May 22, 2025: member

    As mentioned in the description, failing tests are just demonstrating the borked REVERSE_LOCK fixed by #32465.
11. 
    hebasto commented at 9:15 pm on May 22, 2025: member

    Concept ACK.
12. 
    fjahr commented at 11:09 am on May 26, 2025: contributor

    Concept ACK
13. 
    furszy commented at 1:56 pm on May 26, 2025: member

    Code review ACK ee98ad2e9d2be9a72c15c5c1ef93bfd3d5ed015c, will hop into #32465.
14. DrahtBot requested review from hebasto on May 26, 2025
15. DrahtBot requested review from fjahr on May 26, 2025
16. threading: remove unused template instantiations

    These were only required for the ENTER_CRITICAL_SECTION macro.

    2d40a45562
17. 
    theuni commented at 3:52 pm on May 28, 2025: member

    Pushed a small additional commit that removes a now-unnecessary template instantiation. Just a little janitorial work, but it will make some future changes more obviously correct.
18. 
    fanquake commented at 10:12 am on May 29, 2025: member

    https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223

    0[12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
    1[12:12:21.305]    23 |         AssertLockNotHeld(mutex);
    2[12:12:21.305]       |         ^
    3[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
    4[12:12:21.305]   141 | #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
    5[12:12:21.305]       |                               ^
    6[12:12:21.305] 1 error generated.
    

19. 
    theuni commented at 2:55 pm on May 29, 2025: member

    https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223

    0[12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
    1[12:12:21.305]    23 |         AssertLockNotHeld(mutex);
    2[12:12:21.305]       |         ^
    3[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
    4[12:12:21.305]   141 | #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
    5[12:12:21.305]       |                               ^
    6[12:12:21.305] 1 error generated.
    

    As expected :) From description:

    I added a test for the reverse lock here in the form of a compiler warning in reverselock_tests.cpp to simulate that possibility. This PR will therefore cause a new warning (and should fail a warnings-as-errors ci check) until #32465 is merged and this is rebased on top of it.

20. fanquake marked this as a draft on May 30, 2025
21. DrahtBot added the label Needs rebase on Jun 17, 2025
22. 
    DrahtBot commented at 6:45 pm on June 17, 2025: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.
23. 
    fanquake commented at 2:01 pm on July 1, 2025: member

    @theuni want to rebase this now that #32465 is in?
24. fanquake added this to the milestone 30.0 on Jul 15, 2025
25. 
    fanquake commented at 2:58 pm on July 15, 2025: member

    Put this on 30.0.

Contributors
theuni DrahtBot hebasto fjahr furszy fanquake

Review Requested
fjahr hebasto

Labels
Needs rebase CI failed

Milestone
30.0