Skip to content

Persist spks derived from KeychainTxOutIndex #1963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 27, 2025

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented May 22, 2025

Replaces #1960
Fixes #1964

Description

Users with large wallet and/or complex descriptors may experience slow startup of KeychainTxOutIndex. This PR addresses this problem by providing the option to persist derived spks so that they no longer need to be re-derived on startup.

The IndexedTxGraph API has been changed for better ergonomics.

Compared to #1960, this is a more longterm solution that does not depend on multi-threading logic.

Changelog notice

Changed
  - `KeychainTxOutIndex::new` to take in an additional parameter `persist_spks` to control whether derived spks are cached and persisted across restarts. The default of `persist_spks` is false.
  - `KeychainTxOutIndex` methods (`lookahead_to_target, `next_unused_spk`, `reveal_next_spk`) now return changesets as they may derive spks to be persisted.
  - The `InsertDescriptorError` type now wraps descriptors in `Box` to reduce enum size.

Added
  - `spk_cache` field to `indexer::keychain_txout::ChangeSet` which persists derived spks.
  - `IndexedTxGraph::from_changeset` - allows constructing from `indexed_tx_graph::ChangeSet` and only indexing when ready.
  - `IndexedTxGraph::reindex` method.

Fixed
  - `KeychainTxOutIndex::reveal_to_target` so that it actually returns `None` if the `keychain` does not exist.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@evanlinjin evanlinjin force-pushed the feature/spk_cache branch 4 times, most recently from 7afd8b6 to c279c99 Compare May 22, 2025 08:25
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from c279c99 to d299dae Compare May 22, 2025 08:41
@evanlinjin evanlinjin requested a review from ValuedMammal May 22, 2025 10:50
@evanlinjin evanlinjin added the api A breaking API change label May 22, 2025
@evanlinjin evanlinjin moved this to In Progress in BDK Chain May 22, 2025
@notmandatory notmandatory added this to the Wallet 2.0.0 milestone May 22, 2025
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from 90b6813 to 76875e7 Compare May 23, 2025 00:51
* When merging changesets, assert that spk of a given descriptor id &
  derivation index does not get changed.
* When reading spk from cache, check the spk by deriving it.
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from fb838d9 to d761265 Compare May 23, 2025 01:36
This incentivies constructing `KeychainTxOutIndex` from a changeset
before inserting descriptors (to make use of the spk cache).
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from 9d97b29 to 4bc8cc0 Compare May 23, 2025 08:35
Also added staging changes to `ChangeSet::spk_cache`. This way, we can
avoid returning `ChangeSet`s for `apply_changeset` and
`insert_descriptor`.

* `KeychainTxOutIndex::new` now takes in an additional `use_spk_cache`
  parameter.
* Fixed `reveal_to_target` method to actually return `None` if the
  keychain does not exist.
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from 4bc8cc0 to bbbc054 Compare May 23, 2025 08:36
* `new` is now intended to construct a fresh indexed-tx-graph
* `from_changeset` is added for constructing indexed-tx-graph from a
  previously persisted changeset
* added `reindex` for calling after indexer mutations that require it
* reintroduce `Default` impl
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from bbbc054 to 3126cd2 Compare May 23, 2025 10:34
@evanlinjin evanlinjin force-pushed the feature/spk_cache branch from 8fef7c7 to 603f133 Compare May 23, 2025 11:52
@evanlinjin evanlinjin marked this pull request as ready for review May 23, 2025 12:12
Do not reference last revealed table, in case none are revealed.
Correct SQL column name.
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 62767f0

@ValuedMammal ValuedMammal requested a review from LagginTimes May 26, 2025 15:50
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Chain May 26, 2025
@notmandatory
Copy link
Member

notmandatory commented May 27, 2025

Might have a problem with the sqlite store. I'm manually testing and looks like it's storing the same script for all indexes. I'll keep looking but wanted to give a headsup not to merge this yet.
Nevermind, it's working fine, my sqlite select just wan't showing the blog data.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 62767f0

tested with bdk-cli with cache enabled and sqlite schema updated and new table is being populated.

@ValuedMammal ValuedMammal merged commit e217542 into bitcoindevkit:master May 27, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain May 27, 2025
@ValuedMammal ValuedMammal mentioned this pull request May 27, 2025
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement IndexedTxGraph::reindex
3 participants