From bd38645da54cfaa24935cd0001bef5a1a0054eb2 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sat, 10 Aug 2024 20:41:02 +0100 Subject: [PATCH] When adding an account, check whether any component of its UFVK (if available) collides with an existing imported or derived FVK. This does not check for collisions on IVK for `Incoming` accounts. Signed-off-by: Daira-Emma Hopwood --- zcash_client_backend/src/data_api.rs | 9 ++- zcash_client_sqlite/src/error.rs | 1 + zcash_client_sqlite/src/lib.rs | 89 ++++++++++++++++++++++++---- zcash_client_sqlite/src/wallet.rs | 13 +++- 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 0686e70cf..55baaea9d 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1704,7 +1704,8 @@ impl AccountBirthday { /// 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 added to the wallet, and then a later call to one of the methods -/// would produce the same UFVK, an error will be returned. This can occur in the +/// 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 @@ -1715,10 +1716,8 @@ impl AccountBirthday { /// via [`WalletWrite::create_account`], or explicitly via a call to /// [`WalletWrite::import_account_ufvk`] or [`WalletWrite::import_account_hd`]. /// -/// Note that accounts having UFVKs that are not identical but have shared -/// components (for example, two accounts having the same Sapling FVK, one -/// of which also has an Orchard FVK while the other does not) are currently -/// allowed. This will not be allowed in future. +/// 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. diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index b52f7bf3a..b66d33932 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/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. diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index c8811a80d..bf801cd21 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/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( + st: &mut TestState, + 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,18 +2198,21 @@ 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( @@ -2166,9 +2220,11 @@ mod tests { 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")] diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 683fb77d3..823b9e1f4 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -357,6 +357,14 @@ pub(crate) fn add_account( viewing_key: ViewingKey, birthday: &AccountBirthday, ) -> Result { + 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( 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 = ?",