💬 fanquake commented on pull request "ci: re-add Valgrind job to the CI":
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433277209)
Happy to document that it is excluded because it is very slow compared to all other tests. Also happy to re-enable it, if we are fine to add ~13 minutes of runtime to this job, for a single slow test (it'd be about on par with ASAN at that point, so I guess still reasonable).
(https://github.com/bitcoin/bitcoin/pull/33411#discussion_r2433277209)
Happy to document that it is excluded because it is very slow compared to all other tests. Also happy to re-enable it, if we are fine to add ~13 minutes of runtime to this job, for a single slow test (it'd be about on par with ASAN at that point, so I guess still reasonable).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433141523)
Is there a reason we are checking for a null hash and return `nullptr` instead of an assertion? I see the c++ wrapper is throwing a runtime error in case of a `nullptr` return value for this (through the `Handle` constructor).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433141523)
Is there a reason we are checking for a null hash and return `nullptr` instead of an assertion? I see the c++ wrapper is throwing a runtime error in case of a `nullptr` return value for this (through the `Handle` constructor).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433069372)
Here "the chain" might suggest the active chain, but the function appears to work regardless - it simply returns the parent block tree entry via the `pprev` pointer, even if the block is no longer in the active chain (assuming block tree entries remain valid for the chainstate manager's lifetime).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433069372)
Here "the chain" might suggest the active chain, but the function appears to work regardless - it simply returns the parent block tree entry via the `pprev` pointer, even if the block is no longer in the active chain (assuming block tree entries remain valid for the chainstate manager's lifetime).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433243223)
Since `CBlockIndex` already has a calculated hash, can we return a non-owned `btck_BlockHash` here? (like `btck_transaction_get_txid()` for example)
A related question: why `GetBlockHash()` method in `CBlockIndex` returns a copied hash (unlike `GetHash()` in `CTransaction`)?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433243223)
Since `CBlockIndex` already has a calculated hash, can we return a non-owned `btck_BlockHash` here? (like `btck_transaction_get_txid()` for example)
A related question: why `GetBlockHash()` method in `CBlockIndex` returns a copied hash (unlike `GetHash()` in `CTransaction`)?
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433161387)
```suggestion
typedef void (*btck_ValidationInterfacePowValidBlock)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockConnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
```
nit: for consistency.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433161387)
```suggestion
typedef void (*btck_ValidationInterfacePowValidBlock)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockConnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry);
```
nit: for consistency.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433200559)
```suggestion
* @brief Returns the block height where the transaction that created this coin was included in the active chain.
```
nit
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433200559)
```suggestion
* @brief Returns the block height where the transaction that created this coin was included in the active chain.
```
nit
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433285220)
Good to mention about the lifetime of the returned pointer in the doc; also for `btck_transaction_out_point_get_txid()` (it seems we have been explicit about this for the rest of the returned const ptrs in the api).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433285220)
Good to mention about the lifetime of the returned pointer in the doc; also for `btck_transaction_out_point_get_txid()` (it seems we have been explicit about this for the rest of the returned const ptrs in the api).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433255059)
```suggestion
/**
* Opaque data structure for holding a transaction out point.
*
* Holds the txid and output index it is pointing to.
*/
typedef struct btck_TransactionOutPoint btck_TransactionOutPoint;
```
nit: not sure of this one, but i see we are using `txid` everywhere else.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2433255059)
```suggestion
/**
* Opaque data structure for holding a transaction out point.
*
* Holds the txid and output index it is pointing to.
*/
typedef struct btck_TransactionOutPoint btck_TransactionOutPoint;
```
nit: not sure of this one, but i see we are using `txid` everywhere else.
💬 glozow commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407451665)
Given the mini_miner_selection fuzz target is removed in #33629, it might be appropriate to close this? Thanks for working on this fwiw!
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407451665)
Given the mini_miner_selection fuzz target is removed in #33629, it might be appropriate to close this? Thanks for working on this fwiw!
✅ glozow closed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062)
(https://github.com/bitcoin/bitcoin/pull/33062)
💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3407457684)
There's not much activity on this PR and this refactors code that will be refactored again in #33629. I think it's best to close this. Please leave a comment if you think this is a mistake.
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3407457684)
There's not much activity on this PR and this refactors code that will be refactored again in #33629. I think it's best to close this. Please leave a comment if you think this is a mistake.
👍 hebasto approved a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3341520000)
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
My Guix build:
```
x86_64
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82 guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21
...
(https://github.com/bitcoin/bitcoin/pull/33625#pullrequestreview-3341520000)
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
My Guix build:
```
x86_64
22b8a342b9e47069d05d1fca40437ce2b19820e3f3c6b7ef17f2132a462500f9 guix-build-879c21045eba/output/aarch64-linux-gnu/SHA256SUMS.part
8f156c6e1cea62efb25007ab62ce02292eac014fe81f526249e4ff6724292a0c guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21045eba-aarch64-linux-gnu-debug.tar.gz
9afd57b78623c0ca553a6e8bf794baf8e31ee55b3dcbe9e265b02810cfaebb82 guix-build-879c21045eba/output/aarch64-linux-gnu/bitcoin-879c21
...
✅ glozow closed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803)
(https://github.com/bitcoin/bitcoin/pull/31803)
💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407517424)
> Given the mini_miner_selection fuzz target is removed in #33629 (see [c88360c](https://github.com/bitcoin/bitcoin/commit/c88360cd82b6ba59b345d6d573b791e4964930d2)), it might be appropriate to close this? Thanks for working on this fwiw!
Makes sense, thanks for the heads up.
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407517424)
> Given the mini_miner_selection fuzz target is removed in #33629 (see [c88360c](https://github.com/bitcoin/bitcoin/commit/c88360cd82b6ba59b345d6d573b791e4964930d2)), it might be appropriate to close this? Thanks for working on this fwiw!
Makes sense, thanks for the heads up.
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433436966)
> I'd like to echo the concern other reviewers raised about separating the thread pool from the script parallelization work. Since script parallelization is already quite complex, would you consider starting with a simpler 2-threaded implementation in this PR, and introducing the general-purpose thread pool in a separate PR?
Adding 2 threads or `n` threads represents the exact same work. The code complexity comes from the two-step procedure that allows sequential dumps after parallel block da
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433436966)
> I'd like to echo the concern other reviewers raised about separating the thread pool from the script parallelization work. Since script parallelization is already quite complex, would you consider starting with a simpler 2-threaded implementation in this PR, and introducing the general-purpose thread pool in a separate PR?
Adding 2 threads or `n` threads represents the exact same work. The code complexity comes from the two-step procedure that allows sequential dumps after parallel block da
...
🤔 glozow reviewed a pull request: "policy: don't CheckEphemeralSpends on reorg"
(https://github.com/bitcoin/bitcoin/pull/33616#pullrequestreview-3341571869)
This seems to make sense. I'd feel more confident if we add some more test cases for topologies and reorg scenarios that this does (not) apply to?
(https://github.com/bitcoin/bitcoin/pull/33616#pullrequestreview-3341571869)
This seems to make sense. I'd feel more confident if we add some more test cases for topologies and reorg scenarios that this does (not) apply to?
💬 glozow commented on pull request "policy: don't CheckEphemeralSpends on reorg":
(https://github.com/bitcoin/bitcoin/pull/33616#discussion_r2433433677)
I think it's impossible for `m_bypass_limits` to be true in this function, so undoing this diff doesn't make a difference. Could we omit it or `Assume(!args.m_bypass_limits)`?
(https://github.com/bitcoin/bitcoin/pull/33616#discussion_r2433433677)
I think it's impossible for `m_bypass_limits` to be true in this function, so undoing this diff doesn't make a difference. Could we omit it or `Assume(!args.m_bypass_limits)`?
💬 brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3407583434)
> Tested, and `--skipreorg` successfully reduces test runtime from `60s to 35s`.
>
> I’m not very sure about this approach — splitting was also suggested here https://github.com/bitcoin/bitcoin/issues/16613#issuecomment-548952103 I think it’s a better long-term solution, but could be done in a follow-up.
I agree on that for the long-term, I could address on a follow-up and keep this as a simple solution for now.
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3407583434)
> Tested, and `--skipreorg` successfully reduces test runtime from `60s to 35s`.
>
> I’m not very sure about this approach — splitting was also suggested here https://github.com/bitcoin/bitcoin/issues/16613#issuecomment-548952103 I think it’s a better long-term solution, but could be done in a follow-up.
I agree on that for the long-term, I could address on a follow-up and keep this as a simple solution for now.
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433482378)
> Such simple PRs
The thread pool, tests, and fuzz harness are 560 lines. More than half the additions to this PR. I don't think it's fair to say that would be a simple PR.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433482378)
> Such simple PRs
The thread pool, tests, and fuzz harness are 560 lines. More than half the additions to this PR. I don't think it's fair to say that would be a simple PR.
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2432963713)
This doesn't combine well with `invalidateblock`: e.g. when invalidating the tip, the log just gets spammed with warnings as we cycle through new peers:
```
2025-10-15T14:45:32Z UpdateTip: new best=000000092ca51c152010fd76552e12d9c9c7f410ebd04012dcff49eeecb6cfff height=274019 version=0x20000000 log2_work=42.943635 tx=27881630 date='2025-10-15T14:27:13Z' progress=1.000000 cache=71.8MiB(503926txo)
...
2025-10-15T14:45:50Z UpdateTip: new best=0000000b9e7f0760c48d76ef301616dbaa1421788b29911c02
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2432963713)
This doesn't combine well with `invalidateblock`: e.g. when invalidating the tip, the log just gets spammed with warnings as we cycle through new peers:
```
2025-10-15T14:45:32Z UpdateTip: new best=000000092ca51c152010fd76552e12d9c9c7f410ebd04012dcff49eeecb6cfff height=274019 version=0x20000000 log2_work=42.943635 tx=27881630 date='2025-10-15T14:27:13Z' progress=1.000000 cache=71.8MiB(503926txo)
...
2025-10-15T14:45:50Z UpdateTip: new best=0000000b9e7f0760c48d76ef301616dbaa1421788b29911c02
...