Skip to content

Test v3 migration#1013

Open
thunderbiscuit wants to merge 3 commits into
bitcoindevkit:masterfrom
thunderbiscuit:test/v3-migration
Open

Test v3 migration#1013
thunderbiscuit wants to merge 3 commits into
bitcoindevkit:masterfrom
thunderbiscuit:test/v3-migration

Conversation

@thunderbiscuit
Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit commented May 27, 2026

This PR adds old databases (v0.32, v1, v2) to the Android assets, and tests the migration paths for each of those into a v3 wallet.

Notes

You will notice that there is a bit of redundancy/crossover between the new DatabaseVersionCompatTest and the current PersistenceTest. As is, the PersistenceTest is really using v2 databases, and so I didn't want to mess with this and combine the two ideas, even though eventually I think we should clean this up. The tests in PersistenceTest are not wrong, because we know (from the new tests in DatabaseVersionCompatTest) that the v2 to v3 conversion is clean and seamless, and so it's all good. The only think I'd like to fix is that they don't merge the ideas of testing (1) using a v2 db into a v3 wallet, and (2) loading from persistence itself (single and double descriptor wallets, as well as the v0.32 migration).

Checklists

Documentation

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing
  • I've added exactly one changelog:* label
  • I've linked the relevant upstream docs or specs above

Copy link
Copy Markdown
Collaborator

@reez reez left a comment

Choose a reason for hiding this comment

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

Thanks for adding these migration items and tests!

Your note about the overlap with PersistenceTest makes sense to me, I think keeping DatabaseVersionCompatTest separate is fine for sure.

I left one question on the v0.32 migration test path, plus a couple of non blocking nits.

Nothing else jumped out to me overall this looks good.

persister = newV3DB,
)

wallet.revealAddressesTo(KeychainKind.EXTERNAL, 6u)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this use the preV1Keychains values we just read instead of hardcoding 6u and 0u here? I think that would make the test cover the migration path more directly (read old metadata, apply indices to new v3 wallet, then persist/reload + check that next external/internal addresses are 7 and 1).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes absolutely, that's an oversight for sure (hardcoding the correct answer into my test 😆)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep this covers the hardcoded part. Should we also persist and then reload from newV3DB before checking the next addresses? I think that would make this prove the migrated v3 persistence state too instead of only the live wallet after calling revealAddressesTo

Comment thread bdk-android/lib/src/androidTest/assets/README.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants