💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406561682)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Is passing `spent_outputs_len` and verifying it is the same size with the tx output size necessary here? at first pass I see no harm in iterating through the tx outputs and appending them to the vector.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406561682)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Is passing `spent_outputs_len` and verifying it is the same size with the tx output size necessary here? at first pass I see no harm in iterating through the tx outputs and appending them to the vector.
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406655796)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Shouldn't the * operator return a reference and -> operator return a pointer, maybe I am missing something?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406655796)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Shouldn't the * operator return a reference and -> operator return a pointer, maybe I am missing something?
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406606561)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Should we have an opaque script type? then btc_scriptpubkey_create will return it, later if we want to represent scriptSig or witness script we dont have to define their own opaque type.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406606561)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Should we have an opaque script type? then btc_scriptpubkey_create will return it, later if we want to represent scriptSig or witness script we dont have to define their own opaque type.
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406574903)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
```
The flags very combined in an invalid way.
```
This comment is ambiguous what do you mean?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406574903)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
```
The flags very combined in an invalid way.
```
This comment is ambiguous what do you mean?
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406770176)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Why aren't returning *this by reference and making a copy here same in the move assignment operator?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406770176)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Why aren't returning *this by reference and making a copy here same in the move assignment operator?
💬 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
...