💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2423789917)
Had a few other cases. First 2 are confirmed to be the same issue - seeding randomness entropy from Windows performance data obtained through the Registry API during startup hangs when attempting to release a semaphore deep in Microsoft code. Turned off the once-per-hour CI schedule on my master-branch for now. More work to follow next week.

💬 davidgumberg commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2424219301)
> seeding randomness entropy from Windows performance data obtained through the Registry API during startup hangs when attempting to release a semaphore deep in Microsoft code.

The call to `RegQueryValueExA` in `RandAddSeedPerfmon`:

https://github.com/bitcoin/bitcoin/blob/e8f72aefd20049eac81b150e7f0d33709acd18ed/src/randomenv.cpp#L80-L91

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2446546723)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1

Based on `git range-diff 38e24df...0de3e96e333090548a43e5e870c4cb8941d6baf1` .

The cmake tidy-up looks good to me, and I examined newly built binaries for probe and semaphore info. I didn't re-run the testing from my previous ACK.

📝 theuni opened a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466)
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 `CountingSe
...

🤔 shahsb reviewed a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466#pullrequestreview-2831290143)
LGTM.. Thanks for making the changes!

💬 purpleKarrot commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2869668262)
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

👍 TheCharlatan approved a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466#pullrequestreview-2832753782)
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68

💬 theuni commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2884199007)
> 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 gran
...

💬 hebasto commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2895066428)
Concept ACK.

👍 hebasto approved a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466#pullrequestreview-2854910068)
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68, I have reviewed the code and it looks OK.

💬 hebasto commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#discussion_r2098433690)
nit: It doesn't appear to be used anywhere at the moment.

💬 theuni commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#discussion_r2098634952)
Correct. This is just for symmetry with [std::binary_semaphore](https://en.cppreference.com/w/cpp/thread/counting_semaphore)

💬 achow101 commented on pull request "threading: drop CSemaphore in favor of c++20 std::counting_semaphore":
(https://github.com/bitcoin/bitcoin/pull/32466#issuecomment-2895509210)
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68

🚀 achow101 merged a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466)

💬 theStack commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3246836456)
> Can you share the output of `readelf -n ./build/bin/bitcoind | grep outbound_message -A 5 -B 3`?

Sure:
```
$ readelf -n ./build/bin/bitcoind | grep outbound_message -A 5 -B 3
Arguments: -8@x21 8@x22 8@x23 4@x20 8@x0
stapsdt 0x00000057 NT_STAPSDT (SystemTap probe descriptors)
Provider: net
Name: outbound_message
Location: 0x00000000000cf3ac, Base: 0x00000000009ca828, Semaphore: 0x0000000000b19428
Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
st
...

💬 theStack commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249097001)
> > ```
> > Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
> > ```
>
> Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?

They are quite different on a `-fno-inline` build:

```
Name: outbound_message
Location: 0x000000000012ed28, Base: 0x000000000111c6d8, Semaphore: 0x00000000016594d0
Arguments: -8@x23 8@x24 8@x25 8@x26 8@x27 8@x0
```

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399079463)
I don't think we can do that. We need to set these here for other threads to read. These are only read from other threads, never written to. We also only read from other threads after the main thread has released the counting_semaphore, so we know the pointers are synced across the threads.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2429350877)
> I have tried std::jthread

I looked at this, but it doesn't really add anything to this implementation. We could have a `std::stop_token` for each thread, but we would have to `request_stop()` each `jthread` before releasing the semaphore in the destructor anyway. So it doesn't let us remove the destructor, and saves a line for not having to declare `m_request_stop`. I don't think it's worth it to use `jthread`s here.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3419748327)
Updated to use `std::barrier` for the completion synchronization instead of acquiring a semaphore for each thread, as suggested by @l0rinc .

🤔 l0rinc requested changes to a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3482342931)
After the tests, I have reviewed the `ThreadPool` as well now - first iteration, have a few questions, I expect a few more rounds of review.
I left some modernization suggestions and fine-grained commit requests to make the change easily reviewable.

I still share many of @andrewtoth's concerns regarding RAII vs Start/Stop/Interrupt and I think we can modernize the pool from `std::condition_variable` with locks to C++20 `std::counting_semaphore`. Besides my suggestions below I think we should
...