threading: remove ancient CRITICAL_SECTION macros #32592

pull theuni wants to merge 5 commits into bitcoin:master from theuni:remove-critsect changing 5 files +18 −41
  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.

    Edit: Rebased on top of #32465, so this should now pass tests.

  2. 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
    ACK TheCharlatan, maflcko, fjahr, furszy
    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

    No conflicts as of last run.

  3. DrahtBot added the label CI failed on May 22, 2025
  4. 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.

  5. theuni force-pushed on May 22, 2025
  6. 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.
  7. hebasto commented at 9:15 pm on May 22, 2025: member
    Concept ACK.
  8. fjahr commented at 11:09 am on May 26, 2025: contributor
    Concept ACK
  9. furszy commented at 1:56 pm on May 26, 2025: member
    Code review ACK ee98ad2e9d2be9a72c15c5c1ef93bfd3d5ed015c, will hop into #32465.
  10. DrahtBot requested review from hebasto on May 26, 2025
  11. DrahtBot requested review from fjahr on May 26, 2025
  12. 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.
  13. 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.
  14. 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.

  15. fanquake marked this as a draft on May 30, 2025
  16. DrahtBot added the label Needs rebase on Jun 17, 2025
  17. fanquake commented at 2:01 pm on July 1, 2025: member
    @theuni want to rebase this now that #32465 is in?
  18. fanquake added this to the milestone 30.0 on Jul 15, 2025
  19. fanquake commented at 2:58 pm on July 15, 2025: member
    Put this on 30.0.
  20. maflcko commented at 3:27 pm on August 7, 2025: member
    This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it’ll likely miss the milestone.
  21. fanquake commented at 1:35 pm on August 19, 2025: member

    This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it’ll likely miss the milestone.

    Feature freeze doesn’t seem important here, given this isn’t a feature; it’s part of a cleanup of which the prior PRs have already landed, so landing this any time before branch off seems fine (assuming it gets rebased, have pinged @theuni).

  22. tests: get rid of remaining manual critsect usage c88b1cbf57
  23. tests: Add Assertions in reverse_lock tests to exercise thread-safety annotations 3ddd554d31
  24. threading: use a reverse lock rather than manual critsect macros 
    No functional change.
    0d0e0a39b4
  25. threading: remove obsolete critsect macros b537a6a6db
  26. threading: remove unused template instantiations 
    These were only required for the ENTER_CRITICAL_SECTION macro.
    46ca7712cb
  27. theuni force-pushed on Aug 22, 2025
  28. theuni marked this as ready for review on Aug 22, 2025
  29. DrahtBot removed the label CI failed on Aug 22, 2025
  30. DrahtBot removed the label Needs rebase on Aug 22, 2025
  31. in src/rpc/mining.cpp:707 in 46ca7712cb 
    703@@ -704,7 +704,7 @@ static RPCHelpMan getblocktemplate()
704     NodeContext& node = EnsureAnyNodeContext(request.context);
705     ChainstateManager& chainman = EnsureChainman(node);
706     Mining& miner = EnsureMining(node);
707-    LOCK(cs_main);
708+    WAIT_LOCK(cs_main, csmain_lock);
    TheCharlatan commented at 9:22 pm on August 22, 2025:

    Not really related to this PR, but I really don’t like how long the scope of the lock is here. We could reduce this a bit by doing something like

     0diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
 1index b710c605bc..750ca72bb4 100644
 2--- a/src/rpc/mining.cpp
 3+++ b/src/rpc/mining.cpp
 4@@ -707,2 +706,0 @@ static RPCHelpMan getblocktemplate()
 5-    WAIT_LOCK(cs_main, csmain_lock);
 6-    uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
 7@@ -737,0 +736 @@ static RPCHelpMan getblocktemplate()
 8+            LOCK(cs_main);
 9@@ -775,0 +775,3 @@ static RPCHelpMan getblocktemplate()
10+    WAIT_LOCK(cs_main, cs_main_lock);
11+    uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
12+
13@@ -814 +816 @@ static RPCHelpMan getblocktemplate()
14-            REVERSE_LOCK(csmain_lock, cs_main);
15+            REVERSE_LOCK(cs_main_lock, cs_main);

    but then going on to hold it through the entire marshalling block also does not seem ideal.

  32. TheCharlatan approved
  33. TheCharlatan commented at 9:22 pm on August 22, 2025: contributor
    ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763
  34. DrahtBot requested review from furszy on Aug 22, 2025
  35. maflcko commented at 6:56 am on August 23, 2025: member

    review ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763 📌

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763 📌
3V2n3Eh8Fc5sBB4pkAgZwhO+niN3JRgn02KN6u3xaWJyYem/L+0JNJeiBY543bvMUZO6moB6cpFgOcoXMz3FDDA==
  36. fjahr commented at 9:05 am on August 23, 2025: contributor
    Code review ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763
  37. furszy commented at 10:02 am on August 23, 2025: member
    ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763
  38. fanquake merged this on Aug 23, 2025
  39. fanquake closed this on Aug 23, 2025


theuni DrahtBot hebasto fjahr furszy fanquake maflcko TheCharlatan


fjahr hebasto

Milestone
30.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: 2025-08-31 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me