diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 86cbe6337b..327c93d612 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -76,6 +76,8 @@ and this library adheres to Rust's notion of constraint on its `` 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 diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index 1627698bab..2cbef319e0 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/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; diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 6c512a2043..9106a3883a 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -149,6 +149,9 @@ pub enum ChangeError { transparent: Vec, /// The identifiers for Sapling inputs having no current economic value sapling: Vec, + /// The identifiers for Orchard inputs having no current economic value + #[cfg(feature = "orchard")] + orchard: Vec, }, /// An error occurred that was specific to the change selection strategy in use. StrategyError(E), @@ -169,9 +172,13 @@ impl ChangeError { 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 fmt::Display for ChangeError { 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) diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 31c511daa7..a8a791a527 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/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( transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, - dust_output_policy: &DustOutputPolicy, - default_dust_threshold: NonNegativeAmount, - change_memo: Option, - _fallback_change_pool: ShieldedProtocol, -) -> Result> +) -> Result> where E: From + From, { 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( + _net_flows: &NetFlows, + _fallback_change_pool: ShieldedProtocol, +) -> Result<(ShieldedProtocol, usize, usize), ChangeError> +where + E: From + From, +{ // 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, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, + dust_output_policy: &DustOutputPolicy, + default_dust_threshold: NonNegativeAmount, + change_memo: Option, + _fallback_change_pool: ShieldedProtocol, +) -> Result> +where + E: From + From, +{ + let overflow = || ChangeError::StrategyError(E::from(BalanceError::Overflow)); + let underflow = || ChangeError::StrategyError(E::from(BalanceError::Underflow)); + + let net_flows = calculate_net_flows::( + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + )?; + let (change_pool, sapling_change, _orchard_change) = + single_change_output_policy::(&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, diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 3b703f67e3..ebc2aec867 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/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,32 +93,73 @@ impl ChangeStrategy for SingleOutputChangeStrategy { }) .collect(); + #[cfg(feature = "orchard")] + let mut orchard_dust: Vec = orchard + .inputs() + .iter() + .filter_map(|i| { + if orchard_fees::InputView::::value(i) < self.fee_rule.marginal_fee() { + Some(orchard_fees::InputView::::note_id(i).clone()) + } else { + None + } + }) + .collect(); + #[cfg(not(feature = "orchard"))] + let mut orchard_dust: Vec = 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::( + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + )?; + let (_, sapling_change, orchard_change) = + single_change_output_policy::( + &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); @@ -126,6 +167,12 @@ impl ChangeStrategy for SingleOutputChangeStrategy { .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, }); } }