Skip to content

Conversation

@evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Aug 3, 2025

Description

We should ensure we populate the cache (from the changeset) before doing any other operation on KeychainTxOutIndex (i.e. inserting descriptors).

Otherwise we will end up re-deriving spks which have already been cached.

The test is also corrected to check that we don't duplicate deriving spks. A helper function check_cache_cs is added to make the tests more thorough and easier to read.

Changelog notice

Fixed:
* Recovering from spk-cache now works properly.

Checklists

All Submissions:

Bugfixes:

  • I've added tests to reproduce the issue which are now passing

* [ ] This pull request breaks the existing API
* [ ] I'm linking the issue being fixed by this PR

We should ensure we populate the cache (from the changeset) before doing
any other operation on `KeychainTxOutIndex` (i.e. inserting descriptors).

Otherwise we will end up deriving spks which have already been cached.

The test is also corrected to check that we don't duplicate deriving
spks. A helper function `check_cache_cs` is added to make the tests more
thorough and easier to read.
@evanlinjin evanlinjin self-assigned this Aug 3, 2025
@evanlinjin evanlinjin added the bug Something isn't working label Aug 3, 2025
@evanlinjin evanlinjin moved this to Needs Review in BDK Wallet Aug 3, 2025
@evanlinjin evanlinjin added this to the Wallet 2.1.0 milestone Aug 3, 2025
@evanlinjin evanlinjin requested review from ValuedMammal and notmandatory and removed request for ValuedMammal August 3, 2025 13:15
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 e2814e8

Thank you @evanlinjin

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK e2814e8

@ValuedMammal ValuedMammal merged commit 20edf98 into bitcoindevkit:master Aug 5, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Aug 5, 2025
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

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants