threading: drop CSemaphore in favor of c++20 std::counting_semaphore #32466

1. 
   theuni commented at 1:00 am on May 10, 2025: member

   This is relatively simple, but done in a bunch of commits to enable scripted diffs.

   I wanted to add a semaphore in a branch I’ve been working on, but it was unclear if I should use std::counting_semaphore or stick with our old CSemaphore. I couldn’t decide, so I just decided to remove all doubt and get rid of ours :)

   This replaces our old CSemaphore with std::counting_semaphore everywhere we used it. CSemaphoreGrant is still there as an RAII wrapper, but is now called CountingSemaphoreGrant and BinarySemaphoreGrant to match. Those have been moved out of sync.h to their own file.

2. threading: rename CSemaphore methods to match std::semaphore 7b816c4e00
3. threading: add temporary semaphore aliases d870bc9451
4. scripted-diff: rename CSemaphoreGrant and CSemaphore for net

   -BEGIN VERIFY SCRIPT-
   sed -i -e 's|CSemaphoreGrant|SemaphoreGrant|g' -e 's|CSemaphore|Semaphore|g' src/net.h src/net.cpp
   -END VERIFY SCRIPT-

   6790ad27f1
5. wallet: change the write semaphore to a BinarySemaphore

   Follow-up commits will make a distinction between Semaphore and
   BinarySemaphore.

   793166d381
6. scripted-diff: rename CSemaphore and CSemaphoreGrant

   CountingSemaphore and CountingSemaphoreGrant model std::counting_semaphore.
   
   -BEGIN VERIFY SCRIPT-
   sed -i -e 's|CSemaphoreGrant|CountingSemaphoreGrant|g' -e 's|CSemaphore|CountingSemaphore|g' src/sync.h
   -END VERIFY SCRIPT-

   e6ce5f9e78
7. threading: make CountingSemaphore/CountingSemaphoreGrant template types 1acacfbad7
8. threading: replace CountingSemaphore with std::counting_semaphore f21365c4fc
9. scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones

   -BEGIN VERIFY SCRIPT-
   sed -i 's|BinarySemaphore|std::binary_semaphore|g' src/wallet/sqlite.h
   sed -i 's|SemaphoreGrant|CountingGrant|g' src/net.h src/net.cpp
   sed -i 's|Semaphore|std::counting_semaphore<>|g' src/net.h src/net.cpp
   sed -i 's|CountingGrant|CountingSemaphoreGrant<>|g' src/net.h src/net.cpp
   
   -END VERIFY SCRIPT-

   1f89e2a49a
10. threading: semaphore: remove temporary convenience types fd15469892
11. 
    DrahtBot commented at 1:00 am on May 10, 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/32466.

    Reviews

    See the guideline for information on the review process.

    Type	Reviewers
    ACK	purpleKarrot, TheCharlatan, hebasto, achow101

    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:

    • #32394 (net: make m_nodes_mutex non-recursive by vasild)
    • #32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
    • #32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
    • #30988 (Split CConnman by vasild)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #28584 (Fuzz: extend CConnman tests by vasild)

    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.

12. DrahtBot added the label CI failed on May 10, 2025
13. 
    DrahtBot commented at 2:45 am on May 10, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41976150929 LLM reason (✨ experimental): The CI failure is caused by a linting error related to a missing include guard in src/semaphore_grant.h.

    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.

14. threading: semaphore: move CountingSemaphoreGrant to its own header 6f7052a7b9
15. theuni force-pushed on May 10, 2025
16. DrahtBot removed the label CI failed on May 10, 2025
17. laanwj added the label Utils/log/libs on May 10, 2025
18. 
    shahsb commented at 7:36 am on May 11, 2025: none

    LGTM.. Thanks for making the changes!
19. 
    purpleKarrot commented at 10:08 am on May 11, 2025: contributor

    When looking at the new file alone, I see several oportunities for improvement: Drop the f prefix, use default member initializers, use std::exchange. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.

    The code would have been easier to review if the last commit was not part of this PR, but it looks good.

    ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68

20. TheCharlatan approved
21. 
    TheCharlatan commented at 10:40 am on May 12, 2025: contributor

    ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
22. 
    theuni commented at 3:14 pm on May 15, 2025: member

    When looking at the new file alone, I see several oportunities for improvement: Drop the f prefix, use default member initializers, use std::exchange. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.

    Indeed. I have some follow-up work that introduces a GrantableSemaphore and rewrites CountingSemaphoreGrant to be correct-by-construction (where a successfully created grant guarantees that a slot is held as it does not allow releasing or transfer). I considered closing this PR in favor of just moving straight to that, but I think it makes sense to modernize as a first step, then debate the semantics of those changes next.

23. 
    hebasto commented at 4:11 pm on May 20, 2025: member

    Concept ACK.
24. hebasto approved
25. 
    hebasto commented at 4:38 pm on May 20, 2025: member

    ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68, I have reviewed the code and it looks OK.
26. in src/semaphore_grant.h:91 in 6f7052a7b9

    86+    {
    87+        return fHaveGrant;
    88+    }
    89+};
    90+
    91+using BinarySemaphoreGrant = CountingSemaphoreGrant<1>;
    

    ---

    ---

    

    hebasto commented at 4:41 pm on May 20, 2025:

    nit: It doesn’t appear to be used anywhere at the moment.

    ---

    

    theuni commented at 6:37 pm on May 20, 2025:

    Correct. This is just for symmetry with std::binary_semaphore
27. 
    achow101 commented at 7:03 pm on May 20, 2025: member

    ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
28. achow101 merged this on May 20, 2025
29. achow101 closed this on May 20, 2025

    ---

Contributors
theuni DrahtBot shahsb purpleKarrot TheCharlatan hebasto achow101

Labels
Utils/log/libs