Merge pull request #1489 from daira/check-for-colliding-component-fvks

Check for colliding component FVKs

zcash_client_backend/src/data_api.rs

@@ -1703,11 +1703,24 @@ impl AccountBirthday {
 /// - If [`WalletWrite::import_account_hd`] is used to import accounts with non-sequential
 ///   ZIP 32 account indices from the same seed, a call to [`WalletWrite::create_account`]
 ///   will use the ZIP 32 account index just after the highest-numbered existing account.
-/// - If an account is imported via [`WalletWrite::import_account_ufvk`], and then a later
-///   call to either [`WalletWrite::create_account`] or [`WalletWrite::import_account_hd`]
-///   would produce the same UFVK, an error will be returned.
-///   - A future change to this trait might introduce a method to "upgrade" an imported
-///     account with derivation information. See [zcash/librustzcash#1284] for details.
+/// - If an account is added to the wallet, and then a later call to one of the methods
+///   would produce a UFVK that collides with that account on any FVK component (i.e.
+///   Sapling, Orchard, or transparent), an error will be returned. This can occur in the
+///   following cases:
+///   - An account is created via [`WalletWrite::create_account`] with an auto-selected
+///     ZIP 32 account index, and that index is later imported explicitly via either
+///     [`WalletWrite::import_account_ufvk`] or [`WalletWrite::import_account_hd`].
+///   - An account is imported via [`WalletWrite::import_account_ufvk`] or
+///     [`WalletWrite::import_account_hd`], and then the ZIP 32 account index
+///     corresponding to that account's UFVK is later imported either implicitly
+///     via [`WalletWrite::create_account`], or explicitly via a call to
+///     [`WalletWrite::import_account_ufvk`] or [`WalletWrite::import_account_hd`].
+///
+/// Note that an error will be returned on an FVK collision even if the UFVKs do not
+/// match exactly, e.g. if they have different subsets of components.
+///
+/// A future change to this trait might introduce a method to "upgrade" an imported
+/// account with derivation information. See [zcash/librustzcash#1284] for details.
 ///
 /// Users of the `WalletWrite` trait should generally distinguish in their APIs and wallet
 /// UIs between creating a new account, and importing an account that previously existed.

zcash_client_sqlite/src/error.rs

@@ -74,6 +74,7 @@ pub enum SqliteClientError {
     AccountUnknown,
 
     /// The account being added collides with an existing account in the wallet with the given ID.
+    /// The collision can be on the seed and ZIP-32 account index, or a shared FVK component.
     AccountCollision(AccountId),
 
     /// The account was imported, and ZIP-32 derivation information is not known for it.

zcash_client_sqlite/src/lib.rs

@@ -2022,15 +2022,19 @@ extern crate assert_matches;
 
 #[cfg(test)]
 mod tests {
-    use secrecy::{Secret, SecretVec};
+    use secrecy::{ExposeSecret, Secret, SecretVec};
     use zcash_client_backend::data_api::{
         chain::ChainState, Account, AccountBirthday, AccountPurpose, AccountSource, WalletRead,
         WalletWrite,
     };
-    use zcash_keys::keys::UnifiedSpendingKey;
+    use zcash_keys::keys::{UnifiedFullViewingKey, UnifiedSpendingKey};
     use zcash_primitives::block::BlockHash;
 
-    use crate::{error::SqliteClientError, testing::TestBuilder, AccountId, DEFAULT_UA_REQUEST};
+    use crate::{
+        error::SqliteClientError,
+        testing::{TestBuilder, TestState},
+        AccountId, DEFAULT_UA_REQUEST,
+    };
 
     #[cfg(feature = "unstable")]
     use {
@@ -2135,8 +2139,55 @@ mod tests {
             AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index_2);
     }
 
+    fn check_collisions<C>(
+        st: &mut TestState<C>,
+        ufvk: &UnifiedFullViewingKey,
+        birthday: &AccountBirthday,
+        existing_id: AccountId,
+    ) {
+        assert_matches!(
+            st.wallet_mut().import_account_ufvk(ufvk, birthday, AccountPurpose::Spending),
+            Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
+
+        // Remove the transparent component so that we don't have a match on the full UFVK.
+        // That should still produce an AccountCollision error.
+        #[cfg(feature = "transparent-inputs")]
+        {
+            assert!(ufvk.transparent().is_some());
+            let subset_ufvk = UnifiedFullViewingKey::new(
+                None,
+                ufvk.sapling().cloned(),
+                #[cfg(feature = "orchard")]
+                ufvk.orchard().cloned(),
+                #[cfg(not(feature = "orchard"))]
+                None, // see zcash/librustzcash#1488
+            )
+            .unwrap();
+            assert_matches!(
+                st.wallet_mut().import_account_ufvk(&subset_ufvk, birthday, AccountPurpose::Spending),
+                Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
+        }
+
+        // Remove the Orchard component so that we don't have a match on the full UFVK.
+        // That should still produce an AccountCollision error.
+        #[cfg(feature = "orchard")]
+        {
+            assert!(ufvk.orchard().is_some());
+            let subset_ufvk = UnifiedFullViewingKey::new(
+                #[cfg(feature = "transparent-inputs")]
+                ufvk.transparent().cloned(),
+                ufvk.sapling().cloned(),
+                None,
+            )
+            .unwrap();
+            assert_matches!(
+                st.wallet_mut().import_account_ufvk(&subset_ufvk, birthday, AccountPurpose::Spending),
+                Err(SqliteClientError::AccountCollision(id)) if id == existing_id);
+        }
+    }
+
     #[test]
-    pub(crate) fn import_account_hd_1_twice() {
+    pub(crate) fn import_account_hd_1_then_conflicts() {
         let mut st = TestBuilder::new().build();
 
         let birthday = AccountBirthday::from_parts(
@@ -2147,28 +2198,33 @@ mod tests {
         let seed = Secret::new(vec![0u8; 32]);
         let zip32_index_1 = zip32::AccountId::ZERO.next().unwrap();
 
-        let first = st
+        let (first_account, _) = st
             .wallet_mut()
             .import_account_hd(&seed, zip32_index_1, &birthday)
             .unwrap();
+        let ufvk = first_account.ufvk().unwrap();
 
         assert_matches!(
             st.wallet_mut().import_account_hd(&seed, zip32_index_1, &birthday),
-            Err(SqliteClientError::AccountCollision(id)) if id == first.0.id());
+            Err(SqliteClientError::AccountCollision(id)) if id == first_account.id());
+
+        check_collisions(&mut st, ufvk, &birthday, first_account.id());
     }
 
     #[test]
-    pub(crate) fn import_account_ufvk() {
+    pub(crate) fn import_account_ufvk_then_conflicts() {
         let mut st = TestBuilder::new().build();
 
         let birthday = AccountBirthday::from_parts(
             ChainState::empty(st.wallet().params.sapling.unwrap() - 1, BlockHash([0; 32])),
             None,
         );
 
-        let seed = vec![0u8; 32];
-        let usk = UnifiedSpendingKey::from_seed(&st.wallet().params, &seed, zip32::AccountId::ZERO)
-            .unwrap();
+        let seed = Secret::new(vec![0u8; 32]);
+        let zip32_index_0 = zip32::AccountId::ZERO;
+        let usk =
+            UnifiedSpendingKey::from_seed(&st.wallet().params, seed.expose_secret(), zip32_index_0)
+                .unwrap();
         let ufvk = usk.to_unified_full_viewing_key();
 
         let account = st
@@ -2186,10 +2242,16 @@ mod tests {
                 purpose: AccountPurpose::Spending
             }
         );
+
+        assert_matches!(
+            st.wallet_mut().import_account_hd(&seed, zip32_index_0, &birthday),
+            Err(SqliteClientError::AccountCollision(id)) if id == account.id());
+
+        check_collisions(&mut st, &ufvk, &birthday, account.id());
     }
 
     #[test]
-    pub(crate) fn create_account_then_conflicting_import_account_ufvk() {
+    pub(crate) fn create_account_then_conflicts() {
         let mut st = TestBuilder::new().build();
 
         let birthday = AccountBirthday::from_parts(
@@ -2198,13 +2260,16 @@ mod tests {
         );
 
         let seed = Secret::new(vec![0u8; 32]);
+        let zip32_index_0 = zip32::AccountId::ZERO;
         let seed_based = st.wallet_mut().create_account(&seed, &birthday).unwrap();
         let seed_based_account = st.wallet().get_account(seed_based.0).unwrap().unwrap();
         let ufvk = seed_based_account.ufvk().unwrap();
 
         assert_matches!(
-            st.wallet_mut().import_account_ufvk(ufvk, &birthday, AccountPurpose::Spending),
+            st.wallet_mut().import_account_hd(&seed, zip32_index_0, &birthday),
             Err(SqliteClientError::AccountCollision(id)) if id == seed_based.0);
+
+        check_collisions(&mut st, ufvk, &birthday, seed_based.0);
     }
 
     #[cfg(feature = "transparent-inputs")]

zcash_client_sqlite/src/wallet.rs

@@ -357,6 +357,14 @@ pub(crate) fn add_account<P: consensus::Parameters>(
     viewing_key: ViewingKey,
     birthday: &AccountBirthday,
 ) -> Result<Account, SqliteClientError> {
+    if let Some(ufvk) = viewing_key.ufvk() {
+        // Check whether any component of this UFVK collides with an existing imported or derived FVK.
+        if let Some(existing_account) = get_account_for_ufvk(conn, params, ufvk)? {
+            return Err(SqliteClientError::AccountCollision(existing_account.id()));
+        }
+    }
+    // TODO(#1490): check for IVK collisions.
+
     let (hd_seed_fingerprint, hd_account_index, spending_key_available) = match kind {
         AccountSource::Derived {
             seed_fingerprint,
@@ -427,8 +435,9 @@ pub(crate) fn add_account<P: consensus::Parameters>(
             rusqlite::Error::SqliteFailure(f, s)
                 if f.code == rusqlite::ErrorCode::ConstraintViolation =>
             {
-                // An account conflict occurred.
-                // Make a best effort to determine the AccountId of the pre-existing row
+                // An account conflict occurred. This should already have been caught by
+                // the check using `get_account_for_ufvk` above, but in case it wasn't,
+                // make a best effort to determine the AccountId of the pre-existing row
                 // and provide that to our caller.
                 if let Ok(id) = conn.query_row(
                     "SELECT id FROM accounts WHERE ufvk = ?",