Move most ephemeral address index handling into helper functions in

`zcash_client_sqlite::wallet::transparent::ephemeral`. Also report the
account id and index for `SqliteClientError::ReachedGapLimit`.

Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

zcash_client_backend/src/wallet.rs

@@ -633,6 +633,7 @@ impl OvkPolicy {
 }
 
 /// Metadata related to the ZIP 32 derivation of a transparent address.
+/// This is implicitly scoped to an account.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg(feature = "transparent-inputs")]
 pub struct TransparentAddressMetadata {

zcash_client_sqlite/src/error.rs

@@ -15,6 +15,7 @@ use crate::PRUNING_DEPTH;
 
 #[cfg(feature = "transparent-inputs")]
 use {
+    crate::AccountId,
     zcash_client_backend::encoding::TransparentCodecError,
     zcash_primitives::{legacy::TransparentAddress, transaction::TxId},
 };
@@ -114,9 +115,10 @@ pub enum SqliteClientError {
     BalanceError(BalanceError),
 
     /// The proposal cannot be constructed until transactions with previously reserved
-    /// ephemeral address outputs have been mined.
+    /// ephemeral address outputs have been mined. The parameters are the account id and
+    /// the index that could not safely be reserved.
     #[cfg(feature = "transparent-inputs")]
-    ReachedGapLimit,
+    ReachedGapLimit(AccountId, u32),
 
     /// An ephemeral address would be reused, or incorrectly used as an external address.
     /// The parameters are the address in string form, and if it is known to have been
@@ -175,7 +177,10 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::UnsupportedPoolType(t) => write!(f, "Pool type is not currently supported: {}", t),
             SqliteClientError::BalanceError(e) => write!(f, "Balance error: {}", e),
             #[cfg(feature = "transparent-inputs")]
-            SqliteClientError::ReachedGapLimit => write!(f, "The proposal cannot be constructed until transactions with previously reserved ephemeral address outputs have been mined."),
+            SqliteClientError::ReachedGapLimit(account_id, bad_index) => write!(f,
+                "The proposal cannot be constructed until transactions with previously reserved ephemeral address outputs have been mined. \
+                 The ephemeral address in account {account_id:?} at index {bad_index} could not be safely reserved.",
+            ),
             #[cfg(feature = "transparent-inputs")]
             SqliteClientError::EphemeralAddressReuse(address_str, Some(txid)) => write!(f, "The ephemeral address {address_str} previously used in txid {txid} would be reused."),
             #[cfg(feature = "transparent-inputs")]

zcash_client_sqlite/src/wallet/init.rs

@@ -177,7 +177,7 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet
             unreachable!("we don't call methods that require a known chain height")
         }
         #[cfg(feature = "transparent-inputs")]
-        SqliteClientError::ReachedGapLimit => {
+        SqliteClientError::ReachedGapLimit(_, _) => {
             unreachable!("we don't do ephemeral address tracking")
         }
         #[cfg(feature = "transparent-inputs")]

zcash_client_sqlite/src/wallet/transparent.rs

@@ -1,19 +1,24 @@
 //! Functions for transparent input support in the wallet.
+use std::cmp::max;
+use std::collections::{HashMap, HashSet};
+
 use rusqlite::OptionalExtension;
 use rusqlite::{named_params, Connection, Row};
-use std::collections::HashMap;
-use std::collections::HashSet;
-use zcash_client_backend::data_api::AccountBalance;
-use zcash_keys::address::Address;
-use zcash_keys::keys::AddressGenerationError;
 use zip32::{DiversifierIndex, Scope};
 
 use zcash_address::unified::{Encoding, Ivk, Uivk};
-use zcash_client_backend::wallet::{TransparentAddressMetadata, WalletTransparentOutput};
+use zcash_client_backend::{
-use zcash_keys::encoding::{encode_transparent_address_p, AddressCodec};
+    data_api::AccountBalance,
+    keys::AddressGenerationError,
+    wallet::{TransparentAddressMetadata, WalletTransparentOutput},
+};
+use zcash_keys::{
+    address::Address,
+    encoding::{encode_transparent_address_p, AddressCodec},
+};
 use zcash_primitives::{
     legacy::{
-        keys::{EphemeralIvk, IncomingViewingKey, NonHardenedChildIndex, TransparentKeyScope},
+        keys::{EphemeralIvk, IncomingViewingKey, NonHardenedChildIndex},
         Script, TransparentAddress,
     },
     transaction::{
@@ -28,6 +33,8 @@ use crate::{SqlTransaction, WalletDb};
 
 use super::{chain_tip_height, get_account, get_account_ids};
 
+mod ephemeral;
+
 pub(crate) fn detect_spending_accounts<'a>(
     conn: &Connection,
     spent: impl Iterator<Item = &'a OutPoint>,
@@ -494,7 +501,7 @@ pub(crate) fn find_account_for_transparent_output<P: consensus::Parameters>(
         return Ok(Some(account_id));
     }
 
-    // Note that this does not search ephemeral addresses that have not yet been reserved.
+    // Search ephemeral addresses that have already been reserved.
     if let Some(account_id) = conn
         .query_row(
             "SELECT account_id FROM ephemeral_addresses WHERE address = :address",
@@ -741,14 +748,6 @@ pub(crate) fn get_ephemeral_ivk<P: consensus::Parameters>(
         .derive_ephemeral_ivk()?)
 }
 
-// Same as Bitcoin.
-const GAP_LIMIT: i32 = 20;
-
-const EPHEMERAL_SCOPE: TransparentKeyScope = match TransparentKeyScope::custom(2) {
-    Some(s) => s,
-    None => unreachable!(),
-};
-
 /// Returns a vector with all ephemeral transparent addresses potentially belonging to this wallet.
 /// If `for_detection` is true, this includes addresses for an additional GAP_LIMIT indices.
 pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
@@ -771,30 +770,19 @@ pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
         first_unused_index = i32::try_from(raw_index)
             .map_err(|e| SqliteClientError::CorruptedData(e.to_string()))?
             .checked_add(1);
-        let address_index = NonHardenedChildIndex::from_index(raw_index).expect("just checked");
+        let address_index = NonHardenedChildIndex::from_index(raw_index).unwrap();
-        result.insert(
+        let address = TransparentAddress::decode(params, &addr_str)?;
-            TransparentAddress::decode(params, &addr_str)?,
+        result.insert(address, Some(ephemeral::metadata(address_index)));
-            Some(TransparentAddressMetadata::new(
-                EPHEMERAL_SCOPE,
-                address_index,
-            )),
-        );
     }
 
     if for_detection {
         if let Some(first) = first_unused_index {
             let ephemeral_ivk = get_ephemeral_ivk(conn, params, account_id)?;
 
-            for index in first..=first.saturating_add(GAP_LIMIT - 1) {
+            for raw_index in ephemeral::range_after(first, ephemeral::GAP_LIMIT) {
-                let address_index =
+                let address_index = NonHardenedChildIndex::from_index(raw_index).unwrap();
-                    NonHardenedChildIndex::from_index(index as u32).expect("valid index");
+                let address = ephemeral_ivk.derive_ephemeral_address(address_index)?;
-                result.insert(
+                result.insert(address, Some(ephemeral::metadata(address_index)));
-                    ephemeral_ivk.derive_ephemeral_address(address_index)?,
-                    Some(TransparentAddressMetadata::new(
-                        EPHEMERAL_SCOPE,
-                        address_index,
-                    )),
-                );
             }
         }
     }
@@ -827,52 +815,18 @@ pub(crate) fn reserve_next_n_ephemeral_addresses<P: consensus::Parameters>(
     assert!(n > 0);
 
     let ephemeral_ivk = get_ephemeral_ivk(wdb.conn.0, &wdb.params, account_id)?;
+    let last_reserved_index = ephemeral::last_reserved_index(wdb.conn.0, account_id)?;
+    let last_safe_index = ephemeral::last_safe_index(wdb.conn.0, account_id)?;
+    let allocation = ephemeral::range_after(last_reserved_index, n);
 
-    // The inner join with `transactions` excludes addresses for which
+    if allocation.clone().count() < n.try_into().unwrap() {
-    // `mined_in_tx` is NULL. The query also excludes addresses observed
+        return Err(SqliteClientError::AddressGeneration(
-    // to have been mined in a transaction that we currently see as unmined.
-    // This is conservative in terms of avoiding violation of the gap
-    // invariant: it can only cause us to get to the end of the gap (and
-    // start reporting `ReachedGapLimit` errors) sooner.
-    let last_gap_index: i32 = wdb
-        .conn
-        .0
-        .query_row(
-            "SELECT address_index FROM ephemeral_addresses
-             JOIN transactions t ON t.id_tx = mined_in_tx
-             WHERE account_id = :account_id AND t.mined_height IS NOT NULL
-             ORDER BY address_index DESC LIMIT 1",
-            named_params![":account_id": account_id.0],
-            |row| row.get::<_, u32>(0),
-        )
-        .optional()?
-        .map_or(Ok(-1i32), |i| {
-            i32::try_from(i).map_err(|e| SqliteClientError::CorruptedData(e.to_string()))
-        })?
-        .saturating_add(GAP_LIMIT);
-
-    let (first_index, last_index) = wdb
-        .conn
-        .0
-        .query_row(
-            "SELECT address_index FROM ephemeral_addresses
-             WHERE account_id = :account_id
-             ORDER BY address_index DESC LIMIT 1",
-            named_params![":account_id": account_id.0],
-            |row| row.get::<_, u32>(0),
-        )
-        .optional()?
-        .map_or(Ok(-1i32), |i| {
-            i32::try_from(i).map_err(|e| SqliteClientError::CorruptedData(e.to_string()))
-        })
-        .map(|i: i32| i.checked_add(1).zip(i.checked_add(n.try_into().ok()?)))?
-        .ok_or(SqliteClientError::AddressGeneration(
             AddressGenerationError::DiversifierSpaceExhausted,
-        ))?;
+        ));
-
+    }
-    assert!(last_index >= first_index);
+    if *allocation.end() > last_safe_index {
-    if last_index > last_gap_index {
+        let unsafe_index = max(*allocation.start(), last_safe_index.saturating_add(1));
-        return Err(SqliteClientError::ReachedGapLimit);
+        return Err(SqliteClientError::ReachedGapLimit(account_id, unsafe_index));
     }
 
     // used_in_tx and mined_in_tx are initially NULL
@@ -881,20 +835,17 @@ pub(crate) fn reserve_next_n_ephemeral_addresses<P: consensus::Parameters>(
          VALUES (:account_id, :address_index, :address)",
     )?;
 
-    (first_index..=last_index)
+    allocation
-        .map(|address_index| {
+        .map(|raw_index| {
-            let child = NonHardenedChildIndex::from_index(address_index as u32)
+            let address_index = NonHardenedChildIndex::from_index(raw_index).unwrap();
-                .expect("valid by construction");
+            let address = ephemeral_ivk.derive_ephemeral_address(address_index)?;
-            let address = ephemeral_ivk.derive_ephemeral_address(child)?;
+
             stmt_insert_ephemeral_address.execute(named_params![
                 ":account_id": account_id.0,
-                ":address_index": address_index,
+                ":address_index": raw_index,
                 ":address": encode_transparent_address_p(&wdb.params, &address)
             ])?;
-            Ok((
+            Ok((address, ephemeral::metadata(address_index)))
-                address,
-                TransparentAddressMetadata::new(EPHEMERAL_SCOPE, child),
-            ))
         })
         .collect()
 }

zcash_client_sqlite/src/wallet/transparent/ephemeral.rs

@@ -0,0 +1,98 @@
+//! Functions for wallet support of ephemeral transparent addresses.
+use std::ops::RangeInclusive;
+
+use rusqlite::{named_params, OptionalExtension};
+
+use zcash_client_backend::wallet::TransparentAddressMetadata;
+use zcash_primitives::legacy::keys::{NonHardenedChildIndex, TransparentKeyScope};
+
+use crate::{error::SqliteClientError, AccountId};
+
+/// The number of ephemeral addresses that can be safely reserved without observing any
+/// of them to be mined. This is the same as the gap limit in Bitcoin.
+pub(crate) const GAP_LIMIT: i32 = 20;
+
+// The custom scope used for derivation of ephemeral addresses.
+// TODO: consider moving this to `zcash_primitives::legacy::keys`, or else
+// provide a way to derive `ivk`s for custom scopes in general there, so that
+// the constant isn't duplicated.
+pub(crate) const EPHEMERAL_SCOPE: TransparentKeyScope = match TransparentKeyScope::custom(2) {
+    Some(s) => s,
+    None => unreachable!(),
+};
+
+// Returns `TransparentAddressMetadata` in the ephemeral scope for the
+// given address index.
+pub(crate) fn metadata(address_index: NonHardenedChildIndex) -> TransparentAddressMetadata {
+    TransparentAddressMetadata::new(EPHEMERAL_SCOPE, address_index)
+}
+
+/// Returns the last reserved ephemeral address index in the given account,
+/// or -1 if the account has no reserved ephemeral addresses.
+pub(crate) fn last_reserved_index(
+    conn: &rusqlite::Connection,
+    account_id: AccountId,
+) -> Result<i32, SqliteClientError> {
+    match conn
+        .query_row(
+            "SELECT address_index FROM ephemeral_addresses
+         WHERE account_id = :account_id
+         ORDER BY address_index DESC
+         LIMIT 1",
+            named_params![":account_id": account_id.0],
+            |row| row.get::<_, i32>(0),
+        )
+        .optional()?
+    {
+        Some(i) if i < 0 => Err(SqliteClientError::CorruptedData(
+            "negative index".to_owned(),
+        )),
+        Some(i) => Ok(i),
+        None => Ok(-1),
+    }
+}
+
+/// Returns the last ephemeral address index in the given account that
+/// would not violate the gap invariant if used.
+pub(crate) fn last_safe_index(
+    conn: &rusqlite::Connection,
+    account_id: AccountId,
+) -> Result<u32, SqliteClientError> {
+    // The inner join with `transactions` excludes addresses for which
+    // `mined_in_tx` is NULL. The query also excludes addresses observed
+    // to have been mined in a transaction that we currently see as unmined.
+    // This is conservative in terms of avoiding violation of the gap
+    // invariant: it can only cause us to get to the end of the gap sooner.
+    let last_mined_index: i32 = match conn
+        .query_row(
+            "SELECT address_index FROM ephemeral_addresses
+             JOIN transactions t ON t.id_tx = mined_in_tx
+             WHERE account_id = :account_id AND t.mined_height IS NOT NULL
+             ORDER BY address_index DESC
+             LIMIT 1",
+            named_params![":account_id": account_id.0],
+            |row| row.get::<_, i32>(0),
+        )
+        .optional()?
+    {
+        Some(i) if i < 0 => Err(SqliteClientError::CorruptedData(
+            "negative index".to_owned(),
+        )),
+        Some(i) => Ok(i),
+        None => Ok(-1),
+    }?;
+    Ok(u32::try_from(last_mined_index.saturating_add(GAP_LIMIT)).unwrap())
+}
+
+/// Utility function to return an `InclusiveRange<u32>` that starts at `i + 1`
+/// and is of length up to `n`. The range is truncated if necessary to end at
+/// the maximum valid address index, `i32::MAX`.
+///
+/// Precondition: `i >= -1 and n > 0`
+pub(crate) fn range_after(i: i32, n: i32) -> RangeInclusive<u32> {
+    assert!(i >= -1);
+    assert!(n > 0);
+    let first = u32::try_from(i64::from(i) + 1).unwrap();
+    let last = u32::try_from(i.saturating_add(n)).unwrap();
+    first..=last
+}