diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d4fc361df7..1b82c0bb93 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -59,6 +59,8 @@ and this library adheres to Rust's notion of - Arguments to `ScannedBlock::from_parts` have changed. - Changes to the `WalletRead` trait: - Added `Account` associated type. + - Added `validate_seed` method. + - Added `is_seed_relevant_to_any_derived_accounts` method. - Added `get_account` method. - Added `get_derived_account` method. - `get_account_for_ufvk` now returns `Self::Account` instead of a bare @@ -80,7 +82,6 @@ and this library adheres to Rust's notion of - `type OrchardShardStore` - `fn with_orchard_tree_mut` - `fn put_orchard_subtree_roots` - - Added method `WalletRead::validate_seed` - Removed `Error::AccountNotFound` variant. - `WalletSummary::new` now takes an additional `next_orchard_subtree_index` argument when the `orchard` feature flag is enabled. diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 901a368439..c27d9b1371 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -75,7 +75,9 @@ use self::{ use crate::{ address::UnifiedAddress, decrypt::DecryptedOutput, - keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey}, + keys::{ + UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedIncomingViewingKey, UnifiedSpendingKey, + }, proto::service::TreeState, wallet::{Note, NoteId, ReceivedNote, Recipient, WalletTransparentOutput, WalletTx}, ShieldedProtocol, @@ -342,8 +344,16 @@ pub trait Account { /// Accounts for which this returns `None` cannot be used in wallet contexts, because /// they are unable to maintain an accurate balance. fn ufvk(&self) -> Option<&UnifiedFullViewingKey>; + + /// Returns the UIVK that the wallet backend has stored for the account. + /// + /// All accounts are required to have at least an incoming viewing key. This gives no + /// indication about whether an account can be used in a wallet context; for that, use + /// [`Account::ufvk`]. + fn uivk(&self) -> UnifiedIncomingViewingKey; } +#[cfg(any(test, feature = "test-dependencies"))] impl Account for (A, UnifiedFullViewingKey) { fn id(&self) -> A { self.0 @@ -356,9 +366,14 @@ impl Account for (A, UnifiedFullViewingKey) { fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { Some(&self.1) } + + fn uivk(&self) -> UnifiedIncomingViewingKey { + self.1.to_unified_incoming_viewing_key() + } } -impl Account for (A, Option) { +#[cfg(any(test, feature = "test-dependencies"))] +impl Account for (A, UnifiedIncomingViewingKey) { fn id(&self) -> A { self.0 } @@ -368,7 +383,11 @@ impl Account for (A, Option) { } fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { - self.1.as_ref() + None + } + + fn uivk(&self) -> UnifiedIncomingViewingKey { + self.1.clone() } } @@ -721,6 +740,13 @@ pub trait WalletRead { seed: &SecretVec, ) -> Result; + /// Checks whether the given seed is relevant to any of the derived accounts (where + /// [`Account::source`] is [`AccountSource::Derived`]) in the wallet. + fn is_seed_relevant_to_any_derived_accounts( + &self, + seed: &SecretVec, + ) -> Result; + /// Returns the account corresponding to a given [`UnifiedFullViewingKey`], if any. fn get_account_for_ufvk( &self, @@ -1700,6 +1726,13 @@ pub mod testing { Ok(false) } + fn is_seed_relevant_to_any_derived_accounts( + &self, + _seed: &SecretVec, + ) -> Result { + Ok(false) + } + fn get_account_for_ufvk( &self, _ufvk: &UnifiedFullViewingKey, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index d4242f89de..1fadd340f4 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -32,10 +32,12 @@ and this library adheres to Rust's notion of - Added `UnknownZip32Derivation` - Added `BadAccountData` - Removed `DiversifierIndexOutOfRange` + - Removed `InvalidNoteId` - `zcash_client_sqlite::wallet`: - `init::WalletMigrationError` has added variants: - `WalletMigrationError::AddressGeneration` - `WalletMigrationError::CannotRevert` + - `WalletMigrationError::SeedNotRelevant` - The `v_transactions` and `v_tx_outputs` views now include Orchard notes. ## [0.9.1] - 2024-03-09 diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 1271e9ca70..6796c41421 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -30,10 +30,6 @@ pub enum SqliteClientError { /// The rcm value for a note cannot be decoded to a valid JubJub point. InvalidNote, - /// The note id associated with a witness being stored corresponds to a - /// sent note, not a received note. - InvalidNoteId, - /// Illegal attempt to reinitialize an already-initialized wallet database. TableNotEmpty, @@ -138,8 +134,6 @@ impl fmt::Display for SqliteClientError { } SqliteClientError::Protobuf(e) => write!(f, "Failed to parse protobuf-encoded record: {}", e), SqliteClientError::InvalidNote => write!(f, "Invalid note"), - SqliteClientError::InvalidNoteId => - write!(f, "The note ID associated with an inserted witness must correspond to a received note."), SqliteClientError::RequestedRewindInvalid(h, r) => write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_DEPTH, h, r), SqliteClientError::Bech32DecodeError(e) => write!(f, "{}", e), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 469299c175..6b8fd29822 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -321,38 +321,13 @@ impl, P: consensus::Parameters> WalletRead for W account_index, } = account.source() { - let seed_fingerprint_match = - SeedFingerprint::from_seed(seed.expose_secret()).ok_or_else(|| { - SqliteClientError::BadAccountData( - "Seed must be between 32 and 252 bytes in length.".to_owned(), - ) - })? == seed_fingerprint; - - let usk = UnifiedSpendingKey::from_seed( + wallet::seed_matches_derived_account( &self.params, - &seed.expose_secret()[..], + seed, + &seed_fingerprint, account_index, + &account.uivk(), ) - .map_err(|_| SqliteClientError::KeyDerivationError(account_index))?; - - // Keys are not comparable with `Eq`, but addresses are, so we derive what should - // be equivalent addresses for each key and use those to check for key equality. - let ufvk_match = UnifiedAddressRequest::all().map_or( - Ok::<_, Self::Error>(false), - |ua_request| { - Ok(usk - .to_unified_full_viewing_key() - .default_address(ua_request)? - == account.default_address(ua_request)?) - }, - )?; - - if seed_fingerprint_match != ufvk_match { - // If these mismatch, it suggests database corruption. - return Err(SqliteClientError::CorruptedData(format!("Seed fingerprint match: {seed_fingerprint_match}, ufvk match: {ufvk_match}"))); - } - - Ok(seed_fingerprint_match && ufvk_match) } else { Err(SqliteClientError::UnknownZip32Derivation) } @@ -362,6 +337,39 @@ impl, P: consensus::Parameters> WalletRead for W } } + fn is_seed_relevant_to_any_derived_accounts( + &self, + seed: &SecretVec, + ) -> Result { + for account_id in self.get_account_ids()? { + let account = self.get_account(account_id)?.expect("account ID exists"); + + // If the account is imported, the seed _might_ be relevant, but the only + // way we could determine that is by brute-forcing the ZIP 32 account + // index space, which we're not going to do. The method name indicates to + // the caller that we only check derived accounts. + if let AccountSource::Derived { + seed_fingerprint, + account_index, + } = account.source() + { + if wallet::seed_matches_derived_account( + &self.params, + seed, + &seed_fingerprint, + account_index, + &account.uivk(), + )? { + // The seed is relevant to this account. No need to check any others. + return Ok(true); + } + } + } + + // The seed was not relevant to any of the accounts in the wallet. + Ok(false) + } + fn get_account_for_ufvk( &self, ufvk: &UnifiedFullViewingKey, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 858907a9d8..b6681ade7f 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -66,6 +66,7 @@ use incrementalmerkletree::Retention; use rusqlite::{self, named_params, params, OptionalExtension}; +use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; use zip32::fingerprint::SeedFingerprint; @@ -75,7 +76,9 @@ use std::io::{self, Cursor}; use std::num::NonZeroU32; use std::ops::RangeInclusive; use tracing::debug; -use zcash_keys::keys::{AddressGenerationError, UnifiedAddressRequest, UnifiedIncomingViewingKey}; +use zcash_keys::keys::{ + AddressGenerationError, UnifiedAddressRequest, UnifiedIncomingViewingKey, UnifiedSpendingKey, +}; use zcash_client_backend::{ address::{Address, UnifiedAddress}, @@ -223,6 +226,10 @@ impl zcash_client_backend::data_api::Account for Account { fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { self.viewing_key.ufvk() } + + fn uivk(&self) -> UnifiedIncomingViewingKey { + self.viewing_key.uivk() + } } impl ViewingKey { @@ -241,6 +248,49 @@ impl ViewingKey { } } +pub(crate) fn seed_matches_derived_account( + params: &P, + seed: &SecretVec, + seed_fingerprint: &SeedFingerprint, + account_index: zip32::AccountId, + uivk: &UnifiedIncomingViewingKey, +) -> Result { + let seed_fingerprint_match = + &SeedFingerprint::from_seed(seed.expose_secret()).ok_or_else(|| { + SqliteClientError::BadAccountData( + "Seed must be between 32 and 252 bytes in length.".to_owned(), + ) + })? == seed_fingerprint; + + // Keys are not comparable with `Eq`, but addresses are, so we derive what should + // be equivalent addresses for each key and use those to check for key equality. + let uivk_match = + match UnifiedSpendingKey::from_seed(params, &seed.expose_secret()[..], account_index) { + // If we can't derive a USK from the given seed with the account's ZIP 32 + // account index, then we immediately know the UIVK won't match because wallet + // accounts are required to have a known UIVK. + Err(_) => false, + Ok(usk) => UnifiedAddressRequest::all().map_or( + Ok::<_, SqliteClientError>(false), + |ua_request| { + Ok(usk + .to_unified_full_viewing_key() + .default_address(ua_request)? + == uivk.default_address(ua_request)?) + }, + )?, + }; + + if seed_fingerprint_match != uivk_match { + // If these mismatch, it suggests database corruption. + Err(SqliteClientError::CorruptedData(format!( + "Seed fingerprint match: {seed_fingerprint_match}, uivk match: {uivk_match}" + ))) + } else { + Ok(seed_fingerprint_match && uivk_match) + } +} + pub(crate) fn pool_code(pool_type: PoolType) -> i64 { // These constants are *incidentally* shared with the typecodes // for unified addresses, but this is exclusively an internal diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 26bd72a35c..20d01b08b1 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -1,6 +1,7 @@ //! Functions for initializing the various databases. use std::fmt; +use std::rc::Rc; use schemer::{Migrator, MigratorError}; use schemer_rusqlite::RusqliteAdapter; @@ -8,11 +9,11 @@ use secrecy::SecretVec; use shardtree::error::ShardTreeError; use uuid::Uuid; -use zcash_client_backend::keys::AddressGenerationError; +use zcash_client_backend::{data_api::WalletRead, keys::AddressGenerationError}; use zcash_primitives::{consensus, transaction::components::amount::BalanceError}; use super::commitment_tree; -use crate::WalletDb; +use crate::{error::SqliteClientError, WalletDb}; mod migrations; @@ -21,6 +22,17 @@ pub enum WalletMigrationError { /// The seed is required for the migration. SeedRequired, + /// A seed was provided that is not relevant to any of the accounts within the wallet. + /// + /// Specifically, it is not relevant to any account for which [`Account::source`] is + /// [`AccountSource::Derived`]. We do not check whether the seed is relevant to any + /// imported account, because that would require brute-forcing the ZIP 32 account + /// index space. + /// + /// [`Account::source`]: zcash_client_backend::data_api::Account::source + /// [`AccountSource::Derived`]: zcash_client_backend::data_api::AccountSource::Derived + SeedNotRelevant, + /// Decoding of an existing value from its serialized form has failed. CorruptedData(String), @@ -73,6 +85,12 @@ impl fmt::Display for WalletMigrationError { "The wallet seed is required in order to update the database." ) } + WalletMigrationError::SeedNotRelevant => { + write!( + f, + "The provided seed is not relevant to any derived accounts in the database." + ) + } WalletMigrationError::CorruptedData(reason) => { write!(f, "Wallet database is corrupted: {}", reason) } @@ -101,6 +119,58 @@ impl std::error::Error for WalletMigrationError { } } +/// Helper to enable calling regular `WalletDb` methods inside the migration code. +/// +/// In this context we can know the full set of errors that are generated by any call we +/// make, so we mark errors as unreachable instead of adding new `WalletMigrationError` +/// variants. +fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> WalletMigrationError { + match e { + SqliteClientError::CorruptedData(e) => WalletMigrationError::CorruptedData(e), + SqliteClientError::Protobuf(e) => WalletMigrationError::CorruptedData(e.to_string()), + SqliteClientError::InvalidNote => { + WalletMigrationError::CorruptedData("invalid note".into()) + } + SqliteClientError::Bech32DecodeError(e) => { + WalletMigrationError::CorruptedData(e.to_string()) + } + SqliteClientError::HdwalletError(e) => WalletMigrationError::CorruptedData(e.to_string()), + SqliteClientError::TransparentAddress(e) => { + WalletMigrationError::CorruptedData(e.to_string()) + } + SqliteClientError::DbError(e) => WalletMigrationError::DbError(e), + SqliteClientError::Io(e) => WalletMigrationError::CorruptedData(e.to_string()), + SqliteClientError::InvalidMemo(e) => WalletMigrationError::CorruptedData(e.to_string()), + SqliteClientError::AddressGeneration(e) => WalletMigrationError::AddressGeneration(e), + SqliteClientError::BadAccountData(e) => WalletMigrationError::CorruptedData(e), + SqliteClientError::CommitmentTree(e) => WalletMigrationError::CommitmentTree(e), + SqliteClientError::UnsupportedPoolType(pool) => WalletMigrationError::CorruptedData( + format!("Wallet DB contains unsupported pool type {}", pool), + ), + SqliteClientError::BalanceError(e) => WalletMigrationError::BalanceError(e), + SqliteClientError::TableNotEmpty => unreachable!("wallet already initialized"), + SqliteClientError::BlockConflict(_) + | SqliteClientError::NonSequentialBlocks + | SqliteClientError::RequestedRewindInvalid(_, _) + | SqliteClientError::KeyDerivationError(_) + | SqliteClientError::AccountIdDiscontinuity + | SqliteClientError::AccountIdOutOfRange + | SqliteClientError::AddressNotRecognized(_) + | SqliteClientError::CacheMiss(_) => { + unreachable!("we only call WalletRead methods; mutations can't occur") + } + SqliteClientError::AccountUnknown => { + unreachable!("all accounts are known in migration context") + } + SqliteClientError::UnknownZip32Derivation => { + unreachable!("we don't call methods that require operating on imported accounts") + } + SqliteClientError::ChainHeightUnknown => { + unreachable!("we don't call methods that require a known chain height") + } + } +} + /// Sets up the internal structure of the data database. /// /// This procedure will automatically perform migration operations to update the wallet database to @@ -109,26 +179,67 @@ impl std::error::Error for WalletMigrationError { /// operation of this procedure is idempotent, so it is safe (though not required) to invoke this /// operation every time the wallet is opened. /// +/// In order to correctly apply migrations to accounts derived from a seed, sometimes the +/// optional `seed` argument is required. This function should first be invoked with +/// `seed` set to `None`; if a pending migration requires the seed, the function returns +/// `Err(schemer::MigratorError::Migration { error: WalletMigrationError::SeedRequired, .. })`. +/// The caller can then re-call this function with the necessary seed. +/// +/// > Note that currently only one seed can be provided; as such, wallets containing +/// > accounts derived from several different seeds are unsupported, and will result in an +/// > error. Support for multi-seed wallets is being tracked in [zcash/librustzcash#1284]. +/// +/// When the `seed` argument is provided, the seed is checked against the database for +/// _relevance_: if any account in the wallet for which [`Account::source`] is +/// [`AccountSource::Derived`] can be derived from the given seed, the seed is relevant to +/// the wallet. If the given seed is not relevant, the function returns +/// `Err(schemer::MigratorError::Migration { error: WalletMigrationError::SeedNotRelevant, .. })` +/// or `Err(schemer::MigratorError::Adapter(WalletMigrationError::SeedNotRelevant))`. +/// +/// We do not check whether the seed is relevant to any imported account, because that +/// would require brute-forcing the ZIP 32 account index space. Consequentially, imported +/// accounts are not migrated. +/// /// It is safe to use a wallet database previously created without the ability to create /// transparent spends with a build that enables transparent spends (via use of the /// `transparent-inputs` feature flag.) The reverse is unsafe, as wallet balance calculations would /// ignore the transparent UTXOs already controlled by the wallet. /// +/// [zcash/librustzcash#1284]: https://github.com/zcash/librustzcash/issues/1284 +/// [`Account::source`]: zcash_client_backend::data_api::Account::source +/// [`AccountSource::Derived`]: zcash_client_backend::data_api::AccountSource::Derived /// /// # Examples /// /// ``` -/// use secrecy::Secret; -/// use tempfile::NamedTempFile; +/// # use std::error::Error; +/// # use secrecy::SecretVec; +/// # use tempfile::NamedTempFile; /// use zcash_primitives::consensus::Network; /// use zcash_client_sqlite::{ /// WalletDb, -/// wallet::init::init_wallet_db, +/// wallet::init::{WalletMigrationError, init_wallet_db}, /// }; /// -/// let data_file = NamedTempFile::new().unwrap(); -/// let mut db = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap(); -/// init_wallet_db(&mut db, Some(Secret::new(vec![]))).unwrap(); +/// # fn main() -> Result<(), Box> { +/// # let data_file = NamedTempFile::new().unwrap(); +/// # let get_data_db_path = || data_file.path(); +/// # let load_seed = || -> Result<_, String> { Ok(SecretVec::new(vec![])) }; +/// let mut db = WalletDb::for_path(get_data_db_path(), Network::TestNetwork)?; +/// match init_wallet_db(&mut db, None) { +/// Err(e) +/// if matches!( +/// e.source().and_then(|e| e.downcast_ref()), +/// Some(&WalletMigrationError::SeedRequired) +/// ) => +/// { +/// let seed = load_seed()?; +/// init_wallet_db(&mut db, Some(seed)) +/// } +/// res => res, +/// }?; +/// # Ok(()) +/// # } /// ``` // TODO: It would be possible to make the transition from providing transparent support to no // longer providing transparent support safe, by including a migration that verifies that no @@ -148,6 +259,8 @@ fn init_wallet_db_internal( seed: Option>, target_migrations: &[Uuid], ) -> Result<(), MigratorError> { + let seed = seed.map(Rc::new); + // Turn off foreign keys, and ensure that table replacement/modification // does not break views wdb.conn @@ -161,7 +274,7 @@ fn init_wallet_db_internal( let mut migrator = Migrator::new(adapter); migrator - .register_multiple(migrations::all_migrations(&wdb.params, seed)) + .register_multiple(migrations::all_migrations(&wdb.params, seed.clone())) .expect("Wallet migration registration should have been successful."); if target_migrations.is_empty() { migrator.up(None)?; @@ -173,6 +286,17 @@ fn init_wallet_db_internal( wdb.conn .execute("PRAGMA foreign_keys = ON", []) .map_err(|e| MigratorError::Adapter(WalletMigrationError::from(e)))?; + + // Now that the migration succeeded, check whether the seed is relevant to the wallet. + if let Some(seed) = seed { + if !wdb + .is_seed_relevant_to_any_derived_accounts(&seed) + .map_err(sqlite_client_error_to_wallet_migration_error)? + { + return Err(WalletMigrationError::SeedNotRelevant.into()); + } + } + Ok(()) } @@ -204,7 +328,7 @@ mod tests { testing::TestBuilder, wallet::scanning::priority_code, WalletDb, DEFAULT_UA_REQUEST, }; - use super::init_wallet_db; + use super::{init_wallet_db, WalletMigrationError}; #[cfg(feature = "transparent-inputs")] use { @@ -1293,10 +1417,14 @@ mod tests { let network = Network::MainNetwork; let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap(); + assert_matches!(init_wallet_db(&mut db_data, None), Ok(_)); + let seed = test_vectors::UNIFIED[0].root_seed; assert_matches!( init_wallet_db(&mut db_data, Some(Secret::new(seed.to_vec()))), - Ok(_) + Err(schemer::MigratorError::Adapter( + WalletMigrationError::SeedNotRelevant + )) ); let birthday = AccountBirthday::from_sapling_activation(&network); diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 044065082a..9ee86f7c70 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -32,9 +32,8 @@ use super::WalletMigrationError; pub(super) fn all_migrations( params: &P, - seed: Option>, + seed: Option>>, ) -> Vec>> { - let seed = Rc::new(seed); // initial_setup // / \ // utxos_table ufvk_support diff --git a/zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs b/zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs index 1e4363872b..a7099a61aa 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs @@ -17,7 +17,7 @@ use super::{add_account_birthdays, receiving_key_scopes, v_transactions_note_uni pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x1b104345_f27e_42da_a9e3_1de22694da43); pub(crate) struct Migration { - pub(super) seed: Rc>>, + pub(super) seed: Option>>, pub(super) params: P, } @@ -83,9 +83,26 @@ impl RusqliteMigration for Migration

{ if transaction.query_row("SELECT COUNT(*) FROM accounts", [], |row| { Ok(row.get::<_, u32>(0)? > 0) })? { - if let Some(seed) = &self.seed.as_ref() { + if let Some(seed) = &self.seed { let seed_id = SeedFingerprint::from_seed(seed.expose_secret()) .expect("Seed is between 32 and 252 bytes in length."); + + // We track whether we have determined seed relevance or not, in order to + // correctly report errors when checking the seed against an account: + // + // - If we encounter an error with the first account, we can assert that + // the seed is not relevant to the wallet by assuming that: + // - All accounts are from the same seed (which is historically the only + // use case that this migration supported), and + // - All accounts in the wallet must have been able to derive their USKs + // (in order to derive UIVKs). + // + // - Once the seed has been determined to be relevant (because it matched + // the first account), any subsequent account derivation failure is + // proving wrong our second assumption above, and we report this as + // corrupted data. + let mut seed_is_relevant = false; + let mut q = transaction.prepare("SELECT * FROM accounts")?; let mut rows = q.query([])?; while let Some(row) = rows.next()? { @@ -110,17 +127,28 @@ impl RusqliteMigration for Migration

{ })?, ) .map_err(|_| { - WalletMigrationError::CorruptedData( - "Unable to derive spending key from seed.".to_string(), - ) + if seed_is_relevant { + WalletMigrationError::CorruptedData( + "Unable to derive spending key from seed.".to_string(), + ) + } else { + WalletMigrationError::SeedNotRelevant + } })?; let expected_ufvk = usk.to_unified_full_viewing_key(); if ufvk != expected_ufvk.encode(&self.params) { - return Err(WalletMigrationError::CorruptedData( - "UFVK does not match expected value.".to_string(), - )); + return Err(if seed_is_relevant { + WalletMigrationError::CorruptedData( + "UFVK does not match expected value.".to_string(), + ) + } else { + WalletMigrationError::SeedNotRelevant + }); } + // We made it past one derived account, so the seed must be relevant. + seed_is_relevant = true; + let uivk = ufvk_parsed .to_unified_incoming_viewing_key() .encode(&self.params); diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs index 88fd58af65..b4af042352 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs @@ -31,7 +31,7 @@ pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0xbe57ef3b_388e_42ea_97e2_ pub(super) struct Migration

{ pub(super) params: P, - pub(super) seed: Rc>>, + pub(super) seed: Option>>, } impl

schemer::Migration for Migration

{ @@ -68,20 +68,43 @@ impl RusqliteMigration for Migration

{ let mut stmt_fetch_accounts = transaction.prepare("SELECT account, address FROM accounts")?; + // We track whether we have determined seed relevance or not, in order to + // correctly report errors when checking the seed against an account: + // + // - If we encounter an error with the first account, we can assert that the seed + // is not relevant to the wallet by assuming that: + // - All accounts are from the same seed (which is historically the only use + // case that this migration supported), and + // - All accounts in the wallet must have been able to derive their USKs (in + // order to derive UIVKs). + // + // - Once the seed has been determined to be relevant (because it matched the + // first account), any subsequent account derivation failure is proving wrong + // our second assumption above, and we report this as corrupted data. + let mut seed_is_relevant = false; + let ua_request = UnifiedAddressRequest::unsafe_new(false, true, UA_TRANSPARENT); let mut rows = stmt_fetch_accounts.query([])?; while let Some(row) = rows.next()? { // We only need to check for the presence of the seed if we have keys that // need to be migrated; otherwise, it's fine to not supply the seed if this // migration is being used to initialize an empty database. - if let Some(seed) = &self.seed.as_ref() { + if let Some(seed) = &self.seed { let account: u32 = row.get(0)?; let account = AccountId::try_from(account).map_err(|_| { WalletMigrationError::CorruptedData("Account ID is invalid".to_owned()) })?; let usk = UnifiedSpendingKey::from_seed(&self.params, seed.expose_secret(), account) - .unwrap(); + .map_err(|_| { + if seed_is_relevant { + WalletMigrationError::CorruptedData( + "Unable to derive spending key from seed.".to_string(), + ) + } else { + WalletMigrationError::SeedNotRelevant + } + })?; let ufvk = usk.to_unified_full_viewing_key(); let address: String = row.get(1)?; @@ -97,11 +120,15 @@ impl RusqliteMigration for Migration

{ WalletMigrationError::CorruptedData("Derivation should have produced a UFVK containing a Sapling component.".to_owned()))?; let (idx, expected_address) = dfvk.default_address(); if decoded_address != expected_address { - return Err(WalletMigrationError::CorruptedData( + return Err(if seed_is_relevant { + WalletMigrationError::CorruptedData( format!("Decoded Sapling address {} does not match the ufvk's Sapling address {} at {:?}.", address, Address::Sapling(expected_address).encode(&self.params), - idx))); + idx)) + } else { + WalletMigrationError::SeedNotRelevant + }); } } Address::Transparent(_) => { @@ -111,15 +138,22 @@ impl RusqliteMigration for Migration

{ Address::Unified(decoded_address) => { let (expected_address, idx) = ufvk.default_address(ua_request)?; if decoded_address != expected_address { - return Err(WalletMigrationError::CorruptedData( + return Err(if seed_is_relevant { + WalletMigrationError::CorruptedData( format!("Decoded unified address {} does not match the ufvk's default address {} at {:?}.", address, Address::Unified(expected_address).encode(&self.params), - idx))); + idx)) + } else { + WalletMigrationError::SeedNotRelevant + }); } } } + // We made it past one derived account, so the seed must be relevant. + seed_is_relevant = true; + let ufvk_str: String = ufvk.encode(&self.params); let address_str: String = ufvk.default_address(ua_request)?.0.encode(&self.params);