From 73ab884073183cf7a87e05ddf5e303b88c761cf0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Nov 2022 17:44:49 -0600 Subject: [PATCH] Implement ZIP 317 fee estimation, calculation, & change selection --- zcash_client_backend/CHANGELOG.md | 4 +- zcash_client_backend/src/fees.rs | 1 + zcash_client_backend/src/fees/zip317.rs | 336 ++++++++++++++++++ zcash_client_sqlite/src/wallet/transact.rs | 121 ++++++- zcash_primitives/CHANGELOG.md | 6 +- .../src/transaction/components/amount.rs | 13 +- zcash_primitives/src/transaction/fees.rs | 1 + .../src/transaction/fees/zip317.rs | 139 ++++++++ 8 files changed, 614 insertions(+), 7 deletions(-) create mode 100644 zcash_client_backend/src/fees/zip317.rs create mode 100644 zcash_primitives/src/transaction/fees/zip317.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d631e8b12..720c7b35c 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -71,8 +71,10 @@ and this library adheres to Rust's notion of - `TransactionBalance` - `BasicFixedFeeChangeStrategy` - a `ChangeStrategy` implementation that reproduces current wallet change behavior - - `fixed` a new module containing of change selection strategies for the + - `fixed`, a new module containing of change selection strategies for the existing fixed fee rule. + - `zip317`, a new module containing change selection strategies for the ZIP + 317 fee rule. - New experimental APIs that should be considered unstable, and are likely to be modified and/or moved to a different module in a future release: diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 55b9a7a9e..aaaca4ed7 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -12,6 +12,7 @@ use zcash_primitives::{ }; pub mod fixed; +pub mod zip317; /// A proposed change amount and output pool. #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs new file mode 100644 index 000000000..fb03298bf --- /dev/null +++ b/zcash_client_backend/src/fees/zip317.rs @@ -0,0 +1,336 @@ +//! Change strategies designed to implement the ZIP 317 fee rules. +//! +//! Change selection in ZIP 317 requires careful handling of low-valued inputs +//! to ensure that inputs added to a transaction do not cause fees to rise by +//! an amount greater than their value. +use core::cmp::Ordering; + +use zcash_primitives::{ + consensus::{self, BlockHeight}, + transaction::{ + components::{ + amount::{Amount, BalanceError}, + sapling::fees as sapling, + transparent::fees as transparent, + }, + fees::{ + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + FeeRule, + }, + }, +}; + +use super::{ + ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, TransactionBalance, +}; + +/// A change strategy that and proposes change as a single output to the most current supported +/// shielded pool and delegates fee calculation to the provided fee rule. +pub struct SingleOutputChangeStrategy { + fee_rule: Zip317FeeRule, +} + +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 + /// fee parameters. + pub fn new(fee_rule: Zip317FeeRule) -> Self { + Self { fee_rule } + } +} + +impl ChangeStrategy for SingleOutputChangeStrategy { + type FeeRule = Zip317FeeRule; + type Error = Zip317FeeError; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn compute_balance( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_inputs: &[impl sapling::InputView], + sapling_outputs: &[impl sapling::OutputView], + dust_output_policy: &DustOutputPolicy, + ) -> Result> { + let mut transparent_dust: Vec<_> = transparent_inputs + .iter() + .filter_map(|i| { + // for now, we're just assuming p2pkh inputs, so we don't check the size of the input + // script + if i.coin().value < self.fee_rule.marginal_fee() { + Some(i.outpoint().clone()) + } else { + None + } + }) + .collect(); + + let mut sapling_dust: Vec<_> = sapling_inputs + .iter() + .filter_map(|i| { + if i.value() < self.fee_rule.marginal_fee() { + Some(i.note_id().clone()) + } else { + None + } + }) + .collect(); + + // 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()) { + 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. + let s_non_dust = sapling_inputs.len() - sapling_dust.len(); + let s_allowed_dust = (sapling_outputs.len() + 1).saturating_sub(s_non_dust); + + let available_grace_inputs = self + .fee_rule + .grace_actions() + .saturating_sub(t_non_dust) + .saturating_sub(s_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); + + if available_grace_inputs > 0 { + // if we have available grace inputs, allocate them first to transparent dust + // and then to sapling dust + 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); + } + + // truncate the lists of inputs to be disregarded in input selection to just the + // disallowed lengths this has the effect of prioritizing inputs for inclusion by the + // order of th original input slices, with the most preferred inputs first + transparent_dust.reverse(); + transparent_dust.truncate(t_disallowed_dust); + sapling_dust.reverse(); + sapling_dust.truncate(s_disallowed_dust); + + if !(transparent_dust.is_empty() && sapling_dust.is_empty()) { + return Err(ChangeError::DustInputs { + transparent: transparent_dust, + sapling: sapling_dust, + }); + } + } + + let overflow = || ChangeError::StrategyError(Zip317FeeError::from(BalanceError::Overflow)); + let underflow = + || ChangeError::StrategyError(Zip317FeeError::from(BalanceError::Underflow)); + + let t_in = transparent_inputs + .iter() + .map(|t_in| t_in.coin().value) + .sum::>() + .ok_or_else(overflow)?; + let t_out = transparent_outputs + .iter() + .map(|t_out| t_out.value()) + .sum::>() + .ok_or_else(overflow)?; + let sapling_in = sapling_inputs + .iter() + .map(|s_in| s_in.value()) + .sum::>() + .ok_or_else(overflow)?; + let sapling_out = sapling_outputs + .iter() + .map(|s_out| s_out.value()) + .sum::>() + .ok_or_else(overflow)?; + + let fee_amount = self + .fee_rule + .fee_required( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs.len(), + sapling_outputs.len() + 1, + ) + .map_err(ChangeError::StrategyError)?; + + let total_in = (t_in + sapling_in).ok_or_else(overflow)?; + + let total_out = [t_out, sapling_out, fee_amount] + .iter() + .sum::>() + .ok_or_else(overflow)?; + + let proposed_change = (total_in - total_out).ok_or_else(underflow)?; + match proposed_change.cmp(&Amount::zero()) { + Ordering::Less => Err(ChangeError::InsufficientFunds { + available: total_in, + required: total_out, + }), + Ordering::Equal => TransactionBalance::new(vec![], fee_amount).ok_or_else(overflow), + Ordering::Greater => { + let dust_threshold = dust_output_policy + .dust_threshold() + .unwrap_or_else(|| self.fee_rule.marginal_fee()); + + if dust_threshold > proposed_change { + match dust_output_policy.action() { + DustAction::Reject => { + let shortfall = + (dust_threshold - proposed_change).ok_or_else(underflow)?; + + Err(ChangeError::InsufficientFunds { + available: total_in, + required: (total_in + shortfall).ok_or_else(overflow)?, + }) + } + DustAction::AllowDustChange => TransactionBalance::new( + vec![ChangeValue::Sapling(proposed_change)], + fee_amount, + ) + .ok_or_else(overflow), + DustAction::AddDustToFee => { + TransactionBalance::new(vec![], (fee_amount + proposed_change).unwrap()) + .ok_or_else(overflow) + } + } + } else { + TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount) + .ok_or_else(overflow) + } + } + } + } +} + +#[cfg(test)] +mod tests { + + use zcash_primitives::{ + consensus::{Network, NetworkUpgrade, Parameters}, + transaction::{ + components::{amount::Amount, transparent::TxOut}, + fees::zip317::FeeRule as Zip317FeeRule, + }, + }; + + use super::SingleOutputChangeStrategy; + use crate::{ + data_api::wallet::input_selection::SaplingPayment, + fees::{ + tests::{TestSaplingInput, TestTransparentInput}, + ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, + }, + }; + + #[test] + fn change_without_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[TestSaplingInput { + note_id: 0, + value: Amount::from_u64(55000).unwrap(), + }], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(Amount::from_u64(5000).unwrap())] + && balance.fee_required() == Amount::from_u64(10000).unwrap() + ); + } + + #[test] + fn change_with_allowable_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[ + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(49000).unwrap(), + }, + TestSaplingInput { + note_id: 1, + value: Amount::from_u64(1000).unwrap(), + }, + ], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Ok(balance) if balance.proposed_change().is_empty() + && balance.fee_required() == Amount::from_u64(10000).unwrap() + ); + } + + #[test] + fn change_with_disallowed_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[ + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(29000).unwrap(), + }, + TestSaplingInput { + note_id: 1, + value: Amount::from_u64(20000).unwrap(), + }, + TestSaplingInput { + note_id: 2, + value: Amount::from_u64(1000).unwrap(), + }, + ], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + // We will get an error here, because the dust input now isn't free to add + // to the transaction. + assert_matches!( + result, + Err(ChangeError::DustInputs { sapling, .. }) if sapling == vec![2] + ); + } +} diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index c099798f7..23e87c5de 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/zcash_client_sqlite/src/wallet/transact.rs @@ -193,17 +193,23 @@ mod tests { consensus::{BlockHeight, BranchId}, legacy::TransparentAddress, sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver}, - transaction::{components::Amount, Transaction}, + transaction::{components::Amount, fees::zip317::FeeRule as Zip317FeeRule, Transaction}, zip32::sapling::ExtendedSpendingKey, }; use zcash_client_backend::{ + address::RecipientAddress, data_api::{ - self, chain::scan_cached_blocks, wallet::create_spend_to_address, WalletRead, - WalletWrite, + self, + chain::scan_cached_blocks, + error::Error, + wallet::{create_spend_to_address, input_selection::GreedyInputSelector, spend}, + WalletRead, WalletWrite, }, + fees::{zip317, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::OvkPolicy, + zip321::{Payment, TransactionRequest}, }; use crate::{ @@ -832,4 +838,113 @@ mod tests { Ok(_) ); } + + #[test] + fn zip317_spend() { + let cache_file = NamedTempFile::new().unwrap(); + let db_cache = BlockDb(Connection::open(cache_file.path()).unwrap()); + init_cache_database(&db_cache).unwrap(); + + let data_file = NamedTempFile::new().unwrap(); + let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); + init_wallet_db(&mut db_data, None).unwrap(); + + // Add an account to the wallet + let mut ops = db_data.get_update_ops().unwrap(); + let seed = Secret::new([0u8; 32].to_vec()); + let (_, usk) = ops.create_account(&seed).unwrap(); + let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); + + // Add funds to the wallet + let (cb, _) = fake_compact_block( + sapling_activation_height(), + BlockHash([0; 32]), + &dfvk, + AddressType::Internal, + Amount::from_u64(50000).unwrap(), + ); + insert_into_cache(&db_cache, &cb); + + // Add 10 dust notes to the wallet + for i in 1..=10 { + let (cb, _) = fake_compact_block( + sapling_activation_height() + i, + cb.hash(), + &dfvk, + AddressType::DefaultExternal, + Amount::from_u64(1000).unwrap(), + ); + insert_into_cache(&db_cache, &cb); + } + + let mut db_write = db_data.get_update_ops().unwrap(); + scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); + + // Verified balance matches total balance + let total = Amount::from_u64(60000).unwrap(); + let (_, anchor_height) = db_data.get_target_and_anchor_heights(1).unwrap().unwrap(); + assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), total); + assert_eq!( + get_balance_at(&db_data, AccountId::from(0), anchor_height).unwrap(), + total + ); + + let input_selector = GreedyInputSelector::new( + zip317::SingleOutputChangeStrategy::new(Zip317FeeRule::standard()), + DustOutputPolicy::default(), + ); + + // This first request will fail due to insufficient non-dust funds + let req = TransactionRequest::new(vec![Payment { + recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), + amount: Amount::from_u64(50000).unwrap(), + memo: None, + label: None, + message: None, + other_params: vec![], + }]) + .unwrap(); + + assert_matches!( + spend( + &mut db_write, + &tests::network(), + test_prover(), + &input_selector, + &usk, + req, + OvkPolicy::Sender, + 1, + ), + Err(Error::InsufficientFunds { available, required }) + if available == Amount::from_u64(51000).unwrap() + && required == Amount::from_u64(60000).unwrap() + ); + + // This request will succeed, spending a single dust input to pay the 10000 + // ZAT fee in addition to the 41000 ZAT output to the recipient + let req = TransactionRequest::new(vec![Payment { + recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), + amount: Amount::from_u64(41000).unwrap(), + memo: None, + label: None, + message: None, + other_params: vec![], + }]) + .unwrap(); + + assert_matches!( + spend( + &mut db_write, + &tests::network(), + test_prover(), + &input_selector, + &usk, + req, + OvkPolicy::Sender, + 1, + ), + Ok(_) + ); + } } diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 10b701eeb..5a9bae7ca 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -27,8 +27,10 @@ and this library adheres to Rust's notion of and types related to fee calculations. - `FeeRule` a trait that describes how to compute the fee required for a transaction given inputs and outputs to the transaction. - - `zcash_primitives::transaction::fees::fixed` a new module containing an implementation - of the existing fixed fee rule. + - `fixed`, a new module containing an implementation of the existing fixed + fee rule. + - `zip317`, a new module containing an implementation of the ZIP 317 fee + rules. - Added to `zcash_primitives::transaction::components::sapling::builder` - `SaplingBuilder::{inputs, outputs}`: accessors for Sapling builder state. - `zcash_primitives::transaction::components::sapling::fees` diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 11881f0e6..532e225f8 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -1,6 +1,6 @@ use std::convert::TryFrom; use std::iter::Sum; -use std::ops::{Add, AddAssign, Neg, Sub, SubAssign}; +use std::ops::{Add, AddAssign, Mul, Neg, Sub, SubAssign}; use memuse::DynamicUsage; use orchard::value as orchard; @@ -205,6 +205,17 @@ impl Neg for Amount { } } +impl Mul for Amount { + type Output = Option; + + fn mul(self, rhs: usize) -> Option { + let rhs: i64 = rhs.try_into().ok()?; + self.0 + .checked_mul(rhs) + .and_then(|i| Amount::try_from(i).ok()) + } +} + impl TryFrom for Amount { type Error = (); diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index 2a9a9021a..e29913acc 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -9,6 +9,7 @@ use crate::{ use crate::transaction::components::tze::fees as tze; pub mod fixed; +pub mod zip317; /// A trait that represents the ability to compute the fees that must be paid /// by a transaction having a specified set of inputs and outputs. diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs new file mode 100644 index 000000000..6f5218129 --- /dev/null +++ b/zcash_primitives/src/transaction/fees/zip317.rs @@ -0,0 +1,139 @@ +//! Types related to implementing a [`FeeRule`] provides [ZIP 317] fee calculation. +//! +//! [`FeeRule`]: crate::transaction::fees::FeeRule +//! [ZIP 317]: https//zips.z.cash/zip-0317 +use core::cmp::max; + +use crate::{ + consensus::{self, BlockHeight}, + legacy::TransparentAddress, + transaction::components::{ + amount::{Amount, BalanceError}, + transparent::{fees as transparent, OutPoint}, + }, +}; + +/// A [`FeeRule`] implementation that implements the [ZIP 317] fee rule. +/// +/// This fee rule supports only P2pkh transparent inputs; an error will be returned if a coin +/// containing a non-p2pkh script is provided as an input. This fee rule may slightly overestimate +/// fees in case where the user is attempting to spend more than ~150 transparent inputs. +/// +/// [`FeeRule`]: crate::transaction::fees::FeeRule +/// [ZIP 317]: https//zips.z.cash/zip-0317 +#[derive(Clone, Debug)] +pub struct FeeRule { + marginal_fee: Amount, + grace_actions: usize, + p2pkh_standard_input_size: usize, + p2pkh_standard_output_size: usize, +} + +impl FeeRule { + /// Construct a new FeeRule using the standard [ZIP 317] constants. + /// + /// [ZIP 317]: https//zips.z.cash/zip-0317 + pub fn standard() -> Self { + Self { + marginal_fee: Amount::from_u64(5000).unwrap(), + grace_actions: 2, + p2pkh_standard_input_size: 150, + p2pkh_standard_output_size: 34, + } + } + + /// Construct a new FeeRule instance with the specified parameter values. + /// + /// Returns `None` if either `p2pkh_standard_input_size` or `p2pkh_standard_output_size` are + /// zero. + pub fn non_standard( + marginal_fee: Amount, + grace_actions: usize, + p2pkh_standard_input_size: usize, + p2pkh_standard_output_size: usize, + ) -> Option { + if p2pkh_standard_input_size == 0 || p2pkh_standard_output_size == 0 { + None + } else { + Some(Self { + marginal_fee, + grace_actions, + p2pkh_standard_input_size, + p2pkh_standard_output_size, + }) + } + } + + /// Returns the ZIP 317 marginal fee. + pub fn marginal_fee(&self) -> Amount { + self.marginal_fee + } + /// Returns the ZIP 317 number of grace actions + pub fn grace_actions(&self) -> usize { + self.grace_actions + } + /// Returns the ZIP 317 standard P2PKH input size + pub fn p2pkh_standard_input_size(&self) -> usize { + self.p2pkh_standard_input_size + } + /// Returns the ZIP 317 standard P2PKH output size + pub fn p2pkh_standard_output_size(&self) -> usize { + self.p2pkh_standard_output_size + } +} + +/// Errors that can occur in ZIP 317 fee computation +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum FeeError { + /// An overflow or underflow of amount computation occurred. + Balance(BalanceError), + /// Transparent inputs provided to the fee calculation included coins that do not pay to + /// standard P2pkh scripts. + NonP2pkhInputs(Vec), +} + +impl From for FeeError { + fn from(err: BalanceError) -> Self { + FeeError::Balance(err) + } +} + +impl super::FeeRule for FeeRule { + type Error = FeeError; + + fn fee_required( + &self, + _params: &P, + _target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_input_count: usize, + sapling_output_count: usize, + ) -> Result { + let non_p2pkh_inputs: Vec<_> = transparent_inputs + .iter() + .filter_map(|t_in| match t_in.coin().script_pubkey.address() { + Some(TransparentAddress::PublicKey(_)) => None, + _ => Some(t_in.outpoint()), + }) + .cloned() + .collect(); + + if !non_p2pkh_inputs.is_empty() { + return Err(FeeError::NonP2pkhInputs(non_p2pkh_inputs)); + } + + let t_in_total_size = transparent_inputs.len() * 150; + let t_out_total_size = transparent_outputs.len() * 34; + + let ceildiv = |num: usize, den: usize| (num + den - 1) / den; + + let logical_actions = max( + ceildiv(t_in_total_size, self.p2pkh_standard_input_size), + ceildiv(t_out_total_size, self.p2pkh_standard_output_size), + ) + max(sapling_input_count, sapling_output_count); + + (self.marginal_fee * max(self.grace_actions, logical_actions)) + .ok_or_else(|| BalanceError::Overflow.into()) + } +}