`zcash_client_backend::{fixed,standard,zip317}::SingleOutputChangeStr…

…ategy`

now implement a slightly different strategy for choosing whether there will
be any change. Transactions that have any shielded non-change flows will
always have a change output (possibly zero-valued). This can avoid leaking
information about note amounts in some cases.

Signed-off-by: Daira-Emma Hopwood <[email protected]>

zcash_client_backend/CHANGELOG.md

@@ -20,6 +20,11 @@ and this library adheres to Rust's notion of
 
 ### Changed
 - MSRV is now 1.70.0.
+- `zcash_client_backend::{fixed,standard,zip317}::SingleOutputChangeStrategy`
+  now implement a slightly different strategy for choosing whether there will
+  be any change. Transactions that have any shielded non-change flows will
+  always have a change output (possibly zero-valued). This can avoid leaking
+  information about note amounts in some cases.
 - `zcash_client_backend::zip321` has been extracted to, and is now a reexport 
   of the root module of the `zip321` crate. Several of the APIs of this module
   have changed as a consequence of this extraction; please see the `zip321`

zcash_client_backend/src/fees.rs

@@ -250,20 +250,20 @@ impl<NoteRefT> From<BalanceError> for ChangeError<BalanceError, NoteRefT> {
     }
 }
 
-/// An enumeration of actions to tak when a transaction would potentially create dust
-/// outputs (outputs that are likely to be without economic value due to fee rules.)
+/// An enumeration of actions to take when a transaction would potentially create dust
+/// outputs (outputs that are likely to be without economic value due to fee rules).
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum DustAction {
     /// Do not allow creation of dust outputs; instead, require that additional inputs be provided.
     Reject,
     /// Explicitly allow the creation of dust change amounts greater than the specified value.
     AllowDustChange,
-    /// Allow dust amounts to be added to the transaction fee
+    /// Allow dust amounts to be added to the transaction fee.
     AddDustToFee,
 }
 
 /// A policy describing how a [`ChangeStrategy`] should treat potentially dust-valued change
-/// outputs (outputs that are likely to be without economic value due to fee rules.)
+/// outputs (outputs that are likely to be without economic value due to fee rules).
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct DustOutputPolicy {
     action: DustAction,
@@ -275,7 +275,7 @@ impl DustOutputPolicy {
     ///
     /// A dust policy created with `None` as the dust threshold will delegate determination
     /// of the dust threshold to the change strategy that is evaluating the strategy; this
-    /// recommended, but an explicit value (including zero) may be provided to explicitly
+    /// is recommended, but an explicit value (including zero) may be provided to explicitly
     /// override the determination of the change strategy.
     pub fn new(action: DustAction, dust_threshold: Option<NonNegativeAmount>) -> Self {
         Self {
@@ -284,7 +284,7 @@ impl DustOutputPolicy {
         }
     }
 
-    /// Returns the action to take in the event that a dust change amount would be produced
+    /// Returns the action to take in the event that a dust change amount would be produced.
     pub fn action(&self) -> DustAction {
         self.action
     }

zcash_client_backend/src/fees/common.rs

@@ -6,7 +6,7 @@ use zcash_primitives::{
         fees::{transparent, FeeRule},
     },
 };
-use zcash_protocol::ShieldedProtocol;
+use zcash_protocol::{value::ZatBalance, ShieldedProtocol};
 
 use super::{
     sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy,
@@ -25,6 +25,19 @@ pub(crate) struct NetFlows {
     orchard_out: NonNegativeAmount,
 }
 
+impl NetFlows {
+    fn total_in(&self) -> Result<NonNegativeAmount, BalanceError> {
+        (self.t_in + self.sapling_in + self.orchard_in).ok_or(BalanceError::Overflow)
+    }
+    fn total_out(&self) -> Result<NonNegativeAmount, BalanceError> {
+        (self.t_out + self.sapling_out + self.orchard_out).ok_or(BalanceError::Overflow)
+    }
+    fn balance(&self) -> Result<ZatBalance, BalanceError> {
+        (ZatBalance::from(self.total_in()?) - ZatBalance::from(self.total_out()?))
+            .ok_or(BalanceError::Underflow)
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 pub(crate) fn calculate_net_flows<NoteRefT: Clone, F: FeeRule, E>(
     transparent_inputs: &[impl transparent::InputView],
@@ -90,38 +103,40 @@ where
     })
 }
 
-pub(crate) fn single_change_output_policy<NoteRefT: Clone, F: FeeRule, E>(
-    _net_flows: &NetFlows,
-    _fallback_change_pool: ShieldedProtocol,
-) -> Result<(ShieldedProtocol, usize, usize), ChangeError<E, NoteRefT>>
-where
-    E: From<F::Error> + From<BalanceError>,
-{
+#[allow(clippy::bool_to_int_with_if)]
+pub(crate) fn single_change_output_policy(
+    net_flows: &NetFlows,
+    fallback_change_pool: ShieldedProtocol,
+) -> Result<(ShieldedProtocol, usize, usize), BalanceError> {
     // TODO: implement a less naive strategy for selecting the pool to which change will be sent.
+
+    // If the flows are not transparent, there is always change (possibly zero-valued).
     #[cfg(feature = "orchard")]
-    let (change_pool, sapling_change, orchard_change) =
-        if _net_flows.orchard_in.is_positive() || _net_flows.orchard_out.is_positive() {
-            // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs
-            (ShieldedProtocol::Orchard, 0, 1)
-        } else if _net_flows.sapling_in.is_positive() || _net_flows.sapling_out.is_positive() {
-            // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any
-            // Sapling outputs, so that we avoid pool-crossing.
-            (ShieldedProtocol::Sapling, 1, 0)
-        } else {
-            // This is a fully-transparent transaction, so the caller gets to decide
-            // where to shield change.
-            match _fallback_change_pool {
-                ShieldedProtocol::Orchard => (_fallback_change_pool, 0, 1),
-                ShieldedProtocol::Sapling => (_fallback_change_pool, 1, 0),
-            }
-        };
-    #[cfg(not(feature = "orchard"))]
-    let (change_pool, sapling_change, orchard_change) = (ShieldedProtocol::Sapling, 1, 0);
+    if net_flows.orchard_in.is_positive() || net_flows.orchard_out.is_positive() {
+        // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs.
+        return Ok((ShieldedProtocol::Orchard, 0, 1));
+    }
+    if net_flows.sapling_in.is_positive() || net_flows.sapling_out.is_positive() {
+        // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any
+        // Sapling outputs, so that we avoid pool-crossing.
+        return Ok((ShieldedProtocol::Sapling, 1, 0));
+    }
 
-    Ok((change_pool, sapling_change, orchard_change))
+    // The flows are transparent, so there may not be change. If there is, the caller
+    // gets to decide where to shield it.
+    let num_change_outputs = if net_flows.balance()? == ZatBalance::zero() {
+        1
+    } else {
+        0
+    };
+    Ok(match fallback_change_pool {
+        ShieldedProtocol::Orchard => (fallback_change_pool, 0, num_change_outputs),
+        ShieldedProtocol::Sapling => (fallback_change_pool, num_change_outputs, 0),
+    })
 }
 
 #[allow(clippy::too_many_arguments)]
+#[allow(unused_variables)]
 pub(crate) fn single_change_output_balance<
     P: consensus::Parameters,
     NoteRefT: Clone,
@@ -138,7 +153,7 @@ pub(crate) fn single_change_output_balance<
     dust_output_policy: &DustOutputPolicy,
     default_dust_threshold: NonNegativeAmount,
     change_memo: Option<MemoBytes>,
-    _fallback_change_pool: ShieldedProtocol,
+    fallback_change_pool: ShieldedProtocol,
 ) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
 where
     E: From<F::Error> + From<BalanceError>,
@@ -153,8 +168,9 @@ where
         #[cfg(feature = "orchard")]
         orchard,
     )?;
-    let (change_pool, sapling_change, _orchard_change) =
-        single_change_output_policy::<NoteRefT, F, E>(&net_flows, _fallback_change_pool)?;
+    let (change_pool, sapling_change, orchard_change) =
+        single_change_output_policy(&net_flows, fallback_change_pool)
+            .map_err(|e| ChangeError::StrategyError(e.into()))?;
 
     let sapling_input_count = sapling
         .bundle_type()
@@ -173,7 +189,7 @@ where
         .bundle_type()
         .num_actions(
             orchard.inputs().len(),
-            orchard.outputs().len() + _orchard_change,
+            orchard.outputs().len() + orchard_change,
         )
         .map_err(ChangeError::BundleError)?;
     #[cfg(not(feature = "orchard"))]
@@ -191,19 +207,35 @@ where
         )
         .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?;
 
-    let total_in =
-        (net_flows.t_in + net_flows.sapling_in + net_flows.orchard_in).ok_or_else(overflow)?;
-    let total_out = (net_flows.t_out + net_flows.sapling_out + net_flows.orchard_out + fee_amount)
+    let total_in = net_flows
+        .total_in()
+        .map_err(|e| ChangeError::StrategyError(E::from(e)))?;
+    let total_out_plus_fee = (net_flows
+        .total_out()
+        .map_err(|e| ChangeError::StrategyError(E::from(e)))?
+        + fee_amount)
         .ok_or_else(overflow)?;
 
-    let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds {
-        available: total_in,
-        required: total_out,
-    })?;
+    let proposed_change =
+        (total_in - total_out_plus_fee).ok_or(ChangeError::InsufficientFunds {
+            available: total_in,
+            required: total_out_plus_fee,
+        })?;
 
-    if proposed_change.is_zero() {
-        TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow())
+    let (change, fee) = if proposed_change.is_zero() && sapling_change == 0 && orchard_change == 0 {
+        (vec![], fee_amount)
     } else {
+        let simple_case = || {
+            (
+                vec![ChangeValue::shielded(
+                    change_pool,
+                    proposed_change,
+                    change_memo,
+                )],
+                fee_amount,
+            )
+        };
+
         let dust_threshold = dust_output_policy
             .dust_threshold()
             .unwrap_or(default_dust_threshold);
@@ -213,36 +245,20 @@ where
                 DustAction::Reject => {
                     let shortfall = (dust_threshold - proposed_change).ok_or_else(underflow)?;
 
-                    Err(ChangeError::InsufficientFunds {
+                    return Err(ChangeError::InsufficientFunds {
                         available: total_in,
                         required: (total_in + shortfall).ok_or_else(overflow)?,
-                    })
+                    });
+                }
+                DustAction::AllowDustChange => simple_case(),
+                DustAction::AddDustToFee => {
+                    (vec![], (fee_amount + proposed_change).ok_or_else(overflow)?)
                 }
-                DustAction::AllowDustChange => TransactionBalance::new(
-                    vec![ChangeValue::shielded(
-                        change_pool,
-                        proposed_change,
-                        change_memo,
-                    )],
-                    fee_amount,
-                )
-                .map_err(|_| overflow()),
-                DustAction::AddDustToFee => TransactionBalance::new(
-                    vec![],
-                    (fee_amount + proposed_change).ok_or_else(overflow)?,
-                )
-                .map_err(|_| overflow()),
             }
         } else {
-            TransactionBalance::new(
-                vec![ChangeValue::shielded(
-                    change_pool,
-                    proposed_change,
-                    change_memo,
-                )],
-                fee_amount,
-            )
-            .map_err(|_| overflow())
+            simple_case()
         }
-    }
+    };
+
+    TransactionBalance::new(change, fee).map_err(|_| overflow())
 }