From 071d7c51d777a207f66a287c249ee4dbec7e77a8 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Fri, 8 Mar 2024 23:05:37 -0700 Subject: [PATCH 01/22] Add `UnifiedIncomingViewingKey` struct Also update sqlite to utilize the new struct --- zcash_client_sqlite/src/wallet.rs | 52 +- .../init/migrations/full_account_ids.rs | 10 +- zcash_keys/CHANGELOG.md | 9 + zcash_keys/src/keys.rs | 554 +++++++++++++++++- 4 files changed, 564 insertions(+), 61 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 0a8a02a4e..7d2f8b793 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -75,7 +75,9 @@ use std::num::NonZeroU32; use std::ops::RangeInclusive; use tracing::debug; use zcash_address::unified::{Encoding, Ivk, Uivk}; -use zcash_keys::keys::{AddressGenerationError, HdSeedFingerprint, UnifiedAddressRequest}; +use zcash_keys::keys::{ + AddressGenerationError, HdSeedFingerprint, UnifiedAddressRequest, UnifiedIncomingViewingKey, +}; use zcash_client_backend::{ address::{Address, UnifiedAddress}, @@ -200,7 +202,7 @@ pub(crate) enum ImportedAccount { /// An account that was imported via its full viewing key. Full(Box), /// An account that was imported via its incoming viewing key. - Incoming(Uivk), + Incoming(Box), } /// Describes an account in terms of its UVK or ZIP-32 origins. @@ -225,7 +227,7 @@ impl Account { match self { Account::Zip32(HdSeedAccount(_, _, ufvk)) => ufvk.default_address(request), Account::Imported(ImportedAccount::Full(ufvk)) => ufvk.default_address(request), - Account::Imported(ImportedAccount::Incoming(_uivk)) => todo!(), + Account::Imported(ImportedAccount::Incoming(uivk)) => uivk.default_address(request), } } } @@ -304,50 +306,34 @@ fn get_sql_values_for_account_parameters<'a, P: consensus::Parameters>( hd_seed_fingerprint: Some(hdaccount.hd_seed_fingerprint().as_bytes()), hd_account_index: Some(hdaccount.account_index().into()), ufvk: Some(hdaccount.ufvk().encode(params)), - uivk: ufvk_to_uivk(hdaccount.ufvk(), params)?, + uivk: hdaccount + .ufvk() + .to_unified_incoming_viewing_key() + .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))? + .to_uivk() + .encode(¶ms.network_type()), }, Account::Imported(ImportedAccount::Full(ufvk)) => AccountSqlValues { account_type: AccountType::Imported.into(), hd_seed_fingerprint: None, hd_account_index: None, ufvk: Some(ufvk.encode(params)), - uivk: ufvk_to_uivk(ufvk, params)?, + uivk: ufvk + .to_unified_incoming_viewing_key() + .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))? + .to_uivk() + .encode(¶ms.network_type()), }, Account::Imported(ImportedAccount::Incoming(uivk)) => AccountSqlValues { account_type: AccountType::Imported.into(), hd_seed_fingerprint: None, hd_account_index: None, ufvk: None, - uivk: uivk.encode(¶ms.network_type()), + uivk: uivk.to_uivk().encode(¶ms.network_type()), }, }) } -pub(crate) fn ufvk_to_uivk( - ufvk: &UnifiedFullViewingKey, - params: &P, -) -> Result { - let mut ivks: Vec = Vec::new(); - if let Some(orchard) = ufvk.orchard() { - ivks.push(Ivk::Orchard(orchard.to_ivk(Scope::External).to_bytes())); - } - if let Some(sapling) = ufvk.sapling() { - let ivk = sapling.to_external_ivk(); - ivks.push(Ivk::Sapling(ivk.to_bytes())); - } - #[cfg(feature = "transparent-inputs")] - if let Some(tfvk) = ufvk.transparent() { - let tivk = tfvk.derive_external_ivk()?; - ivks.push(Ivk::P2pkh(tivk.serialize().try_into().map_err(|_| { - SqliteClientError::BadAccountData("Unable to serialize transparent IVK.".to_string()) - })?)); - } - - let uivk = zcash_address::unified::Uivk::try_from_items(ivks) - .map_err(|e| SqliteClientError::BadAccountData(format!("Unable to derive UIVK: {}", e)))?; - Ok(uivk.encode(¶ms.network_type())) -} - pub(crate) fn add_account( conn: &rusqlite::Transaction, params: &P, @@ -1202,6 +1188,8 @@ pub(crate) fn get_account, P: Parameters>( "UIVK network type does not match wallet network type".to_string(), )); } + let uivk = UnifiedIncomingViewingKey::from_uivk(&uivk) + .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))?; match account_type { AccountType::Zip32 => Ok(Some(Account::Zip32(HdSeedAccount::new( @@ -1222,7 +1210,7 @@ pub(crate) fn get_account, P: Parameters>( AccountType::Imported => Ok(Some(Account::Imported(if let Some(ufvk) = ufvk { ImportedAccount::Full(Box::new(ufvk)) } else { - ImportedAccount::Incoming(uivk) + ImportedAccount::Incoming(Box::new(uivk)) }))), } } 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 fae568533..ec1913b23 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 @@ -1,10 +1,11 @@ use std::{collections::HashSet, rc::Rc}; -use crate::wallet::{init::WalletMigrationError, ufvk_to_uivk, AccountType}; +use crate::wallet::{init::WalletMigrationError, AccountType}; use rusqlite::{named_params, Transaction}; use schemer_rusqlite::RusqliteMigration; use secrecy::{ExposeSecret, SecretVec}; use uuid::Uuid; +use zcash_address::unified::Encoding; use zcash_client_backend::keys::UnifiedSpendingKey; use zcash_keys::keys::{HdSeedFingerprint, UnifiedFullViewingKey}; use zcash_primitives::consensus; @@ -113,8 +114,11 @@ impl RusqliteMigration for Migration

{ )); } - let uivk = ufvk_to_uivk(&ufvk_parsed, &self.params) - .map_err(|e| WalletMigrationError::CorruptedData(e.to_string()))?; + let uivk = ufvk_parsed + .to_unified_incoming_viewing_key() + .map_err(|e| WalletMigrationError::CorruptedData(e.to_string()))? + .to_uivk() + .encode(&self.params.network_type()); transaction.execute(r#" INSERT INTO accounts_new (id, account_type, hd_seed_fingerprint, hd_account_index, ufvk, uivk, birthday_height, recover_until_height) diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 79654d4c9..08a1f9f57 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -11,6 +11,11 @@ and this library adheres to Rust's notion of - `zcash_keys::address::Address::has_receiver` - `impl Display for zcash_keys::keys::AddressGenerationError` - `impl std::error::Error for zcash_keys::keys::AddressGenerationError` +- `zcash_keys::keys::UnifiedError` +- `zcash_keys::keys::UnifiedFullViewingKey::from_ufvk` +- `zcash_keys::keys::UnifiedFullViewingKey::to_ufvk` +- `zcash_keys::keys::UnifiedFullViewingKey::to_unified_incoming_viewing_key` +- `zcash_keys::keys::UnifiedIncomingViewingKey` ### Changed - `zcash_keys::keys::AddressGenerationError` has a new variant @@ -18,6 +23,10 @@ and this library adheres to Rust's notion of - `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}` now return `Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError>` (instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`). +- `zcash_keys::keys::AddressGenerationError` + - Dropped `Clone` trait + - Added `UnifiedError` variant. + - Added `HDWalletError` variant. ### Fixed - `UnifiedFullViewingKey::find_address` can now find an address for a diversifier diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 27dfb8393..c48e12a1e 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -3,8 +3,8 @@ use blake2b_simd::Params as blake2bParams; use secrecy::{ExposeSecret, SecretVec}; use std::{error, fmt}; -use zcash_address::unified::{self, Container, Encoding, Typecode}; -use zcash_protocol::consensus::{self, NetworkConstants}; +use zcash_address::unified::{self, Container, Encoding, Typecode, Ufvk, Uivk}; +use zcash_protocol::{consensus, ShieldedProtocol}; use zip32::{AccountId, DiversifierIndex}; use crate::address::UnifiedAddress; @@ -15,6 +15,10 @@ use { zcash_primitives::legacy::keys::{self as legacy, IncomingViewingKey, NonHardenedChildIndex}, }; +#[cfg(any(feature = "sapling", feature = "orchard"))] +// Your code here +use zcash_protocol::consensus::NetworkConstants; + #[cfg(all( feature = "transparent-inputs", any(test, feature = "test-dependencies") @@ -472,7 +476,7 @@ impl UnifiedSpendingKey { } /// Errors that can occur in the generation of unified addresses. -#[derive(Clone, Debug)] +#[derive(Debug)] pub enum AddressGenerationError { /// The requested diversifier index was outside the range of valid transparent /// child address indices. @@ -492,6 +496,26 @@ pub enum AddressGenerationError { /// A Unified address cannot be generated without at least one shielded receiver being /// included. ShieldedReceiverRequired, + + // An error occurred while deriving a key or address from an HD wallet. + UnifiedError(UnifiedError), + + // An error occurred while deriving a transparent key or address from an HD wallet. + #[cfg(feature = "transparent-inputs")] + HDWalletError(hdwallet::error::Error), +} + +impl From for AddressGenerationError { + fn from(e: UnifiedError) -> Self { + AddressGenerationError::UnifiedError(e) + } +} + +#[cfg(feature = "transparent-inputs")] +impl From for AddressGenerationError { + fn from(e: hdwallet::error::Error) -> Self { + AddressGenerationError::HDWalletError(e) + } } impl fmt::Display for AddressGenerationError { @@ -505,6 +529,7 @@ impl fmt::Display for AddressGenerationError { i ) } + #[cfg(feature = "sapling")] AddressGenerationError::InvalidSaplingDiversifierIndex(i) => { write!( f, @@ -535,6 +560,13 @@ impl fmt::Display for AddressGenerationError { AddressGenerationError::ShieldedReceiverRequired => { write!(f, "A Unified Address requires at least one shielded (Sapling or Orchard) receiver.") } + AddressGenerationError::UnifiedError(e) => { + write!(f, "UnifiedError: {}", e) + } + #[cfg(feature = "transparent-inputs")] + AddressGenerationError::HDWalletError(e) => { + write!(f, "HDWalletError: {}", e) + } } } } @@ -601,9 +633,43 @@ impl UnifiedAddressRequest { } } +/// Errors that may be returned from the [UnifiedFullViewingKey] and [UnifiedIncomingViewingKey] types. +#[derive(Debug)] +pub enum UnifiedError { + InvalidShieldedKey(ShieldedProtocol), + #[cfg(feature = "transparent-inputs")] + InvalidTransparentKey(hdwallet::error::Error), + #[cfg(feature = "transparent-inputs")] + HDWallet(hdwallet::error::Error), +} + +#[cfg(feature = "transparent-inputs")] +impl From for UnifiedError { + fn from(e: hdwallet::error::Error) -> Self { + UnifiedError::HDWallet(e) + } +} + +impl fmt::Display for UnifiedError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match &self { + UnifiedError::InvalidShieldedKey(p) => { + write!(f, "Invalid key for shielded protocol: {:?}", p) + } + #[cfg(feature = "transparent-inputs")] + UnifiedError::InvalidTransparentKey(e) => { + write!(f, "Invalid key for transparent protocol: {:?}", e) + } + #[cfg(feature = "transparent-inputs")] + UnifiedError::HDWallet(e) => { + write!(f, "HDWallet error: {}", e) + } + } + } +} + /// A [ZIP 316](https://zips.z.cash/zip-0316) unified full viewing key. #[derive(Clone, Debug)] -#[doc(hidden)] pub struct UnifiedFullViewingKey { #[cfg(feature = "transparent-inputs")] transparent: Option, @@ -662,6 +728,13 @@ impl UnifiedFullViewingKey { )); } + Self::from_ufvk(&ufvk).map_err(|e| e.to_string()) + } + + /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. + /// + /// [ZIP 316]: https://zips.z.cash/zip-0316 + pub fn from_ufvk(ufvk: &Ufvk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -677,21 +750,21 @@ impl UnifiedFullViewingKey { .filter_map(|receiver| match receiver { #[cfg(feature = "orchard")] unified::Fvk::Orchard(data) => orchard::keys::FullViewingKey::from_bytes(data) - .ok_or("Invalid Orchard FVK in Unified FVK") + .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Orchard)) .map(|addr| { orchard = Some(addr); None }) .transpose(), #[cfg(not(feature = "orchard"))] - unified::Fvk::Orchard(data) => Some(Ok::<_, &'static str>(( + unified::Fvk::Orchard(data) => Some(Ok::<_, UnifiedError>(( u32::from(unified::Typecode::Orchard), data.to_vec(), ))), #[cfg(feature = "sapling")] unified::Fvk::Sapling(data) => { sapling::DiversifiableFullViewingKey::from_bytes(data) - .ok_or("Invalid Sapling FVK in Unified FVK") + .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Sapling)) .map(|pa| { sapling = Some(pa); None @@ -699,20 +772,20 @@ impl UnifiedFullViewingKey { .transpose() } #[cfg(not(feature = "sapling"))] - unified::Fvk::Sapling(data) => Some(Ok::<_, &'static str>(( + unified::Fvk::Sapling(data) => Some(Ok::<_, UnifiedError>(( u32::from(unified::Typecode::Sapling), data.to_vec(), ))), #[cfg(feature = "transparent-inputs")] unified::Fvk::P2pkh(data) => legacy::AccountPubKey::deserialize(data) - .map_err(|_| "Invalid transparent FVK in Unified FVK") + .map_err(UnifiedError::InvalidTransparentKey) .map(|tfvk| { transparent = Some(tfvk); None }) .transpose(), #[cfg(not(feature = "transparent-inputs"))] - unified::Fvk::P2pkh(data) => Some(Ok::<_, &'static str>(( + unified::Fvk::P2pkh(data) => Some(Ok::<_, UnifiedError>(( u32::from(unified::Typecode::P2pkh), data.to_vec(), ))), @@ -733,6 +806,11 @@ impl UnifiedFullViewingKey { /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. pub fn encode(&self, params: &P) -> String { + self.to_ufvk().encode(¶ms.network_type()) + } + + /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. + pub fn to_ufvk(&self) -> Ufvk { let items = std::iter::empty().chain(self.unknown.iter().map(|(typecode, data)| { unified::Fvk::Unknown { typecode: *typecode, @@ -761,9 +839,27 @@ impl UnifiedFullViewingKey { .map(unified::Fvk::P2pkh), ); - let ufvk = unified::Ufvk::try_from_items(items.collect()) - .expect("UnifiedFullViewingKey should only be constructed safely"); - ufvk.encode(¶ms.network_type()) + unified::Ufvk::try_from_items(items.collect()) + .expect("UnifiedFullViewingKey should only be constructed safely") + } + + /// Derives a Unified Incoming Viewing Key from this Unified Full Viewing Key. + pub fn to_unified_incoming_viewing_key( + &self, + ) -> Result { + Ok(UnifiedIncomingViewingKey { + #[cfg(feature = "transparent-inputs")] + transparent: self + .transparent + .as_ref() + .map(|t| t.derive_external_ivk()) + .transpose()?, + #[cfg(feature = "sapling")] + sapling: self.sapling.as_ref().map(|s| s.to_external_ivk()), + #[cfg(feature = "orchard")] + orchard: self.orchard.as_ref().map(|o| o.to_ivk(Scope::External)), + unknown: Vec::new(), + }) } /// Returns the transparent component of the unified key at the @@ -785,6 +881,221 @@ impl UnifiedFullViewingKey { self.orchard.as_ref() } + /// Attempts to derive the Unified Address for the given diversifier index and + /// receiver types. + /// + /// Returns `None` if the specified index does not produce a valid diversifier. + pub fn address( + &self, + j: DiversifierIndex, + request: UnifiedAddressRequest, + ) -> Result { + self.to_unified_incoming_viewing_key()?.address(j, request) + } + + /// Searches the diversifier space starting at diversifier index `j` for one which will + /// produce a valid diversifier, and return the Unified Address constructed using that + /// diversifier along with the index at which the valid diversifier was found. + /// + /// Returns an `Err(AddressGenerationError)` if no valid diversifier exists or if the features + /// required to satisfy the unified address request are not properly enabled. + #[allow(unused_mut)] + pub fn find_address( + &self, + mut j: DiversifierIndex, + request: UnifiedAddressRequest, + ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { + self.to_unified_incoming_viewing_key()? + .find_address(j, request) + } + + /// Find the Unified Address corresponding to the smallest valid diversifier index, along with + /// that index. + /// + /// Returns an `Err(AddressGenerationError)` if no valid diversifier exists or if the features + /// required to satisfy the unified address request are not properly enabled. + pub fn default_address( + &self, + request: UnifiedAddressRequest, + ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { + self.find_address(DiversifierIndex::new(), request) + } +} + +/// A [ZIP 316](https://zips.z.cash/zip-0316) unified incoming viewing key. +#[derive(Clone, Debug)] +pub struct UnifiedIncomingViewingKey { + #[cfg(feature = "transparent-inputs")] + transparent: Option, + #[cfg(feature = "sapling")] + sapling: Option<::sapling::zip32::IncomingViewingKey>, + #[cfg(feature = "orchard")] + orchard: Option, + /// Stores the unrecognized elements of the unified encoding. + unknown: Vec<(u32, Vec)>, +} + +impl UnifiedIncomingViewingKey { + /// Construct a new unified incoming viewing key, if the required components are present. + pub fn new( + #[cfg(feature = "transparent-inputs")] transparent: Option< + zcash_primitives::legacy::keys::ExternalIvk, + >, + #[cfg(feature = "sapling")] sapling: Option<::sapling::zip32::IncomingViewingKey>, + #[cfg(feature = "orchard")] orchard: Option, + // TODO: Implement construction of UIVKs with metadata items. + ) -> Option { + #[cfg(feature = "orchard")] + let has_orchard = orchard.is_some(); + #[cfg(not(feature = "orchard"))] + let has_orchard = false; + #[cfg(feature = "sapling")] + let has_sapling = sapling.is_some(); + #[cfg(not(feature = "sapling"))] + let has_sapling = false; + + if has_orchard || has_sapling { + Some(UnifiedIncomingViewingKey { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + // We don't allow constructing new UFVKs with unknown items, but we store + // this to allow parsing such UFVKs. + unknown: vec![], + }) + } else { + None + } + } + + /// Constructs a unified incoming viewing key from a parsed unified encoding. + pub fn from_uivk(uivk: &Uivk) -> Result { + #[cfg(feature = "orchard")] + let mut orchard = None; + #[cfg(feature = "sapling")] + let mut sapling = None; + #[cfg(feature = "transparent-inputs")] + let mut transparent = None; + + // We can use as-parsed order here for efficiency, because we're breaking out the + // receivers we support from the unknown receivers. + let unknown = uivk + .items_as_parsed() + .iter() + .filter_map(|receiver| match receiver { + #[cfg(feature = "orchard")] + unified::Ivk::Orchard(data) => { + Option::from(orchard::keys::IncomingViewingKey::from_bytes(data)) + .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Orchard)) + .map(|addr| { + orchard = Some(addr); + None + }) + .transpose() + } + #[cfg(not(feature = "orchard"))] + unified::Ivk::Orchard(data) => Some(Ok::<_, UnifiedError>(( + u32::from(unified::Typecode::Orchard), + data.to_vec(), + ))), + #[cfg(feature = "sapling")] + unified::Ivk::Sapling(data) => { + Option::from(::sapling::zip32::IncomingViewingKey::from_bytes(data)) + .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Sapling)) + .map(|pa| { + sapling = Some(pa); + None + }) + .transpose() + } + #[cfg(not(feature = "sapling"))] + unified::Ivk::Sapling(data) => Some(Ok::<_, UnifiedError>(( + u32::from(unified::Typecode::Sapling), + data.to_vec(), + ))), + #[cfg(feature = "transparent-inputs")] + unified::Ivk::P2pkh(data) => legacy::ExternalIvk::deserialize(data) + .map_err(UnifiedError::InvalidTransparentKey) + .map(|tivk| { + transparent = Some(tivk); + None + }) + .transpose(), + #[cfg(not(feature = "transparent-inputs"))] + unified::Ivk::P2pkh(data) => Some(Ok::<_, UnifiedError>(( + u32::from(unified::Typecode::P2pkh), + data.to_vec(), + ))), + unified::Ivk::Unknown { typecode, data } => Some(Ok((*typecode, data.clone()))), + }) + .collect::>()?; + + Ok(Self { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + unknown, + }) + } + + /// Converts this unified incoming viewing key to a unified encoding. + pub fn to_uivk(&self) -> Uivk { + let items = std::iter::empty().chain(self.unknown.iter().map(|(typecode, data)| { + unified::Ivk::Unknown { + typecode: *typecode, + data: data.clone(), + } + })); + #[cfg(feature = "orchard")] + let items = items.chain( + self.orchard + .as_ref() + .map(|ivk| ivk.to_bytes()) + .map(unified::Ivk::Orchard), + ); + #[cfg(feature = "sapling")] + let items = items.chain( + self.sapling + .as_ref() + .map(|divk| divk.to_bytes()) + .map(unified::Ivk::Sapling), + ); + #[cfg(feature = "transparent-inputs")] + let items = items.chain( + self.transparent + .as_ref() + .map(|tivk| tivk.serialize().try_into().unwrap()) + .map(unified::Ivk::P2pkh), + ); + + unified::Uivk::try_from_items(items.collect()) + .expect("UnifiedIncomingViewingKey should only be constructed safely.") + } + + /// Returns the Transparent external IVK, if present. + #[cfg(feature = "transparent-inputs")] + pub fn transparent(&self) -> &Option { + &self.transparent + } + + /// Returns the Sapling IVK, if present. + #[cfg(feature = "sapling")] + pub fn sapling(&self) -> &Option<::sapling::zip32::IncomingViewingKey> { + &self.sapling + } + + /// Returns the Orchard IVK, if present. + #[cfg(feature = "orchard")] + pub fn orchard(&self) -> &Option { + &self.orchard + } + /// Attempts to derive the Unified Address for the given diversifier index and /// receiver types. /// @@ -792,20 +1103,20 @@ impl UnifiedFullViewingKey { pub fn address( &self, _j: DiversifierIndex, - _request: UnifiedAddressRequest, + request: UnifiedAddressRequest, ) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; - if _request.has_orchard { + if request.has_orchard { #[cfg(not(feature = "orchard"))] return Err(AddressGenerationError::ReceiverTypeNotSupported( Typecode::Orchard, )); #[cfg(feature = "orchard")] - if let Some(ofvk) = &self.orchard { + if let Some(oivk) = &self.orchard { let orchard_j = orchard::keys::DiversifierIndex::from(*_j.as_bytes()); - orchard = Some(ofvk.address_at(orchard_j, Scope::External)) + orchard = Some(oivk.address_at(orchard_j)) } else { return Err(AddressGenerationError::KeyNotAvailable(Typecode::Orchard)); } @@ -813,20 +1124,19 @@ impl UnifiedFullViewingKey { #[cfg(feature = "sapling")] let mut sapling = None; - if _request.has_sapling { + if request.has_sapling { #[cfg(not(feature = "sapling"))] return Err(AddressGenerationError::ReceiverTypeNotSupported( Typecode::Sapling, )); #[cfg(feature = "sapling")] - if let Some(extfvk) = &self.sapling { + if let Some(divk) = &self.sapling { // If a Sapling receiver type is requested, we must be able to construct an // address; if we're unable to do so, then no Unified Address exists at this // diversifier and we use `?` to early-return from this method. sapling = Some( - extfvk - .address(_j) + divk.address_at(_j) .ok_or(AddressGenerationError::InvalidSaplingDiversifierIndex(_j))?, ); } else { @@ -836,14 +1146,14 @@ impl UnifiedFullViewingKey { #[cfg(feature = "transparent-inputs")] let mut transparent = None; - if _request.has_p2pkh { + if request.has_p2pkh { #[cfg(not(feature = "transparent-inputs"))] return Err(AddressGenerationError::ReceiverTypeNotSupported( Typecode::P2pkh, )); #[cfg(feature = "transparent-inputs")] - if let Some(tfvk) = self.transparent.as_ref() { + if let Some(tivk) = self.transparent.as_ref() { // If a transparent receiver type is requested, we must be able to construct an // address; if we're unable to do so, then no Unified Address exists at this // diversifier. @@ -851,8 +1161,7 @@ impl UnifiedFullViewingKey { .ok_or(AddressGenerationError::InvalidTransparentChildIndex(_j))?; transparent = Some( - tfvk.derive_external_ivk() - .and_then(|tivk| tivk.derive_address(transparent_j)) + tivk.derive_address(transparent_j) .map_err(|_| AddressGenerationError::InvalidTransparentChildIndex(_j))?, ); } else { @@ -953,8 +1262,13 @@ pub mod testing { #[cfg(test)] mod tests { + use crate::keys::UnifiedIncomingViewingKey; + use super::UnifiedFullViewingKey; use proptest::prelude::proptest; + use zcash_address::unified::{Encoding, Uivk}; + #[cfg(feature = "orchard")] + use zip32::Scope; #[cfg(any( feature = "orchard", @@ -1199,10 +1513,198 @@ mod tests { } } + #[test] + fn uivk_round_trip() { + #[cfg(feature = "orchard")] + let orchard = { + let sk = + orchard::keys::SpendingKey::from_zip32_seed(&[0; 32], 0, AccountId::ZERO).unwrap(); + Some(orchard::keys::FullViewingKey::from(&sk).to_ivk(Scope::External)) + }; + + #[cfg(feature = "sapling")] + let sapling = { + let extsk = sapling::spending_key(&[0; 32], 0, AccountId::ZERO); + Some(extsk.to_diversifiable_full_viewing_key().to_external_ivk()) + }; + + #[cfg(feature = "transparent-inputs")] + let transparent = { + let privkey = + AccountPrivKey::from_seed(&MAIN_NETWORK, &[0; 32], AccountId::ZERO).unwrap(); + Some(privkey.to_account_pubkey().derive_external_ivk().unwrap()) + }; + + let uivk = UnifiedIncomingViewingKey::new( + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + ); + + #[cfg(not(any(feature = "orchard", feature = "sapling")))] + assert!(uivk.is_none()); + + #[cfg(any(feature = "orchard", feature = "sapling"))] + { + let uivk = uivk.expect("Orchard or Sapling ivk is present."); + let encoded = uivk.to_uivk().encode(&Network::Main); + + // Test encoded form against known values; these test vectors contain Orchard receivers + // that will be treated as unknown if the `orchard` feature is not enabled. + let encoded_with_t = "uivk1z28yg638vjwusmf0zc9ad2j0mpv6s42wc5kqt004aaqfu5xxxgu7mdcydn9qf723fnryt34s6jyxyw0jt7spq04c3v9ze6qe9gjjc5aglz8zv5pqtw58czd0actynww5n85z3052kzgy6cu0fyjafyp4sr4kppyrrwhwev2rr0awq6m8d66esvk6fgacggqnswg5g9gkv6t6fj9ajhyd0gmel4yzscprpzduncc0e2lywufup6fvzf6y8cefez2r99pgge5yyfuus0r60khgu895pln5e7nn77q6s9kh2uwf6lrfu06ma2kd7r05jjvl4hn6nupge8fajh0cazd7mkmz23t79w"; + let _encoded_no_t = "uivk1020vq9j5zeqxh303sxa0zv2hn9wm9fev8x0p8yqxdwyzde9r4c90fcglc63usj0ycl2scy8zxuhtser0qrq356xfy8x3vyuxu7f6gas75svl9v9m3ctuazsu0ar8e8crtx7x6zgh4kw8xm3q4rlkpm9er2wefxhhf9pn547gpuz9vw27gsdp6c03nwlrxgzhr2g6xek0x8l5avrx9ue9lf032tr7kmhqf3nfdxg7ldfgx6yf09g"; + + // We test the full roundtrip only with the `sapling` and `orchard` features enabled, + // because we will not generate these parts of the encoding if the UIVK does not have an + // these parts. + #[cfg(all(feature = "sapling", feature = "orchard"))] + { + #[cfg(feature = "transparent-inputs")] + assert_eq!(encoded, encoded_with_t); + #[cfg(not(feature = "transparent-inputs"))] + assert_eq!(encoded, _encoded_no_t); + } + + let decoded = + UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(&encoded).unwrap().1).unwrap(); + let reencoded = decoded.to_uivk().encode(&Network::Main); + assert_eq!(encoded, reencoded); + + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded.transparent.map(|t| t.serialize()), + uivk.transparent.as_ref().map(|t| t.serialize()), + ); + #[cfg(feature = "sapling")] + assert_eq!( + decoded.sapling.map(|s| s.to_bytes()), + uivk.sapling.map(|s| s.to_bytes()), + ); + #[cfg(feature = "orchard")] + assert_eq!( + decoded.orchard.map(|o| o.to_bytes()), + uivk.orchard.map(|o| o.to_bytes()), + ); + + let decoded_with_t = + UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(encoded_with_t).unwrap().1) + .unwrap(); + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded_with_t.transparent.map(|t| t.serialize()), + uivk.transparent.as_ref().map(|t| t.serialize()), + ); + + // Both Orchard and Sapling enabled + #[cfg(all( + feature = "orchard", + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 0); + #[cfg(all( + feature = "orchard", + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + + // Orchard enabled + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); + + // Sapling enabled + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); + } + } + + #[test] + #[cfg(feature = "transparent-inputs")] + fn uivk_derivation() { + use crate::keys::UnifiedAddressRequest; + + use super::UnifiedSpendingKey; + + for tv in test_vectors::UNIFIED { + let usk = UnifiedSpendingKey::from_seed( + &MAIN_NETWORK, + &tv.root_seed, + AccountId::try_from(tv.account).unwrap(), + ) + .expect("seed produced a valid unified spending key"); + + let d_idx = DiversifierIndex::from(tv.diversifier_index); + let uivk = usk + .to_unified_full_viewing_key() + .to_unified_incoming_viewing_key() + .unwrap(); + + // The test vectors contain some diversifier indices that do not generate + // valid Sapling addresses, so skip those. + #[cfg(feature = "sapling")] + if uivk.sapling().as_ref().unwrap().address_at(d_idx).is_none() { + continue; + } + + let ua = uivk + .address(d_idx, UnifiedAddressRequest::unsafe_new(false, true, true)) + .unwrap_or_else(|err| { + panic!( + "unified address generation failed for account {}: {:?}", + tv.account, err + ) + }); + + match Address::decode(&MAIN_NETWORK, tv.unified_addr) { + Some(Address::Unified(tvua)) => { + // We always derive transparent and Sapling receivers, but not + // every value in the test vectors has these present. + if tvua.transparent().is_some() { + assert_eq!(tvua.transparent(), ua.transparent()); + } + #[cfg(feature = "sapling")] + if tvua.sapling().is_some() { + assert_eq!(tvua.sapling(), ua.sapling()); + } + } + _other => { + panic!( + "{} did not decode to a valid unified address", + tv.unified_addr + ); + } + } + } + } + proptest! { #[test] #[cfg(feature = "unstable")] - fn prop_usk_roundtrip(usk in arb_unified_spending_key(Network::MainNetwork)) { + fn prop_usk_roundtrip(usk in arb_unified_spending_key(zcash_protocol::consensus::Network::MainNetwork)) { let encoded = usk.to_bytes(Era::Orchard); let encoded_len = { From 2b3060d138bcac207f7af2cac0948c4fad65a4ba Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Sat, 9 Mar 2024 21:22:25 -0700 Subject: [PATCH 02/22] Apply updated style for key discovery Co-authored-by: Kris Nuttycombe --- zcash_keys/src/keys.rs | 91 +++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 46 deletions(-) diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index c48e12a1e..8eefd34f0 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -980,58 +980,57 @@ impl UnifiedIncomingViewingKey { #[cfg(feature = "transparent-inputs")] let mut transparent = None; + let mut unknown = vec![]; + // We can use as-parsed order here for efficiency, because we're breaking out the // receivers we support from the unknown receivers. - let unknown = uivk - .items_as_parsed() - .iter() - .filter_map(|receiver| match receiver { - #[cfg(feature = "orchard")] + for receiver in uivk.items_as_parsed() { + match receiver { unified::Ivk::Orchard(data) => { - Option::from(orchard::keys::IncomingViewingKey::from_bytes(data)) - .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Orchard)) - .map(|addr| { - orchard = Some(addr); - None - }) - .transpose() + #[cfg(feature = "orchard")] + { + orchard = Some( + Option::from(orchard::keys::IncomingViewingKey::from_bytes(data)) + .ok_or(UnifiedError::InvalidShieldedKey( + ShieldedProtocol::Orchard, + ))?, + ); + } + + #[cfg(not(feature = "orchard"))] + unknown.push((u32::from(unified::Typecode::Orchard), data.to_vec())); } - #[cfg(not(feature = "orchard"))] - unified::Ivk::Orchard(data) => Some(Ok::<_, UnifiedError>(( - u32::from(unified::Typecode::Orchard), - data.to_vec(), - ))), - #[cfg(feature = "sapling")] unified::Ivk::Sapling(data) => { - Option::from(::sapling::zip32::IncomingViewingKey::from_bytes(data)) - .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Sapling)) - .map(|pa| { - sapling = Some(pa); - None - }) - .transpose() + #[cfg(feature = "sapling")] + { + sapling = Some( + Option::from(::sapling::zip32::IncomingViewingKey::from_bytes(data)) + .ok_or(UnifiedError::InvalidShieldedKey( + ShieldedProtocol::Sapling, + ))?, + ); + } + + #[cfg(not(feature = "sapling"))] + unknown.push((u32::from(unified::Typecode::Sapling), data.to_vec())); } - #[cfg(not(feature = "sapling"))] - unified::Ivk::Sapling(data) => Some(Ok::<_, UnifiedError>(( - u32::from(unified::Typecode::Sapling), - data.to_vec(), - ))), - #[cfg(feature = "transparent-inputs")] - unified::Ivk::P2pkh(data) => legacy::ExternalIvk::deserialize(data) - .map_err(UnifiedError::InvalidTransparentKey) - .map(|tivk| { - transparent = Some(tivk); - None - }) - .transpose(), - #[cfg(not(feature = "transparent-inputs"))] - unified::Ivk::P2pkh(data) => Some(Ok::<_, UnifiedError>(( - u32::from(unified::Typecode::P2pkh), - data.to_vec(), - ))), - unified::Ivk::Unknown { typecode, data } => Some(Ok((*typecode, data.clone()))), - }) - .collect::>()?; + unified::Ivk::P2pkh(data) => { + #[cfg(feature = "transparent-inputs")] + { + transparent = Some( + legacy::ExternalIvk::deserialize(data) + .map_err(UnifiedError::InvalidTransparentKey)?, + ); + } + + #[cfg(not(feature = "transparent-inputs"))] + unknown.push((u32::from(unified::Typecode::P2pkh), data.to_vec())); + } + unified::Ivk::Unknown { typecode, data } => { + unknown.push((*typecode, data.clone())); + } + } + } Ok(Self { #[cfg(feature = "transparent-inputs")] From c99338a7a134483dcff735a3d6a8c7bfc66d9b6d Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Wed, 13 Mar 2024 18:59:37 -0600 Subject: [PATCH 03/22] Merge new error type into existing one --- zcash_client_sqlite/src/wallet.rs | 23 +++---- zcash_keys/CHANGELOG.md | 5 +- zcash_keys/src/keys.rs | 105 ++++++++++++------------------ 3 files changed, 54 insertions(+), 79 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index c52a47b39..22ba9c066 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -298,14 +298,13 @@ struct AccountSqlValues<'a> { hd_seed_fingerprint: Option<&'a [u8]>, hd_account_index: Option, ufvk: Option<&'a UnifiedFullViewingKey>, - uivk: String, + uivk: UnifiedIncomingViewingKey, } /// Returns (account_type, hd_seed_fingerprint, hd_account_index, ufvk, uivk) for a given account. -fn get_sql_values_for_account_parameters<'a, P: consensus::Parameters>( - account: &'a Account, - params: &P, -) -> Result, SqliteClientError> { +fn get_sql_values_for_account_parameters( + account: &Account, +) -> Result { Ok(match account { Account::Zip32(hdaccount) => AccountSqlValues { account_type: AccountType::Zip32.into(), @@ -315,9 +314,7 @@ fn get_sql_values_for_account_parameters<'a, P: consensus::Parameters>( uivk: hdaccount .ufvk() .to_unified_incoming_viewing_key() - .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))? - .to_uivk() - .encode(¶ms.network_type()), + .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))?, }, Account::Imported(ImportedAccount::Full(ufvk)) => AccountSqlValues { account_type: AccountType::Imported.into(), @@ -326,16 +323,14 @@ fn get_sql_values_for_account_parameters<'a, P: consensus::Parameters>( ufvk: Some(ufvk), uivk: ufvk .to_unified_incoming_viewing_key() - .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))? - .to_uivk() - .encode(¶ms.network_type()), + .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))?, }, Account::Imported(ImportedAccount::Incoming(uivk)) => AccountSqlValues { account_type: AccountType::Imported.into(), hd_seed_fingerprint: None, hd_account_index: None, ufvk: None, - uivk: uivk.to_uivk().encode(¶ms.network_type()), + uivk: (**uivk).clone(), }, }) } @@ -346,7 +341,7 @@ pub(crate) fn add_account( account: Account, birthday: AccountBirthday, ) -> Result { - let args = get_sql_values_for_account_parameters(&account, params)?; + let args = get_sql_values_for_account_parameters(&account)?; let orchard_item = args .ufvk @@ -382,7 +377,7 @@ pub(crate) fn add_account( ":hd_seed_fingerprint": args.hd_seed_fingerprint, ":hd_account_index": args.hd_account_index, ":ufvk": args.ufvk.map(|ufvk| ufvk.encode(params)), - ":uivk": args.uivk, + ":uivk": args.uivk.to_uivk().encode(¶ms.network_type()), ":orchard_fvk_item_cache": orchard_item, ":sapling_fvk_item_cache": sapling_item, ":p2pkh_fvk_item_cache": transparent_item, diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 08a1f9f57..65763612d 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -11,7 +11,6 @@ and this library adheres to Rust's notion of - `zcash_keys::address::Address::has_receiver` - `impl Display for zcash_keys::keys::AddressGenerationError` - `impl std::error::Error for zcash_keys::keys::AddressGenerationError` -- `zcash_keys::keys::UnifiedError` - `zcash_keys::keys::UnifiedFullViewingKey::from_ufvk` - `zcash_keys::keys::UnifiedFullViewingKey::to_ufvk` - `zcash_keys::keys::UnifiedFullViewingKey::to_unified_incoming_viewing_key` @@ -25,8 +24,8 @@ and this library adheres to Rust's notion of (instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`). - `zcash_keys::keys::AddressGenerationError` - Dropped `Clone` trait - - Added `UnifiedError` variant. - - Added `HDWalletError` variant. +- `zcash_keys::keys::DerivationError` + - Added `InvalidShieldedKey` variant. ### Fixed - `UnifiedFullViewingKey::find_address` can now find an address for a diversifier diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 9bffc6be0..c6d389c37 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -1,7 +1,10 @@ //! Helper functions for managing light client key material. use blake2b_simd::Params as blake2bParams; use secrecy::{ExposeSecret, SecretVec}; -use std::{error, fmt}; +use std::{ + error, + fmt::{self, Display}, +}; use zcash_address::unified::{self, Container, Encoding, Typecode, Ufvk, Uivk}; use zcash_protocol::{consensus, ShieldedProtocol}; @@ -138,12 +141,27 @@ fn to_transparent_child_index(j: DiversifierIndex) -> Option) -> fmt::Result { + match self { + DerivationError::InvalidShieldedKey(protocol) => { + write!(f, "Invalid shielded key for protocol {:?}", protocol) + } + #[cfg(feature = "orchard")] + DerivationError::Orchard(e) => write!(f, "Orchard error: {}", e), + #[cfg(feature = "transparent-inputs")] + DerivationError::Transparent(e) => write!(f, "Transparent error: {}", e), + } + } +} + /// A version identifier for the encoding of unified spending keys. /// /// Each era corresponds to a range of block heights. During an era, the unified spending key @@ -498,23 +516,19 @@ pub enum AddressGenerationError { ShieldedReceiverRequired, // An error occurred while deriving a key or address from an HD wallet. - UnifiedError(UnifiedError), - - // An error occurred while deriving a transparent key or address from an HD wallet. - #[cfg(feature = "transparent-inputs")] - HDWalletError(hdwallet::error::Error), -} - -impl From for AddressGenerationError { - fn from(e: UnifiedError) -> Self { - AddressGenerationError::UnifiedError(e) - } + Derivation(DerivationError), } #[cfg(feature = "transparent-inputs")] impl From for AddressGenerationError { fn from(e: hdwallet::error::Error) -> Self { - AddressGenerationError::HDWalletError(e) + AddressGenerationError::Derivation(DerivationError::Transparent(e)) + } +} + +impl From for AddressGenerationError { + fn from(e: DerivationError) -> Self { + AddressGenerationError::Derivation(e) } } @@ -560,13 +574,7 @@ impl fmt::Display for AddressGenerationError { AddressGenerationError::ShieldedReceiverRequired => { write!(f, "A Unified Address requires at least one shielded (Sapling or Orchard) receiver.") } - AddressGenerationError::UnifiedError(e) => { - write!(f, "UnifiedError: {}", e) - } - #[cfg(feature = "transparent-inputs")] - AddressGenerationError::HDWalletError(e) => { - write!(f, "HDWalletError: {}", e) - } + AddressGenerationError::Derivation(e) => write!(f, "Error deriving address: {}", e), } } } @@ -633,38 +641,10 @@ impl UnifiedAddressRequest { } } -/// Errors that may be returned from the [UnifiedFullViewingKey] and [UnifiedIncomingViewingKey] types. -#[derive(Debug)] -pub enum UnifiedError { - InvalidShieldedKey(ShieldedProtocol), - #[cfg(feature = "transparent-inputs")] - InvalidTransparentKey(hdwallet::error::Error), - #[cfg(feature = "transparent-inputs")] - HDWallet(hdwallet::error::Error), -} - #[cfg(feature = "transparent-inputs")] -impl From for UnifiedError { +impl From for DerivationError { fn from(e: hdwallet::error::Error) -> Self { - UnifiedError::HDWallet(e) - } -} - -impl fmt::Display for UnifiedError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match &self { - UnifiedError::InvalidShieldedKey(p) => { - write!(f, "Invalid key for shielded protocol: {:?}", p) - } - #[cfg(feature = "transparent-inputs")] - UnifiedError::InvalidTransparentKey(e) => { - write!(f, "Invalid key for transparent protocol: {:?}", e) - } - #[cfg(feature = "transparent-inputs")] - UnifiedError::HDWallet(e) => { - write!(f, "HDWallet error: {}", e) - } - } + DerivationError::Transparent(e) } } @@ -734,7 +714,7 @@ impl UnifiedFullViewingKey { /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. /// /// [ZIP 316]: https://zips.z.cash/zip-0316 - pub fn from_ufvk(ufvk: &Ufvk) -> Result { + pub fn from_ufvk(ufvk: &Ufvk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -750,7 +730,9 @@ impl UnifiedFullViewingKey { .filter_map(|receiver| match receiver { #[cfg(feature = "orchard")] unified::Fvk::Orchard(data) => orchard::keys::FullViewingKey::from_bytes(data) - .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Orchard)) + .ok_or(DerivationError::InvalidShieldedKey( + ShieldedProtocol::Orchard, + )) .map(|addr| { orchard = Some(addr); None @@ -764,7 +746,9 @@ impl UnifiedFullViewingKey { #[cfg(feature = "sapling")] unified::Fvk::Sapling(data) => { sapling::DiversifiableFullViewingKey::from_bytes(data) - .ok_or(UnifiedError::InvalidShieldedKey(ShieldedProtocol::Sapling)) + .ok_or(DerivationError::InvalidShieldedKey( + ShieldedProtocol::Sapling, + )) .map(|pa| { sapling = Some(pa); None @@ -778,7 +762,7 @@ impl UnifiedFullViewingKey { ))), #[cfg(feature = "transparent-inputs")] unified::Fvk::P2pkh(data) => legacy::AccountPubKey::deserialize(data) - .map_err(UnifiedError::InvalidTransparentKey) + .map_err(DerivationError::Transparent) .map(|tfvk| { transparent = Some(tfvk); None @@ -846,7 +830,7 @@ impl UnifiedFullViewingKey { /// Derives a Unified Incoming Viewing Key from this Unified Full Viewing Key. pub fn to_unified_incoming_viewing_key( &self, - ) -> Result { + ) -> Result { Ok(UnifiedIncomingViewingKey { #[cfg(feature = "transparent-inputs")] transparent: self @@ -972,7 +956,7 @@ impl UnifiedIncomingViewingKey { } /// Constructs a unified incoming viewing key from a parsed unified encoding. - pub fn from_uivk(uivk: &Uivk) -> Result { + pub fn from_uivk(uivk: &Uivk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -991,7 +975,7 @@ impl UnifiedIncomingViewingKey { { orchard = Some( Option::from(orchard::keys::IncomingViewingKey::from_bytes(data)) - .ok_or(UnifiedError::InvalidShieldedKey( + .ok_or(DerivationError::InvalidShieldedKey( ShieldedProtocol::Orchard, ))?, ); @@ -1005,7 +989,7 @@ impl UnifiedIncomingViewingKey { { sapling = Some( Option::from(::sapling::zip32::IncomingViewingKey::from_bytes(data)) - .ok_or(UnifiedError::InvalidShieldedKey( + .ok_or(DerivationError::InvalidShieldedKey( ShieldedProtocol::Sapling, ))?, ); @@ -1017,10 +1001,7 @@ impl UnifiedIncomingViewingKey { unified::Ivk::P2pkh(data) => { #[cfg(feature = "transparent-inputs")] { - transparent = Some( - legacy::ExternalIvk::deserialize(data) - .map_err(UnifiedError::InvalidTransparentKey)?, - ); + transparent = Some(legacy::ExternalIvk::deserialize(data)?); } #[cfg(not(feature = "transparent-inputs"))] From 9d6a8b69417a15a7339cf70b9e448318a82f8053 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 13 Mar 2024 20:14:43 -0600 Subject: [PATCH 04/22] zcash_keys: Use `DecodingError` instead of `DerivationError` for key parsing. --- zcash_client_sqlite/src/wallet.rs | 3 +- zcash_keys/CHANGELOG.md | 7 ++- zcash_keys/src/keys.rs | 81 ++++++++++++++++++------------- 3 files changed, 52 insertions(+), 39 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index bdcdfddb8..5f11ebb9b 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -74,7 +74,7 @@ use std::io::{self, Cursor}; use std::num::NonZeroU32; use std::ops::RangeInclusive; use tracing::debug; -use zcash_address::unified::{Encoding, Ivk, Uivk}; +use zcash_address::unified::{Encoding, Uivk}; use zcash_keys::keys::{ AddressGenerationError, HdSeedFingerprint, UnifiedAddressRequest, UnifiedIncomingViewingKey, }; @@ -120,6 +120,7 @@ use { crate::UtxoId, rusqlite::Row, std::collections::BTreeSet, + zcash_address::unified::Ivk, zcash_client_backend::wallet::{TransparentAddressMetadata, WalletTransparentOutput}, zcash_primitives::{ legacy::{ diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 65763612d..e29d0fe90 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -11,21 +11,20 @@ and this library adheres to Rust's notion of - `zcash_keys::address::Address::has_receiver` - `impl Display for zcash_keys::keys::AddressGenerationError` - `impl std::error::Error for zcash_keys::keys::AddressGenerationError` +- `zcash_keys::keys::DecodingError` - `zcash_keys::keys::UnifiedFullViewingKey::from_ufvk` - `zcash_keys::keys::UnifiedFullViewingKey::to_ufvk` - `zcash_keys::keys::UnifiedFullViewingKey::to_unified_incoming_viewing_key` - `zcash_keys::keys::UnifiedIncomingViewingKey` ### Changed -- `zcash_keys::keys::AddressGenerationError` has a new variant - `DiversifierSpaceExhausted`. - `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}` now return `Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError>` (instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`). - `zcash_keys::keys::AddressGenerationError` - Dropped `Clone` trait -- `zcash_keys::keys::DerivationError` - - Added `InvalidShieldedKey` variant. + - Added `KeyDecoding` variant. + - Added `DiversifierSpaceExhausted` variant. ### Fixed - `UnifiedFullViewingKey::find_address` can now find an address for a diversifier diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 350ae858d..0120d6167 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -7,7 +7,7 @@ use std::{ }; use zcash_address::unified::{self, Container, Encoding, Typecode, Ufvk, Uivk}; -use zcash_protocol::{consensus, ShieldedProtocol}; +use zcash_protocol::consensus; use zip32::{AccountId, DiversifierIndex}; use crate::address::UnifiedAddress; @@ -139,9 +139,7 @@ fn to_transparent_child_index(j: DiversifierIndex) -> Option) -> fmt::Result { match self { - DerivationError::InvalidShieldedKey(protocol) => { - write!(f, "Invalid shielded key for protocol {:?}", protocol) - } #[cfg(feature = "orchard")] DerivationError::Orchard(e) => write!(f, "Orchard error: {}", e), #[cfg(feature = "transparent-inputs")] @@ -177,28 +172,40 @@ pub enum Era { } /// A type for errors that can occur when decoding keys from their serialized representations. -#[cfg(feature = "unstable")] #[derive(Debug, PartialEq, Eq)] pub enum DecodingError { + #[cfg(feature = "unstable")] ReadError(&'static str), + #[cfg(feature = "unstable")] EraInvalid, + #[cfg(feature = "unstable")] EraMismatch(Era), + #[cfg(feature = "unstable")] TypecodeInvalid, + #[cfg(feature = "unstable")] LengthInvalid, + #[cfg(feature = "unstable")] LengthMismatch(Typecode, u32), + #[cfg(feature = "unstable")] InsufficientData(Typecode), + /// The key data could not be decoded from its string representation to a valid key. KeyDataInvalid(Typecode), } -#[cfg(feature = "unstable")] impl std::fmt::Display for DecodingError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { + #[cfg(feature = "unstable")] DecodingError::ReadError(s) => write!(f, "Read error: {}", s), + #[cfg(feature = "unstable")] DecodingError::EraInvalid => write!(f, "Invalid era"), + #[cfg(feature = "unstable")] DecodingError::EraMismatch(e) => write!(f, "Era mismatch: actual {:?}", e), + #[cfg(feature = "unstable")] DecodingError::TypecodeInvalid => write!(f, "Invalid typecode"), + #[cfg(feature = "unstable")] DecodingError::LengthInvalid => write!(f, "Invalid length"), + #[cfg(feature = "unstable")] DecodingError::LengthMismatch(t, l) => { write!( f, @@ -206,10 +213,11 @@ impl std::fmt::Display for DecodingError { l, t ) } + #[cfg(feature = "unstable")] DecodingError::InsufficientData(t) => { write!(f, "Insufficient data for typecode {:?}", t) } - DecodingError::KeyDataInvalid(t) => write!(f, "Invalid key data for typecode {:?}", t), + DecodingError::KeyDataInvalid(t) => write!(f, "Invalid key data for key type {:?}", t), } } } @@ -514,8 +522,10 @@ pub enum AddressGenerationError { /// A Unified address cannot be generated without at least one shielded receiver being /// included. ShieldedReceiverRequired, - // An error occurred while deriving a key or address from an HD wallet. + /// An error occurred while deriving a key or address from an HD wallet. Derivation(DerivationError), + /// An error occurred while decoding a key from its encoded representation. + KeyDecoding(DecodingError), } #[cfg(feature = "transparent-inputs")] @@ -531,6 +541,12 @@ impl From for AddressGenerationError { } } +impl From for AddressGenerationError { + fn from(e: DecodingError) -> Self { + AddressGenerationError::KeyDecoding(e) + } +} + impl fmt::Display for AddressGenerationError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { @@ -574,6 +590,11 @@ impl fmt::Display for AddressGenerationError { write!(f, "A Unified Address requires at least one shielded (Sapling or Orchard) receiver.") } AddressGenerationError::Derivation(e) => write!(f, "Error deriving address: {}", e), + AddressGenerationError::KeyDecoding(e) => write!( + f, + "Error decoding key from its serialized representation: {}.", + e + ), } } } @@ -713,7 +734,7 @@ impl UnifiedFullViewingKey { /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. /// /// [ZIP 316]: https://zips.z.cash/zip-0316 - pub fn from_ufvk(ufvk: &Ufvk) -> Result { + pub fn from_ufvk(ufvk: &Ufvk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -729,25 +750,21 @@ impl UnifiedFullViewingKey { .filter_map(|receiver| match receiver { #[cfg(feature = "orchard")] unified::Fvk::Orchard(data) => orchard::keys::FullViewingKey::from_bytes(data) - .ok_or(DerivationError::InvalidShieldedKey( - ShieldedProtocol::Orchard, - )) + .ok_or(DecodingError::KeyDataInvalid(Typecode::Orchard)) .map(|addr| { orchard = Some(addr); None }) .transpose(), #[cfg(not(feature = "orchard"))] - unified::Fvk::Orchard(data) => Some(Ok::<_, DerivationError>(( + unified::Fvk::Orchard(data) => Some(Ok::<_, DecodingError>(( u32::from(unified::Typecode::Orchard), data.to_vec(), ))), #[cfg(feature = "sapling")] unified::Fvk::Sapling(data) => { sapling::DiversifiableFullViewingKey::from_bytes(data) - .ok_or(DerivationError::InvalidShieldedKey( - ShieldedProtocol::Sapling, - )) + .ok_or(DecodingError::KeyDataInvalid(Typecode::Sapling)) .map(|pa| { sapling = Some(pa); None @@ -755,20 +772,20 @@ impl UnifiedFullViewingKey { .transpose() } #[cfg(not(feature = "sapling"))] - unified::Fvk::Sapling(data) => Some(Ok::<_, DerivationError>(( + unified::Fvk::Sapling(data) => Some(Ok::<_, DecodingError>(( u32::from(unified::Typecode::Sapling), data.to_vec(), ))), #[cfg(feature = "transparent-inputs")] unified::Fvk::P2pkh(data) => legacy::AccountPubKey::deserialize(data) - .map_err(DerivationError::Transparent) + .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2pkh)) .map(|tfvk| { transparent = Some(tfvk); None }) .transpose(), #[cfg(not(feature = "transparent-inputs"))] - unified::Fvk::P2pkh(data) => Some(Ok::<_, DerivationError>(( + unified::Fvk::P2pkh(data) => Some(Ok::<_, DecodingError>(( u32::from(unified::Typecode::P2pkh), data.to_vec(), ))), @@ -955,7 +972,7 @@ impl UnifiedIncomingViewingKey { } /// Constructs a unified incoming viewing key from a parsed unified encoding. - pub fn from_uivk(uivk: &Uivk) -> Result { + pub fn from_uivk(uivk: &Uivk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -974,9 +991,7 @@ impl UnifiedIncomingViewingKey { { orchard = Some( Option::from(orchard::keys::IncomingViewingKey::from_bytes(data)) - .ok_or(DerivationError::InvalidShieldedKey( - ShieldedProtocol::Orchard, - ))?, + .ok_or(DecodingError::KeyDataInvalid(Typecode::Orchard))?, ); } @@ -988,9 +1003,7 @@ impl UnifiedIncomingViewingKey { { sapling = Some( Option::from(::sapling::zip32::IncomingViewingKey::from_bytes(data)) - .ok_or(DerivationError::InvalidShieldedKey( - ShieldedProtocol::Sapling, - ))?, + .ok_or(DecodingError::KeyDataInvalid(Typecode::Sapling))?, ); } @@ -1000,7 +1013,10 @@ impl UnifiedIncomingViewingKey { unified::Ivk::P2pkh(data) => { #[cfg(feature = "transparent-inputs")] { - transparent = Some(legacy::ExternalIvk::deserialize(data)?); + transparent = Some( + legacy::ExternalIvk::deserialize(data) + .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2pkh))?, + ); } #[cfg(not(feature = "transparent-inputs"))] @@ -1271,10 +1287,7 @@ mod tests { }; #[cfg(feature = "unstable")] - use { - super::{testing::arb_unified_spending_key, Era, UnifiedSpendingKey}, - zcash_primitives::consensus::Network, - }; + use super::{testing::arb_unified_spending_key, Era, UnifiedSpendingKey}; #[cfg(all(feature = "orchard", feature = "unstable"))] use subtle::ConstantTimeEq; @@ -1551,7 +1564,7 @@ mod tests { let decoded = UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(&encoded).unwrap().1).unwrap(); - let reencoded = decoded.to_uivk().encode(&Network::Main); + let reencoded = decoded.to_uivk().encode(&NetworkType::Main); assert_eq!(encoded, reencoded); #[cfg(feature = "transparent-inputs")] From 4d9927b993b10ae407952efd1bcbc423eb532b22 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 14 Mar 2024 08:27:49 -0600 Subject: [PATCH 05/22] zcash_keys: Verify the ability to derive addresses at USK and UFVK construction. --- zcash_client_sqlite/src/wallet.rs | 10 +- .../init/migrations/full_account_ids.rs | 1 - zcash_keys/CHANGELOG.md | 11 +- zcash_keys/src/keys.rs | 229 +++++++++--------- 4 files changed, 122 insertions(+), 129 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 5f11ebb9b..e73d6dd71 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -235,12 +235,10 @@ impl ViewingKey { } } - fn uivk(&self) -> Result { + fn uivk(&self) -> UnifiedIncomingViewingKey { match self { - ViewingKey::Full(ufvk) => Ok(ufvk - .to_unified_incoming_viewing_key() - .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))?), - ViewingKey::Incoming(uivk) => Ok((**uivk).to_owned()), + ViewingKey::Full(ufvk) => ufvk.as_ref().to_unified_incoming_viewing_key(), + ViewingKey::Incoming(uivk) => uivk.as_ref().clone(), } } } @@ -349,7 +347,7 @@ pub(crate) fn add_account( ":hd_seed_fingerprint": hd_seed_fingerprint.as_ref().map(|fp| fp.as_bytes()), ":hd_account_index": hd_account_index.map(u32::from), ":ufvk": viewing_key.ufvk().map(|ufvk| ufvk.encode(params)), - ":uivk": viewing_key.uivk()?.to_uivk().encode(¶ms.network_type()), + ":uivk": viewing_key.uivk().to_uivk().encode(¶ms.network_type()), ":orchard_fvk_item_cache": orchard_item, ":sapling_fvk_item_cache": sapling_item, ":p2pkh_fvk_item_cache": transparent_item, 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 089f799e9..07689421d 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 @@ -122,7 +122,6 @@ impl RusqliteMigration for Migration

{ let uivk = ufvk_parsed .to_unified_incoming_viewing_key() - .map_err(|e| WalletMigrationError::CorruptedData(e.to_string()))? .to_uivk() .encode(&self.params.network_type()); diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index e29d0fe90..93c627aa6 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -18,14 +18,17 @@ and this library adheres to Rust's notion of - `zcash_keys::keys::UnifiedIncomingViewingKey` ### Changed -- `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}` +- `zcash_keys::keys::UnifiedFullViewingKey::{find_address, default_address}` now return `Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError>` (instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`). - `zcash_keys::keys::AddressGenerationError` - - Dropped `Clone` trait - - Added `KeyDecoding` variant. - Added `DiversifierSpaceExhausted` variant. +### Removed +- `UnifiedFullViewingKey::new` has been placed behind the `test-dependencies` + feature flag. UFVKs should only be produced by derivation from the USK, or + parsed from their string representation. + ### Fixed - `UnifiedFullViewingKey::find_address` can now find an address for a diversifier index outside the valid transparent range if you aren't requesting a @@ -37,7 +40,7 @@ and this library adheres to Rust's notion of - `zcash_keys::keys::UnifiedAddressRequest::all` ### Fixed -- A missing application of the `sapling` feature flag was remedied; +- A missing application of the `sapling` feature flag was remedied; prior to this fix it was not possible to use this crate without the `sapling` feature enabled. diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 0120d6167..a5dbbe368 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -7,7 +7,7 @@ use std::{ }; use zcash_address::unified::{self, Container, Encoding, Typecode, Ufvk, Uivk}; -use zcash_protocol::consensus; +use zcash_protocol::consensus::{self, NetworkConstants}; use zip32::{AccountId, DiversifierIndex}; use crate::address::UnifiedAddress; @@ -18,10 +18,6 @@ use { zcash_primitives::legacy::keys::{self as legacy, IncomingViewingKey, NonHardenedChildIndex}, }; -#[cfg(any(feature = "sapling", feature = "orchard"))] -// Your code here -use zcash_protocol::consensus::NetworkConstants; - #[cfg(all( feature = "transparent-inputs", any(test, feature = "test-dependencies") @@ -264,19 +260,37 @@ impl UnifiedSpendingKey { panic!("ZIP 32 seeds MUST be at least 32 bytes"); } - Ok(UnifiedSpendingKey { + UnifiedSpendingKey::from_checked_parts( #[cfg(feature = "transparent-inputs")] - transparent: legacy::AccountPrivKey::from_seed(_params, seed, _account) + legacy::AccountPrivKey::from_seed(_params, seed, _account) .map_err(DerivationError::Transparent)?, #[cfg(feature = "sapling")] - sapling: sapling::spending_key(seed, _params.coin_type(), _account), + sapling::spending_key(seed, _params.coin_type(), _account), #[cfg(feature = "orchard")] - orchard: orchard::keys::SpendingKey::from_zip32_seed( - seed, - _params.coin_type(), - _account, - ) - .map_err(DerivationError::Orchard)?, + orchard::keys::SpendingKey::from_zip32_seed(seed, _params.coin_type(), _account) + .map_err(DerivationError::Orchard)?, + ) + } + + /// Construct a USK from its constituent parts, after verifying that UIVK derivation can + /// succeed. + fn from_checked_parts( + #[cfg(feature = "transparent-inputs")] transparent: legacy::AccountPrivKey, + #[cfg(feature = "sapling")] sapling: sapling::ExtendedSpendingKey, + #[cfg(feature = "orchard")] orchard: orchard::keys::SpendingKey, + ) -> Result { + // Verify that FVK and IVK derivation succeed; we don't want to construct a USK + // that can't derive transparent addresses. + #[cfg(feature = "transparent-inputs")] + let _ = transparent.to_account_pubkey().derive_external_ivk()?; + + Ok(UnifiedSpendingKey { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, }) } @@ -466,14 +480,15 @@ impl UnifiedSpendingKey { let has_transparent = true; if has_orchard && has_sapling && has_transparent { - return Ok(UnifiedSpendingKey { - #[cfg(feature = "orchard")] - orchard: orchard.unwrap(), - #[cfg(feature = "sapling")] - sapling: sapling.unwrap(), + return UnifiedSpendingKey::from_checked_parts( #[cfg(feature = "transparent-inputs")] - transparent: transparent.unwrap(), - }); + transparent.unwrap(), + #[cfg(feature = "sapling")] + sapling.unwrap(), + #[cfg(feature = "orchard")] + orchard.unwrap(), + ) + .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2pkh)); } } } @@ -502,7 +517,7 @@ impl UnifiedSpendingKey { } /// Errors that can occur in the generation of unified addresses. -#[derive(Debug)] +#[derive(Clone, Debug)] pub enum AddressGenerationError { /// The requested diversifier index was outside the range of valid transparent /// child address indices. @@ -522,29 +537,6 @@ pub enum AddressGenerationError { /// A Unified address cannot be generated without at least one shielded receiver being /// included. ShieldedReceiverRequired, - /// An error occurred while deriving a key or address from an HD wallet. - Derivation(DerivationError), - /// An error occurred while decoding a key from its encoded representation. - KeyDecoding(DecodingError), -} - -#[cfg(feature = "transparent-inputs")] -impl From for AddressGenerationError { - fn from(e: hdwallet::error::Error) -> Self { - AddressGenerationError::Derivation(DerivationError::Transparent(e)) - } -} - -impl From for AddressGenerationError { - fn from(e: DerivationError) -> Self { - AddressGenerationError::Derivation(e) - } -} - -impl From for AddressGenerationError { - fn from(e: DecodingError) -> Self { - AddressGenerationError::KeyDecoding(e) - } } impl fmt::Display for AddressGenerationError { @@ -589,12 +581,6 @@ impl fmt::Display for AddressGenerationError { AddressGenerationError::ShieldedReceiverRequired => { write!(f, "A Unified Address requires at least one shielded (Sapling or Orchard) receiver.") } - AddressGenerationError::Derivation(e) => write!(f, "Error deriving address: {}", e), - AddressGenerationError::KeyDecoding(e) => write!( - f, - "Error decoding key from its serialized representation: {}.", - e - ), } } } @@ -682,37 +668,56 @@ pub struct UnifiedFullViewingKey { #[doc(hidden)] impl UnifiedFullViewingKey { - /// Construct a new unified full viewing key, if the required components are present. + /// Construct a new unified full viewing key. + /// + /// This method is only available when the `test-dependencies` feature is enabled, + /// as derivation from the USK or deserialization from the serialized form should + /// be used instead. + #[cfg(any(test, feature = "test-dependencies"))] pub fn new( #[cfg(feature = "transparent-inputs")] transparent: Option, #[cfg(feature = "sapling")] sapling: Option, #[cfg(feature = "orchard")] orchard: Option, // TODO: Implement construction of UFVKs with metadata items. - ) -> Option { - #[cfg(feature = "orchard")] - let has_orchard = orchard.is_some(); - #[cfg(not(feature = "orchard"))] - let has_orchard = false; - #[cfg(feature = "sapling")] - let has_sapling = sapling.is_some(); - #[cfg(not(feature = "sapling"))] - let has_sapling = false; + ) -> Result { + Self::from_checked_parts( + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + // We don't currently allow constructing new UFVKs with unknown items, but we store + // this to allow parsing such UFVKs. + vec![], + ) + } - if has_orchard || has_sapling { - Some(UnifiedFullViewingKey { - #[cfg(feature = "transparent-inputs")] - transparent, - #[cfg(feature = "sapling")] - sapling, - #[cfg(feature = "orchard")] - orchard, - // We don't allow constructing new UFVKs with unknown items, but we store - // this to allow parsing such UFVKs. - unknown: vec![], - }) - } else { - None - } + /// Construct a UFVK from its constituent parts, after verifying that UIVK derivation can + /// succeed. + fn from_checked_parts( + #[cfg(feature = "transparent-inputs")] transparent: Option, + #[cfg(feature = "sapling")] sapling: Option, + #[cfg(feature = "orchard")] orchard: Option, + unknown: Vec<(u32, Vec)>, + ) -> Result { + // Verify that IVK derivation succeeds; we don't want to construct a UFVK + // that can't derive transparent addresses. + #[cfg(feature = "transparent-inputs")] + let _ = transparent + .as_ref() + .map(|t| t.derive_external_ivk()) + .transpose()?; + + Ok(UnifiedFullViewingKey { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + unknown, + }) } /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. @@ -793,7 +798,7 @@ impl UnifiedFullViewingKey { }) .collect::>()?; - Ok(Self { + Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] transparent, #[cfg(feature = "sapling")] @@ -801,7 +806,8 @@ impl UnifiedFullViewingKey { #[cfg(feature = "orchard")] orchard, unknown, - }) + ) + .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2pkh)) } /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. @@ -844,22 +850,19 @@ impl UnifiedFullViewingKey { } /// Derives a Unified Incoming Viewing Key from this Unified Full Viewing Key. - pub fn to_unified_incoming_viewing_key( - &self, - ) -> Result { - Ok(UnifiedIncomingViewingKey { + pub fn to_unified_incoming_viewing_key(&self) -> UnifiedIncomingViewingKey { + UnifiedIncomingViewingKey { #[cfg(feature = "transparent-inputs")] - transparent: self - .transparent - .as_ref() - .map(|t| t.derive_external_ivk()) - .transpose()?, + transparent: self.transparent.as_ref().map(|t| { + t.derive_external_ivk() + .expect("Transparent IVK derivation was checked at construction.") + }), #[cfg(feature = "sapling")] sapling: self.sapling.as_ref().map(|s| s.to_external_ivk()), #[cfg(feature = "orchard")] orchard: self.orchard.as_ref().map(|o| o.to_ivk(Scope::External)), unknown: Vec::new(), - }) + } } /// Returns the transparent component of the unified key at the @@ -890,7 +893,7 @@ impl UnifiedFullViewingKey { j: DiversifierIndex, request: UnifiedAddressRequest, ) -> Result { - self.to_unified_incoming_viewing_key()?.address(j, request) + self.to_unified_incoming_viewing_key().address(j, request) } /// Searches the diversifier space starting at diversifier index `j` for one which will @@ -905,7 +908,7 @@ impl UnifiedFullViewingKey { mut j: DiversifierIndex, request: UnifiedAddressRequest, ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { - self.to_unified_incoming_viewing_key()? + self.to_unified_incoming_viewing_key() .find_address(j, request) } @@ -936,7 +939,12 @@ pub struct UnifiedIncomingViewingKey { } impl UnifiedIncomingViewingKey { - /// Construct a new unified incoming viewing key, if the required components are present. + /// Construct a new unified incoming viewing key. + /// + /// This method is only available when the `test-dependencies` feature is enabled, + /// as derivation from the UFVK or deserialization from the serialized form should + /// be used instead. + #[cfg(any(test, feature = "test-dependencies"))] pub fn new( #[cfg(feature = "transparent-inputs")] transparent: Option< zcash_primitives::legacy::keys::ExternalIvk, @@ -944,30 +952,17 @@ impl UnifiedIncomingViewingKey { #[cfg(feature = "sapling")] sapling: Option<::sapling::zip32::IncomingViewingKey>, #[cfg(feature = "orchard")] orchard: Option, // TODO: Implement construction of UIVKs with metadata items. - ) -> Option { - #[cfg(feature = "orchard")] - let has_orchard = orchard.is_some(); - #[cfg(not(feature = "orchard"))] - let has_orchard = false; - #[cfg(feature = "sapling")] - let has_sapling = sapling.is_some(); - #[cfg(not(feature = "sapling"))] - let has_sapling = false; - - if has_orchard || has_sapling { - Some(UnifiedIncomingViewingKey { - #[cfg(feature = "transparent-inputs")] - transparent, - #[cfg(feature = "sapling")] - sapling, - #[cfg(feature = "orchard")] - orchard, - // We don't allow constructing new UFVKs with unknown items, but we store - // this to allow parsing such UFVKs. - unknown: vec![], - }) - } else { - None + ) -> UnifiedIncomingViewingKey { + UnifiedIncomingViewingKey { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + // We don't allow constructing new UFVKs with unknown items, but we store + // this to allow parsing such UFVKs. + unknown: vec![], } } @@ -1543,7 +1538,6 @@ mod tests { #[cfg(any(feature = "orchard", feature = "sapling"))] { - let uivk = uivk.expect("Orchard or Sapling ivk is present."); let encoded = uivk.to_uivk().encode(&NetworkType::Main); // Test encoded form against known values; these test vectors contain Orchard receivers @@ -1654,8 +1648,7 @@ mod tests { let d_idx = DiversifierIndex::from(tv.diversifier_index); let uivk = usk .to_unified_full_viewing_key() - .to_unified_incoming_viewing_key() - .unwrap(); + .to_unified_incoming_viewing_key(); // The test vectors contain some diversifier indices that do not generate // valid Sapling addresses, so skip those. From b4171caaf43514b5a51d94c4b6b764a70bbec805 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 14 Mar 2024 11:59:58 -0600 Subject: [PATCH 06/22] zcash_keys: Fix no-default-features compilation. --- zcash_keys/CHANGELOG.md | 2 + zcash_keys/src/address.rs | 9 +- zcash_keys/src/keys.rs | 406 +++++++++++++++++++------------------- zcash_keys/src/lib.rs | 6 + 4 files changed, 214 insertions(+), 209 deletions(-) diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 93c627aa6..500318415 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -23,6 +23,8 @@ and this library adheres to Rust's notion of (instead of `Option<(UnifiedAddress, DiversifierIndex)>` for `find_address`). - `zcash_keys::keys::AddressGenerationError` - Added `DiversifierSpaceExhausted` variant. +- At least one of the `orchard`, `sapling`, or `transparent-inputs` features + must be enabled for the `keys` module to be accessible. ### Removed - `UnifiedFullViewingKey::new` has been placed behind the `test-dependencies` diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs index dd1fb9ac4..c26c64a84 100644 --- a/zcash_keys/src/address.rs +++ b/zcash_keys/src/address.rs @@ -342,7 +342,14 @@ impl Address { } } -#[cfg(any(test, feature = "test-dependencies"))] +#[cfg(all( + any( + feature = "orchard", + feature = "sapling", + feature = "transparent-inputs" + ), + any(test, feature = "test-dependencies") +))] pub mod testing { use proptest::prelude::*; use zcash_primitives::consensus::Network; diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index a5dbbe368..aac28cda3 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -7,11 +7,14 @@ use std::{ }; use zcash_address::unified::{self, Container, Encoding, Typecode, Ufvk, Uivk}; -use zcash_protocol::consensus::{self, NetworkConstants}; +use zcash_protocol::consensus; use zip32::{AccountId, DiversifierIndex}; use crate::address::UnifiedAddress; +#[cfg(any(feature = "sapling", feature = "orchard"))] +use zcash_protocol::consensus::NetworkConstants; + #[cfg(feature = "transparent-inputs")] use { std::convert::TryInto, @@ -143,12 +146,16 @@ pub enum DerivationError { } impl Display for DerivationError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { #[cfg(feature = "orchard")] - DerivationError::Orchard(e) => write!(f, "Orchard error: {}", e), + DerivationError::Orchard(e) => write!(_f, "Orchard error: {}", e), #[cfg(feature = "transparent-inputs")] - DerivationError::Transparent(e) => write!(f, "Transparent error: {}", e), + DerivationError::Transparent(e) => write!(_f, "Transparent error: {}", e), + #[cfg(not(any(feature = "orchard", feature = "transparent-inputs")))] + other => { + unreachable!("Unhandled DerivationError variant {:?}", other) + } } } } @@ -1252,21 +1259,20 @@ pub mod testing { #[cfg(test)] mod tests { - use crate::keys::UnifiedIncomingViewingKey; - use super::UnifiedFullViewingKey; use proptest::prelude::proptest; - use zcash_address::unified::{Encoding, Uivk}; + + use {zcash_primitives::consensus::MAIN_NETWORK, zip32::AccountId}; + + #[cfg(any(feature = "sapling", feature = "orchard"))] + use { + super::{UnifiedFullViewingKey, UnifiedIncomingViewingKey}, + zcash_address::unified::{Encoding, Uivk}, + }; + #[cfg(feature = "orchard")] use zip32::Scope; - #[cfg(any( - feature = "orchard", - feature = "sapling", - feature = "transparent-inputs" - ))] - use {zcash_primitives::consensus::MAIN_NETWORK, zip32::AccountId}; - #[cfg(feature = "sapling")] use super::sapling; @@ -1274,10 +1280,7 @@ mod tests { use { crate::{address::Address, encoding::AddressCodec}, zcash_address::test_vectors, - zcash_primitives::legacy::{ - self, - keys::{AccountPrivKey, IncomingViewingKey}, - }, + zcash_primitives::legacy::keys::{AccountPrivKey, IncomingViewingKey}, zip32::DiversifierIndex, }; @@ -1305,19 +1308,19 @@ mod tests { fn pk_to_taddr() { use zcash_primitives::legacy::keys::NonHardenedChildIndex; - let taddr = - legacy::keys::AccountPrivKey::from_seed(&MAIN_NETWORK, &seed(), AccountId::ZERO) - .unwrap() - .to_account_pubkey() - .derive_external_ivk() - .unwrap() - .derive_address(NonHardenedChildIndex::ZERO) - .unwrap() - .encode(&MAIN_NETWORK); + let taddr = AccountPrivKey::from_seed(&MAIN_NETWORK, &seed(), AccountId::ZERO) + .unwrap() + .to_account_pubkey() + .derive_external_ivk() + .unwrap() + .derive_address(NonHardenedChildIndex::ZERO) + .unwrap() + .encode(&MAIN_NETWORK); assert_eq!(taddr, "t1PKtYdJJHhc3Pxowmznkg7vdTwnhEsCvR4".to_string()); } #[test] + #[cfg(any(feature = "orchard", feature = "sapling"))] fn ufvk_round_trip() { #[cfg(feature = "orchard")] let orchard = { @@ -1348,100 +1351,93 @@ mod tests { orchard, ); - #[cfg(not(any(feature = "orchard", feature = "sapling")))] - assert!(ufvk.is_none()); + let ufvk = ufvk.expect("Orchard or Sapling fvk is present."); + let encoded = ufvk.encode(&MAIN_NETWORK); - #[cfg(any(feature = "orchard", feature = "sapling"))] + // Test encoded form against known values; these test vectors contain Orchard receivers + // that will be treated as unknown if the `orchard` feature is not enabled. + let encoded_with_t = "uview1tg6rpjgju2s2j37gkgjq79qrh5lvzr6e0ed3n4sf4hu5qd35vmsh7avl80xa6mx7ryqce9hztwaqwrdthetpy4pc0kce25x453hwcmax02p80pg5savlg865sft9reat07c5vlactr6l2pxtlqtqunt2j9gmvr8spcuzf07af80h5qmut38h0gvcfa9k4rwujacwwca9vu8jev7wq6c725huv8qjmhss3hdj2vh8cfxhpqcm2qzc34msyrfxk5u6dqttt4vv2mr0aajreww5yufpk0gn4xkfm888467k7v6fmw7syqq6cceu078yw8xja502jxr0jgum43lhvpzmf7eu5dmnn6cr6f7p43yw8znzgxg598mllewnx076hljlvynhzwn5es94yrv65tdg3utuz2u3sras0wfcq4adxwdvlk387d22g3q98t5z74quw2fa4wed32escx8dwh4mw35t4jwf35xyfxnu83mk5s4kw2glkgsshmxk"; + let _encoded_no_t = "uview12z384wdq76ceewlsu0esk7d97qnd23v2qnvhujxtcf2lsq8g4hwzpx44fwxssnm5tg8skyh4tnc8gydwxefnnm0hd0a6c6etmj0pp9jqkdsllkr70u8gpf7ndsfqcjlqn6dec3faumzqlqcmtjf8vp92h7kj38ph2786zx30hq2wru8ae3excdwc8w0z3t9fuw7mt7xy5sn6s4e45kwm0cjp70wytnensgdnev286t3vew3yuwt2hcz865y037k30e428dvgne37xvyeal2vu8yjnznphf9t2rw3gdp0hk5zwq00ws8f3l3j5n3qkqgsyzrwx4qzmgq0xwwk4vz2r6vtsykgz089jncvycmem3535zjwvvtvjw8v98y0d5ydwte575gjm7a7k"; + + // We test the full roundtrip only with the `sapling` and `orchard` features enabled, + // because we will not generate these parts of the encoding if the UFVK does not have an + // these parts. + #[cfg(all(feature = "sapling", feature = "orchard"))] { - let ufvk = ufvk.expect("Orchard or Sapling fvk is present."); - let encoded = ufvk.encode(&MAIN_NETWORK); - - // Test encoded form against known values; these test vectors contain Orchard receivers - // that will be treated as unknown if the `orchard` feature is not enabled. - let encoded_with_t = "uview1tg6rpjgju2s2j37gkgjq79qrh5lvzr6e0ed3n4sf4hu5qd35vmsh7avl80xa6mx7ryqce9hztwaqwrdthetpy4pc0kce25x453hwcmax02p80pg5savlg865sft9reat07c5vlactr6l2pxtlqtqunt2j9gmvr8spcuzf07af80h5qmut38h0gvcfa9k4rwujacwwca9vu8jev7wq6c725huv8qjmhss3hdj2vh8cfxhpqcm2qzc34msyrfxk5u6dqttt4vv2mr0aajreww5yufpk0gn4xkfm888467k7v6fmw7syqq6cceu078yw8xja502jxr0jgum43lhvpzmf7eu5dmnn6cr6f7p43yw8znzgxg598mllewnx076hljlvynhzwn5es94yrv65tdg3utuz2u3sras0wfcq4adxwdvlk387d22g3q98t5z74quw2fa4wed32escx8dwh4mw35t4jwf35xyfxnu83mk5s4kw2glkgsshmxk"; - let _encoded_no_t = "uview12z384wdq76ceewlsu0esk7d97qnd23v2qnvhujxtcf2lsq8g4hwzpx44fwxssnm5tg8skyh4tnc8gydwxefnnm0hd0a6c6etmj0pp9jqkdsllkr70u8gpf7ndsfqcjlqn6dec3faumzqlqcmtjf8vp92h7kj38ph2786zx30hq2wru8ae3excdwc8w0z3t9fuw7mt7xy5sn6s4e45kwm0cjp70wytnensgdnev286t3vew3yuwt2hcz865y037k30e428dvgne37xvyeal2vu8yjnznphf9t2rw3gdp0hk5zwq00ws8f3l3j5n3qkqgsyzrwx4qzmgq0xwwk4vz2r6vtsykgz089jncvycmem3535zjwvvtvjw8v98y0d5ydwte575gjm7a7k"; - - // We test the full roundtrip only with the `sapling` and `orchard` features enabled, - // because we will not generate these parts of the encoding if the UFVK does not have an - // these parts. - #[cfg(all(feature = "sapling", feature = "orchard"))] - { - #[cfg(feature = "transparent-inputs")] - assert_eq!(encoded, encoded_with_t); - #[cfg(not(feature = "transparent-inputs"))] - assert_eq!(encoded, _encoded_no_t); - } - - let decoded = UnifiedFullViewingKey::decode(&MAIN_NETWORK, &encoded).unwrap(); - let reencoded = decoded.encode(&MAIN_NETWORK); - assert_eq!(encoded, reencoded); - #[cfg(feature = "transparent-inputs")] - assert_eq!( - decoded.transparent.map(|t| t.serialize()), - ufvk.transparent.as_ref().map(|t| t.serialize()), - ); - #[cfg(feature = "sapling")] - assert_eq!( - decoded.sapling.map(|s| s.to_bytes()), - ufvk.sapling.map(|s| s.to_bytes()), - ); - #[cfg(feature = "orchard")] - assert_eq!( - decoded.orchard.map(|o| o.to_bytes()), - ufvk.orchard.map(|o| o.to_bytes()), - ); - - let decoded_with_t = - UnifiedFullViewingKey::decode(&MAIN_NETWORK, encoded_with_t).unwrap(); - #[cfg(feature = "transparent-inputs")] - assert_eq!( - decoded_with_t.transparent.map(|t| t.serialize()), - ufvk.transparent.as_ref().map(|t| t.serialize()), - ); - - // Both Orchard and Sapling enabled - #[cfg(all( - feature = "orchard", - feature = "sapling", - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 0); - #[cfg(all( - feature = "orchard", - feature = "sapling", - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - - // Orchard enabled - #[cfg(all( - feature = "orchard", - not(feature = "sapling"), - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - #[cfg(all( - feature = "orchard", - not(feature = "sapling"), - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 2); - - // Sapling enabled - #[cfg(all( - not(feature = "orchard"), - feature = "sapling", - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - #[cfg(all( - not(feature = "orchard"), - feature = "sapling", - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 2); + assert_eq!(encoded, encoded_with_t); + #[cfg(not(feature = "transparent-inputs"))] + assert_eq!(encoded, _encoded_no_t); } + + let decoded = UnifiedFullViewingKey::decode(&MAIN_NETWORK, &encoded).unwrap(); + let reencoded = decoded.encode(&MAIN_NETWORK); + assert_eq!(encoded, reencoded); + + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded.transparent.map(|t| t.serialize()), + ufvk.transparent.as_ref().map(|t| t.serialize()), + ); + #[cfg(feature = "sapling")] + assert_eq!( + decoded.sapling.map(|s| s.to_bytes()), + ufvk.sapling.map(|s| s.to_bytes()), + ); + #[cfg(feature = "orchard")] + assert_eq!( + decoded.orchard.map(|o| o.to_bytes()), + ufvk.orchard.map(|o| o.to_bytes()), + ); + + let decoded_with_t = UnifiedFullViewingKey::decode(&MAIN_NETWORK, encoded_with_t).unwrap(); + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded_with_t.transparent.map(|t| t.serialize()), + ufvk.transparent.as_ref().map(|t| t.serialize()), + ); + + // Both Orchard and Sapling enabled + #[cfg(all( + feature = "orchard", + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 0); + #[cfg(all( + feature = "orchard", + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + + // Orchard enabled + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); + + // Sapling enabled + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); } #[test] @@ -1501,6 +1497,7 @@ mod tests { } #[test] + #[cfg(any(feature = "orchard", feature = "sapling"))] fn uivk_round_trip() { use zcash_primitives::consensus::NetworkType; @@ -1533,101 +1530,94 @@ mod tests { orchard, ); - #[cfg(not(any(feature = "orchard", feature = "sapling")))] - assert!(uivk.is_none()); + let encoded = uivk.to_uivk().encode(&NetworkType::Main); - #[cfg(any(feature = "orchard", feature = "sapling"))] + // Test encoded form against known values; these test vectors contain Orchard receivers + // that will be treated as unknown if the `orchard` feature is not enabled. + let encoded_with_t = "uivk1z28yg638vjwusmf0zc9ad2j0mpv6s42wc5kqt004aaqfu5xxxgu7mdcydn9qf723fnryt34s6jyxyw0jt7spq04c3v9ze6qe9gjjc5aglz8zv5pqtw58czd0actynww5n85z3052kzgy6cu0fyjafyp4sr4kppyrrwhwev2rr0awq6m8d66esvk6fgacggqnswg5g9gkv6t6fj9ajhyd0gmel4yzscprpzduncc0e2lywufup6fvzf6y8cefez2r99pgge5yyfuus0r60khgu895pln5e7nn77q6s9kh2uwf6lrfu06ma2kd7r05jjvl4hn6nupge8fajh0cazd7mkmz23t79w"; + let _encoded_no_t = "uivk1020vq9j5zeqxh303sxa0zv2hn9wm9fev8x0p8yqxdwyzde9r4c90fcglc63usj0ycl2scy8zxuhtser0qrq356xfy8x3vyuxu7f6gas75svl9v9m3ctuazsu0ar8e8crtx7x6zgh4kw8xm3q4rlkpm9er2wefxhhf9pn547gpuz9vw27gsdp6c03nwlrxgzhr2g6xek0x8l5avrx9ue9lf032tr7kmhqf3nfdxg7ldfgx6yf09g"; + + // We test the full roundtrip only with the `sapling` and `orchard` features enabled, + // because we will not generate these parts of the encoding if the UIVK does not have an + // these parts. + #[cfg(all(feature = "sapling", feature = "orchard"))] { - let encoded = uivk.to_uivk().encode(&NetworkType::Main); - - // Test encoded form against known values; these test vectors contain Orchard receivers - // that will be treated as unknown if the `orchard` feature is not enabled. - let encoded_with_t = "uivk1z28yg638vjwusmf0zc9ad2j0mpv6s42wc5kqt004aaqfu5xxxgu7mdcydn9qf723fnryt34s6jyxyw0jt7spq04c3v9ze6qe9gjjc5aglz8zv5pqtw58czd0actynww5n85z3052kzgy6cu0fyjafyp4sr4kppyrrwhwev2rr0awq6m8d66esvk6fgacggqnswg5g9gkv6t6fj9ajhyd0gmel4yzscprpzduncc0e2lywufup6fvzf6y8cefez2r99pgge5yyfuus0r60khgu895pln5e7nn77q6s9kh2uwf6lrfu06ma2kd7r05jjvl4hn6nupge8fajh0cazd7mkmz23t79w"; - let _encoded_no_t = "uivk1020vq9j5zeqxh303sxa0zv2hn9wm9fev8x0p8yqxdwyzde9r4c90fcglc63usj0ycl2scy8zxuhtser0qrq356xfy8x3vyuxu7f6gas75svl9v9m3ctuazsu0ar8e8crtx7x6zgh4kw8xm3q4rlkpm9er2wefxhhf9pn547gpuz9vw27gsdp6c03nwlrxgzhr2g6xek0x8l5avrx9ue9lf032tr7kmhqf3nfdxg7ldfgx6yf09g"; - - // We test the full roundtrip only with the `sapling` and `orchard` features enabled, - // because we will not generate these parts of the encoding if the UIVK does not have an - // these parts. - #[cfg(all(feature = "sapling", feature = "orchard"))] - { - #[cfg(feature = "transparent-inputs")] - assert_eq!(encoded, encoded_with_t); - #[cfg(not(feature = "transparent-inputs"))] - assert_eq!(encoded, _encoded_no_t); - } - - let decoded = - UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(&encoded).unwrap().1).unwrap(); - let reencoded = decoded.to_uivk().encode(&NetworkType::Main); - assert_eq!(encoded, reencoded); - #[cfg(feature = "transparent-inputs")] - assert_eq!( - decoded.transparent.map(|t| t.serialize()), - uivk.transparent.as_ref().map(|t| t.serialize()), - ); - #[cfg(feature = "sapling")] - assert_eq!( - decoded.sapling.map(|s| s.to_bytes()), - uivk.sapling.map(|s| s.to_bytes()), - ); - #[cfg(feature = "orchard")] - assert_eq!( - decoded.orchard.map(|o| o.to_bytes()), - uivk.orchard.map(|o| o.to_bytes()), - ); - - let decoded_with_t = - UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(encoded_with_t).unwrap().1) - .unwrap(); - #[cfg(feature = "transparent-inputs")] - assert_eq!( - decoded_with_t.transparent.map(|t| t.serialize()), - uivk.transparent.as_ref().map(|t| t.serialize()), - ); - - // Both Orchard and Sapling enabled - #[cfg(all( - feature = "orchard", - feature = "sapling", - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 0); - #[cfg(all( - feature = "orchard", - feature = "sapling", - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - - // Orchard enabled - #[cfg(all( - feature = "orchard", - not(feature = "sapling"), - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - #[cfg(all( - feature = "orchard", - not(feature = "sapling"), - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 2); - - // Sapling enabled - #[cfg(all( - not(feature = "orchard"), - feature = "sapling", - feature = "transparent-inputs" - ))] - assert_eq!(decoded_with_t.unknown.len(), 1); - #[cfg(all( - not(feature = "orchard"), - feature = "sapling", - not(feature = "transparent-inputs") - ))] - assert_eq!(decoded_with_t.unknown.len(), 2); + assert_eq!(encoded, encoded_with_t); + #[cfg(not(feature = "transparent-inputs"))] + assert_eq!(encoded, _encoded_no_t); } + + let decoded = + UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(&encoded).unwrap().1).unwrap(); + let reencoded = decoded.to_uivk().encode(&NetworkType::Main); + assert_eq!(encoded, reencoded); + + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded.transparent.map(|t| t.serialize()), + uivk.transparent.as_ref().map(|t| t.serialize()), + ); + #[cfg(feature = "sapling")] + assert_eq!( + decoded.sapling.map(|s| s.to_bytes()), + uivk.sapling.map(|s| s.to_bytes()), + ); + #[cfg(feature = "orchard")] + assert_eq!( + decoded.orchard.map(|o| o.to_bytes()), + uivk.orchard.map(|o| o.to_bytes()), + ); + + let decoded_with_t = + UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(encoded_with_t).unwrap().1).unwrap(); + #[cfg(feature = "transparent-inputs")] + assert_eq!( + decoded_with_t.transparent.map(|t| t.serialize()), + uivk.transparent.as_ref().map(|t| t.serialize()), + ); + + // Both Orchard and Sapling enabled + #[cfg(all( + feature = "orchard", + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 0); + #[cfg(all( + feature = "orchard", + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + + // Orchard enabled + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + feature = "orchard", + not(feature = "sapling"), + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); + + // Sapling enabled + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + feature = "transparent-inputs" + ))] + assert_eq!(decoded_with_t.unknown.len(), 1); + #[cfg(all( + not(feature = "orchard"), + feature = "sapling", + not(feature = "transparent-inputs") + ))] + assert_eq!(decoded_with_t.unknown.len(), 2); } #[test] diff --git a/zcash_keys/src/lib.rs b/zcash_keys/src/lib.rs index 992170ab1..eb881a80f 100644 --- a/zcash_keys/src/lib.rs +++ b/zcash_keys/src/lib.rs @@ -16,4 +16,10 @@ pub mod address; pub mod encoding; + +#[cfg(any( + feature = "orchard", + feature = "sapling", + feature = "transparent-inputs" +))] pub mod keys; From 9ddbf1e3e95b2d3033d1cff0258f244640349b74 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 14 Mar 2024 15:21:46 -0600 Subject: [PATCH 07/22] Implement todo! line --- zcash_client_sqlite/src/wallet.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index e73d6dd71..90d2616fc 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -208,7 +208,7 @@ impl Account { ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { match &self.viewing_key { ViewingKey::Full(ufvk) => ufvk.default_address(request), - ViewingKey::Incoming(_uivk) => todo!(), + ViewingKey::Incoming(uivk) => uivk.default_address(request), } } } From 9e1a4327c3b9d7632925add0ef87821c118aa036 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 14 Mar 2024 15:23:40 -0600 Subject: [PATCH 08/22] zcash_keys: Keep the Ufvk and Uivk encodings private. --- zcash_client_sqlite/src/wallet.rs | 18 ++------- .../init/migrations/full_account_ids.rs | 4 +- zcash_keys/CHANGELOG.md | 2 - zcash_keys/src/keys.rs | 40 ++++++++++++++----- 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 90d2616fc..90dc840ae 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -74,7 +74,6 @@ use std::io::{self, Cursor}; use std::num::NonZeroU32; use std::ops::RangeInclusive; use tracing::debug; -use zcash_address::unified::{Encoding, Uivk}; use zcash_keys::keys::{ AddressGenerationError, HdSeedFingerprint, UnifiedAddressRequest, UnifiedIncomingViewingKey, }; @@ -120,7 +119,7 @@ use { crate::UtxoId, rusqlite::Row, std::collections::BTreeSet, - zcash_address::unified::Ivk, + zcash_address::unified::{Encoding, Ivk, Uivk}, zcash_client_backend::wallet::{TransparentAddressMetadata, WalletTransparentOutput}, zcash_primitives::{ legacy::{ @@ -347,7 +346,7 @@ pub(crate) fn add_account( ":hd_seed_fingerprint": hd_seed_fingerprint.as_ref().map(|fp| fp.as_bytes()), ":hd_account_index": hd_account_index.map(u32::from), ":ufvk": viewing_key.ufvk().map(|ufvk| ufvk.encode(params)), - ":uivk": viewing_key.uivk().to_uivk().encode(¶ms.network_type()), + ":uivk": viewing_key.uivk().encode(params), ":orchard_fvk_item_cache": orchard_item, ":sapling_fvk_item_cache": sapling_item, ":p2pkh_fvk_item_cache": transparent_item, @@ -1503,18 +1502,9 @@ pub(crate) fn get_account( )) } else { let uivk_str: String = row.get("uivk")?; - let (network, uivk) = Uivk::decode(&uivk_str).map_err(|e| { - SqliteClientError::CorruptedData(format!("Failure to decode UIVK: {e}")) - })?; - if network != params.network_type() { - return Err(SqliteClientError::CorruptedData( - "UIVK network type does not match wallet network type".to_string(), - )); - } ViewingKey::Incoming(Box::new( - UnifiedIncomingViewingKey::from_uivk(&uivk).map_err(|e| { - SqliteClientError::CorruptedData(format!("Failure to decode UIVK: {e}")) - })?, + UnifiedIncomingViewingKey::decode(params, &uivk_str[..]) + .map_err(SqliteClientError::BadAccountData)?, )) }; 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 07689421d..fa019be21 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 @@ -5,7 +5,6 @@ use rusqlite::{named_params, Transaction}; use schemer_rusqlite::RusqliteMigration; use secrecy::{ExposeSecret, SecretVec}; use uuid::Uuid; -use zcash_address::unified::Encoding; use zcash_client_backend::{data_api::AccountKind, keys::UnifiedSpendingKey}; use zcash_keys::keys::{HdSeedFingerprint, UnifiedFullViewingKey}; use zcash_primitives::consensus; @@ -122,8 +121,7 @@ impl RusqliteMigration for Migration

{ let uivk = ufvk_parsed .to_unified_incoming_viewing_key() - .to_uivk() - .encode(&self.params.network_type()); + .encode(&self.params); #[cfg(feature = "transparent-inputs")] let transparent_item = ufvk_parsed.transparent().map(|k| k.serialize()); diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 500318415..8f2627d98 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -12,8 +12,6 @@ and this library adheres to Rust's notion of - `impl Display for zcash_keys::keys::AddressGenerationError` - `impl std::error::Error for zcash_keys::keys::AddressGenerationError` - `zcash_keys::keys::DecodingError` -- `zcash_keys::keys::UnifiedFullViewingKey::from_ufvk` -- `zcash_keys::keys::UnifiedFullViewingKey::to_ufvk` - `zcash_keys::keys::UnifiedFullViewingKey::to_unified_incoming_viewing_key` - `zcash_keys::keys::UnifiedIncomingViewingKey` diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index aac28cda3..d03b5852c 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -740,13 +740,13 @@ impl UnifiedFullViewingKey { )); } - Self::from_ufvk(&ufvk).map_err(|e| e.to_string()) + Self::parse(&ufvk).map_err(|e| e.to_string()) } /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. /// /// [ZIP 316]: https://zips.z.cash/zip-0316 - pub fn from_ufvk(ufvk: &Ufvk) -> Result { + pub fn parse(ufvk: &Ufvk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -823,7 +823,7 @@ impl UnifiedFullViewingKey { } /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. - pub fn to_ufvk(&self) -> Ufvk { + fn to_ufvk(&self) -> Ufvk { let items = std::iter::empty().chain(self.unknown.iter().map(|(typecode, data)| { unified::Fvk::Unknown { typecode: *typecode, @@ -973,8 +973,24 @@ impl UnifiedIncomingViewingKey { } } + /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. + /// + /// [ZIP 316]: https://zips.z.cash/zip-0316 + pub fn decode(params: &P, encoding: &str) -> Result { + let (net, ufvk) = unified::Uivk::decode(encoding).map_err(|e| e.to_string())?; + let expected_net = params.network_type(); + if net != expected_net { + return Err(format!( + "UIVK is for network {:?} but we expected {:?}", + net, expected_net, + )); + } + + Self::parse(&ufvk).map_err(|e| e.to_string()) + } + /// Constructs a unified incoming viewing key from a parsed unified encoding. - pub fn from_uivk(uivk: &Uivk) -> Result { + fn parse(uivk: &Uivk) -> Result { #[cfg(feature = "orchard")] let mut orchard = None; #[cfg(feature = "sapling")] @@ -1041,8 +1057,13 @@ impl UnifiedIncomingViewingKey { }) } + /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. + pub fn encode(&self, params: &P) -> String { + self.render().encode(¶ms.network_type()) + } + /// Converts this unified incoming viewing key to a unified encoding. - pub fn to_uivk(&self) -> Uivk { + fn render(&self) -> Uivk { let items = std::iter::empty().chain(self.unknown.iter().map(|(typecode, data)| { unified::Ivk::Unknown { typecode: *typecode, @@ -1530,7 +1551,7 @@ mod tests { orchard, ); - let encoded = uivk.to_uivk().encode(&NetworkType::Main); + let encoded = uivk.render().encode(&NetworkType::Main); // Test encoded form against known values; these test vectors contain Orchard receivers // that will be treated as unknown if the `orchard` feature is not enabled. @@ -1548,9 +1569,8 @@ mod tests { assert_eq!(encoded, _encoded_no_t); } - let decoded = - UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(&encoded).unwrap().1).unwrap(); - let reencoded = decoded.to_uivk().encode(&NetworkType::Main); + let decoded = UnifiedIncomingViewingKey::parse(&Uivk::decode(&encoded).unwrap().1).unwrap(); + let reencoded = decoded.render().encode(&NetworkType::Main); assert_eq!(encoded, reencoded); #[cfg(feature = "transparent-inputs")] @@ -1570,7 +1590,7 @@ mod tests { ); let decoded_with_t = - UnifiedIncomingViewingKey::from_uivk(&Uivk::decode(encoded_with_t).unwrap().1).unwrap(); + UnifiedIncomingViewingKey::parse(&Uivk::decode(encoded_with_t).unwrap().1).unwrap(); #[cfg(feature = "transparent-inputs")] assert_eq!( decoded_with_t.transparent.map(|t| t.serialize()), From ab3e790bfca626779a6878e9793c6d09cee81e32 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 13 Mar 2024 21:49:28 -0600 Subject: [PATCH 09/22] zcash_client_backend: Rename `AccountKind` to `AccountSource` --- zcash_client_backend/CHANGELOG.md | 2 +- zcash_client_backend/src/data_api.rs | 12 +++--- zcash_client_sqlite/src/lib.rs | 8 ++-- zcash_client_sqlite/src/wallet.rs | 38 +++++++++---------- zcash_client_sqlite/src/wallet/init.rs | 4 +- .../init/migrations/full_account_ids.rs | 6 +-- 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 2b3921032..d4fc361df 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -16,7 +16,7 @@ and this library adheres to Rust's notion of - `Account` - `AccountBalance::with_orchard_balance_mut` - `AccountBirthday::orchard_frontier` - - `AccountKind` + - `AccountSource` - `BlockMetadata::orchard_tree_size` - `DecryptedTransaction::{new, tx(), orchard_outputs()}` - `NoteRetention` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 53ac896b1..901a36843 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -317,7 +317,7 @@ impl AccountBalance { /// The kinds of accounts supported by `zcash_client_backend`. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] -pub enum AccountKind { +pub enum AccountSource { /// An account derived from a known seed. Derived { seed_fingerprint: SeedFingerprint, @@ -335,7 +335,7 @@ pub trait Account { /// Returns whether this account is derived or imported, and the derivation parameters /// if applicable. - fn kind(&self) -> AccountKind; + fn source(&self) -> AccountSource; /// Returns the UFVK that the wallet backend has stored for the account, if any. /// @@ -349,8 +349,8 @@ impl Account for (A, UnifiedFullViewingKey) { self.0 } - fn kind(&self) -> AccountKind { - AccountKind::Imported + fn source(&self) -> AccountSource { + AccountSource::Imported } fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { @@ -363,8 +363,8 @@ impl Account for (A, Option) { self.0 } - fn kind(&self) -> AccountKind { - AccountKind::Imported + fn source(&self) -> AccountSource { + AccountSource::Imported } fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 15d83fc2b..469299c17 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -60,7 +60,7 @@ use zcash_client_backend::{ self, chain::{BlockSource, ChainState, CommitmentTreeRoot}, scanning::{ScanPriority, ScanRange}, - Account, AccountBirthday, AccountKind, BlockMetadata, DecryptedTransaction, InputSource, + Account, AccountBirthday, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SentTransaction, SpendableNotes, WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }, @@ -316,10 +316,10 @@ impl, P: consensus::Parameters> WalletRead for W seed: &SecretVec, ) -> Result { if let Some(account) = self.get_account(account_id)? { - if let AccountKind::Derived { + if let AccountSource::Derived { seed_fingerprint, account_index, - } = account.kind() + } = account.source() { let seed_fingerprint_match = SeedFingerprint::from_seed(seed.expose_secret()).ok_or_else(|| { @@ -527,7 +527,7 @@ impl WalletWrite for WalletDb let account_id = wallet::add_account( wdb.conn.0, &wdb.params, - AccountKind::Derived { + AccountSource::Derived { seed_fingerprint, account_index, }, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index c4964257e..7202ffebe 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -82,8 +82,8 @@ use zcash_client_backend::{ address::{Address, UnifiedAddress}, data_api::{ scanning::{ScanPriority, ScanRange}, - AccountBalance, AccountBirthday, AccountKind, BlockMetadata, Ratio, SentTransactionOutput, - WalletSummary, SAPLING_SHARD_HEIGHT, + AccountBalance, AccountBirthday, AccountSource, BlockMetadata, Ratio, + SentTransactionOutput, WalletSummary, SAPLING_SHARD_HEIGHT, }, encoding::AddressCodec, keys::UnifiedFullViewingKey, @@ -139,13 +139,13 @@ pub(crate) mod scanning; pub(crate) const BLOCK_SAPLING_FRONTIER_ABSENT: &[u8] = &[0x0]; -fn parse_account_kind( +fn parse_account_source( account_kind: u32, hd_seed_fingerprint: Option<[u8; 32]>, hd_account_index: Option, -) -> Result { +) -> Result { match (account_kind, hd_seed_fingerprint, hd_account_index) { - (0, Some(seed_fp), Some(account_index)) => Ok(AccountKind::Derived { + (0, Some(seed_fp), Some(account_index)) => Ok(AccountSource::Derived { seed_fingerprint: SeedFingerprint::from_bytes(seed_fp), account_index: zip32::AccountId::try_from(account_index).map_err(|_| { SqliteClientError::CorruptedData( @@ -153,7 +153,7 @@ fn parse_account_kind( ) })?, }), - (1, None, None) => Ok(AccountKind::Imported), + (1, None, None) => Ok(AccountSource::Imported), (0, None, None) | (1, Some(_), Some(_)) => Err(SqliteClientError::CorruptedData( "Wallet DB account_kind constraint violated".to_string(), )), @@ -163,10 +163,10 @@ fn parse_account_kind( } } -fn account_kind_code(value: AccountKind) -> u32 { +fn account_kind_code(value: AccountSource) -> u32 { match value { - AccountKind::Derived { .. } => 0, - AccountKind::Imported => 1, + AccountSource::Derived { .. } => 0, + AccountSource::Imported => 1, } } @@ -190,7 +190,7 @@ pub(crate) enum ViewingKey { #[derive(Debug, Clone)] pub struct Account { account_id: AccountId, - kind: AccountKind, + kind: AccountSource, viewing_key: ViewingKey, } @@ -216,7 +216,7 @@ impl zcash_client_backend::data_api::Account for Account { self.account_id } - fn kind(&self) -> AccountKind { + fn source(&self) -> AccountSource { self.kind } @@ -324,16 +324,16 @@ pub(crate) fn ufvk_to_uivk( pub(crate) fn add_account( conn: &rusqlite::Transaction, params: &P, - kind: AccountKind, + kind: AccountSource, viewing_key: ViewingKey, birthday: AccountBirthday, ) -> Result { let (hd_seed_fingerprint, hd_account_index) = match kind { - AccountKind::Derived { + AccountSource::Derived { seed_fingerprint, account_index, } => (Some(seed_fingerprint), Some(account_index)), - AccountKind::Imported => (None, None), + AccountSource::Imported => (None, None), }; let orchard_item = viewing_key @@ -734,7 +734,7 @@ pub(crate) fn get_account_for_ufvk( ], |row| { let account_id = row.get::<_, u32>(0).map(AccountId)?; - let kind = parse_account_kind(row.get(1)?, row.get(2)?, row.get(3)?)?; + let kind = parse_account_source(row.get(1)?, row.get(2)?, row.get(3)?)?; // We looked up the account by FVK components, so the UFVK column must be // non-null. @@ -802,7 +802,7 @@ pub(crate) fn get_derived_account( }?; Ok(Account { account_id, - kind: AccountKind::Derived { + kind: AccountSource::Derived { seed_fingerprint: *seed, account_index, }, @@ -1511,7 +1511,7 @@ pub(crate) fn get_account( let row = result.next()?; match row { Some(row) => { - let kind = parse_account_kind( + let kind = parse_account_source( row.get("account_kind")?, row.get("hd_seed_fingerprint")?, row.get("hd_account_index")?, @@ -2700,7 +2700,7 @@ mod tests { use std::num::NonZeroU32; use sapling::zip32::ExtendedSpendingKey; - use zcash_client_backend::data_api::{AccountBirthday, AccountKind, WalletRead}; + use zcash_client_backend::data_api::{AccountBirthday, AccountSource, WalletRead}; use zcash_primitives::{block::BlockHash, transaction::components::amount::NonNegativeAmount}; use crate::{ @@ -2857,7 +2857,7 @@ mod tests { let expected_account_index = zip32::AccountId::try_from(0).unwrap(); assert_matches!( account_parameters.kind, - AccountKind::Derived{account_index, ..} if account_index == expected_account_index + AccountSource::Derived{account_index, ..} if account_index == expected_account_index ); } diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 00147527d..1b0feee4d 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -1282,7 +1282,7 @@ mod tests { #[test] #[cfg(feature = "transparent-inputs")] fn account_produces_expected_ua_sequence() { - use zcash_client_backend::data_api::{AccountBirthday, AccountKind, WalletRead}; + use zcash_client_backend::data_api::{AccountBirthday, AccountSource, WalletRead}; let network = Network::MainNetwork; let data_file = NamedTempFile::new().unwrap(); @@ -1301,7 +1301,7 @@ mod tests { db_data.get_account(account_id), Ok(Some(account)) if matches!( account.kind, - AccountKind::Derived{account_index, ..} if account_index == zip32::AccountId::ZERO, + AccountSource::Derived{account_index, ..} if account_index == zip32::AccountId::ZERO, ) ); 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 9de422b9b..40e479d07 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 @@ -5,7 +5,7 @@ use rusqlite::{named_params, Transaction}; use schemer_rusqlite::RusqliteMigration; use secrecy::{ExposeSecret, SecretVec}; use uuid::Uuid; -use zcash_client_backend::{data_api::AccountKind, keys::UnifiedSpendingKey}; +use zcash_client_backend::{data_api::AccountSource, keys::UnifiedSpendingKey}; use zcash_keys::keys::UnifiedFullViewingKey; use zcash_primitives::consensus; use zip32::fingerprint::SeedFingerprint; @@ -45,11 +45,11 @@ impl RusqliteMigration for Migration

{ type Error = WalletMigrationError; fn up(&self, transaction: &Transaction) -> Result<(), WalletMigrationError> { - let account_kind_derived = account_kind_code(AccountKind::Derived { + let account_kind_derived = account_kind_code(AccountSource::Derived { seed_fingerprint: SeedFingerprint::from_bytes([0; 32]), account_index: zip32::AccountId::ZERO, }); - let account_kind_imported = account_kind_code(AccountKind::Imported); + let account_kind_imported = account_kind_code(AccountSource::Imported); transaction.execute_batch( &format!(r#" PRAGMA foreign_keys = OFF; From 8b8757ce659f981eb6a3e0aa69c27d9115f3a783 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 15 Mar 2024 16:50:53 +0000 Subject: [PATCH 10/22] zcash_client_sqlite: Fix ambiguities in transaction views Co-authored-by: Kris Nuttycombe --- zcash_client_sqlite/src/testing.rs | 100 ++++++++++++++++++ zcash_client_sqlite/src/testing/pool.rs | 3 + zcash_client_sqlite/src/wallet/init.rs | 38 ++++--- .../init/migrations/orchard_received_notes.rs | 38 ++++--- 4 files changed, 147 insertions(+), 32 deletions(-) diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index e969b5152..472e094cf 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -63,6 +63,7 @@ use zcash_primitives::{ zip32::DiversifierIndex, }; use zcash_protocol::local_consensus::LocalNetwork; +use zcash_protocol::value::{ZatBalance, Zatoshis}; use crate::{ chain::init::init_cache_database, @@ -958,6 +959,105 @@ impl TestState { ) .unwrap() } + + /// Returns a vector of transaction summaries + pub(crate) fn get_tx_history( + &self, + ) -> Result>, SqliteClientError> { + let mut stmt = self.wallet().conn.prepare_cached( + "SELECT * + FROM v_transactions + ORDER BY mined_height DESC, tx_index DESC", + )?; + + let results = stmt + .query_and_then::, SqliteClientError, _, _>([], |row| { + Ok(TransactionSummary { + account_id: AccountId(row.get("account_id")?), + txid: TxId::from_bytes(row.get("txid")?), + expiry_height: row + .get::<_, Option>("expiry_height")? + .map(BlockHeight::from), + mined_height: row + .get::<_, Option>("mined_height")? + .map(BlockHeight::from), + account_value_delta: ZatBalance::from_i64(row.get("account_balance_delta")?)?, + fee_paid: row + .get::<_, Option>("fee_paid")? + .map(Zatoshis::from_nonnegative_i64) + .transpose()?, + has_change: row.get("has_change")?, + sent_note_count: row.get("sent_note_count")?, + received_note_count: row.get("received_note_count")?, + memo_count: row.get("memo_count")?, + expired_unmined: row.get("expired_unmined")?, + }) + })? + .collect::, _>>()?; + + Ok(results) + } +} + +pub(crate) struct TransactionSummary { + account_id: AccountId, + txid: TxId, + expiry_height: Option, + mined_height: Option, + account_value_delta: ZatBalance, + fee_paid: Option, + has_change: bool, + sent_note_count: usize, + received_note_count: usize, + memo_count: usize, + expired_unmined: bool, +} + +#[allow(dead_code)] +impl TransactionSummary { + pub(crate) fn account_id(&self) -> &AccountId { + &self.account_id + } + + pub(crate) fn txid(&self) -> TxId { + self.txid + } + + pub(crate) fn expiry_height(&self) -> Option { + self.expiry_height + } + + pub(crate) fn mined_height(&self) -> Option { + self.mined_height + } + + pub(crate) fn account_value_delta(&self) -> ZatBalance { + self.account_value_delta + } + + pub(crate) fn fee_paid(&self) -> Option { + self.fee_paid + } + + pub(crate) fn has_change(&self) -> bool { + self.has_change + } + + pub(crate) fn sent_note_count(&self) -> usize { + self.sent_note_count + } + + pub(crate) fn received_note_count(&self) -> usize { + self.received_note_count + } + + pub(crate) fn expired_unmined(&self) -> bool { + self.expired_unmined + } + + pub(crate) fn memo_count(&self) -> usize { + self.memo_count + } } /// Trait used by tests that require a full viewing key. diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 235c640f0..24d8e955b 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -271,6 +271,9 @@ pub(crate) fn send_single_step_proposed_transfer() { .get_memo(NoteId::new(sent_tx_id, T::SHIELDED_PROTOCOL, 12345)), Ok(None) ); + + let tx_history = st.get_tx_history().unwrap(); + assert_eq!(tx_history.len(), 2); } #[cfg(feature = "transparent-inputs")] diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 00147527d..e58429470 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -552,14 +552,14 @@ mod tests { // v_received_notes "CREATE VIEW v_received_notes AS SELECT - id, - tx, + sapling_received_notes.id AS id_within_pool_table, + sapling_received_notes.tx, 2 AS pool, sapling_received_notes.output_index AS output_index, account_id, - value, + sapling_received_notes.value, is_change, - memo, + sapling_received_notes.memo, spent, sent_notes.id AS sent_note_id FROM sapling_received_notes @@ -568,14 +568,14 @@ mod tests { (sapling_received_notes.tx, 2, sapling_received_notes.output_index) UNION SELECT - id, - tx, + orchard_received_notes.id AS id_within_pool_table, + orchard_received_notes.tx, 3 AS pool, orchard_received_notes.action_index AS output_index, account_id, - value, + orchard_received_notes.value, is_change, - memo, + orchard_received_notes.memo, spent, sent_notes.id AS sent_note_id FROM orchard_received_notes @@ -650,11 +650,12 @@ mod tests { "CREATE VIEW v_transactions AS WITH notes AS ( - SELECT v_received_notes.id AS id, - v_received_notes.account_id AS account_id, + -- Shielded notes received in this transaction + SELECT v_received_notes.account_id AS account_id, transactions.block AS block, transactions.txid AS txid, v_received_notes.pool AS pool, + id_within_pool_table, v_received_notes.value AS value, CASE WHEN v_received_notes.is_change THEN 1 @@ -673,22 +674,24 @@ mod tests { JOIN transactions ON transactions.id_tx = v_received_notes.tx UNION - SELECT utxos.id AS id, - utxos.received_by_account_id AS account_id, + -- Transparent TXOs received in this transaction + SELECT utxos.received_by_account_id AS account_id, utxos.height AS block, utxos.prevout_txid AS txid, 0 AS pool, + utxos.id AS id_within_pool_table, utxos.value_zat AS value, 0 AS is_change, 1 AS received_count, 0 AS memo_present FROM utxos UNION - SELECT v_received_notes.id AS id, - v_received_notes.account_id AS account_id, + -- Shielded notes spent in this transaction + SELECT v_received_notes.account_id AS account_id, transactions.block AS block, transactions.txid AS txid, v_received_notes.pool AS pool, + id_within_pool_table, -v_received_notes.value AS value, 0 AS is_change, 0 AS received_count, @@ -697,11 +700,12 @@ mod tests { JOIN transactions ON transactions.id_tx = v_received_notes.spent UNION - SELECT utxos.id AS id, - utxos.received_by_account_id AS account_id, + -- Transparent TXOs spent in this transaction + SELECT utxos.received_by_account_id AS account_id, transactions.block AS block, transactions.txid AS txid, 0 AS pool, + utxos.id AS id_within_pool_table, -utxos.value_zat AS value, 0 AS is_change, 0 AS received_count, @@ -710,6 +714,8 @@ mod tests { JOIN transactions ON transactions.id_tx = utxos.spent_in_tx ), + -- Obtain a count of the notes that the wallet created in each transaction, + -- not counting change notes. sent_note_counts AS ( SELECT sent_notes.from_account_id AS account_id, transactions.txid AS txid, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/orchard_received_notes.rs b/zcash_client_sqlite/src/wallet/init/migrations/orchard_received_notes.rs index 014f80dc4..7e174164b 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/orchard_received_notes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/orchard_received_notes.rs @@ -70,14 +70,14 @@ impl RusqliteMigration for Migration { &format!( "CREATE VIEW v_received_notes AS SELECT - id, - tx, + sapling_received_notes.id AS id_within_pool_table, + sapling_received_notes.tx, {sapling_pool_code} AS pool, sapling_received_notes.output_index AS output_index, account_id, - value, + sapling_received_notes.value, is_change, - memo, + sapling_received_notes.memo, spent, sent_notes.id AS sent_note_id FROM sapling_received_notes @@ -86,14 +86,14 @@ impl RusqliteMigration for Migration { (sapling_received_notes.tx, {sapling_pool_code}, sapling_received_notes.output_index) UNION SELECT - id, - tx, + orchard_received_notes.id AS id_within_pool_table, + orchard_received_notes.tx, {orchard_pool_code} AS pool, orchard_received_notes.action_index AS output_index, account_id, - value, + orchard_received_notes.value, is_change, - memo, + orchard_received_notes.memo, spent, sent_notes.id AS sent_note_id FROM orchard_received_notes @@ -110,11 +110,12 @@ impl RusqliteMigration for Migration { CREATE VIEW v_transactions AS WITH notes AS ( - SELECT v_received_notes.id AS id, - v_received_notes.account_id AS account_id, + -- Shielded notes received in this transaction + SELECT v_received_notes.account_id AS account_id, transactions.block AS block, transactions.txid AS txid, v_received_notes.pool AS pool, + id_within_pool_table, v_received_notes.value AS value, CASE WHEN v_received_notes.is_change THEN 1 @@ -133,22 +134,24 @@ impl RusqliteMigration for Migration { JOIN transactions ON transactions.id_tx = v_received_notes.tx UNION - SELECT utxos.id AS id, - utxos.received_by_account_id AS account_id, + -- Transparent TXOs received in this transaction + SELECT utxos.received_by_account_id AS account_id, utxos.height AS block, utxos.prevout_txid AS txid, {transparent_pool_code} AS pool, + utxos.id AS id_within_pool_table, utxos.value_zat AS value, 0 AS is_change, 1 AS received_count, 0 AS memo_present FROM utxos UNION - SELECT v_received_notes.id AS id, - v_received_notes.account_id AS account_id, + -- Shielded notes spent in this transaction + SELECT v_received_notes.account_id AS account_id, transactions.block AS block, transactions.txid AS txid, v_received_notes.pool AS pool, + id_within_pool_table, -v_received_notes.value AS value, 0 AS is_change, 0 AS received_count, @@ -157,11 +160,12 @@ impl RusqliteMigration for Migration { JOIN transactions ON transactions.id_tx = v_received_notes.spent UNION - SELECT utxos.id AS id, - utxos.received_by_account_id AS account_id, + -- Transparent TXOs spent in this transaction + SELECT utxos.received_by_account_id AS account_id, transactions.block AS block, transactions.txid AS txid, {transparent_pool_code} AS pool, + utxos.id AS id_within_pool_table, -utxos.value_zat AS value, 0 AS is_change, 0 AS received_count, @@ -170,6 +174,8 @@ impl RusqliteMigration for Migration { JOIN transactions ON transactions.id_tx = utxos.spent_in_tx ), + -- Obtain a count of the notes that the wallet created in each transaction, + -- not counting change notes. sent_note_counts AS ( SELECT sent_notes.from_account_id AS account_id, transactions.txid AS txid, From 85d79fbb8aeed12f75d63a757f837427a8d50199 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 20:30:12 +0000 Subject: [PATCH 11/22] zcash_client_sqlite: Distinguish "seed not relevant" in migration errors --- zcash_client_sqlite/CHANGELOG.md | 1 + zcash_client_sqlite/src/wallet/init.rs | 17 +++++++ .../init/migrations/full_account_ids.rs | 40 ++++++++++++++--- .../wallet/init/migrations/ufvk_support.rs | 44 ++++++++++++++++--- 4 files changed, 91 insertions(+), 11 deletions(-) diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index d4242f89d..db7bc856c 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -36,6 +36,7 @@ and this library adheres to Rust's notion of - `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/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 26bd72a35..87584248b 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -21,6 +21,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 +84,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) } 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 1e4363872..2727f34df 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 @@ -86,6 +86,23 @@ impl RusqliteMigration for Migration

{ if let Some(seed) = &self.seed.as_ref() { 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 88fd58af6..58c69a97d 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ufvk_support.rs @@ -68,6 +68,21 @@ 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()? { @@ -81,7 +96,15 @@ impl RusqliteMigration for Migration

{ })?; 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); From 703e50ae03791905fa8b8d2eb1a9d09e08e5706a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 20:59:52 +0000 Subject: [PATCH 12/22] Add `Account::uivk` The blanket `impl Account for (A, Option)` is removed because we cannot know the UIVK for `(A, None)`. We instead provide a blanket impl for `(A, UnifiedIncomingViewingKey)`. We also move both of them behind `test-dependencies` because they are only intended for testing purposes. --- zcash_client_backend/src/data_api.rs | 25 ++++++++++++++++++++++--- zcash_client_sqlite/src/wallet.rs | 4 ++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 901a36843..909aa6884 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() } } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 858907a9d..d1a9615f9 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -223,6 +223,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 { From c67b17dc962112a37f6b3ee9f096b6d744a994f9 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 21:13:39 +0000 Subject: [PATCH 13/22] zcash_client_sqlite: Extract `seed_matches_derived_account` helper --- zcash_client_sqlite/src/lib.rs | 33 +++--------------------- zcash_client_sqlite/src/wallet.rs | 42 ++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 469299c17..fb9ca3777 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) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index d1a9615f9..413b0c9ec 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}, @@ -245,6 +248,43 @@ 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; + + let usk = UnifiedSpendingKey::from_seed(params, &seed.expose_secret()[..], account_index) + .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 uivk_match = + 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 From 2d8a7dc4af4f284efb0a8d9838b849bb297175a1 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 22:00:56 +0000 Subject: [PATCH 14/22] zcash_client_sqlite: Remove `SqliteClientError::InvalidNoteId` --- zcash_client_sqlite/CHANGELOG.md | 1 + zcash_client_sqlite/src/error.rs | 6 ------ 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index db7bc856c..1fadd340f 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -32,6 +32,7 @@ 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` diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 1271e9ca7..6796c4142 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), From 8c7f8d07ba3938e2933f567f6ef0662b7683d778 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 22:28:50 +0000 Subject: [PATCH 15/22] zcash_client_sqlite: Fix bug in `WalletDb::validate_seed` The previous implementation was mixing the caller-provided seed with the wallet-provided ZIP 32 account index, and throwing an error if the USK derivation failed. We instead need to count that as a mismatch, because the wallet account's actual seed would derive a USK fine (because wallet accounts are required to have a known UIVK). --- zcash_client_sqlite/src/wallet.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 413b0c9ec..b6681ade7 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -262,18 +262,24 @@ pub(crate) fn seed_matches_derived_account( ) })? == seed_fingerprint; - let usk = UnifiedSpendingKey::from_seed(params, &seed.expose_secret()[..], account_index) - .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 uivk_match = - 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)?) - })?; + 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. From e6bc21b4613ef17690023c64d026e497fbc4fe3c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 22:41:39 +0000 Subject: [PATCH 16/22] Add `WalletRead::is_seed_relevant_to_any_derived_accounts` --- zcash_client_backend/CHANGELOG.md | 3 ++- zcash_client_backend/src/data_api.rs | 14 ++++++++++++ zcash_client_sqlite/src/lib.rs | 33 ++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d4fc361df..1b82c0bb9 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 909aa6884..c27d9b137 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -740,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, @@ -1719,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/src/lib.rs b/zcash_client_sqlite/src/lib.rs index fb9ca3777..6b8fd2982 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -337,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, From 4fa0547b84e3ffe43554e8053cc84e55ccb859e3 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 18 Mar 2024 22:42:01 +0000 Subject: [PATCH 17/22] zcash_client_sqlite: Always check for seed relevance in `init_wallet_db` Closes zcash/librustzcash#1283. --- zcash_client_sqlite/src/wallet/init.rs | 133 ++++++++++++++++-- .../src/wallet/init/migrations.rs | 3 +- .../init/migrations/full_account_ids.rs | 4 +- .../wallet/init/migrations/ufvk_support.rs | 4 +- 4 files changed, 127 insertions(+), 17 deletions(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 87584248b..20d01b08b 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; @@ -118,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 @@ -126,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 @@ -165,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 @@ -178,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)?; @@ -190,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(()) } @@ -221,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 { @@ -1310,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 044065082..9ee86f7c7 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 2727f34df..a7099a61a 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,7 +83,7 @@ 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."); 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 58c69a97d..b4af04235 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

{ @@ -89,7 +89,7 @@ impl RusqliteMigration for Migration

{ // 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()) From 3090aff87f6964247f6c5a0d7e8ab20c9ef06392 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 19 Mar 2024 03:43:03 +0000 Subject: [PATCH 18/22] Distinguish seed relevance when no derived accounts are present During wallet migration in particular, the absence of _any_ accounts is expected, and all seeds should be treated as relevant (because accounts cannot be added before a wallet is initialized). --- zcash_client_backend/CHANGELOG.md | 1 + zcash_client_backend/src/data_api.rs | 33 ++++++++++++++++----- zcash_client_sqlite/Cargo.toml | 2 +- zcash_client_sqlite/src/lib.rs | 33 +++++++++++++++------ zcash_client_sqlite/src/wallet/init.rs | 40 +++++++++++++++++++++----- 5 files changed, 86 insertions(+), 23 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 1b82c0bb9..9873992fc 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -22,6 +22,7 @@ and this library adheres to Rust's notion of - `NoteRetention` - `ScannedBlock::orchard` - `ScannedBlockCommitments::orchard` + - `SeedRelevance` - `SentTransaction::new` - `SpendableNotes` - `ORCHARD_SHARD_HEIGHT` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index c27d9b137..50841a87f 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -64,6 +64,7 @@ use std::{ }; use incrementalmerkletree::{frontier::Frontier, Retention}; +use nonempty::NonEmpty; use secrecy::SecretVec; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; use zip32::fingerprint::SeedFingerprint; @@ -742,10 +743,13 @@ pub trait WalletRead { /// 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( + /// + /// This API does not check whether the seed is relevant to any imported account, + /// because that would require brute-forcing the ZIP 32 account index space. + fn seed_relevance_to_derived_accounts( &self, seed: &SecretVec, - ) -> Result; + ) -> Result, Self::Error>; /// Returns the account corresponding to a given [`UnifiedFullViewingKey`], if any. fn get_account_for_ufvk( @@ -907,6 +911,21 @@ pub trait WalletRead { } } +/// The relevance of a seed to a given wallet. +/// +/// This is the return type for [`WalletRead::seed_relevance_to_derived_accounts`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum SeedRelevance { + /// The seed is relevant to at least one derived account within the wallet. + Relevant { account_ids: NonEmpty }, + /// The seed is not relevant to any of the derived accounts within the wallet. + NotRelevant, + /// The wallet contains no derived accounts. + NoDerivedAccounts, + /// The wallet contains no accounts. + NoAccounts, +} + /// Metadata describing the sizes of the zcash note commitment trees as of a particular block. #[derive(Debug, Clone, Copy)] pub struct BlockMetadata { @@ -1632,8 +1651,8 @@ pub mod testing { chain::{ChainState, CommitmentTreeRoot}, scanning::ScanRange, AccountBirthday, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, - ScannedBlock, SentTransaction, SpendableNotes, WalletCommitmentTrees, WalletRead, - WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, WalletCommitmentTrees, + WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }; #[cfg(feature = "transparent-inputs")] @@ -1726,11 +1745,11 @@ pub mod testing { Ok(false) } - fn is_seed_relevant_to_any_derived_accounts( + fn seed_relevance_to_derived_accounts( &self, _seed: &SecretVec, - ) -> Result { - Ok(false) + ) -> Result, Self::Error> { + Ok(SeedRelevance::NoAccounts) } fn get_account_for_ufvk( diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 2c247b393..45e475452 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -45,6 +45,7 @@ tracing.workspace = true # - Serialization byteorder.workspace = true +nonempty.workspace = true prost.workspace = true group.workspace = true jubjub.workspace = true @@ -82,7 +83,6 @@ bls12_381.workspace = true incrementalmerkletree = { workspace = true, features = ["test-dependencies"] } pasta_curves.workspace = true shardtree = { workspace = true, features = ["legacy-api", "test-dependencies"] } -nonempty.workspace = true orchard = { workspace = true, features = ["test-dependencies"] } proptest.workspace = true rand_chacha.workspace = true diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 6b8fd2982..bf822eeff 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -37,6 +37,7 @@ use maybe_rayon::{ prelude::{IndexedParallelIterator, ParallelIterator}, slice::ParallelSliceMut, }; +use nonempty::NonEmpty; use rusqlite::{self, Connection}; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, ShardTree}; @@ -61,8 +62,8 @@ use zcash_client_backend::{ chain::{BlockSource, ChainState, CommitmentTreeRoot}, scanning::{ScanPriority, ScanRange}, Account, AccountBirthday, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, - NullifierQuery, ScannedBlock, SentTransaction, SpendableNotes, WalletCommitmentTrees, - WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + NullifierQuery, ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, + WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }, keys::{ AddressGenerationError, UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey, @@ -337,11 +338,16 @@ impl, P: consensus::Parameters> WalletRead for W } } - fn is_seed_relevant_to_any_derived_accounts( + fn seed_relevance_to_derived_accounts( &self, seed: &SecretVec, - ) -> Result { + ) -> Result, Self::Error> { + let mut has_accounts = false; + let mut has_derived = false; + let mut relevant_account_ids = vec![]; + for account_id in self.get_account_ids()? { + has_accounts = true; let account = self.get_account(account_id)?.expect("account ID exists"); // If the account is imported, the seed _might_ be relevant, but the only @@ -353,6 +359,8 @@ impl, P: consensus::Parameters> WalletRead for W account_index, } = account.source() { + has_derived = true; + if wallet::seed_matches_derived_account( &self.params, seed, @@ -360,14 +368,23 @@ impl, P: consensus::Parameters> WalletRead for W account_index, &account.uivk(), )? { - // The seed is relevant to this account. No need to check any others. - return Ok(true); + // The seed is relevant to this account. + relevant_account_ids.push(account_id); } } } - // The seed was not relevant to any of the accounts in the wallet. - Ok(false) + Ok( + if let Some(account_ids) = NonEmpty::from_vec(relevant_account_ids) { + SeedRelevance::Relevant { account_ids } + } else if has_derived { + SeedRelevance::NotRelevant + } else if has_accounts { + SeedRelevance::NoDerivedAccounts + } else { + SeedRelevance::NoAccounts + }, + ) } fn get_account_for_ufvk( diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 20d01b08b..5e128d46b 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -9,7 +9,10 @@ use secrecy::SecretVec; use shardtree::error::ShardTreeError; use uuid::Uuid; -use zcash_client_backend::{data_api::WalletRead, keys::AddressGenerationError}; +use zcash_client_backend::{ + data_api::{SeedRelevance, WalletRead}, + keys::AddressGenerationError, +}; use zcash_primitives::{consensus, transaction::components::amount::BalanceError}; use super::commitment_tree; @@ -289,11 +292,18 @@ fn init_wallet_db_internal( // 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) + match wdb + .seed_relevance_to_derived_accounts(&seed) .map_err(sqlite_client_error_to_wallet_migration_error)? { - return Err(WalletMigrationError::SeedNotRelevant.into()); + SeedRelevance::Relevant { .. } => (), + // Every seed is relevant to a wallet with no accounts; this is most likely a + // new wallet database being initialized for the first time. + SeedRelevance::NoAccounts => (), + // No seed is relevant to a wallet that only has imported accounts. + SeedRelevance::NotRelevant | SeedRelevance::NoDerivedAccounts => { + return Err(WalletMigrationError::SeedNotRelevant.into()) + } } } @@ -1419,12 +1429,16 @@ mod tests { let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap(); assert_matches!(init_wallet_db(&mut db_data, None), Ok(_)); + // Prior to adding any accounts, every seed phrase is relevant to the wallet. let seed = test_vectors::UNIFIED[0].root_seed; + let other_seed = [7; 32]; assert_matches!( init_wallet_db(&mut db_data, Some(Secret::new(seed.to_vec()))), - Err(schemer::MigratorError::Adapter( - WalletMigrationError::SeedNotRelevant - )) + Ok(()) + ); + assert_matches!( + init_wallet_db(&mut db_data, Some(Secret::new(other_seed.to_vec()))), + Ok(()) ); let birthday = AccountBirthday::from_sapling_activation(&network); @@ -1439,6 +1453,18 @@ mod tests { ) ); + // After adding an account, only the real seed phrase is relevant to the wallet. + assert_matches!( + init_wallet_db(&mut db_data, Some(Secret::new(seed.to_vec()))), + Ok(()) + ); + assert_matches!( + init_wallet_db(&mut db_data, Some(Secret::new(other_seed.to_vec()))), + Err(schemer::MigratorError::Adapter( + WalletMigrationError::SeedNotRelevant + )) + ); + for tv in &test_vectors::UNIFIED[..3] { if let Some(Address::Unified(tvua)) = Address::decode(&Network::MainNetwork, tv.unified_addr) From 3c1e82a0c87ba4e6b9b69014d3fc218881ff2fe3 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 19 Mar 2024 17:44:04 +0000 Subject: [PATCH 19/22] zcash_client_sqlite: Add missing feature flags to error helper fn --- zcash_client_sqlite/src/wallet/init.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 5e128d46b..5772674ea 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -137,6 +137,7 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet SqliteClientError::Bech32DecodeError(e) => { WalletMigrationError::CorruptedData(e.to_string()) } + #[cfg(feature = "transparent-inputs")] SqliteClientError::HdwalletError(e) => WalletMigrationError::CorruptedData(e.to_string()), SqliteClientError::TransparentAddress(e) => { WalletMigrationError::CorruptedData(e.to_string()) @@ -158,10 +159,13 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet | SqliteClientError::KeyDerivationError(_) | SqliteClientError::AccountIdDiscontinuity | SqliteClientError::AccountIdOutOfRange - | SqliteClientError::AddressNotRecognized(_) | SqliteClientError::CacheMiss(_) => { unreachable!("we only call WalletRead methods; mutations can't occur") } + #[cfg(feature = "transparent-inputs")] + SqliteClientError::AddressNotRecognized(_) => { + unreachable!("we only call WalletRead methods; mutations can't occur") + } SqliteClientError::AccountUnknown => { unreachable!("all accounts are known in migration context") } From b189fe7a36df68d7c233b58639b4f5e752f370f6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 19 Mar 2024 14:05:45 +0000 Subject: [PATCH 20/22] Remove `orchard` feature flag from behind `zcash_unstable` cfg flag --- .github/workflows/ci.yml | 1 - zcash_client_backend/src/lib.rs | 5 ----- 2 files changed, 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ebf418cfe..3e3e10aa8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,6 @@ jobs: - state: Orchard extra_flags: orchard - rustflags: '--cfg zcash_unstable="orchard"' - state: NU6 rustflags: '--cfg zcash_unstable="nu6"' diff --git a/zcash_client_backend/src/lib.rs b/zcash_client_backend/src/lib.rs index 7c5d32cef..a6928a1b3 100644 --- a/zcash_client_backend/src/lib.rs +++ b/zcash_client_backend/src/lib.rs @@ -83,8 +83,3 @@ pub use zcash_protocol::{PoolType, ShieldedProtocol}; #[cfg(test)] #[macro_use] extern crate assert_matches; - -#[cfg(all(feature = "orchard", not(zcash_unstable = "orchard")))] -core::compile_error!( - "The `orchard` feature flag requires the `zcash_unstable=\"orchard\"` RUSTFLAG." -); From 4f7c5bd722f6592a2fcd111adaf443bbdd10cd08 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 18 Mar 2024 15:29:16 -0600 Subject: [PATCH 21/22] zcash_client_sqlite: Fix `scan_complete` tests. --- Cargo.lock | 11 +-- Cargo.toml | 7 +- zcash_client_backend/CHANGELOG.md | 1 + zcash_client_backend/src/data_api/chain.rs | 17 +++- zcash_client_backend/src/proto.rs | 12 +++ zcash_client_sqlite/src/testing.rs | 97 ++++++++++++++++++---- zcash_client_sqlite/src/wallet/orchard.rs | 1 + zcash_client_sqlite/src/wallet/sapling.rs | 1 + zcash_client_sqlite/src/wallet/scanning.rs | 92 ++++++++++++-------- 9 files changed, 175 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 820c7cf97..33f697f20 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1087,10 +1087,12 @@ dependencies = [ [[package]] name = "incrementalmerkletree" version = "0.5.0" -source = "git+https://github.com/nuttycom/incrementalmerkletree?rev=fa147c89c6c98a03bba745538f4e68d4eaed5146#fa147c89c6c98a03bba745538f4e68d4eaed5146" +source = "git+https://github.com/zcash/incrementalmerkletree?rev=e1a7a80212c22e5a8912d05860f7eb6899c56a7c#e1a7a80212c22e5a8912d05860f7eb6899c56a7c" dependencies = [ "either", "proptest", + "rand", + "rand_core", ] [[package]] @@ -1475,7 +1477,7 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5" [[package]] name = "orchard" version = "0.7.1" -source = "git+https://github.com/zcash/orchard?rev=e74879dd0ad0918f4ffe0826e03905cd819981bd#e74879dd0ad0918f4ffe0826e03905cd819981bd" +source = "git+https://github.com/zcash/orchard?rev=33474bdbfd7268e1f84718078d47f63d01a879d5#33474bdbfd7268e1f84718078d47f63d01a879d5" dependencies = [ "aes", "bitvec", @@ -2103,8 +2105,7 @@ dependencies = [ [[package]] name = "sapling-crypto" version = "0.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0db258736b34dfc6bec50fab2afb94c1845d90370b38309ebf9bb166cc951251" +source = "git+https://github.com/zcash/sapling-crypto?rev=22412ae07644813253feb064d1692b0823242853#22412ae07644813253feb064d1692b0823242853" dependencies = [ "aes", "bellman", @@ -2244,7 +2245,7 @@ dependencies = [ [[package]] name = "shardtree" version = "0.2.0" -source = "git+https://github.com/nuttycom/incrementalmerkletree?rev=fa147c89c6c98a03bba745538f4e68d4eaed5146#fa147c89c6c98a03bba745538f4e68d4eaed5146" +source = "git+https://github.com/zcash/incrementalmerkletree?rev=e1a7a80212c22e5a8912d05860f7eb6899c56a7c#e1a7a80212c22e5a8912d05860f7eb6899c56a7c" dependencies = [ "assert_matches", "bitflags 2.4.1", diff --git a/Cargo.toml b/Cargo.toml index ab50918ed..2e904303f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -122,6 +122,7 @@ panic = 'abort' codegen-units = 1 [patch.crates-io] -orchard = { git = "https://github.com/zcash/orchard", rev = "e74879dd0ad0918f4ffe0826e03905cd819981bd" } -incrementalmerkletree = { git = "https://github.com/nuttycom/incrementalmerkletree", rev = "fa147c89c6c98a03bba745538f4e68d4eaed5146" } -shardtree = { git = "https://github.com/nuttycom/incrementalmerkletree", rev = "fa147c89c6c98a03bba745538f4e68d4eaed5146" } +orchard = { git = "https://github.com/zcash/orchard", rev = "33474bdbfd7268e1f84718078d47f63d01a879d5" } +incrementalmerkletree = { git = "https://github.com/zcash/incrementalmerkletree", rev = "e1a7a80212c22e5a8912d05860f7eb6899c56a7c" } +sapling = { git = "https://github.com/zcash/sapling-crypto", package = "sapling-crypto", rev = "22412ae07644813253feb064d1692b0823242853" } +shardtree = { git = "https://github.com/zcash/incrementalmerkletree", rev = "e1a7a80212c22e5a8912d05860f7eb6899c56a7c" } diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 9873992fc..2211d07e6 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -30,6 +30,7 @@ and this library adheres to Rust's notion of - `WalletSummary::next_orchard_subtree_index` - `chain::ChainState` - `chain::ScanSummary::{spent_orchard_note_count, received_orchard_note_count}` + - `impl Debug for chain::CommitmentTreeRoot` - `zcash_client_backend::fees`: - `orchard` - `ChangeValue::orchard` diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index cb4e14e40..21e136d71 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -155,7 +155,10 @@ use std::ops::Range; use incrementalmerkletree::frontier::Frontier; use subtle::ConditionallySelectable; -use zcash_primitives::consensus::{self, BlockHeight}; +use zcash_primitives::{ + block::BlockHash, + consensus::{self, BlockHeight}, +}; use crate::{ data_api::{NullifierQuery, WalletWrite}, @@ -172,6 +175,7 @@ use super::WalletRead; /// /// This stores the block height at which the leaf that completed the subtree was /// added, and the root hash of the complete subtree. +#[derive(Debug)] pub struct CommitmentTreeRoot { subtree_end_height: BlockHeight, root_hash: H, @@ -291,6 +295,7 @@ impl ScanSummary { #[derive(Debug, Clone)] pub struct ChainState { block_height: BlockHeight, + block_hash: BlockHash, final_sapling_tree: Frontier, #[cfg(feature = "orchard")] final_orchard_tree: @@ -299,9 +304,10 @@ pub struct ChainState { impl ChainState { /// Construct a new empty chain state. - pub fn empty(block_height: BlockHeight) -> Self { + pub fn empty(block_height: BlockHeight, block_hash: BlockHash) -> Self { Self { block_height, + block_hash, final_sapling_tree: Frontier::empty(), #[cfg(feature = "orchard")] final_orchard_tree: Frontier::empty(), @@ -311,6 +317,7 @@ impl ChainState { /// Construct a new [`ChainState`] from its constituent parts. pub fn new( block_height: BlockHeight, + block_hash: BlockHash, final_sapling_tree: Frontier, #[cfg(feature = "orchard")] final_orchard_tree: Frontier< orchard::tree::MerkleHashOrchard, @@ -319,6 +326,7 @@ impl ChainState { ) -> Self { Self { block_height, + block_hash, final_sapling_tree, #[cfg(feature = "orchard")] final_orchard_tree, @@ -330,6 +338,11 @@ impl ChainState { self.block_height } + /// Return the hash of the block. + pub fn block_hash(&self) -> BlockHash { + self.block_hash + } + /// Returns the frontier of the Sapling note commitment tree as of the end of the block at /// [`Self::block_height`]. pub fn final_sapling_tree( diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 69d89070c..3e58bc309 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -295,10 +295,22 @@ impl service::TreeState { /// /// [`scan_cached_blocks`]: crate::data_api::chain::scan_cached_blocks pub fn to_chain_state(&self) -> io::Result { + let mut hash_bytes = hex::decode(&self.hash).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("Block hash is not valid hex: {:?}", e), + ) + })?; + // Zcashd hex strings for block hashes are byte-reversed. + hash_bytes.reverse(); + Ok(ChainState::new( self.height .try_into() .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "Invalid block height"))?, + BlockHash::try_from_slice(&hash_bytes).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "Invalid block hash length.") + })?, self.sapling_tree()?.to_frontier(), #[cfg(feature = "orchard")] self.orchard_tree()?.to_frontier(), diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 472e094cf..1ce7f74df 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -6,12 +6,14 @@ use std::{collections::BTreeMap, convert::Infallible}; use std::fs::File; use group::ff::Field; +use incrementalmerkletree::Retention; use nonempty::NonEmpty; use prost::Message; use rand_chacha::ChaChaRng; use rand_core::{CryptoRng, RngCore, SeedableRng}; use rusqlite::{params, Connection}; use secrecy::{Secret, SecretVec}; +use shardtree::error::ShardTreeError; use tempfile::NamedTempFile; #[cfg(feature = "unstable")] @@ -28,13 +30,14 @@ use zcash_client_backend::{ address::Address, data_api::{ self, - chain::{scan_cached_blocks, BlockSource, ScanSummary}, + chain::{scan_cached_blocks, BlockSource, CommitmentTreeRoot, ScanSummary}, wallet::{ create_proposed_transactions, create_spend_to_address, input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector}, propose_standard_transfer_to_address, propose_transfer, spend, }, - AccountBalance, AccountBirthday, WalletRead, WalletSummary, WalletWrite, + AccountBalance, AccountBirthday, WalletCommitmentTrees, WalletRead, WalletSummary, + WalletWrite, }, keys::UnifiedSpendingKey, proposal::Proposal, @@ -190,7 +193,6 @@ impl TestBuilder { #[derive(Clone, Debug)] pub(crate) struct CachedBlock { - hash: BlockHash, chain_state: ChainState, sapling_end_size: u32, orchard_end_size: u32, @@ -199,19 +201,13 @@ pub(crate) struct CachedBlock { impl CachedBlock { fn none(sapling_activation_height: BlockHeight) -> Self { Self { - hash: BlockHash([0; 32]), - chain_state: ChainState::empty(sapling_activation_height), + chain_state: ChainState::empty(sapling_activation_height, BlockHash([0; 32])), sapling_end_size: 0, orchard_end_size: 0, } } - fn at( - hash: BlockHash, - chain_state: ChainState, - sapling_end_size: u32, - orchard_end_size: u32, - ) -> Self { + fn at(chain_state: ChainState, sapling_end_size: u32, orchard_end_size: u32) -> Self { assert_eq!( chain_state.final_sapling_tree().tree_size() as u32, sapling_end_size @@ -223,7 +219,6 @@ impl CachedBlock { ); Self { - hash, chain_state, sapling_end_size, orchard_end_size, @@ -258,9 +253,9 @@ impl CachedBlock { }); Self { - hash: cb.hash(), chain_state: ChainState::new( cb.height(), + cb.hash(), sapling_final_tree, #[cfg(feature = "orchard")] orchard_final_tree, @@ -323,6 +318,67 @@ where self.cache.insert(&compact_block) } + /// Ensure that the provided chain state and subtree roots exist in the wallet's note + /// commitment tree(s). This may result in a conflict if either the provided subtree roots or + /// the chain state conflict with existing note commitment tree data. + pub(crate) fn establish_chain_state( + &mut self, + state: ChainState, + prior_sapling_roots: &[CommitmentTreeRoot], + #[cfg(feature = "orchard")] prior_orchard_roots: &[CommitmentTreeRoot], + ) -> Result<(), ShardTreeError> { + self.wallet_mut() + .put_sapling_subtree_roots(0, prior_sapling_roots)?; + #[cfg(feature = "orchard")] + self.wallet_mut() + .put_orchard_subtree_roots(0, prior_orchard_roots)?; + + self.wallet_mut().with_sapling_tree_mut(|t| { + t.insert_frontier( + state.final_sapling_tree().clone(), + Retention::Checkpoint { + id: state.block_height(), + is_marked: false, + }, + ) + })?; + let final_sapling_tree_size = state.final_sapling_tree().tree_size() as u32; + + #[cfg(feature = "orchard")] + self.wallet_mut().with_orchard_tree_mut(|t| { + t.insert_frontier( + state.final_orchard_tree().clone(), + Retention::Checkpoint { + id: state.block_height(), + is_marked: false, + }, + ) + })?; + + let _final_orchard_tree_size = 0; + #[cfg(feature = "orchard")] + let _final_orchard_tree_size = state.final_orchard_tree().tree_size() as u32; + + self.insert_cached_block(state, final_sapling_tree_size, _final_orchard_tree_size); + Ok(()) + } + + fn insert_cached_block( + &mut self, + chain_state: ChainState, + sapling_end_size: u32, + orchard_end_size: u32, + ) { + self.cached_blocks.insert( + chain_state.block_height(), + CachedBlock { + chain_state, + sapling_end_size, + orchard_end_size, + }, + ); + } + /// Creates a fake block at the expected next height containing a single output of the /// given value, and inserts it into the cache. pub(crate) fn generate_next_block( @@ -337,7 +393,7 @@ where let (res, nf) = self.generate_block_at( height, - prior_cached_block.hash, + prior_cached_block.chain_state.block_hash(), fvk, req, value, @@ -376,7 +432,7 @@ where // we need to generate a new prior cached block that the block to be generated can // successfully chain from, with the provided tree sizes. if prior_cached_block.chain_state.block_height() == height - 1 { - assert_eq!(prev_hash, prior_cached_block.hash); + assert_eq!(prev_hash, prior_cached_block.chain_state.block_hash()); } else { let final_sapling_tree = (prior_cached_block.sapling_end_size..initial_sapling_tree_size).fold( @@ -400,9 +456,9 @@ where ); prior_cached_block = CachedBlock::at( - prev_hash, ChainState::new( height - 1, + prev_hash, final_sapling_tree, #[cfg(feature = "orchard")] final_orchard_tree, @@ -452,7 +508,7 @@ where let cb = fake_compact_block_spending( &self.network(), height, - prior_cached_block.hash, + prior_cached_block.chain_state.block_hash(), note, fvk, to.into(), @@ -508,7 +564,7 @@ where let cb = fake_compact_block_from_tx( height, - prior_cached_block.hash, + prior_cached_block.chain_state.block_hash(), tx_index, tx, prior_cached_block.sapling_end_size, @@ -613,6 +669,11 @@ impl TestState { self.db_data.params } + /// Exposes the random number source for the test state + pub(crate) fn rng(&mut self) -> &mut ChaChaRng { + &mut self.rng + } + /// Convenience method for obtaining the Sapling activation height for the network under test. pub(crate) fn sapling_activation_height(&self) -> BlockHeight { self.db_data diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 3d0a575ad..638857859 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -378,6 +378,7 @@ pub(crate) mod tests { impl ShieldedPoolTester for OrchardPoolTester { const SHIELDED_PROTOCOL: ShieldedProtocol = ShieldedProtocol::Orchard; const TABLES_PREFIX: &'static str = ORCHARD_TABLES_PREFIX; + // const MERKLE_TREE_DEPTH: u8 = {orchard::NOTE_COMMITMENT_TREE_DEPTH as u8}; type Sk = SpendingKey; type Fvk = FullViewingKey; diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 14b886100..ddb0fce79 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -399,6 +399,7 @@ pub(crate) mod tests { impl ShieldedPoolTester for SaplingPoolTester { const SHIELDED_PROTOCOL: ShieldedProtocol = ShieldedProtocol::Sapling; const TABLES_PREFIX: &'static str = SAPLING_TABLES_PREFIX; + // const MERKLE_TREE_DEPTH: u8 = sapling::NOTE_COMMITMENT_TREE_DEPTH; type Sk = ExtendedSpendingKey; type Fvk = DiversifiableFullViewingKey; diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 11f1a9d32..511213e77 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -579,12 +579,14 @@ pub(crate) fn update_chain_tip( #[cfg(test)] pub(crate) mod tests { - use incrementalmerkletree::{frontier::Frontier, Hashable, Level, Position}; + use std::num::NonZeroU8; + + use incrementalmerkletree::{frontier::Frontier, Hashable, Position}; use sapling::Node; use secrecy::SecretVec; use zcash_client_backend::data_api::{ - chain::CommitmentTreeRoot, + chain::{ChainState, CommitmentTreeRoot}, scanning::{spanning_tree::testing::scan_range, ScanPriority}, AccountBirthday, Ratio, WalletRead, WalletWrite, SAPLING_SHARD_HEIGHT, }; @@ -611,9 +613,7 @@ pub(crate) mod tests { zcash_client_backend::data_api::ORCHARD_SHARD_HEIGHT, }; - // FIXME: This requires fixes to the test framework. #[test] - #[cfg(feature = "orchard")] fn sapling_scan_complete() { scan_complete::(); } @@ -624,55 +624,75 @@ pub(crate) mod tests { scan_complete::(); } - // FIXME: This requires fixes to the test framework. - #[allow(dead_code)] fn scan_complete() { use ScanPriority::*; - + let initial_height_offset = 310; let mut st = TestBuilder::new() .with_block_cache() .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let dfvk = T::test_account_fvk(&st); let sapling_activation_height = st.sapling_activation_height(); - assert_matches!( - // In the following, we don't care what the root hashes are, they just need to be - // distinct. - T::put_subtree_roots( - &mut st, - 0, - &[ - CommitmentTreeRoot::from_parts( - sapling_activation_height + 100, - T::empty_tree_root(Level::from(0)) - ), - CommitmentTreeRoot::from_parts( - sapling_activation_height + 200, - T::empty_tree_root(Level::from(1)) - ), - CommitmentTreeRoot::from_parts( - sapling_activation_height + 300, - T::empty_tree_root(Level::from(2)) - ), - ] - ), - Ok(()) - ); - // We'll start inserting leaf notes 5 notes after the end of the third subtree, with a gap // of 10 blocks. After `scan_cached_blocks`, the scan queue should have a requested scan // range of 300..310 with `FoundNote` priority, 310..320 with `Scanned` priority. // We set both Sapling and Orchard to the same initial tree size for simplicity. - let initial_sapling_tree_size = (0x1 << 16) * 3 + 5; - let initial_orchard_tree_size = (0x1 << 16) * 3 + 5; - let initial_height = sapling_activation_height + 310; + let initial_sapling_tree_size: u32 = (0x1 << 16) * 3 + 5; + let initial_orchard_tree_size: u32 = (0x1 << 16) * 3 + 5; + // Construct a fake chain state for the end of block 300 + let (prior_sapling_frontiers, sapling_initial_tree) = + Frontier::random_with_prior_subtree_roots( + st.rng(), + initial_sapling_tree_size.into(), + NonZeroU8::new(16).unwrap(), + ); + let sapling_subtree_roots = prior_sapling_frontiers + .into_iter() + .zip(0u32..) + .map(|(root, i)| { + CommitmentTreeRoot::from_parts(sapling_activation_height + (100 * (i + 1)), root) + }) + .collect::>(); + + #[cfg(feature = "orchard")] + let (prior_orchard_frontiers, orchard_initial_tree) = + Frontier::random_with_prior_subtree_roots( + st.rng(), + initial_orchard_tree_size.into(), + NonZeroU8::new(16).unwrap(), + ); + #[cfg(feature = "orchard")] + let orchard_subtree_roots = prior_orchard_frontiers + .into_iter() + .zip(0u32..) + .map(|(root, i)| { + CommitmentTreeRoot::from_parts(sapling_activation_height + (100 * (i + 1)), root) + }) + .collect::>(); + + let prior_block_hash = BlockHash([0; 32]); + st.establish_chain_state( + ChainState::new( + sapling_activation_height + initial_height_offset - 1, + prior_block_hash, + sapling_initial_tree, + #[cfg(feature = "orchard")] + orchard_initial_tree, + ), + &sapling_subtree_roots, + #[cfg(feature = "orchard")] + &orchard_subtree_roots, + ) + .unwrap(); + + let dfvk = T::test_account_fvk(&st); let value = NonNegativeAmount::const_from_u64(50000); + let initial_height = sapling_activation_height + initial_height_offset; st.generate_block_at( initial_height, - BlockHash([0; 32]), + prior_block_hash, &dfvk, AddressType::DefaultExternal, value, From 5f1d75937b84cdc51ad81d7eb2a9b0123c57af3e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 19 Mar 2024 17:15:06 -0600 Subject: [PATCH 22/22] zcash_client_backend: Treat protobuf default as the empty tree. Fixes #1280 --- zcash_client_backend/src/proto.rs | 44 +++++++++++++++++++------------ 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 3e58bc309..f9706c08b 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -263,15 +263,19 @@ impl service::TreeState { pub fn sapling_tree( &self, ) -> io::Result> { - let sapling_tree_bytes = hex::decode(&self.sapling_tree).map_err(|e| { - io::Error::new( - io::ErrorKind::InvalidData, - format!("Hex decoding of Sapling tree bytes failed: {:?}", e), + if self.sapling_tree.is_empty() { + Ok(CommitmentTree::empty()) + } else { + let sapling_tree_bytes = hex::decode(&self.sapling_tree).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("Hex decoding of Sapling tree bytes failed: {:?}", e), + ) + })?; + read_commitment_tree::( + &sapling_tree_bytes[..], ) - })?; - read_commitment_tree::( - &sapling_tree_bytes[..], - ) + } } /// Deserializes and returns the Sapling note commitment tree field of the tree state. @@ -280,15 +284,21 @@ impl service::TreeState { &self, ) -> io::Result> { - let orchard_tree_bytes = hex::decode(&self.orchard_tree).map_err(|e| { - io::Error::new( - io::ErrorKind::InvalidData, - format!("Hex decoding of Orchard tree bytes failed: {:?}", e), - ) - })?; - read_commitment_tree::( - &orchard_tree_bytes[..], - ) + if self.orchard_tree.is_empty() { + Ok(CommitmentTree::empty()) + } else { + let orchard_tree_bytes = hex::decode(&self.orchard_tree).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("Hex decoding of Orchard tree bytes failed: {:?}", e), + ) + })?; + read_commitment_tree::< + MerkleHashOrchard, + _, + { orchard::NOTE_COMMITMENT_TREE_DEPTH as u8 }, + >(&orchard_tree_bytes[..]) + } } /// Parses this tree state into a [`ChainState`] for use with [`scan_cached_blocks`].