From aa4312326d8106e2287e60915ea8acc2f8b1ea1d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 16 Jul 2024 13:53:31 -0600 Subject: [PATCH] Use `EphemeralBalance` instead of `EphemeralParameters` The `EphemeralParameters` type makes too many states representable, in particular, it represents that it is possible for a proposal step to have both ephemeral inputs and ephemeral outputs. This change reduces the representable state space to make it so that only one or the other is true, and also makes clear that the change memo must be attached to the change output of the intermediate step, and not to the ultimate output to the final recipient (where we expect there to be no shielded change, and therefore it is not possible to attach a memo.) --- zcash_client_backend/CHANGELOG.md | 5 +- .../src/data_api/wallet/input_selection.rs | 27 ++++--- zcash_client_backend/src/fees.rs | 71 ++++++------------- zcash_client_backend/src/fees/common.rs | 41 ++++++----- zcash_client_backend/src/fees/fixed.rs | 12 ++-- zcash_client_backend/src/fees/standard.rs | 10 +-- zcash_client_backend/src/fees/zip317.rs | 21 +++--- 7 files changed, 80 insertions(+), 107 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c5cc60cd1..e0d0a8cfb 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -28,6 +28,7 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `chain::BlockCache` trait, behind the `sync` feature flag. - `WalletRead::get_spendable_transparent_outputs`. - `zcash_client_backend::fees`: + - `EphemeralBalance` - `ChangeValue::shielded, is_ephemeral` - `ChangeValue::ephemeral_transparent` (when "transparent-inputs" is enabled) - `sapling::EmptyBundleView` @@ -67,10 +68,10 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail return type of `ChangeValue::output_pool` has (unconditionally) changed from `ShieldedProtocol` to `zcash_protocol::PoolType`. - `ChangeStrategy::compute_balance`: this trait method has an additional - `&EphemeralParameters` parameter. If the "transparent-inputs" feature is + `Option<&EphemeralBalance>` parameter. If the "transparent-inputs" feature is enabled, this can be used to specify whether the change memo should be ignored, and the amounts of additional transparent P2PKH inputs and - outputs. Passing `&EphemeralParameters::NONE` will retain the previous + outputs. Passing `None` will retain the previous behaviour (and is necessary when the "transparent-inputs" feature is not enabled). - `zcash_client_backend::input_selection::GreedyInputSelectorError` has 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 162290ef9..a3518795c 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -23,7 +23,7 @@ use zcash_primitives::{ use crate::{ address::{Address, UnifiedAddress}, data_api::{InputSource, SimpleNoteRetention, SpendableNotes}, - fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralParameters}, + fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy}, proposal::{Proposal, ProposalError, ShieldedInputs}, wallet::WalletTransparentOutput, zip321::TransactionRequest, @@ -33,7 +33,7 @@ use crate::{ #[cfg(feature = "transparent-inputs")] use { crate::{ - fees::ChangeValue, + fees::{ChangeValue, EphemeralBalance}, proposal::{Step, StepOutput, StepOutputIndex}, zip321::Payment, }, @@ -460,12 +460,12 @@ where } #[cfg(not(feature = "transparent-inputs"))] - let ephemeral_parameters = EphemeralParameters::NONE; + let ephemeral_balance = None; #[cfg(feature = "transparent-inputs")] - let (ephemeral_parameters, tr1_balance_opt) = { + let (ephemeral_balance, tr1_balance_opt) = { if tr1_transparent_outputs.is_empty() { - (EphemeralParameters::NONE, None) + (None, None) } else { // The ephemeral input going into transaction 1 must be able to pay that // transaction's fee, as well as the TEX address payments. @@ -484,7 +484,7 @@ where #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &self.dust_output_policy, - &EphemeralParameters::new(true, Some(NonNegativeAmount::ZERO), None), + Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)), ) { Err(ChangeError::InsufficientFunds { required, .. }) => required, Ok(_) => NonNegativeAmount::ZERO, // shouldn't happen @@ -502,12 +502,12 @@ where #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &self.dust_output_policy, - &EphemeralParameters::new(true, Some(tr1_required_input_value), None), + Some(&EphemeralBalance::Input(tr1_required_input_value)), )?; assert_eq!(tr1_balance.total(), tr1_balance.fee_required()); ( - EphemeralParameters::new(false, None, Some(tr1_required_input_value)), + Some(EphemeralBalance::Output(tr1_required_input_value)), Some(tr1_balance), ) } @@ -582,7 +582,7 @@ where &orchard_outputs[..], ), &self.dust_output_policy, - &ephemeral_parameters, + ephemeral_balance.as_ref(), ); match balance { @@ -609,8 +609,8 @@ where assert_eq!( *balance.proposed_change().last().expect("nonempty"), ChangeValue::ephemeral_transparent( - ephemeral_parameters - .ephemeral_output_amount() + ephemeral_balance + .and_then(|b| b.ephemeral_output_amount()) .expect("ephemeral output is present") ) ); @@ -773,7 +773,7 @@ where #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &self.dust_output_policy, - &EphemeralParameters::NONE, + None, ); let balance = match trial_balance { @@ -791,8 +791,7 @@ where #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &self.dust_output_policy, - #[cfg(feature = "transparent-inputs")] - &EphemeralParameters::NONE, + None, )? } Err(other) => { diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 51867a9f1..218e9cc76 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -330,61 +330,36 @@ impl Default for DustOutputPolicy { } } -/// `EphemeralParameters` can be used to specify variations on how balance -/// and fees are computed that are relevant to transactions using ephemeral -/// transparent outputs. These are only relevant when the "transparent-inputs" -/// feature is enabled, but to reduce feature-dependent boilerplate, the type -/// and the `EphemeralParameters::NONE` constant are present unconditionally. +/// `EphemeralBalance` describes the ephemeral input or output value for +/// a transaction. It is use in the computation of fees are relevant to transactions using +/// ephemeral transparent outputs. #[derive(Clone, Debug, PartialEq, Eq)] -pub struct EphemeralParameters { - ignore_change_memo: bool, - ephemeral_input_amount: Option, - ephemeral_output_amount: Option, +pub enum EphemeralBalance { + Input(NonNegativeAmount), + Output(NonNegativeAmount), } -impl EphemeralParameters { - /// An `EphemeralParameters` indicating no use of ephemeral inputs - /// or outputs. It has: - /// - /// * `ignore_change_memo: false`, - /// * `ephemeral_input_amount: None`, - /// * `ephemeral_output_amount: None`. - pub const NONE: Self = Self { - ignore_change_memo: false, - ephemeral_input_amount: None, - ephemeral_output_amount: None, - }; +impl EphemeralBalance { + pub fn is_input(&self) -> bool { + matches!(self, EphemeralBalance::Input(_)) + } - /// Returns an `EphemeralParameters` with the following parameters: - /// - /// * `ignore_change_memo`: `true` if the change memo should be - /// ignored for the purpose of deciding whether there will be a - /// change output. - /// * `ephemeral_input_amount`: specifies that there will be an - /// additional P2PKH input of the given amount. - /// * `ephemeral_output_amount`: specifies that there will be an - /// additional P2PKH output of the given amount. - #[cfg(feature = "transparent-inputs")] - pub const fn new( - ignore_change_memo: bool, - ephemeral_input_amount: Option, - ephemeral_output_amount: Option, - ) -> Self { - Self { - ignore_change_memo, - ephemeral_input_amount, - ephemeral_output_amount, + pub fn is_output(&self) -> bool { + matches!(self, EphemeralBalance::Output(_)) + } + + pub fn ephemeral_input_amount(&self) -> Option { + match self { + EphemeralBalance::Input(v) => Some(*v), + EphemeralBalance::Output(_) => None, } } - pub fn ignore_change_memo(&self) -> bool { - self.ignore_change_memo - } - pub fn ephemeral_input_amount(&self) -> Option { - self.ephemeral_input_amount - } pub fn ephemeral_output_amount(&self) -> Option { - self.ephemeral_output_amount + match self { + EphemeralBalance::Input(_) => None, + EphemeralBalance::Output(v) => Some(*v), + } } } @@ -433,7 +408,7 @@ pub trait ChangeStrategy { sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, dust_output_policy: &DustOutputPolicy, - ephemeral_parameters: &EphemeralParameters, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 560d8b4e6..f1b0b7319 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -12,7 +12,7 @@ use zcash_protocol::ShieldedProtocol; use super::{ sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, - EphemeralParameters, TransactionBalance, + EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -49,8 +49,7 @@ pub(crate) fn calculate_net_flows( transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, - ephemeral_input_amount: Option, - ephemeral_output_amount: Option, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result> where E: From + From, @@ -60,13 +59,13 @@ where let t_in = transparent_inputs .iter() .map(|t_in| t_in.coin().value) - .chain(ephemeral_input_amount) + .chain(ephemeral_balance.and_then(|b| b.ephemeral_input_amount())) .sum::>() .ok_or_else(overflow)?; let t_out = transparent_outputs .iter() .map(|t_out| t_out.value()) - .chain(ephemeral_output_amount) + .chain(ephemeral_balance.and_then(|b| b.ephemeral_output_amount())) .sum::>() .ok_or_else(overflow)?; let sapling_in = sapling @@ -162,12 +161,15 @@ pub(crate) fn single_change_output_balance< fallback_change_pool: ShieldedProtocol, marginal_fee: NonNegativeAmount, grace_actions: usize, - ephemeral_parameters: &EphemeralParameters, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result> where E: From + From, { - let change_memo = change_memo.filter(|_| !ephemeral_parameters.ignore_change_memo()); + // The change memo, if any, must be attached to the change in the intermediate step that + // produces the ephemeral output, and so it should be discarded in the ultimate step; this is + // distinguished by identifying that this transaction has ephemeral inputs. + let change_memo = change_memo.filter(|_| ephemeral_balance.map_or(true, |b| !b.is_input())); let overflow = || ChangeError::StrategyError(E::from(BalanceError::Overflow)); let underflow = || ChangeError::StrategyError(E::from(BalanceError::Underflow)); @@ -178,8 +180,7 @@ where sapling, #[cfg(feature = "orchard")] orchard, - ephemeral_parameters.ephemeral_input_amount(), - ephemeral_parameters.ephemeral_output_amount(), + ephemeral_balance, )?; #[allow(unused_variables)] @@ -213,7 +214,7 @@ where marginal_fee, grace_actions, &possible_change[..], - ephemeral_parameters, + ephemeral_balance, )?; } @@ -287,16 +288,16 @@ where .iter() .map(|i| i.serialized_size()) .chain( - ephemeral_parameters - .ephemeral_input_amount() + ephemeral_balance + .and_then(|b| b.ephemeral_input_amount()) .map(|_| transparent::InputSize::STANDARD_P2PKH), ); let transparent_output_sizes = transparent_outputs .iter() .map(|i| i.serialized_size()) .chain( - ephemeral_parameters - .ephemeral_output_amount() + ephemeral_balance + .and_then(|b| b.ephemeral_output_amount()) .map(|_| P2PKH_STANDARD_OUTPUT_SIZE), ); @@ -426,8 +427,8 @@ where }; #[cfg(feature = "transparent-inputs")] change.extend( - ephemeral_parameters - .ephemeral_output_amount() + ephemeral_balance + .and_then(|b| b.ephemeral_output_amount()) .map(ChangeValue::ephemeral_transparent), ); @@ -456,7 +457,7 @@ pub(crate) fn check_for_uneconomic_inputs( marginal_fee: NonNegativeAmount, grace_actions: usize, possible_change: &[(usize, usize, usize)], - ephemeral_parameters: &EphemeralParameters, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result<(), ChangeError> { let mut t_dust: Vec<_> = transparent_inputs .iter() @@ -504,10 +505,8 @@ pub(crate) fn check_for_uneconomic_inputs( } let (t_inputs_len, t_outputs_len) = ( - transparent_inputs.len() - + usize::from(ephemeral_parameters.ephemeral_input_amount().is_some()), - transparent_outputs.len() - + usize::from(ephemeral_parameters.ephemeral_output_amount().is_some()), + transparent_inputs.len() + usize::from(ephemeral_balance.map_or(false, |b| b.is_input())), + transparent_outputs.len() + usize::from(ephemeral_balance.map_or(false, |b| b.is_output())), ); let (s_inputs_len, s_outputs_len) = (sapling.inputs().len(), sapling.outputs().len()); #[cfg(feature = "orchard")] diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index e42e4527d..933b00820 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -13,7 +13,7 @@ use crate::ShieldedProtocol; use super::{ common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, - DustOutputPolicy, EphemeralParameters, TransactionBalance, + DustOutputPolicy, EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -63,7 +63,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, - ephemeral_parameters: &EphemeralParameters, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result> { single_change_output_balance( params, @@ -80,7 +80,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { self.fallback_change_pool, NonNegativeAmount::ZERO, 0, - ephemeral_parameters, + ephemeral_balance, ) } } @@ -100,7 +100,7 @@ mod tests { data_api::wallet::input_selection::SaplingPayment, fees::{ tests::{TestSaplingInput, TestTransparentInput}, - ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, EphemeralParameters, + ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, }, ShieldedProtocol, }; @@ -136,7 +136,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -182,7 +182,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index 8c49e33b3..5c08c7b1a 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -18,7 +18,7 @@ use crate::ShieldedProtocol; use super::{ fixed, sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, - EphemeralParameters, TransactionBalance, + EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -68,7 +68,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, - ephemeral_parameters: &EphemeralParameters, + ephemeral_balance: Option<&EphemeralBalance>, ) -> Result> { #[allow(deprecated)] match self.fee_rule() { @@ -86,7 +86,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(feature = "orchard")] orchard, dust_output_policy, - ephemeral_parameters, + ephemeral_balance, ) .map_err(|e| e.map(Zip317FeeError::Balance)), StandardFeeRule::Zip313 => fixed::SingleOutputChangeStrategy::new( @@ -103,7 +103,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(feature = "orchard")] orchard, dust_output_policy, - ephemeral_parameters, + ephemeral_balance, ) .map_err(|e| e.map(Zip317FeeError::Balance)), StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::new( @@ -120,7 +120,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(feature = "orchard")] orchard, dust_output_policy, - ephemeral_parameters, + ephemeral_balance, ), } } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index a054f9b41..e2c6152c8 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -17,7 +17,7 @@ use crate::ShieldedProtocol; use super::{ common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, - DustOutputPolicy, EphemeralParameters, TransactionBalance, + DustOutputPolicy, EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -67,7 +67,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, - ephemeral_parameters: &EphemeralParameters, + ephemeral_parameters: Option<&EphemeralBalance>, ) -> Result> { single_change_output_balance( params, @@ -108,7 +108,6 @@ mod tests { fees::{ tests::{TestSaplingInput, TestTransparentInput}, ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, - EphemeralParameters, }, ShieldedProtocol, }; @@ -148,7 +147,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -192,7 +191,7 @@ mod tests { ))][..], ), &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -245,7 +244,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, dust_output_policy, - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -289,7 +288,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -333,7 +332,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -383,7 +382,7 @@ mod tests { DustAction::AllowDustChange, Some(NonNegativeAmount::const_from_u64(1000)), ), - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -444,7 +443,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, dust_output_policy, - &EphemeralParameters::NONE, + None, ); assert_matches!( @@ -495,7 +494,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, &DustOutputPolicy::default(), - &EphemeralParameters::NONE, + None, ); // We will get an error here, because the dust input isn't free to add