Merge pull request #1491 from daira/consistent-orchard-enabling

Ensure that `zcash_client_sqlite` only enables "orchard" for `zcash_keys` when its own "orchard" feature is enabled

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;

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

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!(

zcash_client_sqlite/src/wallet.rs

@@ -373,12 +373,17 @@ pub(crate) fn add_account<P: consensus::Parameters>(
         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<Vec<u8>> = 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<P: consensus::Parameters>(
     params: &P,
     ufvk: &UnifiedFullViewingKey,
 ) -> Result<Option<Account>, SqliteClientError> {
+    #[cfg(feature = "orchard")]
+    let orchard_item = ufvk.orchard().map(|k| k.to_bytes());
+    #[cfg(not(feature = "orchard"))]
+    let orchard_item: Option<Vec<u8>> = 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<P: consensus::Parameters>(
     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| {

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),
         }

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<Vec<u8>> = 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"))]

zcash_client_sqlite/src/wallet/init/migrations/full_account_ids.rs

@@ -172,6 +172,13 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                         .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<Vec<u8>> = 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<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                             ":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,

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!(

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());
                     }
                 }