🤔 furszy reviewed a pull request: "doc: how to update a subtree"
(https://github.com/bitcoin/bitcoin/pull/33568#pullrequestreview-3314862355)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
(https://github.com/bitcoin/bitcoin/pull/33568#pullrequestreview-3314862355)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
💬 Eunovo commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413902914)
https://github.com/bitcoin/bitcoin/pull/26966/commits/0afb05cad29e4c30a2f9eb4295e997967129dc88:
Is it not beneficial for a generic ThreadPool to support assigning tasks in batches to a thread? I expect this to be useful for many small tasks.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413902914)
https://github.com/bitcoin/bitcoin/pull/26966/commits/0afb05cad29e4c30a2f9eb4295e997967129dc88:
Is it not beneficial for a generic ThreadPool to support assigning tasks in batches to a thread? I expect this to be useful for many small tasks.
💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3381673850)
I have a question related to the approach I started taking with the insights coming from the crashes reported here.
following your advice, I'm no longer using one single server thread for everything... now there's one "static" server thread that's responsible for all non-blocking calls (e.g.: fetching some template data or submitting solution to some template)
and for `waitNext`, there's dedicated server thread(s)
the thing is, I still need to cater for incoming Sv2 `CoinbaseOutputConstraints
...
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3381673850)
I have a question related to the approach I started taking with the insights coming from the crashes reported here.
following your advice, I'm no longer using one single server thread for everything... now there's one "static" server thread that's responsible for all non-blocking calls (e.g.: fetching some template data or submitting solution to some template)
and for `waitNext`, there's dedicated server thread(s)
the thing is, I still need to cater for incoming Sv2 `CoinbaseOutputConstraints
...
🤔 ismaelsadeeq reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3304903446)
Halfway through review, I will update this as I go on.
- [x] [kernel: Introduce initial kernel C header API](https://github.com/bitcoin/bitcoin/pull/30595/commits/166138374857eab7d4717471a8aacaa9d6ff2218)166138374857eab7d4717471a8aacaa9d6ff2218
- [x] [kernel: Add logging to kernel library C header](https://github.com/bitcoin/bitcoin/pull/30595/commits/2c686196e70465a5211e866c0ae73804015f1c4e)
- [x] [kernel: Add kernel library context object](https://github.com/bitcoin/bitcoin/pull/30595/commit
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3304903446)
Halfway through review, I will update this as I go on.
- [x] [kernel: Introduce initial kernel C header API](https://github.com/bitcoin/bitcoin/pull/30595/commits/166138374857eab7d4717471a8aacaa9d6ff2218)166138374857eab7d4717471a8aacaa9d6ff2218
- [x] [kernel: Add logging to kernel library C header](https://github.com/bitcoin/bitcoin/pull/30595/commits/2c686196e70465a5211e866c0ae73804015f1c4e)
- [x] [kernel: Add kernel library context object](https://github.com/bitcoin/bitcoin/pull/30595/commit
...
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406541898)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Why do we change amount to value here?
also there is inconsistency later in `btck_script_pubkey_verify` where we call it amount all through, might be better to just stick with value?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406541898)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Why do we change amount to value here?
also there is inconsistency later in `btck_script_pubkey_verify` where we call it amount all through, might be better to just stick with value?
💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406532719)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Will prevented double cast here by caching the vouts by reference first and then using in assert and getting the output be better?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2406532719)
In "kernel: Introduce initial kernel C header API" d192006d7f4e8f8ec7371f5921c7ea154c45f2d9
Will prevented double cast here by caching the vouts by reference first and then using in assert and getting the output be better?
💬 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_
...