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

   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.

Contributors
theuni DrahtBot hebasto

Labels
CI failed