💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414239068)
nice! I like the clearer variable names. used this diff in f284834 and added you as a coauthor.

1 difference with using `pindex` instead of `to_mark_failed` in [the last couple of lines](https://github.com/bitcoin/bitcoin/pull/32950/commits/f284834170d625b86de05d7b98d91eb8c39ab55e#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98L3777) - (ex: `InvalidChainFound(to_mark_failed);`) is that:
- `to_mark_failed` used to track the last disconnected block index
- `pindex` is t
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414243718)
> [0afb05c](https://github.com/bitcoin/bitcoin/commit/0afb05cad29e4c30a2f9eb4295e997967129dc88):
>
> Is it not beneficial for a generic ThreadPool to support assigning tasks in batches to threads? I expect this to be useful when submitting many small tasks.

I don't think that's thread pool responsibility. Generic thread pools are designed to execute tasks efficiently, but they don't dictate their granularity (mainly because they do not know the content of the task). If batching is benefici
...

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414248250)
Ok, but now we also need `std::vector<std::thread> m_workers;` guarded by `m_mutex`. Since it can be written by `Stop` and also read in `Submit`. So it also needs to be locked at the end of `Stop` and in `WorkersCount`.

💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#issuecomment-3382105823)
thanks @stickies-v! I've addressed your review comments.
- added a test in https://github.com/bitcoin/bitcoin/pull/32950/commits/cfadc8038c08f9804c81af7950164483761f1db5
- used the `{disconnected,new}_tip` variable tracking suggestions in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405494993

💬 hebasto commented on issue "build: Qt deprecated-declarations warnings":
(https://github.com/bitcoin/bitcoin/issues/33571#issuecomment-3382153671)
Out of curiosity, which distro or package manager ships Qt 6.10?

💬 janb84 commented on pull request "[WIP] doc: update Guix install instructions":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3382155241)
Is there a reason (you know) why the project does not mention / recommend to install guix by using the shell installer [as mentioned in the install doc of guix](https://guix.gnu.org/manual/en/html_node/Binary-Installation.html)? Wouldn't that be the answer to this problem?

💬 fanquake commented on pull request "[WIP] doc: update Guix install instructions":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3382162152)
> Is there a reason (you know) why the project does not mention / recommend to install guix by using the shell installer

We do: https://github.com/bitcoin/bitcoin/blob/master/contrib/guix/INSTALL.md#options-1-and-2-using-the-official-shell-installer-script-or-binary-tarball.

💬 kevkevinpal commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382168362)
> The v30 has still support for 13+. see [https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`](https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987%60) Think it's still to early to change this if v30 has still full support for that version.

I don't think this change will be part of v30, so it should still be good for review

💬 fanquake commented on issue "build: Qt deprecated-declarations warnings":
(https://github.com/bitcoin/bitcoin/issues/33571#issuecomment-3382205958)
Probably any rolling distro? `brew` will be once https://github.com/Homebrew/homebrew-core/pull/247354 is merged.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414334630)
> Ok, but now we also need `std::vector<std::thread> m_workers;` guarded by `m_mutex`. Since it can be written by `Stop` and also read in `Submit`. So it also needs to be locked at the end of `Stop` and in `WorkersCount`.

True. Pushed. Thanks!

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414350038)
Yeah, I found the test worthwhile. Taken. Thanks for sharing!
Allowing task submission from any thread should be a must for me. It otherwise defeats the purpose of having a shared thread pool if you can only submit tasks from a single place.

This also pushed me further. Added another test for the case where the pool is about to shut down while a thread is concurrently submitting a new task.

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414346916)
This comment is stale now.

💬 m3dwards commented on pull request "[WIP] doc: update Guix install instructions":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3382246481)
From reading the blog post it seems that the main issue is around how Guix chooses to manage releases making it hard for distro package maintainers to release backports. With a small userbase the effort doesn't seem worth it for them.

So perhaps the instructions should just focus on installing the guix binary from upstream.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414359830)
removed it. I think it's obvious enough why it's not applicable.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414363696)
done.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414369286)
Ups, updated. Thanks.
Also moved the condition variable comment above the member declaration (this caused a nasty bug that we shouldn't forget).

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414369729)
These 2 checks are redundant since they are both set atomically. Should just pick one for clarity.

💬 glozow commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382275989)
> The v30 has still support for 13+. see [https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`](https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987%60)
> Think it's still to early to change this if v30 has still full support for that version.

Note that this PR is against master, not 30.x

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414378684)
Actually, we must use worker count check. Since we can Start with 0 worker threads but still set interrupt to false, so this would deadlock with only the interrupt check.
Maybe guard against starting with 0 threads and then pick either?

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414383643)
> These 2 checks are redundant since they are both set atomically. Should just pick one for clarity.

Not really. What if the `ThreadPool` was just constructed but not started? Only using `m_interrupt` here would allow submission when there are no workers.