zcash_client_sqlite: Always check for seed relevance in init_wallet_db

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.

zcash_client_backend/src/data_api.rs

@@ -75,7 +75,9 @@
 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 @@
     /// 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<A: Copy> Account<A> for (A, UnifiedFullViewingKey) {
     fn id(&self) -> A {
         self.0
@@ -356,9 +366,14 @@
     fn ufvk(&self) -> Option<&UnifiedFullViewingKey> {
         Some(&self.1)
     }
+
+    fn uivk(&self) -> UnifiedIncomingViewingKey {
+        self.1.to_unified_incoming_viewing_key()
+    }
 }
 
-impl<A: Copy> Account<A> for (A, Option<UnifiedFullViewingKey>) {
+#[cfg(any(test, feature = "test-dependencies"))]
+impl<A: Copy> Account<A> for (A, UnifiedIncomingViewingKey) {
     fn id(&self) -> A {
         self.0
     }
@@ -368,7 +383,11 @@
     }
 
     fn ufvk(&self) -> Option<&UnifiedFullViewingKey> {
-        self.1.as_ref()
+        None
+    }
+
+    fn uivk(&self) -> UnifiedIncomingViewingKey {
+        self.1.clone()
     }
 }
 
@@ -721,6 +740,13 @@
         seed: &SecretVec<u8>,
     ) -> Result<bool, Self::Error>;
 
+    /// 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<u8>,
+    ) -> Result<bool, Self::Error>;
+
     /// Returns the account corresponding to a given [`UnifiedFullViewingKey`], if any.
     fn get_account_for_ufvk(
         &self,
@@ -1700,6 +1726,13 @@
             Ok(false)
         }
 
+        fn is_seed_relevant_to_any_derived_accounts(
+            &self,
+            _seed: &SecretVec<u8>,
+        ) -> Result<bool, Self::Error> {
+            Ok(false)
+        }
+
         fn get_account_for_ufvk(
             &self,
             _ufvk: &UnifiedFullViewingKey,

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

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),

zcash_client_sqlite/src/lib.rs

@@ -321,38 +321,13 @@
                 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 @@
         }
     }
 
+    fn is_seed_relevant_to_any_derived_accounts(
+        &self,
+        seed: &SecretVec<u8>,
+    ) -> Result<bool, Self::Error> {
+        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,

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::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 @@
     fn ufvk(&self) -> Option<&UnifiedFullViewingKey> {
         self.viewing_key.ufvk()
     }
+
+    fn uivk(&self) -> UnifiedIncomingViewingKey {
+        self.viewing_key.uivk()
+    }
 }
 
 impl ViewingKey {
@@ -241,6 +248,49 @@
     }
 }
 
+pub(crate) fn seed_matches_derived_account<P: consensus::Parameters>(
+    params: &P,
+    seed: &SecretVec<u8>,
+    seed_fingerprint: &SeedFingerprint,
+    account_index: zip32::AccountId,
+    uivk: &UnifiedIncomingViewingKey,
+) -> Result<bool, SqliteClientError> {
+    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