💬 ryanofsky commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422090129)
In commit "util: introduce general purpose thread pool" (3943630a4cea56b040377b33476803745055510c)

Would be nice to avoid shared_ptr here. Apparently it is needed because std::function objects are required to be copyable and packaged tasks aren't copyable. Could fix this by avoiding std::function which would also simplify the Submit() function:

<details><summary>diff</summary>
<p>

```diff
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -47,7 +47,7 @@ class ThreadPool {

...

👋 theuni's pull request is ready for review: "multiprocess: Fix high overhead from message logging"
(https://github.com/bitcoin/bitcoin/pull/33517)

💬 1440000bytes commented on issue "No way to clear command history in RPC console or reset the console without restarting the node":
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3392342670)
Related pull request: https://github.com/bitcoin-core/gui/pull/901

💬 ryanofsky commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422124854)
In commit "util: introduce general purpose thread pool" (3943630a4cea56b040377b33476803745055510c)

I'm a little unclear what this swap is trying to do. I would expect the point of swapping into a temporary vector would be to destroy the callables without holding m_mutex (since they could own resources and take time to destroy). But it looks like the `empty` vector is destroyed while `m_mutex` is still locked, so that isn't happening. Also I don't understand why `m_mutex` is locked twice in th
...

💬 mzumsande commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2421835077)
I don't understand the need for the sanity cleanup. Shouldn't the logic from `WorkerThread` guarantee that `m_work_queue` is empty at this point, so that we could just assert that here?

🤔 mzumsande reviewed a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3325595624)
Reviewed only the first two commits so far.

It could make sense to introduce the threadpool in an extra PR first, since it is very generic and can be used for multiple things, in case there are some reviewers who don't feel comfortable ACKing the pretty involved index changes, but would want to ACK the threadpool changes. (just a suggestion, not necessary just for my sake since I happen know the index code)

💬 mzumsande commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422052807)
Is this meant to be used by tests only, or are there possible other use cases? If it's the latter, it might be helpful to return a bool that indicates whether a task was executed or not (empty queue).

💬 mzumsande commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422034019)
nit: could add Test case 0) in the description

💬 mzumsande commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422078863)
`-indexworkers=0` is allowed according to the doc, but it will trigger the assert `num_workers > 0` in the `ThreadPool`. Shouldn't create a `ThreadPool` in this case.

💬 Crypt-iQ commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2422152798)
I completely forgot about this and realize it's too late to adjust.

💬 fanquake commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3392426742)
Guix Build (x86_64):
```bash
c0f071c9bd3236f8faf679f16e9abfb4fba11021135fa70725fb0658196ec707 guix-build-73bea6d39e41/output/aarch64-linux-gnu/SHA256SUMS.part
c09f085cfb26dac9db776fe20729007b3b346da7a136aaaa57c2cb245e0c43f0 guix-build-73bea6d39e41/output/aarch64-linux-gnu/bitcoin-73bea6d39e41-aarch64-linux-gnu-debug.tar.gz
31167a791279e8e68e7187d4782d6badfec67bb4290a34ec2b53bdbdcbf4fc68 guix-build-73bea6d39e41/output/aarch64-linux-gnu/bitcoin-73bea6d39e41-aarch64-linux-gnu.tar.gz
fe0fbc1
...

🤔 kevkevinpal reviewed a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3326017240)
Concept ACK

I think it makes sense to change to an `enum` instead of just a `bool`. I added a few comments but I have still yet to pull and build on my local machine

💬 kevkevinpal commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2422144497)
This isn't a method
```suggestion
* Enum to signal to broadcast a local transaction.
```

💬 kevkevinpal commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2422155953)
I don't think we need the `what` variable if it's just used as a string to print directly after the if block
```suggestion
switch (broadcast_method) {
case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL:
WalletLogPrintf("Submitting wtx %s to mempool and for broadcast to peers\n", wtx.GetHash().ToString());
break;
case node::ADD_TO_MEMPOOL_NO_BROADCAST:
WalletLogPrintf("Submitting wtx %s to mempool without broadcast\n", wtx.GetHash().ToString());
break;

...

💬 kevkevinpal commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2422147741)
Maybe I'm confused but why include this switch statement if previously we didnt need to? I don't see any if statement for `relay` here

💬 optout21 commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3392437781)
> * It looks to me like the `expected_code_path` assert is confirming that the `std::logic_error` exception throws at least once and that test coverage is complete.

I don't think it checks that exception is thrown at least once, but it checks that either there is no exception, or if there is, it is that specific exception. With this change there is an assert to make sure the exception is the one expected. So I think the test code change is correct, but I agree that the "that all codepaths res
...

💬 RandyMcMillan commented on pull request "rpcnestedtests.cpp:remove qtest-obsolete members":
(https://github.com/bitcoin-core/gui/pull/900#discussion_r2422197293)
note: https://github.com/bitcoin-core/gui/actions/runs/18416608653/job/52481652961

only failure - which suggests the tests aren't actually testing qt version <= 6.2

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2422282858)
I have already applied your previous suggestion and added you as coauthor.

💬 GaloisField2718 commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3392867370)
https://github.com/bitcoin/bitcoin/blob/v30.0/doc/release-notes.md

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3393048219)
> Should probably only be available if advanced coin control is enabled in settings, and maybe only on a menu even then (if it made sense to do it as a tab, it should be within the main window, not a new window).
>
>
>
> The SVG source for the new icon should also be included.

Thanks for the comments! I will check soon!