💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2409921699)
In "kernel: Add logging to kernel library C header" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
This is gone in next commit, but we can just have it use the `DestroyFunc` when it is introduced.
```suggestion
if (ptr) DestroyFunc(ptr);
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2409921699)
In "kernel: Add logging to kernel library C header" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
This is gone in next commit, but we can just have it use the `DestroyFunc` when it is introduced.
```suggestion
if (ptr) DestroyFunc(ptr);
```
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2409964043)
In kernel: Add logging to kernel library C header" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
I happen to see 4 loggers are connected 3 consecutively and one at the end, while in the code only three loggers were instantiated.
```
Running 5 test cases...
kernel: 2025-10-07T09:13:15.773635Z [unknown] [all:info] Using the 'arm_shani(1way;2way)' SHA256 implementation
kernel: 2025-10-07T09:13:15.876100Z [unknown] [kernel:debug] Logger connected.
kernel: 2025-10-07T09:13:15.876104Z [unknown
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2409964043)
In kernel: Add logging to kernel library C header" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
I happen to see 4 loggers are connected 3 consecutively and one at the end, while in the code only three loggers were instantiated.
```
Running 5 test cases...
kernel: 2025-10-07T09:13:15.773635Z [unknown] [all:info] Using the 'arm_shani(1way;2way)' SHA256 implementation
kernel: 2025-10-07T09:13:15.876100Z [unknown] [kernel:debug] Logger connected.
kernel: 2025-10-07T09:13:15.876104Z [unknown
...
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3381735920)
Nice work, Cool c++ as well :)
A couple of approach questions and some things I noted while reviewing
- Thread Safety: The library does not appear to be thread-safe. Concurrent instantiation of objects such as items, context, or logger can create duplicates. We should either add proper synchronization within the library or clearly document that users are responsible for ensuring thread safety. I'm leaning towards making it thread-safe since as a stateful library.
- C API Overhead: The c
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3381735920)
Nice work, Cool c++ as well :)
A couple of approach questions and some things I noted while reviewing
- Thread Safety: The library does not appear to be thread-safe. Concurrent instantiation of objects such as items, context, or logger can create duplicates. We should either add proper synchronization within the library or clearly document that users are responsible for ensuring thread safety. I'm leaning towards making it thread-safe since as a stateful library.
- C API Overhead: The c
...
💬 hodlinator commented on pull request "ci: Check macos-cross executables on macOS":
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3381737041)
Broadly agree with maflcko. Despite WSL (not sure of the status of that on CI), Windows is overall less similar to Linux than macOS is to Linux, so it is probably more likely to uncover issues.
I guess the downside of nightly is that figuring out which commit introduces a failure lands on build-oriented people. Until we learn CI to git bisect. So assuming that CI-cost is low, I can see how this PR may be preferable. Maybe there's a middle ground of only running upon merge into master?
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3381737041)
Broadly agree with maflcko. Despite WSL (not sure of the status of that on CI), Windows is overall less similar to Linux than macOS is to Linux, so it is probably more likely to uncover issues.
I guess the downside of nightly is that figuring out which commit introduces a failure lands on build-oriented people. Until we learn CI to git bisect. So assuming that CI-cost is low, I can see how this PR may be preferable. Maybe there's a middle ground of only running upon merge into master?
💬 Crypt-iQ commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3381753793)
crACK 0a757acf66d905566729ec4140159cbc68a95ac7
> I think this could be fixed by exchanging the order of [this](https://github.com/bitcoin/bitcoin/blob/af6cffa36d12d8b9f504024c080f1559373aba2a/src/net_processing.cpp#L1490-L1497) and [this](https://github.com/bitcoin/bitcoin/blob/af6cffa36d12d8b9f504024c080f1559373aba2a/src/net_processing.cpp#L1499-L1507) code block. I'm not sure if this is something that needs fixing though because the current patch should avoid this problem too...
Since th
...
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3381753793)
crACK 0a757acf66d905566729ec4140159cbc68a95ac7
> I think this could be fixed by exchanging the order of [this](https://github.com/bitcoin/bitcoin/blob/af6cffa36d12d8b9f504024c080f1559373aba2a/src/net_processing.cpp#L1490-L1497) and [this](https://github.com/bitcoin/bitcoin/blob/af6cffa36d12d8b9f504024c080f1559373aba2a/src/net_processing.cpp#L1499-L1507) code block. I'm not sure if this is something that needs fixing though because the current patch should avoid this problem too...
Since th
...
💬 Crypt-iQ commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2414015067)
I guess there's no way to quickly set pindexLastCommonBlock if it's outside of our active chain, so skipping ahead works only when it's inside the active chain (even if we had the blocks to do so)?
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2414015067)
I guess there's no way to quickly set pindexLastCommonBlock if it's outside of our active chain, so skipping ahead works only when it's inside the active chain (even if we had the blocks to do so)?
💬 mstorsjo commented on pull request "refactor: replace `const char*` exceptions with `std::runtime_error`":
(https://github.com/bitcoin/bitcoin/pull/33569#issuecomment-3381846204)
Also, FWIW, the issue isn't about throwing a `const char *`, it's about whether the pointer is caught by value or by reference, as `catch (const char*)` or `catch (const char* &)`. For larger types (like classes), I can see some performance benefit from catching by reference, but for a pointer sized value like this, there shouldn't be any performance difference. It's of course possible that the distinction isn't quite as easy and clear between those two as it was in the reduced testcase in https
...
(https://github.com/bitcoin/bitcoin/pull/33569#issuecomment-3381846204)
Also, FWIW, the issue isn't about throwing a `const char *`, it's about whether the pointer is caught by value or by reference, as `catch (const char*)` or `catch (const char* &)`. For larger types (like classes), I can see some performance benefit from catching by reference, but for a pointer sized value like this, there shouldn't be any performance difference. It's of course possible that the distinction isn't quite as easy and clear between those two as it was in the reduced testcase in https
...
📝 kevkevinpal opened a pull request: "doc: bump the template macOS version since 14 is now the minimum supported version"
(https://github.com/bitcoin/bitcoin/pull/33573)
Motivated by https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3361601497
The minimum version of MacOS for this repo is now 14 and above so it makes sense to update the issue template to reflect that.
(https://github.com/bitcoin/bitcoin/pull/33573)
Motivated by https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3361601497
The minimum version of MacOS for this repo is now 14 and above so it makes sense to update the issue template to reflect that.
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414186294)
> In the above scenario, the blockstorage.cpp logic on master would mark C as BLOCK_FAILED_CHILD, whereas on this PR it wouldn't be marked as BLOCK_FAILED_VALID
No, this doesn't change logic and both master and this PR would mark `C` as `BLOCK_FAILED_VALID`.
short answer: because we're iterating through block indices in increasing order of heights:
we'd first mark `B` as `BLOCK_FAILED_VALID` (from `BLOCK_FAILED_CHILD`)
`A -> B -> C` would then look like:
- `A` is marked `BLOCK_FAILED_
...
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2414186294)
> In the above scenario, the blockstorage.cpp logic on master would mark C as BLOCK_FAILED_CHILD, whereas on this PR it wouldn't be marked as BLOCK_FAILED_VALID
No, this doesn't change logic and both master and this PR would mark `C` as `BLOCK_FAILED_VALID`.
short answer: because we're iterating through block indices in increasing order of heights:
we'd first mark `B` as `BLOCK_FAILED_VALID` (from `BLOCK_FAILED_CHILD`)
`A -> B -> C` would then look like:
- `A` is marked `BLOCK_FAILED_
...
📝 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?