💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2478495348)
Non-blocking I'd say, so could be a followup once it's figured out.

💬 ajtowns commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-3468660227)
> Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools (but is in a third location), this makes it clear that there's an interface at least.

Concept ACK! https://github.com/jamesob/verystable is something like this, arguably. It would also be nice to export it onto pypi (or whatever) for easy quick-and-dirty python/bitcoin scripting.

This PR lgtm, tested ACK f535b2fe63255175f7a35882599f9b6b83ac25f1

💬 fjahr commented on pull request "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)":
(https://github.com/bitcoin/bitcoin/pull/32924#issuecomment-3468674950)
tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777

Reviewed code and verified the behavior with the suggestions in the PR description.

💬 ajtowns commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-3468685164)
> Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools (but is in a third location), this makes it clear that there's an interface at least.

Big agree on this, eager to review if someone wants to do it. https://github.com/jamesob/verystable or https://pypi.org/project/verystable/ is somewhat like this, except external. It would also be nice to export it onto pypi (or whatever) for easy quick-and-dirty python
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2478627740)
I think outbound connections are getting disconnected for some reason, latest coverage is [here](https://crypt-iq.github.io/fuzz_coverage_reports/cmpctblock-aflpp-inputs-10292025/).

👋 fanquake's pull request is ready for review: "doc: update Guix INSTALL.md"
(https://github.com/bitcoin/bitcoin/pull/33574)

💬 ajtowns commented on pull request "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs":
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-3468708041)
Conflicts with #32116 ; let's get that merged first?

💬 fanquake commented on pull request "doc: update Guix INSTALL.md":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3468712881)
Updated the PR description, and added 1.5.0 release. Marked this as reviewable, as I think its fine drop the expectations section from 2021.

👍 stickies-v approved a pull request: "test: Use same rpc timeout for authproxy and cli"
(https://github.com/bitcoin/bitcoin/pull/33698#pullrequestreview-3400367122)
tACK 66667d6512294fd5dd02161b7c68c19af0865865

Rationale makes sense, tested old and new behaviour, code LGTM.

💬 fanquake commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468717752)
ACK fae9235d39a9976a725cfc6f32cfacb1bdfcf7a3 - did not test.

🤔 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?