threading: remove ancient CRITICAL_SECTION macros #32592

theuni · 2025-05-22T18:12:56Z

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.

DrahtBot · 2025-05-22T18:12:59Z

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.

DrahtBot · 2025-05-22T19:04:08Z

🚧 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.}

Hints

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.

theuni · 2025-05-22T19:22:09Z

As mentioned in the description, failing tests are just demonstrating the borked REVERSE_LOCK fixed by #32465.

hebasto · 2025-05-22T21:15:18Z

Concept ACK.

fjahr · 2025-05-26T11:09:15Z

Concept ACK

furszy

Code review ACK ee98ad2, will hop into #32465.

theuni · 2025-05-28T15:52:49Z

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.

fanquake · 2025-05-29T10:12:37Z

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

[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]
[12:12:21.305]    23 |         AssertLockNotHeld(mutex);
[12:12:21.305]       |         ^
[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
[12:12:21.305]   141 | #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
[12:12:21.305]       |                               ^
[12:12:21.305] 1 error generated.

theuni · 2025-05-29T14:55:50Z

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

[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]
[12:12:21.305]    23 |         AssertLockNotHeld(mutex);
[12:12:21.305]       |         ^
[12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
[12:12:21.305]   141 | #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
[12:12:21.305]       |                               ^
[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.

fanquake · 2025-07-01T14:01:14Z

@theuni want to rebase this now that #32465 is in?

fanquake · 2025-07-15T14:58:09Z

Put this on 30.0.

maflcko · 2025-08-07T15:27:31Z

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.

fanquake · 2025-08-19T13:35:28Z

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).

… annotations

No functional change.

These were only required for the ENTER_CRITICAL_SECTION macro.

TheCharlatan

ACK 46ca771

TheCharlatan · 2025-08-22T21:22:00Z

src/rpc/mining.cpp

@@ -704,7 +704,7 @@ static RPCHelpMan getblocktemplate()
    NodeContext& node = EnsureAnyNodeContext(request.context);
    ChainstateManager& chainman = EnsureChainman(node);
    Mining& miner = EnsureMining(node);
-    LOCK(cs_main);
+    WAIT_LOCK(cs_main, csmain_lock);


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

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index b710c605bc..750ca72bb4 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -707,2 +706,0 @@ static RPCHelpMan getblocktemplate() - WAIT_LOCK(cs_main, csmain_lock); - uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash}; @@ -737,0 +736 @@ static RPCHelpMan getblocktemplate() + LOCK(cs_main); @@ -775,0 +775,3 @@ static RPCHelpMan getblocktemplate() + WAIT_LOCK(cs_main, cs_main_lock); + uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash}; + @@ -814 +816 @@ static RPCHelpMan getblocktemplate() - REVERSE_LOCK(csmain_lock, cs_main); + REVERSE_LOCK(cs_main_lock, cs_main);

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

maflcko · 2025-08-23T06:56:45Z

review ACK 46ca771 📌

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763 📌
V2n3Eh8Fc5sBB4pkAgZwhO+niN3JRgn02KN6u3xaWJyYem/L+0JNJeiBY543bvMUZO6moB6cpFgOcoXMz3FDDA==

fjahr · 2025-08-23T09:05:00Z

Code review ACK 46ca771

furszy

ACK 46ca771

DrahtBot added the CI failed label May 22, 2025

theuni force-pushed the remove-critsect branch from 3503d51 to ee98ad2 Compare May 22, 2025 19:20

DrahtBot mentioned this pull request May 23, 2025

thread-safety: fix annotations with REVERSE_LOCK #32465

Merged

furszy reviewed May 26, 2025

View reviewed changes

DrahtBot requested review from hebasto and fjahr May 26, 2025 13:56

fanquake marked this pull request as draft May 30, 2025 09:40

DrahtBot added the Needs rebase label Jun 17, 2025

fanquake added this to the 30.0 milestone Jul 15, 2025

theuni added 5 commits August 22, 2025 14:25

tests: get rid of remaining manual critsect usage

c88b1cb

tests: Add Assertions in reverse_lock tests to exercise thread-safety…

3ddd554

… annotations

threading: use a reverse lock rather than manual critsect macros

0d0e0a3

No functional change.

threading: remove obsolete critsect macros

b537a6a

threading: remove unused template instantiations

46ca771

These were only required for the ENTER_CRITICAL_SECTION macro.

theuni force-pushed the remove-critsect branch from 2d40a45 to 46ca771 Compare August 22, 2025 14:29

theuni marked this pull request as ready for review August 22, 2025 14:33

DrahtBot removed CI failed Needs rebase labels Aug 22, 2025

TheCharlatan approved these changes Aug 22, 2025

View reviewed changes

DrahtBot requested a review from furszy August 22, 2025 21:22

furszy reviewed Aug 23, 2025

View reviewed changes

fanquake merged commit 9703b7e into bitcoin:master Aug 23, 2025
19 checks passed

threading: remove ancient CRITICAL_SECTION macros #32592

threading: remove ancient CRITICAL_SECTION macros #32592

Conversation

theuni commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

theuni commented May 22, 2025

Uh oh!

hebasto commented May 22, 2025

Uh oh!

fjahr commented May 26, 2025

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 28, 2025

Uh oh!

fanquake commented May 29, 2025

Uh oh!

theuni commented May 29, 2025

Uh oh!

fanquake commented Jul 1, 2025

Uh oh!

fanquake commented Jul 15, 2025

Uh oh!

maflcko commented Aug 7, 2025

Uh oh!

fanquake commented Aug 19, 2025

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 23, 2025

Uh oh!

fjahr commented Aug 23, 2025

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theuni commented May 22, 2025 •

edited

Loading

DrahtBot commented May 22, 2025 •

edited

Loading

furszy left a comment •

edited

Loading