🤔 l0rinc requested changes to a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3377790750)
I have started reviewing this PR but have only finished the first commit (e1eb4cd3a5eb192cd6d9ee5d255688c06ab2089a).

The depth of tests gives me hope that this transition will be smooth, the tests cover most of the functionality, even a few corner cases I haven't thought of!

It seemed, however, that I had more to say about that than anticipated. I didn't even get to reviewing the threadpool properly, just most of the tests.

I know receiving this amount of feedback can be daunting, but we both
...

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2461223410)
Can we investigate that?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477415901)
I'm not sure this is necessarry

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477334363)
nit: we don't need to include the type here:
```suggestion
constexpr auto TIMEOUT{std::chrono::seconds(120)};
```
https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423291668

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477426689)
Doesn't this prevent inline string literals? Often copy is cheaper than reference, here I'd expect that to be the case and we couldn't need to use constants when constructing the `ThreadPool`

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477395665)
nit: we could use `strprintf` in a few more places:
```suggestion
m_workers.emplace_back(&util::TraceThread, strprintf("%s_pool_%d", m_name, i), [this] { WorkerThread(); });
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477415169)
What's the reason for locking an notifying manually instead of using higher-level primitives - have you tried these with C++20 https://en.cppreference.com/w/cpp/thread/barrier.html instead?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477459325)
the comment mirrors the code, it doesn't really add anything that the code doesn't already say (especially since the comment above already stated the same)

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477491505)
nit: for consistency
```suggestion
for (size_t i{1}; i <= num_tasks; ++i) {
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477455551)
do we really need this extra complexity here?

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478779605)
These two are used in consensus code as well, that's why I mentioned them. And they're part of some template metaprogramming magic which should likely be aligned here. Will leave it up to you of course.

🚀 fanquake merged a pull request: "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)"
(https://github.com/bitcoin/bitcoin/pull/32924)

💬 00w1 commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3468946970)
<img width="948" height="289" alt="Image" src="https://github.com/user-attachments/assets/080212eb-ada9-496e-9c85-d87a9306e2e5" />

🚀 fanquake merged a pull request: "test: Use same rpc timeout for authproxy and cli"
(https://github.com/bitcoin/bitcoin/pull/33698)

💬 dergoegge commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3468958855)
> floppy profile pic in screenshot

<img width="272" height="186" alt="Image" src="https://github.com/user-attachments/assets/88349a98-b1b5-432d-a2af-9a21cb5372ce" />

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478798725)
Sure, but the smallest change here that would fix the bug would be to upcast the `4` multiplier to 64 bits as mentioned in https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2474414992.
I don't mind doing some combination of partial migration and not just the smallest possible diff, but I mentioned the outliers, will let you decide which ones are relevant and which ones to do separately. I don't particularly care about the order.

💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3468964102)
> > the second peer can endlessly send CMPCTBLOCK's
>
> If we're concerned about that, seems better to fix it directly?
>
> ```diff
> --- a/src/net_processing.cpp
> +++ b/src/net_processing.cpp
> @@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
> while (range_flight.first != range_flight.second) {
> if (range_flight.first->second.first == pfrom.GetId()) {
> requested_block_from_this_peer = tr
...

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478801132)
https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390 explains it well, I can get behind both as long as we're consistent

💬 00w1 commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3468967896)
> > floppy profile pic in screenshot
>
> <img alt="Image" width="272" height="186" src="https://private-user-images.githubusercontent.com/8077169/507760105-88349a98-b1b5-432d-a2af-9a21cb5372ce.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjE4NDI1NDQsIm5iZiI6MTc2MTg0MjI0NCwicGF0aCI6Ii84MDc3MTY5LzUwNzc2MDEwNS04ODM0OWE5OC1iMWI1LTQzMmQtYTJhZi05YTIxY2I1MzcyY2UucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU
...

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2478816900)
> These two are used in consensus code as well,

No, they are not. Only one of them is used in psbt, which is not consensus.

One way to check if a function is used somewhere is to `=delete` it and compile.

The following diff compiles:

```diff
diff --git a/src/psbt.h b/src/psbt.h
index f0de079f7b..ec235ed2fc 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -212,7 +212,7 @@ void DeserializeMuSig2ParticipantPubkeys(Stream& s, SpanReader& skey, std::map<C
std::vector<unsigned char>
...