zcash_client_backend: Detect Orchard dust in `zip317::SingleOutputCha…

…ngeStrategy`

zcash_client_backend/CHANGELOG.md

@@ -76,6 +76,8 @@ and this library adheres to Rust's notion of
     constraint on its `<AccountId>` parameter has been strengthened to `Copy`.
 - `zcash_client_backend::fees`:
   - Arguments to `ChangeStrategy::compute_balance` have changed.
+  - `ChangeError::DustInputs` now has an `orchard` field behind the `orchard`
+    feature flag.
 - `zcash_client_backend::proto`:
   - `ProposalDecodingError` has a new variant `TransparentMemo`.
 - `zcash_client_backend::zip321::render::amount_str` now takes a

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -445,8 +445,15 @@ where
                     )
                     .map_err(InputSelectorError::Proposal);
                 }
-                Err(ChangeError::DustInputs { mut sapling, .. }) => {
+                Err(ChangeError::DustInputs {
+                    mut sapling,
+                    #[cfg(feature = "orchard")]
+                    mut orchard,
+                    ..
+                }) => {
                     exclude.append(&mut sapling);
+                    #[cfg(feature = "orchard")]
+                    exclude.append(&mut orchard);
                 }
                 Err(ChangeError::InsufficientFunds { required, .. }) => {
                     amount_required = required;

zcash_client_backend/src/fees.rs

@@ -149,6 +149,9 @@ pub enum ChangeError<E, NoteRefT> {
         transparent: Vec<OutPoint>,
         /// The identifiers for Sapling inputs having no current economic value
         sapling: Vec<NoteRefT>,
+        /// The identifiers for Orchard inputs having no current economic value
+        #[cfg(feature = "orchard")]
+        orchard: Vec<NoteRefT>,
     },
     /// An error occurred that was specific to the change selection strategy in use.
     StrategyError(E),
@@ -169,9 +172,13 @@ impl<E, NoteRefT> ChangeError<E, NoteRefT> {
             ChangeError::DustInputs {
                 transparent,
                 sapling,
+                #[cfg(feature = "orchard")]
+                orchard,
             } => ChangeError::DustInputs {
                 transparent,
                 sapling,
+                #[cfg(feature = "orchard")]
+                orchard,
             },
             ChangeError::StrategyError(e) => ChangeError::StrategyError(f(e)),
             ChangeError::BundleError(e) => ChangeError::BundleError(e),
@@ -194,10 +201,21 @@ impl<CE: fmt::Display, N: fmt::Display> fmt::Display for ChangeError<CE, N> {
             ChangeError::DustInputs {
                 transparent,
                 sapling,
+                #[cfg(feature = "orchard")]
+                orchard,
             } => {
+                #[cfg(feature = "orchard")]
+                let orchard_len = orchard.len();
+                #[cfg(not(feature = "orchard"))]
+                let orchard_len = 0;
+
                 // we can't encode the UA to its string representation because we
                 // don't have network parameters here
-                write!(f, "Insufficient funds: {} dust inputs were present, but would cost more to spend than they are worth.", transparent.len() + sapling.len())
+                write!(
+                    f,
+                    "Insufficient funds: {} dust inputs were present, but would cost more to spend than they are worth.",
+                    transparent.len() + sapling.len() + orchard_len,
+                )
             }
             ChangeError::StrategyError(err) => {
                 write!(f, "{}", err)

zcash_client_backend/src/fees/common.rs

@@ -17,30 +17,26 @@ use super::{
 #[cfg(feature = "orchard")]
 use super::orchard as orchard_fees;
 
+pub(crate) struct NetFlows {
+    t_in: NonNegativeAmount,
+    t_out: NonNegativeAmount,
+    sapling_in: NonNegativeAmount,
+    sapling_out: NonNegativeAmount,
+    orchard_in: NonNegativeAmount,
+    orchard_out: NonNegativeAmount,
+}
+
 #[allow(clippy::too_many_arguments)]
-pub(crate) fn single_change_output_balance<
-    P: consensus::Parameters,
-    NoteRefT: Clone,
-    F: FeeRule,
-    E,
->(
-    params: &P,
-    fee_rule: &F,
-    target_height: BlockHeight,
+pub(crate) fn calculate_net_flows<NoteRefT: Clone, F: FeeRule, E>(
     transparent_inputs: &[impl transparent::InputView],
     transparent_outputs: &[impl transparent::OutputView],
     sapling: &impl sapling_fees::BundleView<NoteRefT>,
     #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
-    dust_output_policy: &DustOutputPolicy,
-    default_dust_threshold: NonNegativeAmount,
-    change_memo: Option<MemoBytes>,
-    _fallback_change_pool: ShieldedProtocol,
-) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
+) -> Result<NetFlows, ChangeError<E, NoteRefT>>
 where
     E: From<F::Error> + From<BalanceError>,
 {
     let overflow = || ChangeError::StrategyError(E::from(BalanceError::Overflow));
-    let underflow = || ChangeError::StrategyError(E::from(BalanceError::Underflow));
 
     let t_in = transparent_inputs
         .iter()
@@ -85,13 +81,30 @@ where
     #[cfg(not(feature = "orchard"))]
     let orchard_out = NonNegativeAmount::ZERO;
 
+    Ok(NetFlows {
+        t_in,
+        t_out,
+        sapling_in,
+        sapling_out,
+        orchard_in,
+        orchard_out,
+    })
+}
+
+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>,
+{
     // TODO: implement a less naive strategy for selecting the pool to which change will be sent.
     #[cfg(feature = "orchard")]
     let (change_pool, sapling_change, orchard_change) =
-        if orchard_in.is_positive() || orchard_out.is_positive() {
+        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 sapling_in.is_positive() || sapling_out.is_positive() {
+        } 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)
@@ -104,7 +117,45 @@ where
             }
         };
     #[cfg(not(feature = "orchard"))]
-    let (change_pool, sapling_change) = (ShieldedProtocol::Sapling, 1);
+    let (change_pool, sapling_change, orchard_change) = (ShieldedProtocol::Sapling, 1, 0);
+
+    Ok((change_pool, sapling_change, orchard_change))
+}
+
+#[allow(clippy::too_many_arguments)]
+pub(crate) fn single_change_output_balance<
+    P: consensus::Parameters,
+    NoteRefT: Clone,
+    F: FeeRule,
+    E,
+>(
+    params: &P,
+    fee_rule: &F,
+    target_height: BlockHeight,
+    transparent_inputs: &[impl transparent::InputView],
+    transparent_outputs: &[impl transparent::OutputView],
+    sapling: &impl sapling_fees::BundleView<NoteRefT>,
+    #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
+    dust_output_policy: &DustOutputPolicy,
+    default_dust_threshold: NonNegativeAmount,
+    change_memo: Option<MemoBytes>,
+    _fallback_change_pool: ShieldedProtocol,
+) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
+where
+    E: From<F::Error> + From<BalanceError>,
+{
+    let overflow = || ChangeError::StrategyError(E::from(BalanceError::Overflow));
+    let underflow = || ChangeError::StrategyError(E::from(BalanceError::Underflow));
+
+    let net_flows = calculate_net_flows::<NoteRefT, F, E>(
+        transparent_inputs,
+        transparent_outputs,
+        sapling,
+        #[cfg(feature = "orchard")]
+        orchard,
+    )?;
+    let (change_pool, sapling_change, _orchard_change) =
+        single_change_output_policy::<NoteRefT, F, E>(&net_flows, _fallback_change_pool)?;
 
     let sapling_input_count = sapling
         .bundle_type()
@@ -123,7 +174,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"))]
@@ -141,8 +192,10 @@ where
         )
         .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?;
 
-    let total_in = (t_in + sapling_in + orchard_in).ok_or_else(overflow)?;
-    let total_out = (t_out + sapling_out + orchard_out + fee_amount).ok_or_else(overflow)?;
+    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)
+        .ok_or_else(overflow)?;
 
     let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds {
         available: total_in,

zcash_client_backend/src/fees/zip317.rs

@@ -16,8 +16,8 @@ use zcash_primitives::{
 use crate::ShieldedProtocol;
 
 use super::{
-    common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy,
-    DustOutputPolicy, TransactionBalance,
+    common::{calculate_net_flows, single_change_output_balance, single_change_output_policy},
+    sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance,
 };
 
 #[cfg(feature = "orchard")]
@@ -93,39 +93,86 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             })
             .collect();
 
+        #[cfg(feature = "orchard")]
+        let mut orchard_dust: Vec<NoteRefT> = orchard
+            .inputs()
+            .iter()
+            .filter_map(|i| {
+                if orchard_fees::InputView::<NoteRefT>::value(i) < self.fee_rule.marginal_fee() {
+                    Some(orchard_fees::InputView::<NoteRefT>::note_id(i).clone())
+                } else {
+                    None
+                }
+            })
+            .collect();
+        #[cfg(not(feature = "orchard"))]
+        let mut orchard_dust: Vec<NoteRefT> = vec![];
+
         // Depending on the shape of the transaction, we may be able to spend up to
         // `grace_actions - 1` dust inputs. If we don't have any dust inputs though,
         // we don't need to worry about any of that.
-        if !(transparent_dust.is_empty() && sapling_dust.is_empty()) {
+        if !(transparent_dust.is_empty() && sapling_dust.is_empty() && orchard_dust.is_empty()) {
             let t_non_dust = transparent_inputs.len() - transparent_dust.len();
             let t_allowed_dust = transparent_outputs.len().saturating_sub(t_non_dust);
 
-            // We add one to the sapling outputs for the (single) change output Note that this
-            // means that wallet-internal shielding transactions are an opportunity to spend a dust
-            // note.
+            // We add one to either the Sapling or Orchard outputs for the (single)
+            // change output. Note that this means that wallet-internal shielding
+            // transactions are an opportunity to spend a dust note.
+            let net_flows = calculate_net_flows::<NoteRefT, Self::FeeRule, Self::Error>(
+                transparent_inputs,
+                transparent_outputs,
+                sapling,
+                #[cfg(feature = "orchard")]
+                orchard,
+            )?;
+            let (_, sapling_change, orchard_change) =
+                single_change_output_policy::<NoteRefT, Self::FeeRule, Self::Error>(
+                    &net_flows,
+                    self.fallback_change_pool,
+                )?;
+
             let s_non_dust = sapling.inputs().len() - sapling_dust.len();
-            let s_allowed_dust = (sapling.outputs().len() + 1).saturating_sub(s_non_dust);
+            let s_allowed_dust =
+                (sapling.outputs().len() + sapling_change).saturating_sub(s_non_dust);
+
+            #[cfg(feature = "orchard")]
+            let (orchard_inputs_len, orchard_outputs_len) =
+                (orchard.inputs().len(), orchard.outputs().len());
+            #[cfg(not(feature = "orchard"))]
+            let (orchard_inputs_len, orchard_outputs_len) = (0, 0);
+
+            let o_non_dust = orchard_inputs_len - orchard_dust.len();
+            let o_allowed_dust = (orchard_outputs_len + orchard_change).saturating_sub(o_non_dust);
 
             let available_grace_inputs = self
                 .fee_rule
                 .grace_actions()
                 .saturating_sub(t_non_dust)
-                .saturating_sub(s_non_dust);
+                .saturating_sub(s_non_dust)
+                .saturating_sub(o_non_dust);
 
             let mut t_disallowed_dust = transparent_dust.len().saturating_sub(t_allowed_dust);
             let mut s_disallowed_dust = sapling_dust.len().saturating_sub(s_allowed_dust);
+            let mut o_disallowed_dust = orchard_dust.len().saturating_sub(o_allowed_dust);
 
             if available_grace_inputs > 0 {
                 // If we have available grace inputs, allocate them first to transparent dust
-                // and then to Sapling dust. The caller has provided inputs that it is willing
-                // to spend, so we don't need to consider privacy effects at this layer.
+                // and then to Sapling dust followed by Orchard dust. The caller has provided
+                // inputs that it is willing to spend, so we don't need to consider privacy
+                // effects at this layer.
                 let t_grace_dust = available_grace_inputs.saturating_sub(t_disallowed_dust);
                 t_disallowed_dust = t_disallowed_dust.saturating_sub(t_grace_dust);
 
                 let s_grace_dust = available_grace_inputs
                     .saturating_sub(t_grace_dust)
                     .saturating_sub(s_disallowed_dust);
                 s_disallowed_dust = s_disallowed_dust.saturating_sub(s_grace_dust);
+
+                let o_grace_dust = available_grace_inputs
+                    .saturating_sub(t_grace_dust)
+                    .saturating_sub(s_grace_dust)
+                    .saturating_sub(o_disallowed_dust);
+                o_disallowed_dust = o_disallowed_dust.saturating_sub(o_grace_dust);
             }
 
             // Truncate the lists of inputs to be disregarded in input selection to just the
@@ -135,11 +182,16 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             transparent_dust.truncate(t_disallowed_dust);
             sapling_dust.reverse();
             sapling_dust.truncate(s_disallowed_dust);
+            orchard_dust.reverse();
+            orchard_dust.truncate(o_disallowed_dust);
 
-            if !(transparent_dust.is_empty() && sapling_dust.is_empty()) {
+            if !(transparent_dust.is_empty() && sapling_dust.is_empty() && orchard_dust.is_empty())
+            {
                 return Err(ChangeError::DustInputs {
                     transparent: transparent_dust,
                     sapling: sapling_dust,
+                    #[cfg(feature = "orchard")]
+                    orchard: orchard_dust,
                 });
             }
         }