👋 fanquake's pull request is ready for review: "doc: update Guix INSTALL.md"
(https://github.com/bitcoin/bitcoin/pull/33574)
(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?
(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.
(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.
(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.
(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
...
(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?
(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
(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
(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`
(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(); });
```
(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?
(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)
(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) {
```
(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?
(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.
(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)
(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" />
(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)
(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" />
(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" />