💬 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.
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414363696)
done.
(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).
(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.
(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
(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?
(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.
(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.
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414391241)
good point - I restricted the log to outbound peers, which makes sense anyway, because it's consistent with the fact that we only disconnect those.
(Besides, I'm curious what the current way to think about unconditional logging should be - be more liberal / care less about spamming? Or keep on being careful about log levels just as before when the rate limiting didn't exist? A bit offtopic here, but maybe something to discuss at CoreDev...)
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2414391241)
good point - I restricted the log to outbound peers, which makes sense anyway, because it's consistent with the fact that we only disconnect those.
(Besides, I'm curious what the current way to think about unconditional logging should be - be more liberal / care less about spamming? Or keep on being careful about log levels just as before when the rate limiting didn't exist? A bit offtopic here, but maybe something to discuss at CoreDev...)
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414395854)
Thanks for pointing this out, `GetStrongRandBytes` does not introduce non-determinism. I'll change the next time I push up by making `rand_path` accept a `bool strong` so that the original behavior when `-testdatadir` is set is unchanged.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2414395854)
Thanks for pointing this out, `GetStrongRandBytes` does not introduce non-determinism. I'll change the next time I push up by making `rand_path` accept a `bool strong` so that the original behavior when `-testdatadir` is set is unchanged.
💬 theuni commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956)
Agreed about avoiding crashing.
I pushed a change that does something different than what you suggested though: assume an unknown log level should only be seen at our most verbose level, which is Trace.
I think that's ok, as it should cause a compilation warning (missed switch value), but otherwise not much change for the user. As it would be an API break that requires downstream attention, I think that's reasonable.
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956)
Agreed about avoiding crashing.
I pushed a change that does something different than what you suggested though: assume an unknown log level should only be seen at our most verbose level, which is Trace.
I think that's ok, as it should cause a compilation warning (missed switch value), but otherwise not much change for the user. As it would be an API break that requires downstream attention, I think that's reasonable.
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414403520)
Right, m_interrupt should be initialized to true.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2414403520)
Right, m_interrupt should be initialized to true.
⚠️ ryanofsky opened an issue: "RFC: Cancelling waitNext calls in the IPC mining interface"
(https://github.com/bitcoin/bitcoin/issues/33575)
It seems that the IPC mining interface needs to provide some way to cancel [`BlockTemplate::waitNext`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/interfaces/mining.h#L63-L74) calls. This should not be hard to implement, but there are a few approaches that could be taken and it would be useful to know which would work best for callers. Context:
---
_Originally posted by @plebhash in [#33554](https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-
...
(https://github.com/bitcoin/bitcoin/issues/33575)
It seems that the IPC mining interface needs to provide some way to cancel [`BlockTemplate::waitNext`](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/interfaces/mining.h#L63-L74) calls. This should not be hard to implement, but there are a few approaches that could be taken and it would be useful to know which would work best for callers. Context:
---
_Originally posted by @plebhash in [#33554](https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-
...