Skip to content

Persist cached SPKs on wallet creation#428

Open
j-kon wants to merge 1 commit intobitcoindevkit:masterfrom
j-kon:fix-387-persist-cached-spks
Open

Persist cached SPKs on wallet creation#428
j-kon wants to merge 1 commit intobitcoindevkit:masterfrom
j-kon:fix-387-persist-cached-spks

Conversation

@j-kon
Copy link
Copy Markdown

@j-kon j-kon commented Apr 1, 2026

Summary

  • flush the indexer's staged SPK cache during wallet creation so persisted wallets write the initial lookahead cache immediately
  • avoid emitting empty staged cache entries when reloading wallets with persisted SPK cache enabled
  • add a regression test proving a wallet created with use_spk_cache(true) does not need an initial reveal before the cache persists

Closes #387.

Testing

  • cargo test --all-features --test persisted_wallet
  • cargo test --all-features

cc @ValuedMammal, @oleonardolima

@ValuedMammal
Copy link
Copy Markdown
Collaborator

The main issue is with the spk_cache_stage AFAICT, because it alters the ChangeSet semantics in subtle and surprising ways. It's not clear to the developer why indexing a null txout would be necessary to "flush the stage".

The SPK cache already acts as a kind of stage. What we want is to pass a &mut ChangeSet directly to replenish_inner_index and then ideally return it from insert_descriptor (or similar). At the very least we can just stage the indexer initial_changeset when creating a Wallet. Then reveal_next_address will only persist 1 newly derived SPK at a time.

Second, make_indexed_graph appears to be doing double duty by constructing the IndexedTxGraph from a changeset during both the creation and loading phases. This is unnecessary since create_with_params just passes an empty changeset. Further, we already assert that the stage must be empty upon loading, so what is the intention of the &mut stage in make_indexed_graph 🤔

@j-kon j-kon force-pushed the fix-387-persist-cached-spks branch from 5f9c03f to dbad84a Compare April 3, 2026 17:49
@j-kon
Copy link
Copy Markdown
Author

j-kon commented Apr 3, 2026

Thanks, this makes sense.

I reworked the patch to follow that direction and pushed an update. Instead of forcing the staged SPK cache out via index_txout, wallet creation now stages index.initial_changeset() directly, so the initial lookahead cache is persisted explicitly at creation time.

I also split the creation and loading paths: creation now builds the index directly and stages its initial changeset, while loading reconstructs the IndexedTxGraph from the persisted changeset without producing new staged changes. That removes the &mut stage coupling from the load path and keeps the "stage is empty upon load" invariant intact.

The regression test is still covering the original issue: a wallet created with use_spk_cache(true) can be reloaded immediately, and the first reveal_next_address only stages the newly derived SPK rather than restaging the initial cache.

I reran cargo test --all-features --test persisted_wallet and cargo test --all-features after the refactor.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.06%. Comparing base (4e202c8) to head (9d78337).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/wallet/mod.rs 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   80.04%   80.06%   +0.02%     
==========================================
  Files          24       24              
  Lines        5336     5348      +12     
  Branches      242      242              
==========================================
+ Hits         4271     4282      +11     
- Misses        987      988       +1     
  Partials       78       78              
Flag Coverage Δ
rust 80.06% <97.67%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@j-kon j-kon force-pushed the fix-387-persist-cached-spks branch from dbad84a to 9d78337 Compare April 4, 2026 01:17
@j-kon j-kon marked this pull request as ready for review April 4, 2026 01:17
@j-kon j-kon marked this pull request as draft April 4, 2026 01:17
@ValuedMammal ValuedMammal added the bug Something isn't working label Apr 14, 2026
@ValuedMammal ValuedMammal added this to the Wallet 3.1.0 milestone Apr 14, 2026
@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Apr 14, 2026
@j-kon j-kon marked this pull request as ready for review April 15, 2026 01:21
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Wallet creation should persist cached SPKs

2 participants