⚠️ fjahr opened an issue: "Script Validation Performance Improvement Tracking Issue"
(https://github.com/bitcoin/bitcoin/issues/32042)
This is a tracking issue for a couple of improvements targeting performance of script validation that are closely related. The topic was [discussed at CoreDev in February 2025](https://gist.github.com/fjahr/b020a16255f044ef62c48e9adf18881a) as well.
Benchmarking
- Pending: Benchmarking across different available threads ([using benchkit](https://github.com/bitcoin-dev-tools/benchkit))
- https://github.com/bitcoin/bitcoin/pull/31689
Schnorr Batch Validation
- https://github.com/bitcoin-core/sec
...
(https://github.com/bitcoin/bitcoin/issues/32042)
This is a tracking issue for a couple of improvements targeting performance of script validation that are closely related. The topic was [discussed at CoreDev in February 2025](https://gist.github.com/fjahr/b020a16255f044ef62c48e9adf18881a) as well.
Benchmarking
- Pending: Benchmarking across different available threads ([using benchkit](https://github.com/bitcoin-dev-tools/benchkit))
- https://github.com/bitcoin/bitcoin/pull/31689
Schnorr Batch Validation
- https://github.com/bitcoin-core/sec
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2718268595)
Anything left to do here?
The goal here is to add the contrib tool, not to fix all non-determinism in all tests. Especially, if they happen only on macOS, I can't really fix it myself anyway, since I don't have macOS. Ideally this is done in a follow-up.
There are three acks, two of which indicate to have tested the changes.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2718268595)
Anything left to do here?
The goal here is to add the contrib tool, not to fix all non-determinism in all tests. Especially, if they happen only on macOS, I can't really fix it myself anyway, since I don't have macOS. Ideally this is done in a follow-up.
There are three acks, two of which indicate to have tested the changes.
👍 hebasto approved a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2678966319)
re-ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8.
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2678966319)
re-ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8.
💬 hebasto commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2718326026)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2718326026)
Concept ACK.
💬 maflcko commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2718335838)
See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2718335838)
See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1991801639)
> Good idea for a follow up PR, a section on removing a feature will be nice.
@ismaelsadeeq I think you might want to take a look at this PR https://github.com/bitcoin/bitcoin/pull/30142
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1991801639)
> Good idea for a follow up PR, a section on removing a feature will be nice.
@ismaelsadeeq I think you might want to take a look at this PR https://github.com/bitcoin/bitcoin/pull/30142
👍 ryanofsky approved a pull request: "depends: patch around PlacementNew issue in capnp"
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2679035874)
Code review ACK 1ef22ce3351708bdd294d675f818880b7c93fffc. Confirmed patch is identical to one merged upstream. Only change since last review was tweaking the file paths and commit data in the patch.
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2679035874)
Code review ACK 1ef22ce3351708bdd294d675f818880b7c93fffc. Confirmed patch is identical to one merged upstream. Only change since last review was tweaking the file paths and commit data in the patch.
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2718373984)
Reading through commit a35a2a05887202de8c49727b6ea9166bf5f1a6f2 I am asking myself if with the addition of `m_target_blockhash`, the state of `m_validity` can just be computed on the fly without having to lug around and assert its state. For tracking the change in validity to `INVALID` once the tip reaches the target block the historical chainstate, could the presence of the `m_target_utxo_hash` (similar to how the belt-and-suspenders check is done later on) be checked instead? And for the curre
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2718373984)
Reading through commit a35a2a05887202de8c49727b6ea9166bf5f1a6f2 I am asking myself if with the addition of `m_target_blockhash`, the state of `m_validity` can just be computed on the fly without having to lug around and assert its state. For tracking the change in validity to `INVALID` once the tip reaches the target block the historical chainstate, could the presence of the `m_target_utxo_hash` (similar to how the belt-and-suspenders check is done later on) be checked instead? And for the curre
...
👍 TheCharlatan approved a pull request: "depends: patch around PlacementNew issue in capnp"
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2679065785)
ACK 1ef22ce3351708bdd294d675f818880b7c93fffc
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2679065785)
ACK 1ef22ce3351708bdd294d675f818880b7c93fffc
🤔 fjahr reviewed a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2679057659)
ACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9
Good addition!
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2679057659)
ACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9
Good addition!
💬 fjahr commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1991821751)
nit: Below doesn't talk about reviewing afaict
``` suggestion
A few guidelines for modifying existing RPC interfaces:
```
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1991821751)
nit: Below doesn't talk about reviewing afaict
``` suggestion
A few guidelines for modifying existing RPC interfaces:
```
🤔 polespinasa reviewed a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2679075922)
Concept ACK.
I would also add some indications on how to deprecate RPCs (https://github.com/bitcoin/bitcoin/issues/31980).
See https://github.com/bitcoin/bitcoin/pull/31278 as an example of a try to deprecate an RPC and the discussions held about the procedure.
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2679075922)
Concept ACK.
I would also add some indications on how to deprecate RPCs (https://github.com/bitcoin/bitcoin/issues/31980).
See https://github.com/bitcoin/bitcoin/pull/31278 as an example of a try to deprecate an RPC and the discussions held about the procedure.
📝 l0rinc opened a pull request: "[IBD] - Tracking PR for speeding up Initial Block Download "
(https://github.com/bitcoin/bitcoin/pull/32043)
During the last Core Dev meeting, it was proposed to create a tracking PR aggregating the individual IBD optimizations - to illustrate how these changes contribute to the broader performance improvement efforts.
## Summary: **18%** full IBD speedup
We don't have many low-hanging fruits anymore, but big speed improvements can also be achieved by many small, focused changes.
Many optimization opportunities are hiding in consensus critical code - this tracking PR provides justification for why
...
(https://github.com/bitcoin/bitcoin/pull/32043)
During the last Core Dev meeting, it was proposed to create a tracking PR aggregating the individual IBD optimizations - to illustrate how these changes contribute to the broader performance improvement efforts.
## Summary: **18%** full IBD speedup
We don't have many low-hanging fruits anymore, but big speed improvements can also be achieved by many small, focused changes.
Many optimization opportunities are hiding in consensus critical code - this tracking PR provides justification for why
...
💬 maflcko commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2718471034)
https://cirrus-ci.com/task/5838171738472448?logs=ci#L2741
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2718471034)
https://cirrus-ci.com/task/5838171738472448?logs=ci#L2741
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2718492650)
> Reading through commit [a35a2a0](https://github.com/bitcoin/bitcoin/commit/a35a2a05887202de8c49727b6ea9166bf5f1a6f2) I am asking myself if with the addition of `m_target_blockhash`, the state of `m_validity` can just be computed on the fly without having to lug around and assert its state.
I think it could probably work but two reasons not to do it might be:
(1) to avoid performance regressions in parts of the code that are assuming it is possible to determine whether a chainstate is val
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2718492650)
> Reading through commit [a35a2a0](https://github.com/bitcoin/bitcoin/commit/a35a2a05887202de8c49727b6ea9166bf5f1a6f2) I am asking myself if with the addition of `m_target_blockhash`, the state of `m_validity` can just be computed on the fly without having to lug around and assert its state.
I think it could probably work but two reasons not to do it might be:
(1) to avoid performance regressions in parts of the code that are assuming it is possible to determine whether a chainstate is val
...
📝 maflcko opened a pull request: "ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env""
(https://github.com/bitcoin/bitcoin/pull/32044)
Seems fine to revert this now. If this still happens it should be rare enough to be trivial to fix via a new push (normal push, force-push, rebase, ...), or to just ignore the failure.
(https://github.com/bitcoin/bitcoin/pull/32044)
Seems fine to revert this now. If this still happens it should be rare enough to be trivial to fix via a new push (normal push, force-push, rebase, ...), or to just ignore the failure.
💬 ryanofsky commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2718577789)
Thanks for creating this. This should make it easier to navigate the other PRs and discuss the overall topic of IBD performance and benchmarking without needing to necessarily repeat it in the individual PRs.
Would be useful to have concept ACKs/NACKs here from others who know more about performance and benchmarking. But from from what I can tell the individual optimizations do not seem very complicated and seem like they should be justified.
---
One suggestion for the PR description ab
...
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2718577789)
Thanks for creating this. This should make it easier to navigate the other PRs and discuss the overall topic of IBD performance and benchmarking without needing to necessarily repeat it in the individual PRs.
Would be useful to have concept ACKs/NACKs here from others who know more about performance and benchmarking. But from from what I can tell the individual optimizations do not seem very complicated and seem like they should be justified.
---
One suggestion for the PR description ab
...
🤔 musaHaruna reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2679357968)
Concept ACK [b1c80fa](https://github.com/bitcoin/bitcoin/commit/b1c80fab0c026a40f2901958abdeedb9dd36c30d)
New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2679357968)
Concept ACK [b1c80fa](https://github.com/bitcoin/bitcoin/commit/b1c80fab0c026a40f2901958abdeedb9dd36c30d)
New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.
💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992051054)
Seems a bit odd to skip rescan when all timestamps are negative. Previously, this would rescan the full chain.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992051054)
Seems a bit odd to skip rescan when all timestamps are negative. Previously, this would rescan the full chain.
⚠️ Sjors opened an issue: "Wallet migration includes rescan"
(https://github.com/bitcoin/bitcoin/issues/32045)
Users may want to migrate legacy wallets they haven't used in years. The migration itself is typically very quick, but it's followed by a rescan.
Depending on whether the user has `-blockfilterindex` enabled, this rescan takes somewhere between long and really really long.
There's no visual indication, only the logs show what's going on:
<img width="383" alt="Image" src="https://github.com/user-attachments/assets/da911b44-8052-4b90-aa16-67625e550e3e" />
Ideally the migration should finish an
...
(https://github.com/bitcoin/bitcoin/issues/32045)
Users may want to migrate legacy wallets they haven't used in years. The migration itself is typically very quick, but it's followed by a rescan.
Depending on whether the user has `-blockfilterindex` enabled, this rescan takes somewhere between long and really really long.
There's no visual indication, only the logs show what's going on:
<img width="383" alt="Image" src="https://github.com/user-attachments/assets/da911b44-8052-4b90-aa16-67625e550e3e" />
Ideally the migration should finish an
...