Minor changes responding to review comments.

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

zcash_client_backend/CHANGELOG.md

@@ -75,6 +75,9 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
     not enabled).
 - `zcash_client_backend::input_selection::GreedyInputSelectorError` has a
   new variant `UnsupportedTexAddress`.
+- `zcash_client_backend::proposal::ProposalError` has new variants
+  `SpendsChange`, `EphemeralOutputLeftUnspent`, and `PaysTexFromShielded`.
+  (the last two are conditional on the "transparent-inputs" feature).
 - `zcash_client_backend::proto::ProposalDecodingError` has a new variant
   `InvalidEphemeralRecipient`.
 - `zcash_client_backend::wallet::Recipient` variants have changed. Instead of

zcash_client_backend/src/data_api.rs

@@ -922,7 +922,7 @@ pub trait WalletRead {
     }
 
     /// Returns the metadata associated with a given transparent receiver in an account
-    /// controlled by this wallet.
+    /// controlled by this wallet, if available.
     ///
     /// This is equivalent to (but may be implemented more efficiently than):
     /// ```compile_fail
@@ -956,13 +956,19 @@ pub trait WalletRead {
         Ok(None)
     }
 
-    /// Returns the set of reserved ephemeral transparent addresses associated with the
-    /// given account controlled by this wallet.
+    /// Returns a set of ephemeral transparent addresses associated with the given
+    /// account controlled by this wallet, along with their metadata.
     ///
-    /// The set contains all ephemeral transparent receivers that are known to have
-    /// been derived under this account. Wallets should scan the chain for UTXOs sent to
-    /// these receivers, but do not need to do so regularly. Under expected usage, outputs
-    /// would only be detected with these receivers in the following situations:
+    /// If `for_detection` is false, the set only includes addresses reserved by
+    /// `reserve_next_n_ephemeral_addresses`. If `for_detection` is true, it includes
+    /// those addresses and also the ones that will be reserved next, for an additional
+    /// `GAP_LIMIT` indices (up to and including the maximum index given by
+    /// `NonHardenedChildIndex::from_index(i32::MAX as u32)`).
+    ///
+    /// Wallets should scan the chain for UTXOs sent to the ephemeral transparent
+    /// receivers obtained with `for_detection` set to `true`, but do not need to do
+    /// so regularly. Under expected usage, outputs would only be detected with these
+    /// receivers in the following situations:
     ///
     /// - This wallet created a payment to a ZIP 320 (TEX) address, but the second
     ///   transaction (that spent the output sent to the ephemeral address) did not get
@@ -1542,6 +1548,8 @@ pub trait WalletWrite: WalletRead {
     /// funds have been received by the currently-available account (in order to enable automated
     /// account recovery).
     ///
+    /// # Panics
+    ///
     /// Panics if the length of the seed is not between 32 and 252 bytes inclusive.
     ///
     /// [ZIP 316]: https://zips.z.cash/zip-0316
@@ -1634,7 +1642,9 @@ pub trait WalletWrite: WalletRead {
     /// the given number of addresses, or if the account identifier does not correspond
     /// to a known account.
     ///
-    /// Precondition: `n < 0x80000000`
+    /// # Panics
+    ///
+    /// Panics if the precondition `n < 0x80000000` does not hold.
     ///
     /// [BIP 44]: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#user-content-Address_gap_limit
     #[cfg(feature = "transparent-inputs")]

zcash_client_backend/src/data_api/wallet.rs

@@ -680,6 +680,7 @@ where
     // TODO: Maybe support spending prior shielded outputs at some point? Doing so would require
     // a higher-level approach in the wallet that waits for transactions with shielded outputs to
     // be mined and only then attempts to perform the next step.
+    #[allow(clippy::never_loop)]
     for input_ref in proposal_step.prior_step_inputs() {
         let (prior_step, _) = prior_step_results
             .get(input_ref.step_index())
@@ -1251,13 +1252,18 @@ where
 
     #[allow(unused_variables)]
     let transparent_outputs = transparent_output_meta.into_iter().enumerate().map(
-        |(n, (recipient, ephemeral_address, value, step_output_index))| {
+        |(n, (recipient, address, value, step_output_index))| {
+            // This assumes that transparent outputs are pushed onto `transparent_output_meta`
+            // with the same indices they have in the transaction's transparent outputs.
+            // We do not reorder transparent outputs; there is no reason to do so because it
+            // would not usefully improve privacy.
             let outpoint = OutPoint::new(txid, n as u32);
+
             let recipient = recipient.map_ephemeral_transparent_outpoint(|()| outpoint.clone());
             #[cfg(feature = "transparent-inputs")]
             unused_transparent_outputs.insert(
                 StepOutput::new(step_index, step_output_index),
-                (ephemeral_address, outpoint),
+                (address, outpoint),
             );
             SentTransactionOutput::from_parts(n, recipient, value, None)
         },

zcash_client_backend/src/fees/common.rs

@@ -521,10 +521,17 @@ pub(crate) fn check_for_uneconomic_inputs<NoteRefT: Clone, E>(
 
     // Return the number of allowed dust inputs from each pool.
     let allowed_dust = |(t_change, s_change, o_change): &(usize, usize, usize)| {
-        // What is the maximum number of dust inputs in each pool, out of the ones we
-        // actually have, that can be economically spent along with the non-dust inputs?
-        // Get an initial estimate by calculating the number of dust inputs in each pool
-        // that will be allowed regardless of padding or grace.
+        // Here we assume a "ZIP 317-like" fee model in which the existence of an output
+        // to a given pool implies that a corresponding input from that pool can be
+        // provided without increasing the fee. (This is also likely to be true for
+        // future fee models if we do not want to penalize use of Orchard relative to
+        // other pools.)
+        //
+        // Under that assumption, we want to calculate the maximum number of dust inputs
+        // from each pool, out of the ones we actually have, that can be economically
+        // spent along with the non-dust inputs. Get an initial estimate by calculating
+        // the number of dust inputs in each pool that will be allowed regardless of
+        // padding or grace.
 
         let t_allowed = min(
             t_dust.len(),
@@ -545,10 +552,12 @@ pub(crate) fn check_for_uneconomic_inputs<NoteRefT: Clone, E>(
         #[cfg(feature = "orchard")]
         let o_req_inputs = o_non_dust + o_allowed;
 
-        // This calculates the hypothetical number of actions with given extra inputs.
-        // The padding rules are subtle (they also depend on `bundle_required` for example).
-        // To reliably tell whether we can add an extra input of a given type, we will need
-        // to call them both with without that input; if the number of actions does not
+        // This calculates the hypothetical number of actions with given extra inputs,
+        // for ZIP 317 and the padding rules in effect. The padding rules for each
+        // pool are subtle (they also depend on `bundle_required` for example), so we
+        // must actually call them rather than try to predict their effect. To tell
+        // whether we can freely add an extra input from a given pool, we need to call
+        // them both with and without that input; if the number of actions does not
         // increase, then the input is free to add.
         let hypothetical_actions = |t_extra, s_extra, _o_extra| {
             let s_spend_count = sapling

zcash_client_backend/src/wallet.rs

@@ -647,6 +647,8 @@ pub struct TransparentAddressMetadata {
 
 #[cfg(feature = "transparent-inputs")]
 impl TransparentAddressMetadata {
+    /// Returns a `TransparentAddressMetadata` in the given scope for the
+    /// given address index.
     pub fn new(scope: TransparentKeyScope, address_index: NonHardenedChildIndex) -> Self {
         Self {
             scope,

zcash_client_sqlite/src/wallet/transparent/ephemeral.rs

@@ -25,23 +25,10 @@ use crate::{error::SqliteClientError, wallet::get_account, AccountId, SqlTransac
 /// 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.
-//
-// This must match the constant used in
-// `zcash_primitives::legacy::keys::AccountPubKey::derive_ephemeral_ivk`.
-//
-// 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.
-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)
+    TransparentAddressMetadata::new(TransparentKeyScope::EPHEMERAL, address_index)
 }
 
 /// Returns the last reserved ephemeral address index in the given account,
@@ -61,9 +48,7 @@ pub(crate) fn last_reserved_index(
         )
         .optional()?
     {
-        Some(i) if i < 0 => Err(SqliteClientError::CorruptedData(
-            "negative index".to_owned(),
-        )),
+        Some(i) if i < 0 => unreachable!("violates constraint address_index_in_range"),
         Some(i) => Ok(i),
         None => Ok(-1),
     }
@@ -92,12 +77,10 @@ pub(crate) fn last_safe_index(
         )
         .optional()?
     {
-        Some(i) if i < 0 => Err(SqliteClientError::CorruptedData(
-            "negative index".to_owned(),
-        )),
-        Some(i) => Ok(i),
-        None => Ok(-1),
-    }?;
+        Some(i) if i < 0 => unreachable!("violates constraint address_index_in_range"),
+        Some(i) => i,
+        None => -1,
+    };
     Ok(u32::try_from(last_mined_index.saturating_add(GAP_LIMIT)).unwrap())
 }
 
@@ -105,7 +88,9 @@ pub(crate) fn last_safe_index(
 /// 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`
+/// # Panics
+///
+/// Panics if the precondition `i >= -1 and n > 0` does not hold.
 pub(crate) fn range_after(i: i32, n: i32) -> RangeInclusive<u32> {
     assert!(i >= -1);
     assert!(n > 0);
@@ -128,8 +113,12 @@ pub(crate) fn get_ephemeral_ivk<P: consensus::Parameters>(
         .derive_ephemeral_ivk()?)
 }
 
-/// 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.
+/// Returns a mapping of ephemeral transparent addresses potentially belonging
+/// to this wallet to their metadata.
+///
+/// If `for_detection` is false, the result only includes reserved addresses.
+/// If `for_detection` is true, it includes addresses for an additional
+/// `GAP_LIMIT` indices, up to the limit.
 pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
     conn: &rusqlite::Connection,
     params: &P,
@@ -172,7 +161,9 @@ pub(crate) fn get_reserved_ephemeral_addresses<P: consensus::Parameters>(
 /// Returns a vector with the next `n` previously unreserved ephemeral addresses for
 /// the given account.
 ///
-/// Precondition: `n < 0x80000000`
+/// # Panics
+///
+/// Panics if the precondition `n < 0x80000000` does not hold.
 ///
 /// # Errors
 ///

zcash_primitives/CHANGELOG.md

@@ -14,6 +14,7 @@ and this library adheres to Rust's notion of
   - `EphemeralIvk`
   - `AccountPubKey::derive_ephemeral_ivk`
   - `TransparentKeyScope::custom` is now `const`.
+  - `TransparentKeyScope::{EXTERNAL, INTERNAL, EPHEMERAL}`
 - `zcash_primitives::legacy::Script::serialized_size`
 - `zcash_primitives::transaction::fees::transparent`:
   - `InputSize`

zcash_primitives/src/legacy/keys.rs

@@ -21,6 +21,10 @@ use super::TransparentAddress;
 pub struct TransparentKeyScope(u32);
 
 impl TransparentKeyScope {
+    /// Returns an arbitrary custom `TransparentKeyScope`. This should be used
+    /// with care: funds associated with keys derived under a custom scope may
+    /// not be recoverable if the wallet seed is restored in another wallet.
+    /// It is usually preferable to use standardized key scopes.
     pub const fn custom(i: u32) -> Option<Self> {
         if i < (1 << 31) {
             Some(TransparentKeyScope(i))
@@ -28,13 +32,24 @@ impl TransparentKeyScope {
             None
         }
     }
+
+    /// The scope used to derive keys for external transparent addresses,
+    /// intended to be used to send funds to this wallet.
+    pub const EXTERNAL: Self = TransparentKeyScope(0);
+
+    /// The scope used to derive keys for internal wallet operations, e.g.
+    /// change or UTXO management.
+    pub const INTERNAL: Self = TransparentKeyScope(1);
+
+    /// The scope used to derive keys for ephemeral transparent addresses.
+    pub const EPHEMERAL: Self = TransparentKeyScope(2);
 }
 
 impl From<zip32::Scope> for TransparentKeyScope {
     fn from(value: zip32::Scope) -> Self {
         match value {
-            zip32::Scope::External => TransparentKeyScope(0),
-            zip32::Scope::Internal => TransparentKeyScope(1),
+            zip32::Scope::External => TransparentKeyScope::EXTERNAL,
+            zip32::Scope::Internal => TransparentKeyScope::INTERNAL,
         }
     }
 }