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

💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468999171)
(force pushed for Alpine)