diff --git a/zcash_client_backend/src/data_api/testing/orchard.rs b/zcash_client_backend/src/data_api/testing/orchard.rs index 4ca673eb2..0c0ce03d8 100644 --- a/zcash_client_backend/src/data_api/testing/orchard.rs +++ b/zcash_client_backend/src/data_api/testing/orchard.rs @@ -75,6 +75,7 @@ impl ShieldedPoolTester for OrchardPoolTester { None, None, ) + .expect("Address construction is valid") .into() } @@ -154,7 +155,9 @@ impl ShieldedPoolTester for OrchardPoolTester { return result.map(|(note, addr, memo)| { ( Note::Orchard(note), - UnifiedAddress::from_receivers(Some(addr), None, None).into(), + UnifiedAddress::from_receivers(Some(addr), None, None) + .expect("Address construction is valid") + .into(), MemoBytes::from_bytes(&memo).expect("correct length"), ) }); diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 9ab62ca6f..e6e0e4b1a 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -24,6 +24,7 @@ and this library adheres to Rust's notion of unknown_data, unknown_metadata }` - `zcash_keys::address::UnifiedAddressRequest::unsafe_new_without_expiry` +- `zcash_keys::keys::UnifiedKeyError` ### Changed - `zcash_keys::address::UnifiedAddressRequest::new` now takes additional @@ -38,17 +39,28 @@ and this library adheres to Rust's notion of - `zcash_keys::keys::UnifiedIncomingViewingKey::default_address` - `zcash_keys::address::UnifiedAddress::from_receivers` is now only available under the `test-dependencies` feature flag. It should not be used for - non-test purposes as it can potentially generate addresses that contain no - receivers. + non-test purposes. +- `zcash_keys::address::UnifiedAddress` can now represent ZIP 316 Revision 1 + unified addresses, which may lack any shielded receivers; addresses are now + constrained to include at least one data item, but are not otherwise + restricted in their receiver composition. - `zcash_keys::keys::UnifiedSpendingKey::default_address` is now failable, and now returns a `Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError>`. - `zcash_keys::address::Address` variants now `Box` their contents to avoid large discrepancies in enum variant sizing. +- `zcash_keys::keys::DecodingError` has added variant `ConstraintViolation`. +- The following methods now return `Result<_, UnifiedKeyError>` instead of + `Result<_, DerivationError>` + - `zcash_keys::keys::UnifiedSpendingKey::from_seed` + - `zcash_keys::keys::UnifiedFullViewingKey::new` + - `zcash_keys::keys::UnifiedFullViewingKey::from_orchard_fvk` + - `zcash_keys::keys::UnifiedFullViewingKey::from_sapling_extended_full_viewing_key` ### Removed - `zcash_keys::address::UnifiedAddress::unknown` (use `unknown_data` instead.) -- `zcash_keys::address::UnifiedAddressRequest::unsafe_new` (use +- `zcash_keys::address::UnifiedAddressRequest::unsafe_new` (use `UnifiedAddressRequest::unsafe_new_without_expiry` instead) +- `zcash_keys::keys::DerivationError` (use `UnifiedKeyError` instead). ## [0.5.0] - 2024-11-14 diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs index 17822fab5..2f689c67b 100644 --- a/zcash_keys/src/address.rs +++ b/zcash_keys/src/address.rs @@ -93,7 +93,7 @@ impl TryFrom for UnifiedAddress { } } - Ok(Self { + Self::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] @@ -103,7 +103,8 @@ impl TryFrom for UnifiedAddress { expiry_height, expiry_time, unknown_metadata, - }) + ) + .ok_or("Unified addresses without data fields are not permitted.") } } @@ -118,36 +119,55 @@ impl UnifiedAddress { #[cfg(feature = "orchard")] orchard: Option, #[cfg(feature = "sapling")] sapling: Option, transparent: Option, - ) -> Self { - Self::new_internal( + ) -> Option { + Self::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, + vec![], None, None, + vec![], ) } - pub(crate) fn new_internal( + pub(crate) fn from_checked_parts( #[cfg(feature = "orchard")] orchard: Option, #[cfg(feature = "sapling")] sapling: Option, transparent: Option, + unknown_data: Vec<(u32, Vec)>, expiry_height: Option, expiry_time: Option, - ) -> Self { - Self { + unknown_metadata: Vec<(u32, Vec)>, + ) -> Option { + let has_transparent = transparent.is_some(); + + #[allow(unused_mut)] + let mut has_shielded = false; + #[cfg(feature = "sapling")] + { + has_shielded = has_shielded || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_shielded = has_shielded || orchard.is_some(); + } + + let has_unknown = !unknown_data.is_empty(); + + (has_transparent || has_shielded || has_unknown).then_some(Self { #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, - unknown_data: vec![], + unknown_data, expiry_height, expiry_time, - unknown_metadata: vec![], - } + unknown_metadata, + }) } /// Returns whether this address has an Orchard receiver. @@ -599,15 +619,25 @@ mod tests { let transparent = None; #[cfg(all(feature = "orchard", feature = "sapling"))] - let ua = UnifiedAddress::new_internal(orchard, sapling, transparent, None, None); + let ua = UnifiedAddress::from_checked_parts( + orchard, + sapling, + transparent, + vec![], + None, + None, + vec![], + ); #[cfg(all(not(feature = "orchard"), feature = "sapling"))] - let ua = UnifiedAddress::new_internal(sapling, transparent, None, None); + let ua = + UnifiedAddress::from_checked_parts(sapling, transparent, vec![], None, None, vec![]); #[cfg(all(feature = "orchard", not(feature = "sapling")))] - let ua = UnifiedAddress::new_internal(orchard, transparent, None, None); + let ua = + UnifiedAddress::from_checked_parts(orchard, transparent, vec![], None, None, vec![]); - let addr = Address::from(ua); + let addr = Address::from(ua.expect("test UAs are constructed in valid configurations")); let addr_str = addr.encode(&MAIN_NETWORK); assert_eq!(Address::decode(&MAIN_NETWORK, &addr_str), Some(addr)); } @@ -616,7 +646,10 @@ mod tests { #[cfg(not(any(feature = "orchard", feature = "sapling")))] fn ua_round_trip() { let transparent = None; - assert_eq!(UnifiedAddress::new_internal(transparent, None, None), None) + assert_eq!( + UnifiedAddress::from_checked_parts(transparent, vec![], None, None, vec![]), + None + ) } #[test] diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index a4dca03d9..c12001e23 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -91,30 +91,40 @@ fn to_transparent_child_index(j: DiversifierIndex) -> Option) -> fmt::Result { match self { + UnifiedKeyError::DataItemRequired => write!( + _f, + "Unified keys must contain at least one non-metadata item." + ), #[cfg(feature = "orchard")] - DerivationError::Orchard(e) => write!(_f, "Orchard error: {}", e), + UnifiedKeyError::Orchard(e) => write!(_f, "Orchard key derivation error: {}", e), #[cfg(feature = "transparent-inputs")] - DerivationError::Transparent(e) => write!(_f, "Transparent error: {}", e), + UnifiedKeyError::Transparent(e) => { + write!(_f, "Transparent key derivation error: {}", e) + } #[cfg(not(any(feature = "orchard", feature = "transparent-inputs")))] other => { - unreachable!("Unhandled DerivationError variant {:?}", other) + unreachable!("Unhandled UnifiedKeyError variant {:?}", other) } } } } -impl std::error::Error for DerivationError {} +impl std::error::Error for UnifiedKeyError {} /// A version identifier for the encoding of unified spending keys. /// @@ -147,8 +157,11 @@ pub enum DecodingError { LengthMismatch(Typecode, u32), #[cfg(feature = "unstable")] InsufficientData(Typecode), - /// The key data could not be decoded from its string representation to a valid key. + /// The key data for the given key type could not be decoded from its string representation to + /// a valid key. KeyDataInvalid(Typecode), + /// Decoding resulted in a value that would violate validity constraints. + ConstraintViolation(String), } impl std::fmt::Display for DecodingError { @@ -177,12 +190,33 @@ impl std::fmt::Display for DecodingError { write!(f, "Insufficient data for typecode {:?}", t) } DecodingError::KeyDataInvalid(t) => write!(f, "Invalid key data for key type {:?}", t), + DecodingError::ConstraintViolation(s) => { + write!(f, "Decoding produced an invalid value: {}", s) + } } } } impl std::error::Error for DecodingError {} +impl From for DecodingError { + fn from(value: UnifiedKeyError) -> Self { + match value { + UnifiedKeyError::DataItemRequired => { + Self::ConstraintViolation("At least one Data Item must be present.".to_owned()) + } + #[cfg(feature = "orchard")] + UnifiedKeyError::Orchard(_) => { + Self::KeyDataInvalid(Typecode::Data(unified::DataTypecode::Orchard)) + } + #[cfg(feature = "transparent-inputs")] + UnifiedKeyError::Transparent(_) => { + Self::KeyDataInvalid(Typecode::Data(unified::DataTypecode::P2pkh)) + } + } + } +} + #[cfg(feature = "unstable")] impl Era { /// Returns the unique identifier for the era. @@ -218,7 +252,7 @@ impl UnifiedSpendingKey { _params: &P, seed: &[u8], _account: AccountId, - ) -> Result { + ) -> Result { if seed.len() < 32 { panic!("ZIP 32 seeds MUST be at least 32 bytes"); } @@ -226,12 +260,12 @@ impl UnifiedSpendingKey { UnifiedSpendingKey::from_checked_parts( #[cfg(feature = "transparent-inputs")] legacy::AccountPrivKey::from_seed(_params, seed, _account) - .map_err(DerivationError::Transparent)?, + .map_err(UnifiedKeyError::Transparent)?, #[cfg(feature = "sapling")] sapling::spending_key(seed, _params.coin_type(), _account), #[cfg(feature = "orchard")] orchard::keys::SpendingKey::from_zip32_seed(seed, _params.coin_type(), _account) - .map_err(DerivationError::Orchard)?, + .map_err(UnifiedKeyError::Orchard)?, ) } @@ -241,7 +275,7 @@ impl UnifiedSpendingKey { #[cfg(feature = "transparent-inputs")] transparent: legacy::AccountPrivKey, #[cfg(feature = "sapling")] sapling: sapling::ExtendedSpendingKey, #[cfg(feature = "orchard")] orchard: orchard::keys::SpendingKey, - ) -> Result { + ) -> 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")] @@ -458,7 +492,7 @@ impl UnifiedSpendingKey { #[cfg(feature = "orchard")] orchard.unwrap(), ) - .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2PKH)); + .map_err(DecodingError::from); } } } @@ -630,14 +664,15 @@ impl UnifiedAddressRequest { /// Construct a new unified address request from its constituent parts. /// - /// Panics: at least one of `has_orchard`, `has_sapling`, or `has_p2pkh` must be `true`. + /// Panics: at least one of `has_orchard` or `has_sapling` must be `true`. P2pkh-only + /// addresses without additional metadata are currently disallowed by ZIP 316. pub const fn unsafe_new_without_expiry( has_orchard: bool, has_sapling: bool, has_p2pkh: bool, ) -> Self { - if !(has_orchard || has_sapling || has_p2pkh) { - panic!("At least one receiver must be requested.") + if !(has_orchard || has_sapling) { + panic!("At least one shielded receiver must be requested.") } Self { @@ -651,9 +686,9 @@ impl UnifiedAddressRequest { } #[cfg(feature = "transparent-inputs")] -impl From for DerivationError { +impl From for UnifiedKeyError { fn from(e: bip32::Error) -> Self { - DerivationError::Transparent(e) + UnifiedKeyError::Transparent(e) } } @@ -684,7 +719,7 @@ impl UnifiedFullViewingKey { #[cfg(feature = "sapling")] sapling: Option, #[cfg(feature = "orchard")] orchard: Option, // TODO: Implement construction of UFVKs with metadata items. - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] transparent, @@ -704,7 +739,7 @@ impl UnifiedFullViewingKey { #[cfg(feature = "unstable-frost")] pub fn from_orchard_fvk( orchard: orchard::keys::FullViewingKey, - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] None, @@ -724,7 +759,7 @@ impl UnifiedFullViewingKey { #[cfg(all(feature = "sapling", feature = "unstable"))] pub fn from_sapling_extended_full_viewing_key( sapling: ExtendedFullViewingKey, - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] None, @@ -751,7 +786,7 @@ impl UnifiedFullViewingKey { expiry_height: Option, expiry_time: Option, unknown_metadata: Vec<(u32, Vec)>, - ) -> Result { + ) -> Result { // Verify that IVK derivation succeeds; we don't want to construct a UFVK // that can't derive transparent addresses. #[cfg(feature = "transparent-inputs")] @@ -760,18 +795,37 @@ impl UnifiedFullViewingKey { .map(|t| t.derive_external_ivk()) .transpose()?; - Ok(UnifiedFullViewingKey { - #[cfg(feature = "transparent-inputs")] - transparent, - #[cfg(feature = "sapling")] - sapling, - #[cfg(feature = "orchard")] - orchard, - unknown_data, - expiry_height, - expiry_time, - unknown_metadata, - }) + #[allow(unused_mut)] + let mut has_data = !unknown_data.is_empty(); + #[cfg(feature = "transparent-inputs")] + { + has_data = has_data || transparent.is_some(); + } + #[cfg(feature = "sapling")] + { + has_data = has_data || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_data = has_data || orchard.is_some(); + } + + if has_data { + Ok(Self { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + unknown_data, + expiry_height, + expiry_time, + unknown_metadata, + }) + } else { + Err(UnifiedKeyError::DataItemRequired) + } } /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. @@ -1132,6 +1186,50 @@ impl UnifiedIncomingViewingKey { } } + fn from_checked_parts( + #[cfg(feature = "transparent-inputs")] transparent: Option< + zcash_primitives::legacy::keys::ExternalIvk, + >, + #[cfg(feature = "sapling")] sapling: Option<::sapling::zip32::IncomingViewingKey>, + #[cfg(feature = "orchard")] orchard: Option, + unknown_data: Vec<(u32, Vec)>, + expiry_height: Option, + expiry_time: Option, + unknown_metadata: Vec<(u32, Vec)>, + ) -> Result { + #[allow(unused_mut)] + let mut has_data = !unknown_data.is_empty(); + #[cfg(feature = "transparent-inputs")] + { + has_data = has_data || transparent.is_some(); + } + #[cfg(feature = "sapling")] + { + has_data = has_data || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_data = has_data || orchard.is_some(); + } + + if has_data { + Ok(Self { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + unknown_data, + expiry_height, + expiry_time, + unknown_metadata, + }) + } else { + Err(UnifiedKeyError::DataItemRequired) + } + } + /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. /// /// [ZIP 316]: https://zips.z.cash/zip-0316 @@ -1216,7 +1314,7 @@ impl UnifiedIncomingViewingKey { } } - Ok(Self { + Ok(Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] transparent, #[cfg(feature = "sapling")] @@ -1227,7 +1325,7 @@ impl UnifiedIncomingViewingKey { expiry_height, expiry_time, unknown_metadata, - }) + )?) } /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. @@ -1436,12 +1534,13 @@ impl UnifiedIncomingViewingKey { #[cfg(not(feature = "transparent-inputs"))] let transparent = None; - Ok(UnifiedAddress::new_internal( + Ok(UnifiedAddress::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, + self.unknown_data.clone(), self.expiry_height .zip(request.expiry_height) .map(|(l, r)| std::cmp::min(l, r)) @@ -1452,7 +1551,9 @@ impl UnifiedIncomingViewingKey { .map(|(l, r)| std::cmp::min(l, r)) .or(self.expiry_time) .or(request.expiry_time), - )) + self.unknown_metadata.clone(), + ) + .expect("UIVK validity constraints are checked at construction.")) } /// Searches the diversifier space starting at diversifier index `j` for one which will @@ -1583,13 +1684,11 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - crate::{address::Address, encoding::AddressCodec}, - zcash_address::test_vectors, + crate::encoding::AddressCodec, zcash_primitives::legacy::keys::{AccountPrivKey, IncomingViewingKey}, - zip32::DiversifierIndex, }; - #[cfg(feature = "unstable")] + #[cfg(all(feature = "unstable", any(feature = "sapling", feature = "orchard")))] use super::{testing::arb_unified_spending_key, Era, UnifiedSpendingKey}; #[cfg(all(feature = "orchard", feature = "unstable"))] @@ -1746,9 +1845,14 @@ mod tests { } #[test] - #[cfg(feature = "transparent-inputs")] + #[cfg(all( + feature = "transparent-inputs", + any(feature = "orchard", feature = "sapling") + ))] fn ufvk_derivation() { - use crate::keys::UnifiedAddressRequest; + use crate::{address::Address, keys::UnifiedAddressRequest}; + use zcash_address::test_vectors; + use zip32::DiversifierIndex; use super::UnifiedSpendingKey; @@ -1934,9 +2038,14 @@ mod tests { } #[test] - #[cfg(feature = "transparent-inputs")] + #[cfg(all( + feature = "transparent-inputs", + any(feature = "sapling", feature = "orchard") + ))] fn uivk_derivation() { - use crate::keys::UnifiedAddressRequest; + use crate::{address::Address, keys::UnifiedAddressRequest}; + use zcash_address::test_vectors; + use zip32::DiversifierIndex; use super::UnifiedSpendingKey; @@ -1998,7 +2107,7 @@ mod tests { proptest! { #[test] - #[cfg(feature = "unstable")] + #[cfg(all(feature = "unstable", any(feature = "orchard", feature = "sapling")))] fn prop_usk_roundtrip(usk in arb_unified_spending_key(zcash_protocol::consensus::Network::MainNetwork)) { let encoded = usk.to_bytes(Era::Orchard); @@ -2027,6 +2136,7 @@ mod tests { #[cfg(feature = "orchard")] assert!(bool::from(decoded.orchard().ct_eq(usk.orchard()))); + #[cfg(feature = "sapling")] assert_eq!(decoded.sapling(), usk.sapling()); #[cfg(feature = "transparent-inputs")]