Skip to content
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

zcash_client_sqlite: Integrate "is relevant seed" logic into database migration #1283

Closed
str4d opened this issue Mar 18, 2024 · 0 comments · Fixed by #1286
Closed

zcash_client_sqlite: Integrate "is relevant seed" logic into database migration #1283

str4d opened this issue Mar 18, 2024 · 0 comments · Fixed by #1286
Labels
S-in-progress Status: Work is currently in progress on this item.

Comments

@str4d
Copy link
Contributor

str4d commented Mar 18, 2024

The mobile SDKs require the ability to check whether a given seed is relevant to a database, before using the seed with the database. This was initially added via a WalletRead::validate_seed method (that validates the seed against a specific account, requiring some non-trivial logic to convert this into a "relevant to any account" check). However, in the context that the mobile SDKs need it, this method has a circular dependency:

  • The caller wants to choose which seed to use (or enable its users to provide a seed), and confirming the seed is relevant requires querying the accounts table.
  • !uerying the accounts table requires that the table has been fully migrated.
  • Migrating the accounts table can require the seed, and currently does after the recent AccountId changes.

This gets more complicated once accounts derived from multiple seeds can coexist in the wallet DB, but we fortunately don't have to solve that problem yet because the migration API currently only takes a single seed, and the seed-requiring migrations return an error if the given seed doesn't match every derived account.

To solve this for the mobile SDKs (and single-seed users more generally), we will change the migration logic to always check that the seed is relevant to at lesat one account, instead of only doing so when a migration runs that requires the seed. The error returned from this new logic will be unified with the "wrong seed" errors returned from the seed-requiring migrations.

After these changes, a caller that has some database (e.g. from a backup) and a candidate seed can then do the following:

  • Attempt to migrate the database without any seed.
    • If the migration requires a seed, attempt to migrate the database with the candidate seed.
      • Caller gets an error if the seed is not relevant to the database.
      • If the migration succeeds, then the candidate seed was relevant to the database.
        • Right now, the caller "knows" that this is the only seed relevant to (the derived accounts in) the database. In future this won't be the case, but that's out of scope for this issue.
    • If the migration succeeds without any seed, call their respective isSeedRelevantToWallet logic (which internally calls WalletRead::validate_seed) to check the candidate seed (and any other candidates).
      • Given that I'm making changes, I will probably just replace WalletRead::validate_seed with WalletRead::is_seed_relevant_to_any_derived_accounts.

This isn't the prettiest UX for downstream users, but it's the simplest we can get working for Zashi 1.0 that doesn't require us to build and maintain self-introspecting queries that need to be valid for every possible accounts table migration state.

@str4d str4d added this to the Zashi 1.0 milestone Mar 18, 2024
@str4d str4d changed the title zcash_client_sqlite: Integrate "is relevant seed" logic into database migration zcash_client_sqlite: Integrate "is relevant seed" logic into database migration Mar 18, 2024
@str4d str4d added the S-in-progress Status: Work is currently in progress on this item. label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-in-progress Status: Work is currently in progress on this item.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant