From a021b9f2776e20f8971139d4f6bdb071ffbf5143 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sun, 11 Aug 2024 18:30:15 +0100 Subject: [PATCH] Ensure that `zcash_client_sqlite` only enables "orchard" for `zcash_keys` when its own "orchard" feature is enabled. fixes #1488 Signed-off-by: Daira-Emma Hopwood --- .../src/data_api/wallet/input_selection.rs | 4 +-- zcash_client_sqlite/Cargo.toml | 2 +- zcash_client_sqlite/src/lib.rs | 2 -- zcash_client_sqlite/src/wallet.rs | 16 ++++++++++-- .../migrations/ensure_orchard_ua_receiver.rs | 12 ++++----- .../init/migrations/ephemeral_addresses.rs | 5 ++++ .../init/migrations/full_account_ids.rs | 11 ++++++-- zcash_keys/src/address.rs | 26 +++++-------------- zcash_keys/src/keys.rs | 8 +++--- 9 files changed, 48 insertions(+), 38 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index c745a0a8a..343905f73 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -441,13 +441,13 @@ where } Address::Unified(addr) => { #[cfg(feature = "orchard")] - if addr.orchard().is_some() { + if addr.has_orchard() { payment_pools.insert(*idx, PoolType::ORCHARD); orchard_outputs.push(OrchardPayment(payment.amount())); continue; } - if addr.sapling().is_some() { + if addr.has_sapling() { payment_pools.insert(*idx, PoolType::SAPLING); sapling_outputs.push(SaplingPayment(payment.amount())); continue; diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 939dfcc1c..6d6bcb801 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -29,7 +29,7 @@ rustdoc-args = ["--cfg", "docsrs"] zcash_address.workspace = true zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] } zcash_encoding.workspace = true -zcash_keys = { workspace = true, features = ["orchard", "sapling"] } +zcash_keys = { workspace = true, features = ["sapling"] } zcash_primitives.workspace = true zcash_protocol.workspace = true zip32.workspace = true diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index bf801cd21..5a7e453af 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -2159,8 +2159,6 @@ mod tests { ufvk.sapling().cloned(), #[cfg(feature = "orchard")] ufvk.orchard().cloned(), - #[cfg(not(feature = "orchard"))] - None, // see zcash/librustzcash#1488 ) .unwrap(); assert_matches!( diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 823b9e1f4..9e5584375 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -373,12 +373,17 @@ pub(crate) fn add_account( AccountSource::Imported { purpose } => (None, None, purpose == AccountPurpose::Spending), }; + #[cfg(feature = "orchard")] let orchard_item = viewing_key .ufvk() .and_then(|ufvk| ufvk.orchard().map(|k| k.to_bytes())); + #[cfg(not(feature = "orchard"))] + let orchard_item: Option> = None; + let sapling_item = viewing_key .ufvk() .and_then(|ufvk| ufvk.sapling().map(|k| k.to_bytes())); + #[cfg(feature = "transparent-inputs")] let transparent_item = viewing_key .ufvk() @@ -685,6 +690,13 @@ pub(crate) fn get_account_for_ufvk( params: &P, ufvk: &UnifiedFullViewingKey, ) -> Result, SqliteClientError> { + #[cfg(feature = "orchard")] + let orchard_item = ufvk.orchard().map(|k| k.to_bytes()); + #[cfg(not(feature = "orchard"))] + let orchard_item: Option> = None; + + let sapling_item = ufvk.sapling().map(|k| k.to_bytes()); + #[cfg(feature = "transparent-inputs")] let transparent_item = ufvk.transparent().map(|k| k.serialize()); #[cfg(not(feature = "transparent-inputs"))] @@ -701,8 +713,8 @@ pub(crate) fn get_account_for_ufvk( let accounts = stmt .query_and_then::<_, SqliteClientError, _, _>( named_params![ - ":orchard_fvk_item_cache": ufvk.orchard().map(|k| k.to_bytes()), - ":sapling_fvk_item_cache": ufvk.sapling().map(|k| k.to_bytes()), + ":orchard_fvk_item_cache": orchard_item, + ":sapling_fvk_item_cache": sapling_item, ":p2pkh_fvk_item_cache": transparent_item, ], |row| { diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs b/zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs index 1637b4da0..11d38dc61 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ensure_orchard_ua_receiver.rs @@ -166,9 +166,9 @@ mod tests { Ok(Address::decode(&db_data.params, &row.get::<_, String>(0)?).unwrap()) }) { Ok(Address::Unified(ua)) => { - assert!(ua.orchard().is_none()); - assert!(ua.sapling().is_some()); - assert_eq!(ua.transparent().is_some(), UA_TRANSPARENT); + assert!(!ua.has_orchard()); + assert!(ua.has_sapling()); + assert_eq!(ua.has_transparent(), UA_TRANSPARENT); } other => panic!("Unexpected result from address decoding: {:?}", other), } @@ -184,9 +184,9 @@ mod tests { Ok(Address::decode(&db_data.params, &row.get::<_, String>(0)?).unwrap()) }) { Ok(Address::Unified(ua)) => { - assert_eq!(ua.orchard().is_some(), UA_ORCHARD); - assert!(ua.sapling().is_some()); - assert_eq!(ua.transparent().is_some(), UA_TRANSPARENT); + assert_eq!(ua.has_orchard(), UA_ORCHARD); + assert!(ua.has_sapling()); + assert_eq!(ua.has_transparent(), UA_TRANSPARENT); } other => panic!("Unexpected result from address decoding: {:?}", other), } diff --git a/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs b/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs index bda03e76e..686719722 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/ephemeral_addresses.rs @@ -133,8 +133,13 @@ mod tests { .map_err(|_| SqliteClientError::KeyDerivationError(account_index))?; let ufvk = usk.to_unified_full_viewing_key(); + #[cfg(feature = "orchard")] let orchard_item = ufvk.orchard().map(|k| k.to_bytes()); + #[cfg(not(feature = "orchard"))] + let orchard_item: Option> = None; + let sapling_item = ufvk.sapling().map(|k| k.to_bytes()); + #[cfg(feature = "transparent-inputs")] let transparent_item = ufvk.transparent().map(|k| k.serialize()); #[cfg(not(feature = "transparent-inputs"))] 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 cd03ed5d4..dd2de2360 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 @@ -172,6 +172,13 @@ impl RusqliteMigration for Migration

{ .to_unified_incoming_viewing_key() .encode(&self.params); + #[cfg(feature = "orchard")] + let orchard_item = ufvk_parsed.orchard().map(|k| k.to_bytes()); + #[cfg(not(feature = "orchard"))] + let orchard_item: Option> = None; + + let sapling_item = ufvk_parsed.sapling().map(|k| k.to_bytes()); + #[cfg(feature = "transparent-inputs")] let transparent_item = ufvk_parsed.transparent().map(|k| k.serialize()); #[cfg(not(feature = "transparent-inputs"))] @@ -220,8 +227,8 @@ impl RusqliteMigration for Migration

{ ":account_index": account_index, ":ufvk": ufvk, ":uivk": uivk, - ":orchard_fvk_item_cache": ufvk_parsed.orchard().map(|k| k.to_bytes()), - ":sapling_fvk_item_cache": ufvk_parsed.sapling().map(|k| k.to_bytes()), + ":orchard_fvk_item_cache": orchard_item, + ":sapling_fvk_item_cache": sapling_item, ":p2pkh_fvk_item_cache": transparent_item, ":birthday_height": birthday_height, ":birthday_sapling_tree_size": birthday_sapling_tree_size, diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs index 7f2abfdcb..665c03a34 100644 --- a/zcash_keys/src/address.rs +++ b/zcash_keys/src/address.rs @@ -415,21 +415,9 @@ impl Address { matches!(pool_type, PoolType::Transparent) } Address::Unified(ua) => match pool_type { - PoolType::Transparent => ua.transparent().is_some(), - PoolType::Shielded(ShieldedProtocol::Sapling) => { - #[cfg(feature = "sapling")] - return ua.sapling().is_some(); - - #[cfg(not(feature = "sapling"))] - return false; - } - PoolType::Shielded(ShieldedProtocol::Orchard) => { - #[cfg(feature = "orchard")] - return ua.orchard().is_some(); - - #[cfg(not(feature = "orchard"))] - return false; - } + PoolType::Transparent => ua.has_transparent(), + PoolType::Shielded(ShieldedProtocol::Sapling) => ua.has_sapling(), + PoolType::Shielded(ShieldedProtocol::Orchard) => ua.has_orchard(), }, } } @@ -540,15 +528,15 @@ mod tests { fn ua_parsing() { for tv in test_vectors::UNIFIED { match Address::decode(&MAIN_NETWORK, tv.unified_addr) { - Some(Address::Unified(_ua)) => { + Some(Address::Unified(ua)) => { assert_eq!( - _ua.transparent().is_some(), + ua.has_transparent(), tv.p2pkh_bytes.is_some() || tv.p2sh_bytes.is_some() ); #[cfg(feature = "sapling")] - assert_eq!(_ua.sapling().is_some(), tv.sapling_raw_addr.is_some()); + assert_eq!(ua.has_sapling(), tv.sapling_raw_addr.is_some()); #[cfg(feature = "orchard")] - assert_eq!(_ua.orchard().is_some(), tv.orchard_raw_addr.is_some()); + assert_eq!(ua.has_orchard(), tv.orchard_raw_addr.is_some()); } Some(_) => { panic!( diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index 51a39fec7..8e61703bf 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -1477,11 +1477,11 @@ mod tests { 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() { + if tvua.has_transparent() { assert_eq!(tvua.transparent(), ua.transparent()); } #[cfg(feature = "sapling")] - if tvua.sapling().is_some() { + if tvua.has_sapling() { assert_eq!(tvua.sapling(), ua.sapling()); } } @@ -1658,11 +1658,11 @@ mod tests { 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() { + if tvua.has_transparent() { assert_eq!(tvua.transparent(), ua.transparent()); } #[cfg(feature = "sapling")] - if tvua.sapling().is_some() { + if tvua.has_sapling() { assert_eq!(tvua.sapling(), ua.sapling()); } }