Merge pull request #1732 from zcash/zcs-migration-fixes

zcash_client_sqlite: Migration fixes and API updates

zcash_client_backend/src/data_api.rs

@@ -2387,8 +2387,9 @@ pub trait WalletWrite: WalletRead {
     ///
     /// Returns `Ok(None)` in the case that it is not possible to generate an address conforming
     /// to the provided request at the specified diversifier index. Such a result might arise from
-    /// either a key not being available or the diversifier index not being valid for a
-    /// [`ReceiverRequirement::Require`]'ed receiver.
+    /// the diversifier index not being valid for a [`ReceiverRequirement::Require`]'ed receiver.
+    /// Some implementations of this trait may return `Err(_)` in some cases to expose more
+    /// information, which is only accessible in a backend-specific context.
     ///
     /// Address generation should fail if an address has already been exposed for the given
     /// diversifier index and the given request produced an address having different receivers than

zcash_client_sqlite/CHANGELOG.md

@@ -20,6 +20,9 @@ and this library adheres to Rust's notion of
   result:
   - `WalletDb::for_path`
   - `WalletDb::from_connection`
+- `zcash_client_sqlite::WalletDb::get_address_for_index` now returns some of
+  its failure modes via `Err(SqliteClientError::AddressGeneration)` instead of
+  `Ok(None)`.
 - `zcash_client_sqlite::error::SqliteClientError` variants have changed:
   - The `EphemeralAddressReuse` variant has been removed and replaced
     by a new generalized `AddressReuse` error variant.

zcash_client_sqlite/src/lib.rs

@@ -1175,21 +1175,27 @@ impl<C: BorrowMut<rusqlite::Connection>, P: consensus::Parameters, CL: Clock> Wa
         request: UnifiedAddressRequest,
     ) -> Result<Option<UnifiedAddress>, Self::Error> {
         if let Some(account) = self.get_account(account)? {
-            if let Ok(address) = account.uivk().address(diversifier_index, request) {
-                let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())?;
-                upsert_address(
-                    self.conn.borrow(),
-                    &self.params,
-                    account.internal_id(),
-                    diversifier_index,
-                    &address,
-                    Some(chain_tip_height.unwrap_or(account.birthday())),
-                    true,
-                )?;
-
-                Ok(Some(address))
-            } else {
-                Ok(None)
+            use zcash_keys::keys::AddressGenerationError::*;
+
+            match account.uivk().address(diversifier_index, request) {
+                Ok(address) => {
+                    let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())?;
+                    upsert_address(
+                        self.conn.borrow(),
+                        &self.params,
+                        account.internal_id(),
+                        diversifier_index,
+                        &address,
+                        Some(chain_tip_height.unwrap_or(account.birthday())),
+                        true,
+                    )?;
+
+                    Ok(Some(address))
+                }
+                #[cfg(feature = "transparent-inputs")]
+                Err(InvalidTransparentChildIndex(_)) => Ok(None),
+                Err(InvalidSaplingDiversifierIndex(_)) => Ok(None),
+                Err(e) => Err(SqliteClientError::AddressGeneration(e)),
             }
         } else {
             Err(SqliteClientError::AccountUnknown)
@@ -1414,6 +1420,7 @@ impl<C: BorrowMut<rusqlite::Connection>, P: consensus::Parameters, CL: Clock> Wa
                     key_scope,
                     &wdb.gap_limits,
                     UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+                    false,
                 )?;
             }
 
@@ -1687,6 +1694,7 @@ impl<C: BorrowMut<rusqlite::Connection>, P: consensus::Parameters, CL: Clock> Wa
                 key_scope,
                 &wdb.gap_limits,
                 UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+                true,
             )?;
 
             Ok(utxo_id)

zcash_client_sqlite/src/wallet.rs

@@ -623,6 +623,7 @@ pub(crate) fn add_account<P: consensus::Parameters>(
             key_scope,
             gap_limits,
             UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+            false,
         )?;
     }
 
@@ -670,6 +671,7 @@ pub(crate) fn get_next_available_address<P: consensus::Parameters, C: Clock>(
                 KeyScope::EXTERNAL,
                 gap_limits,
                 UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+                true,
             )?;
 
             // Select indices from the transparent gap limit that are available for use as
@@ -926,9 +928,19 @@ pub(crate) fn upsert_address<P: consensus::Parameters>(
     )?;
 
     #[cfg(feature = "transparent-inputs")]
-    let transparent_child_index = NonHardenedChildIndex::try_from(diversifier_index)
-        .ok()
-        .map(|i| i.index());
+    let (transparent_child_index, cached_taddr) = {
+        let idx = NonHardenedChildIndex::try_from(diversifier_index)
+            .ok()
+            .map(|i| i.index());
+
+        // This upholds the `transparent_index_consistency` check on the `addresses` table.
+        match (idx, address.transparent()) {
+            (Some(idx), Some(r)) => Ok((Some(idx), Some(r.encode(params)))),
+            (_, None) => Ok((None, None)),
+            (None, Some(addr)) => Err(SqliteClientError::AddressNotRecognized(*addr)),
+        }
+    }?;
+
     #[cfg(not(feature = "transparent-inputs"))]
     let transparent_child_index: Option<u32> = None;
 
@@ -940,13 +952,14 @@ pub(crate) fn upsert_address<P: consensus::Parameters>(
             ":key_scope": KeyScope::EXTERNAL.encode(),
             ":address": &address.encode(params),
             ":transparent_child_index": transparent_child_index,
-            ":cached_transparent_receiver_address": &address.transparent().map(|r| r.encode(params)),
+            ":cached_transparent_receiver_address": &cached_taddr,
             ":exposed_at_height": exposed_at_height.map(u32::from),
             ":force_update_address": force_update_address,
             ":receiver_flags": ReceiverFlags::from(address).bits()
         ],
-        |row| row.get(0).map(AddressRef)
-    ).map_err(SqliteClientError::from)
+        |row| row.get(0).map(AddressRef),
+    )
+    .map_err(SqliteClientError::from)
 }
 
 #[cfg(feature = "transparent-inputs")]
@@ -3403,6 +3416,7 @@ pub(crate) fn store_decrypted_tx<P: consensus::Parameters>(
             key_scope,
             gap_limits,
             UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+            false,
         )?;
     }
 

zcash_client_sqlite/src/wallet/init/migrations.rs

@@ -168,7 +168,7 @@ pub(super) fn all_migrations<P: consensus::Parameters + 'static>(
 #[allow(dead_code)]
 const PUBLIC_MIGRATION_STATES: &[&[Uuid]] = &[
     V_0_4_0, V_0_6_0, V_0_8_0, V_0_9_0, V_0_10_0, V_0_10_3, V_0_11_0, V_0_11_1, V_0_11_2, V_0_12_0,
-    V_0_13_0,
+    V_0_13_0, V_0_14_0, V_0_15_0,
 ];
 
 /// Leaf migrations in the 0.4.0 release.
@@ -228,6 +228,18 @@ const V_0_12_0: &[Uuid] = &[fix_broken_commitment_trees::MIGRATION_ID];
 /// Leaf migrations in the 0.13.0 release.
 const V_0_13_0: &[Uuid] = &[fix_bad_change_flagging::MIGRATION_ID];
 
+/// Leaf migrations in the 0.14.0 release.
+const V_0_14_0: &[Uuid] = &[
+    fix_bad_change_flagging::MIGRATION_ID,
+    add_account_uuids::MIGRATION_ID,
+];
+
+/// Leaf migrations in the 0.15.0 release.
+const V_0_15_0: &[Uuid] = &[
+    fix_bad_change_flagging::MIGRATION_ID,
+    v_transactions_additional_totals::MIGRATION_ID,
+];
+
 pub(super) fn verify_network_compatibility<P: consensus::Parameters>(
     conn: &rusqlite::Connection,
     params: &P,

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

@@ -597,6 +597,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
                     key_scope,
                     &GapLimits::default(),
                     UnifiedAddressRequest::unsafe_custom(Allow, Allow, Require),
+                    false,
                 )?;
             }
         }

zcash_client_sqlite/src/wallet/transparent.rs

@@ -414,10 +414,21 @@ pub(crate) fn generate_gap_addresses<P: consensus::Parameters>(
     key_scope: KeyScope,
     gap_limits: &GapLimits,
     request: UnifiedAddressRequest,
+    require_key: bool,
 ) -> Result<(), SqliteClientError> {
     let account = get_account_internal(conn, params, account_id)?
         .ok_or_else(|| SqliteClientError::AccountUnknown)?;
 
+    if !account.uivk().has_transparent() {
+        if require_key {
+            return Err(SqliteClientError::AddressGeneration(
+                AddressGenerationError::KeyNotAvailable(Typecode::P2pkh),
+            ));
+        } else {
+            return Ok(());
+        }
+    }
+
     let gen_addrs = |key_scope: KeyScope, index: NonHardenedChildIndex| {
         Ok::<_, SqliteClientError>(match key_scope {
             KeyScope::Zip32(zip32::Scope::External) => {
@@ -426,7 +437,7 @@ pub(crate) fn generate_gap_addresses<P: consensus::Parameters>(
                     .uivk()
                     .transparent()
                     .as_ref()
-                    .ok_or(AddressGenerationError::KeyNotAvailable(Typecode::P2pkh))?
+                    .expect("presence of transparent key was checked above.")
                     .derive_address(index)?;
                 (
                     ua.map_or_else(
@@ -448,7 +459,7 @@ pub(crate) fn generate_gap_addresses<P: consensus::Parameters>(
                 let internal_address = account
                     .ufvk()
                     .and_then(|k| k.transparent())
-                    .ok_or(AddressGenerationError::KeyNotAvailable(Typecode::P2pkh))?
+                    .expect("presence of transparent key was checked above.")
                     .derive_internal_ivk()?
                     .derive_address(index)?;
                 (
@@ -460,7 +471,7 @@ pub(crate) fn generate_gap_addresses<P: consensus::Parameters>(
                 let ephemeral_address = account
                     .ufvk()
                     .and_then(|k| k.transparent())
-                    .ok_or(AddressGenerationError::KeyNotAvailable(Typecode::P2pkh))?
+                    .expect("presence of transparent key was checked above.")
                     .derive_ephemeral_ivk()?
                     .derive_ephemeral_address(index)?;
                 (