π fanquake opened a pull request: "[WIP] doc: update Guix install instructions"
(https://github.com/bitcoin/bitcoin/pull/33574)
Not sure what to do here yet, but it's somewhat annoying / concerning that Guix is falling out of being packaged by distros. For some more context, see https://lwn.net/Articles/1035491/.
> However, it is likely that the [Guix](https://guix.gnu.org/en/) package manager will soon be removed from the repositories for Debian 13 and Debian 12 ("bookworm", also called oldstable).
This seems to be happening. You can't `apt install guix` using the current release of Debian. https://packages.debian.o
...
(https://github.com/bitcoin/bitcoin/pull/33574)
Not sure what to do here yet, but it's somewhat annoying / concerning that Guix is falling out of being packaged by distros. For some more context, see https://lwn.net/Articles/1035491/.
> However, it is likely that the [Guix](https://guix.gnu.org/en/) package manager will soon be removed from the repositories for Debian 13 and Debian 12 ("bookworm", also called oldstable).
This seems to be happening. You can't `apt install guix` using the current release of Debian. https://packages.debian.o
...
π¬ janb84 commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414191393)
```suggestion
Linux Kernel 3.17+, macOS 14+, and Windows 10+. Bitcoin
```
Nit: Should this not be 14+ now ? Given https://github.com/bitcoin/bitcoin/pull/33489
(https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414191393)
```suggestion
Linux Kernel 3.17+, macOS 14+, and Windows 10+. Bitcoin
```
Nit: Should this not be 14+ now ? Given https://github.com/bitcoin/bitcoin/pull/33489
π¬ stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414193213)
> The current approach doesn't look robust to me, i.e. it won't catch all BLOCK_FAILED_CHILD flags. Suggested alternative that is more robust and (imo) more readable:
current approach catches all `BLOCK_FAILED_CHILD`. let's discuss it in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414186294.
> git diff on https://github.com/bitcoin/bitcoin/commit/2e040d8b3c861c96c536754a82f4352b635c4d01
thanks! would have used the diff if there was a problem with the current approach.
...
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414193213)
> The current approach doesn't look robust to me, i.e. it won't catch all BLOCK_FAILED_CHILD flags. Suggested alternative that is more robust and (imo) more readable:
current approach catches all `BLOCK_FAILED_CHILD`. let's discuss it in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414186294.
> git diff on https://github.com/bitcoin/bitcoin/commit/2e040d8b3c861c96c536754a82f4352b635c4d01
thanks! would have used the diff if there was a problem with the current approach.
...
π¬ fanquake commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987)
That change isn't in the 30.x branch.
(https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987)
That change isn't in the 30.x branch.
π¬ janb84 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-3382040905)
The v30 has still support for 13+. see https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`
Think it's still to early to change this if v30 has still full support for that version.
(https://github.com/bitcoin/bitcoin/pull/33573#issuecomment-3382040905)
The v30 has still support for 13+. see https://github.com/bitcoin/bitcoin/pull/33559#discussion_r2414194987`
Think it's still to early to change this if v30 has still full support for that version.
π¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414227236)
Nice thread of thoughts! Will try to go over the comments.
Regarding `Submit` being called only from the initial controller thread:
Right now, we are actually submitting tasks from threads that are not the initial one. Each index starts its own "background sync" thread that is responsible for computing the tasks submitted to the thread pool.
I donβt think itβs reasonable to expect a generic thread pool to only accept submissions from a single thread. We must be able to submit tasks from
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414227236)
Nice thread of thoughts! Will try to go over the comments.
Regarding `Submit` being called only from the initial controller thread:
Right now, we are actually submitting tasks from threads that are not the initial one. Each index starts its own "background sync" thread that is responsible for computing the tasks submitted to the thread pool.
I donβt think itβs reasonable to expect a generic thread pool to only accept submissions from a single thread. We must be able to submit tasks from
...
π¬ 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
...
(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
...
(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`.
(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
(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?
(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?
(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.
(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
(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.
(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!
(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.
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414359830)
removed it. I think it's obvious enough why it's not applicable.