From c80893b2fa19de1a0287cdee1bc64aece0dfd2e2 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Oct 2024 12:32:25 -0600 Subject: [PATCH 1/9] zcash_client_backend: Use an explicit struct for change output counts instead of a tuple. --- zcash_client_backend/src/fees/common.rs | 80 ++++++++++++++++++------- 1 file changed, 58 insertions(+), 22 deletions(-) diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index f1b0b7319e..99dcf046b2 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -141,6 +141,29 @@ pub(crate) fn single_change_output_policy( ) } +#[derive(Clone, Copy, Debug)] +pub(crate) struct OutputManifest { + transparent: usize, + sapling: usize, + orchard: usize, +} + +impl OutputManifest { + const ZERO: OutputManifest = OutputManifest { + transparent: 0, + sapling: 0, + orchard: 0, + }; + + pub(crate) fn sapling(&self) -> usize { + self.sapling + } + + pub(crate) fn orchard(&self) -> usize { + self.orchard + } +} + #[allow(clippy::too_many_arguments)] pub(crate) fn single_change_output_balance< P: consensus::Parameters, @@ -200,9 +223,16 @@ where let possible_change = // These are the situations where we might not have a change output. if transparent || (dust_output_policy.action() == DustAction::AddDustToFee && change_memo.is_none()) { - vec![(0, 0, 0), (0, sapling_change, orchard_change)] + vec![ + OutputManifest::ZERO, + OutputManifest { + transparent: 0, + sapling: sapling_change, + orchard: orchard_change + } + ] } else { - vec![(0, sapling_change, orchard_change)] + vec![OutputManifest { transparent: 0, sapling: sapling_change, orchard: orchard_change}] }; check_for_uneconomic_inputs( @@ -456,7 +486,7 @@ pub(crate) fn check_for_uneconomic_inputs( #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, marginal_fee: NonNegativeAmount, grace_actions: usize, - possible_change: &[(usize, usize, usize)], + possible_change: &[OutputManifest], ephemeral_balance: Option<&EphemeralBalance>, ) -> Result<(), ChangeError> { let mut t_dust: Vec<_> = transparent_inputs @@ -519,7 +549,7 @@ pub(crate) fn check_for_uneconomic_inputs( let o_non_dust = o_inputs_len.checked_sub(o_dust.len()).unwrap(); // Return the number of allowed dust inputs from each pool. - let allowed_dust = |(t_change, s_change, o_change): &(usize, usize, usize)| { + let allowed_dust = |change: &OutputManifest| { // Here we assume a "ZIP 317-like" fee model in which the existence of an output // to a given pool implies that a corresponding input from that pool can be // provided without increasing the fee. (This is also likely to be true for @@ -534,15 +564,15 @@ pub(crate) fn check_for_uneconomic_inputs( let t_allowed = min( t_dust.len(), - (t_outputs_len + t_change).saturating_sub(t_non_dust), + (t_outputs_len + change.transparent).saturating_sub(t_non_dust), ); let s_allowed = min( s_dust.len(), - (s_outputs_len + s_change).saturating_sub(s_non_dust), + (s_outputs_len + change.sapling).saturating_sub(s_non_dust), ); let o_allowed = min( o_dust.len(), - (o_outputs_len + o_change).saturating_sub(o_non_dust), + (o_outputs_len + change.orchard).saturating_sub(o_non_dust), ); // We'll be spending the non-dust and allowed dust in each pool. @@ -566,22 +596,24 @@ pub(crate) fn check_for_uneconomic_inputs( let s_output_count = sapling .bundle_type() - .num_outputs(s_req_inputs + s_extra, s_outputs_len + s_change) + .num_outputs(s_req_inputs + s_extra, s_outputs_len + change.sapling) .map_err(ChangeError::BundleError)?; #[cfg(feature = "orchard")] let o_action_count = orchard .bundle_type() - .num_actions(o_req_inputs + _o_extra, o_outputs_len + o_change) + .num_actions(o_req_inputs + _o_extra, o_outputs_len + change.orchard) .map_err(ChangeError::BundleError)?; #[cfg(not(feature = "orchard"))] let o_action_count = 0; // To calculate the number of unused actions, we assume that transparent inputs // and outputs are P2PKH. - Ok(max(t_req_inputs + t_extra, t_outputs_len + t_change) - + max(s_spend_count, s_output_count) - + o_action_count) + Ok( + max(t_req_inputs + t_extra, t_outputs_len + change.transparent) + + max(s_spend_count, s_output_count) + + o_action_count, + ) }; // First calculate the baseline number of logical actions with only the definitely @@ -606,27 +638,31 @@ pub(crate) fn check_for_uneconomic_inputs( } else { (0, 0, 0) }; - Ok(( - t_allowed + t_extra, - s_allowed + s_extra, - o_allowed + o_extra, - )) + Ok(OutputManifest { + transparent: t_allowed + t_extra, + sapling: s_allowed + s_extra, + orchard: o_allowed + o_extra, + }) }; // Find the least number of allowed dust inputs for each pool for any `possible_change`. - let (t_allowed, s_allowed, o_allowed) = possible_change + let allowed = possible_change .iter() .map(allowed_dust) .collect::, _>>()? .into_iter() - .reduce(|(a, b, c), (x, y, z)| (min(a, x), min(b, y), min(c, z))) + .reduce(|l, r| OutputManifest { + transparent: min(l.transparent, r.transparent), + sapling: min(l.sapling, r.sapling), + orchard: min(l.orchard, r.orchard), + }) .expect("possible_change is nonempty"); // The inputs in the tail of each list after the first `*_allowed` are returned as uneconomic. // The caller should order the inputs from most to least preferred to spend. - let t_dust = t_dust.split_off(t_allowed); - let s_dust = s_dust.split_off(s_allowed); - let o_dust = o_dust.split_off(o_allowed); + let t_dust = t_dust.split_off(allowed.transparent); + let s_dust = s_dust.split_off(allowed.sapling); + let o_dust = o_dust.split_off(allowed.orchard); if t_dust.is_empty() && s_dust.is_empty() && o_dust.is_empty() { Ok(()) From f8970b0dbb5cee2dd93467b7b3013a19b92b9e22 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 15 Oct 2024 16:39:14 -0600 Subject: [PATCH 2/9] zcash_client_backend: Add `WalletMeta` type & retrieval method. --- zcash_client_backend/CHANGELOG.md | 7 +- zcash_client_backend/src/data_api.rs | 74 ++++++++++++++++++++ zcash_client_backend/src/data_api/testing.rs | 13 +++- zcash_client_sqlite/src/lib.rs | 35 ++++++++- zcash_client_sqlite/src/wallet/common.rs | 51 ++++++++++++++ 5 files changed, 175 insertions(+), 5 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 67f7e418ab..49d277096e 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -11,6 +11,11 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `Progress` - `WalletSummary::progress` + - `WalletMeta` + +### Changed +- `zcash_client_backend::data_api`: + - `InputSource` has an added method `get_wallet_metadata` ### Changed - Migrated to `arti-client 0.22`. @@ -27,7 +32,7 @@ and this library adheres to Rust's notion of - `GAP_LIMIT` - `WalletSummary::recovery_progress` - `SpendableNotes::{take_sapling, take_orchard}` - - Tests and testing infrastructure have been migrated from the + - Tests and testing infrastructure have been migrated from the `zcash_client_sqlite` internal tests to the `testing` module, and have been generalized so that they may be used for testing arbitrary implementations of the `zcash_client_backend::data_api` interfaces. The following have been diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 38ac7abf2e..bc2dad95c2 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -794,6 +794,69 @@ impl SpendableNotes { } } +/// Metadata about the structure of the wallet for a particular account. +/// +/// At present this just contains counts of unspent outputs in each pool, but it may be extended in +/// the future to contain note values or other more detailed information about wallet structure. +/// +/// Values of this type are intended to be used in selection of change output values. A value of +/// this type may represent filtered data, and may therefore not count all of the unspent notes in +/// the wallet. +pub struct WalletMeta { + sapling_note_count: usize, + #[cfg(feature = "orchard")] + orchard_note_count: usize, +} + +impl WalletMeta { + /// Constructs a new [`WalletMeta`] value from its constituent parts. + pub fn new( + sapling_note_count: usize, + #[cfg(feature = "orchard")] orchard_note_count: usize, + ) -> Self { + Self { + sapling_note_count, + #[cfg(feature = "orchard")] + orchard_note_count, + } + } + + /// Returns the number of unspent notes in the wallet for the given shielded protocol. + pub fn note_count(&self, protocol: ShieldedProtocol) -> usize { + match protocol { + ShieldedProtocol::Sapling => self.sapling_note_count, + #[cfg(feature = "orchard")] + ShieldedProtocol::Orchard => self.orchard_note_count, + #[cfg(not(feature = "orchard"))] + ShieldedProtocol::Orchard => 0, + } + } + + /// Returns the number of unspent Sapling notes belonging to the account for which this was + /// generated. + pub fn sapling_note_count(&self) -> usize { + self.sapling_note_count + } + + /// Returns the number of unspent Orchard notes belonging to the account for which this was + /// generated. + #[cfg(feature = "orchard")] + pub fn orchard_note_count(&self) -> usize { + self.orchard_note_count + } + + /// Returns the total number of unspent shielded notes belonging to the account for which this + /// was generated. + pub fn total_note_count(&self) -> usize { + #[cfg(feature = "orchard")] + let orchard_note_count = self.orchard_note_count; + #[cfg(not(feature = "orchard"))] + let orchard_note_count = 0; + + self.sapling_note_count + orchard_note_count + } +} + /// A trait representing the capability to query a data store for unspent transaction outputs /// belonging to a wallet. #[cfg_attr(feature = "test-dependencies", delegatable_trait)] @@ -838,6 +901,17 @@ pub trait InputSource { exclude: &[Self::NoteRef], ) -> Result, Self::Error>; + /// Returns metadata describing the structure of the wallet for the specified account. + /// + /// The returned metadata value must exclude spent notes and unspent notes having value less + /// than the specified minimum value or identified in the given exclude list. + fn get_wallet_metadata( + &self, + account: Self::AccountId, + min_value: NonNegativeAmount, + exclude: &[Self::NoteRef], + ) -> Result; + /// Fetches the transparent output corresponding to the provided `outpoint`. /// /// Returns `Ok(None)` if the UTXO is not known to belong to the wallet or is not diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 9e5a401d01..2d545789d2 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -56,7 +56,6 @@ use crate::{ ShieldedProtocol, }; -use super::WalletTest; #[allow(deprecated)] use super::{ chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary}, @@ -69,7 +68,8 @@ use super::{ Account, AccountBalance, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, TransactionDataRequest, TransactionStatus, - WalletCommitmentTrees, WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + WalletCommitmentTrees, WalletMeta, WalletRead, WalletSummary, WalletTest, WalletWrite, + SAPLING_SHARD_HEIGHT, }; #[cfg(feature = "transparent-inputs")] @@ -2378,6 +2378,15 @@ impl InputSource for MockWalletDb { ) -> Result, Self::Error> { Ok(SpendableNotes::empty()) } + + fn get_wallet_metadata( + &self, + _account: Self::AccountId, + _min_value: NonNegativeAmount, + _exclude: &[Self::NoteRef], + ) -> Result { + Err(()) + } } impl WalletRead for MockWalletDb { diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 0453989dd0..d8ab67b5e1 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -52,8 +52,8 @@ use zcash_client_backend::{ scanning::{ScanPriority, ScanRange}, Account, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, - SentTransaction, SpendableNotes, TransactionDataRequest, WalletCommitmentTrees, WalletRead, - WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + SentTransaction, SpendableNotes, TransactionDataRequest, WalletCommitmentTrees, WalletMeta, + WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }, keys::{ AddressGenerationError, UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey, @@ -128,6 +128,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, + common::count_outputs, SubtreeProgressEstimator, }; @@ -346,6 +347,36 @@ impl, P: consensus::Parameters> InputSource for min_confirmations, ) } + + fn get_wallet_metadata( + &self, + account_id: Self::AccountId, + min_value: NonNegativeAmount, + exclude: &[Self::NoteRef], + ) -> Result { + let sapling_note_count = count_outputs( + self.conn.borrow(), + account_id, + min_value, + exclude, + ShieldedProtocol::Sapling, + )?; + + #[cfg(feature = "orchard")] + let orchard_note_count = count_outputs( + self.conn.borrow(), + account_id, + min_value, + exclude, + ShieldedProtocol::Orchard, + )?; + + Ok(WalletMeta::new( + sapling_note_count, + #[cfg(feature = "orchard")] + orchard_note_count, + )) + } } impl, P: consensus::Parameters> WalletRead for WalletDb { diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 893a6a7dc1..707a759143 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -225,3 +225,54 @@ where .filter_map(|r| r.transpose()) .collect::>() } + +pub(crate) fn count_outputs( + conn: &rusqlite::Connection, + account: AccountId, + min_value: NonNegativeAmount, + exclude: &[ReceivedNoteId], + protocol: ShieldedProtocol, +) -> Result { + let (table_prefix, _, _) = per_protocol_names(protocol); + + let excluded: Vec = exclude + .iter() + .filter_map(|ReceivedNoteId(p, n)| { + if *p == protocol { + Some(Value::from(*n)) + } else { + None + } + }) + .collect(); + let excluded_ptr = Rc::new(excluded); + + conn.query_row( + &format!( + "SELECT COUNT(*) + FROM {table_prefix}_received_notes rn + INNER JOIN accounts ON accounts.id = rn.account_id + INNER JOIN transactions ON transactions.id_tx = rn.tx + WHERE value >= :min_value + AND accounts.id = :account_id + AND accounts.ufvk IS NOT NULL + AND recipient_key_scope IS NOT NULL + AND transactions.mined_height IS NOT NULL + AND rn.id NOT IN rarray(:exclude) + AND rn.id NOT IN ( + SELECT {table_prefix}_received_note_id + FROM {table_prefix}_received_note_spends + JOIN transactions stx ON stx.id_tx = transaction_id + WHERE stx.block IS NOT NULL -- the spending tx is mined + OR stx.expiry_height IS NULL -- the spending tx will not expire + OR stx.expiry_height > :anchor_height -- the spending tx is unexpired + )" + ), + named_params![ + ":account_id": account.0, + ":min_value": u64::from(min_value), + ":exclude": &excluded_ptr + ], + |row| row.get(0), + ) +} From ff00f1ab2bbac2856daaddf6b49be9d5f8b91bb0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 3/9] zcash_client_backend: Make it possible for change strategies to use wallet metadata. In the process this modifies input selection to take the change strategy as an explicit argument, rather than being wrapped as part of the input selector. --- zcash_client_backend/CHANGELOG.md | 47 +++ zcash_client_backend/src/data_api.rs | 2 +- zcash_client_backend/src/data_api/error.rs | 45 ++- zcash_client_backend/src/data_api/testing.rs | 107 ++--- .../src/data_api/testing/pool.rs | 151 ++++--- .../src/data_api/testing/transparent.rs | 21 +- zcash_client_backend/src/data_api/wallet.rs | 376 ++++++++++-------- .../src/data_api/wallet/input_selection.rs | 234 ++++++----- zcash_client_backend/src/fees.rs | 49 ++- zcash_client_backend/src/fees/fixed.rs | 52 ++- zcash_client_backend/src/fees/standard.rs | 43 +- zcash_client_backend/src/fees/zip317.rs | 90 +++-- zcash_client_backend/src/proposal.rs | 2 +- zcash_client_sqlite/src/testing/pool.rs | 2 +- zcash_client_sqlite/src/wallet.rs | 2 + zcash_client_sqlite/src/wallet/scanning.rs | 19 +- 16 files changed, 751 insertions(+), 491 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 49d277096e..a34a061009 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -12,10 +12,55 @@ and this library adheres to Rust's notion of - `Progress` - `WalletSummary::progress` - `WalletMeta` + - `impl Default for wallet::input_selection::GreedyInputSelector` ### Changed - `zcash_client_backend::data_api`: - `InputSource` has an added method `get_wallet_metadata` + - `error::Error` has additional variant `Error::Change`. This necessitates + the addition of two type parameters to the `Error` type, + `ChangeErrT` and `NoteRefT`. + - The following methods each now take an additional `change_strategy` + argument, along with an associated `ChangeT` type parameter: + - `zcash_client_backend::data_api::wallet::spend` + - `zcash_client_backend::data_api::wallet::propose_transfer` + - `zcash_client_backend::data_api::wallet::propose_shielding`. This method + also now takes an additional `to_account` argument. + - `zcash_client_backend::data_api::wallet::shield_transparent_funds`. This + method also now takes an additional `to_account` argument. + - `wallet::input_selection::InputSelectionError` now has an additional `Change` + variant. This necessitates the addition of two type parameters. + - `wallet::input_selection::InputSelector::propose_transaction` takes an + additional `change_strategy` argument, along with an associated `ChangeT` + type parameter. + - The `wallet::input_selection::InputSelector::FeeRule` associated type has + been removed. The fee rule is now part of the change strategy passed to + `propose_transaction`. + - `wallet::input_selection::ShieldingSelector::propose_shielding` takes an + additional `change_strategy` argument, along with an associated `ChangeT` + type parameter. In addition, it also takes a new `to_account` argument + that identifies the destination account for the shielded notes. + - The `wallet::input_selection::ShieldingSelector::FeeRule` associated type + has been removed. The fee rule is now part of the change strategy passed to + `propose_shielding`. + - The `Change` variant of `wallet::input_selection::GreedyInputSelectorError` + has been removed, along with the additional type parameters it necessitated. + - The arguments to `wallet::input_selection::GreedyInputSelector::new` have + changed. +- `zcash_client_backend::fees::ChangeStrategy` has changed. It has two new + associated types, `MetaSource` and `WalletMeta`, and its `FeeRule` associated + type now has an additional `Clone` bound. In addition, it defines a new + `fetch_wallet_meta` method, and the arguments to `compute_balance` have + changed. +- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new` + now takes an additional `DustOutputPolicy` argument. It also now carries + an additional type parameter. +- `zcash_client_backend::fees::standard::SingleOutputChangeStrategy::new` + now takes an additional `DustOutputPolicy` argument. It also now carries + an additional type parameter. +- `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new` + now takes an additional `DustOutputPolicy` argument. It also now carries + an additional type parameter. ### Changed - Migrated to `arti-client 0.22`. @@ -24,6 +69,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have been removed. Use `WalletSummary::progress` instead. +- `zcash_client_backend::fees`: + - `impl From for ChangeError<...>` ## [0.14.0] - 2024-10-04 diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index bc2dad95c2..03083a5cce 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -794,7 +794,7 @@ impl SpendableNotes { } } -/// Metadata about the structure of the wallet for a particular account. +/// Metadata about the structure of the wallet for a particular account. /// /// At present this just contains counts of unspent outputs in each pool, but it may be extended in /// the future to contain note values or other more detailed information about wallet structure. diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index fd1d35aaae..e32ae56fc1 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -13,6 +13,7 @@ use zcash_primitives::transaction::{ use crate::address::UnifiedAddress; use crate::data_api::wallet::input_selection::InputSelectorError; +use crate::fees::ChangeError; use crate::proposal::ProposalError; use crate::PoolType; @@ -23,7 +24,8 @@ use crate::wallet::NoteId; /// Errors that can occur as a consequence of wallet operations. #[derive(Debug)] -pub enum Error { +pub enum Error +{ /// An error occurred retrieving data from the underlying data source DataSource(DataSourceError), @@ -33,6 +35,9 @@ pub enum Error { /// An error in note selection NoteSelection(SelectionError), + /// An error in change selection during transaction proposal construction + Change(ChangeError), + /// An error in transaction proposal construction Proposal(ProposalError), @@ -98,12 +103,14 @@ pub enum Error { PaysEphemeralTransparentAddress(String), } -impl fmt::Display for Error +impl fmt::Display for Error where DE: fmt::Display, - CE: fmt::Display, + TE: fmt::Display, SE: fmt::Display, FE: fmt::Display, + CE: fmt::Display, + N: fmt::Display, { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { @@ -120,6 +127,9 @@ where Error::NoteSelection(e) => { write!(f, "Note selection encountered the following error: {}", e) } + Error::Change(e) => { + write!(f, "Change output generation failed: {}", e) + } Error::Proposal(e) => { write!(f, "Input selection attempted to construct an invalid proposal: {}", e) } @@ -173,12 +183,14 @@ where } } -impl error::Error for Error +impl error::Error for Error where DE: Debug + Display + error::Error + 'static, - CE: Debug + Display + error::Error + 'static, + TE: Debug + Display + error::Error + 'static, SE: Debug + Display + error::Error + 'static, FE: Debug + Display + 'static, + CE: Debug + Display + error::Error + 'static, + N: Debug + Display + 'static, { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match &self { @@ -192,35 +204,38 @@ where } } -impl From> for Error { +impl From> for Error { fn from(e: builder::Error) -> Self { Error::Builder(e) } } -impl From for Error { +impl From for Error { fn from(e: ProposalError) -> Self { Error::Proposal(e) } } -impl From for Error { +impl From for Error { fn from(e: BalanceError) -> Self { Error::BalanceError(e) } } -impl From> for Error { +impl From> for Error { fn from(value: ConversionError<&'static str>) -> Self { Error::Address(value) } } -impl From> for Error { - fn from(e: InputSelectorError) -> Self { +impl From> + for Error +{ + fn from(e: InputSelectorError) -> Self { match e { InputSelectorError::DataSource(e) => Error::DataSource(e), InputSelectorError::Selection(e) => Error::NoteSelection(e), + InputSelectorError::Change(e) => Error::Change(e), InputSelectorError::Proposal(e) => Error::Proposal(e), InputSelectorError::InsufficientFunds { available, @@ -235,20 +250,20 @@ impl From> for Error } } -impl From for Error { +impl From for Error { fn from(e: sapling::builder::Error) -> Self { Error::Builder(builder::Error::SaplingBuild(e)) } } -impl From for Error { +impl From for Error { fn from(e: transparent::builder::Error) -> Self { Error::Builder(builder::Error::TransparentBuild(e)) } } -impl From> for Error { - fn from(e: ShardTreeError) -> Self { +impl From> for Error { + fn from(e: ShardTreeError) -> Self { Error::CommitmentTree(e) } } diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 2d545789d2..2e095eb5e9 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -31,7 +31,7 @@ use zcash_primitives::{ memo::Memo, transaction::{ components::{amount::NonNegativeAmount, sapling::zip212_enforcement}, - fees::{zip317::FeeError as Zip317FeeError, FeeRule, StandardFeeRule}, + fees::{FeeRule, StandardFeeRule}, Transaction, TxId, }, }; @@ -46,7 +46,10 @@ use zip32::{fingerprint::SeedFingerprint, DiversifierIndex}; use crate::{ address::UnifiedAddress, - fees::{standard, DustOutputPolicy}, + fees::{ + standard::{self, SingleOutputChangeStrategy}, + ChangeStrategy, DustOutputPolicy, + }, keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey}, proposal::Proposal, proto::compact_formats::{ @@ -62,7 +65,7 @@ use super::{ scanning::ScanRange, wallet::{ create_proposed_transactions, create_spend_to_address, - input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector}, + input_selection::{GreedyInputSelector, InputSelector}, propose_standard_transfer_to_address, propose_transfer, spend, }, Account, AccountBalance, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, @@ -874,12 +877,7 @@ where fallback_change_pool: ShieldedProtocol, ) -> Result< NonEmpty, - super::error::Error< - ErrT, - ::Error, - GreedyInputSelectorError::NoteRef>, - Zip317FeeError, - >, + super::wallet::TransferErrT, SingleOutputChangeStrategy>, > { let prover = LocalTxProver::bundled(); let network = self.network().clone(); @@ -901,24 +899,18 @@ where /// Invokes [`spend`] with the given arguments. #[allow(clippy::type_complexity)] - pub fn spend( + pub fn spend( &mut self, input_selector: &InputsT, + change_strategy: &ChangeT, usk: &UnifiedSpendingKey, request: zip321::TransactionRequest, ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, - ) -> Result< - NonEmpty, - super::error::Error< - ErrT, - ::Error, - InputsT::Error, - ::Error, - >, - > + ) -> Result, super::wallet::TransferErrT> where InputsT: InputSelector, + ChangeT: ChangeStrategy, { #![allow(deprecated)] let prover = LocalTxProver::bundled(); @@ -929,6 +921,7 @@ where &prover, &prover, input_selector, + change_strategy, usk, request, ovk_policy, @@ -938,25 +931,28 @@ where /// Invokes [`propose_transfer`] with the given arguments. #[allow(clippy::type_complexity)] - pub fn propose_transfer( + pub fn propose_transfer( &mut self, spend_from_account: ::AccountId, input_selector: &InputsT, + change_strategy: &ChangeT, request: zip321::TransactionRequest, min_confirmations: NonZeroU32, ) -> Result< - Proposal::NoteRef>, - super::error::Error::Error>, + Proposal::NoteRef>, + super::wallet::ProposeTransferErrT, > where InputsT: InputSelector, + ChangeT: ChangeStrategy, { let network = self.network().clone(); - propose_transfer::<_, _, _, Infallible>( + propose_transfer::<_, _, _, _, Infallible>( self.wallet_mut(), &network, spend_from_account, input_selector, + change_strategy, request, min_confirmations, ) @@ -977,11 +973,11 @@ where fallback_change_pool: ShieldedProtocol, ) -> Result< Proposal::NoteRef>, - super::error::Error< - ErrT, + super::wallet::ProposeTransferErrT< + DbT, CommitmentTreeErrT, - GreedyInputSelectorError::NoteRef>, - Zip317FeeError, + GreedyInputSelector, + SingleOutputChangeStrategy, >, > { let network = self.network().clone(); @@ -1011,47 +1007,47 @@ where #[cfg(feature = "transparent-inputs")] #[allow(clippy::type_complexity)] #[allow(dead_code)] - pub fn propose_shielding( + pub fn propose_shielding( &mut self, input_selector: &InputsT, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, from_addrs: &[TransparentAddress], + to_account: ::AccountId, min_confirmations: u32, ) -> Result< - Proposal, - super::error::Error::Error>, + Proposal, + super::wallet::ProposeShieldingErrT, > where InputsT: ShieldingSelector, + ChangeT: ChangeStrategy, { use super::wallet::propose_shielding; let network = self.network().clone(); - propose_shielding::<_, _, _, Infallible>( + propose_shielding::<_, _, _, _, Infallible>( self.wallet_mut(), &network, input_selector, + change_strategy, shielding_threshold, from_addrs, + to_account, min_confirmations, ) } /// Invokes [`create_proposed_transactions`] with the given arguments. #[allow(clippy::type_complexity)] - pub fn create_proposed_transactions( + pub fn create_proposed_transactions( &mut self, usk: &UnifiedSpendingKey, ovk_policy: OvkPolicy, proposal: &Proposal::NoteRef>, ) -> Result< NonEmpty, - super::error::Error< - ErrT, - ::Error, - InputsErrT, - FeeRuleT::Error, - >, + super::wallet::CreateErrT, > where FeeRuleT: FeeRule, @@ -1074,24 +1070,20 @@ where /// [`shield_transparent_funds`]: crate::data_api::wallet::shield_transparent_funds #[cfg(feature = "transparent-inputs")] #[allow(clippy::type_complexity)] - pub fn shield_transparent_funds( + #[allow(clippy::too_many_arguments)] + pub fn shield_transparent_funds( &mut self, input_selector: &InputsT, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, usk: &UnifiedSpendingKey, from_addrs: &[TransparentAddress], + to_account: ::AccountId, min_confirmations: u32, - ) -> Result< - NonEmpty, - super::error::Error< - ErrT, - ::Error, - InputsT::Error, - ::Error, - >, - > + ) -> Result, super::wallet::ShieldErrT> where InputsT: ShieldingSelector, + ChangeT: ChangeStrategy, { use crate::data_api::wallet::shield_transparent_funds; @@ -1103,9 +1095,11 @@ where &prover, &prover, input_selector, + change_strategy, shielding_threshold, usk, from_addrs, + to_account, min_confirmations, ) } @@ -1229,15 +1223,22 @@ impl TestState { /// Helper method for constructing a [`GreedyInputSelector`] with a /// [`standard::SingleOutputChangeStrategy`]. -pub fn input_selector( +pub fn input_selector() -> GreedyInputSelector { + GreedyInputSelector::::new() +} + +pub fn single_output_change_strategy( fee_rule: StandardFeeRule, change_memo: Option<&str>, fallback_change_pool: ShieldedProtocol, -) -> GreedyInputSelector { +) -> standard::SingleOutputChangeStrategy { let change_memo = change_memo.map(|m| MemoBytes::from(m.parse::().unwrap())); - let change_strategy = - standard::SingleOutputChangeStrategy::new(fee_rule, change_memo, fallback_change_pool); - GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()) + standard::SingleOutputChangeStrategy::new( + fee_rule, + change_memo, + fallback_change_pool, + DustOutputPolicy::default(), + ) } // Checks that a protobuf proposal serialized from the provided proposal value correctly parses to diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 22a0a9a437..dfba19e6fd 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -17,9 +17,7 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ components::amount::NonNegativeAmount, - fees::{ - fixed::FeeRule as FixedFeeRule, zip317::FeeError as Zip317FeeError, StandardFeeRule, - }, + fees::{fixed::FeeRule as FixedFeeRule, StandardFeeRule}, Transaction, }, }; @@ -38,16 +36,22 @@ use crate::{ self, chain::{self, ChainState, CommitmentTreeRoot, ScanSummary}, error::Error, - testing::{input_selector, AddressType, FakeCompactOutput, InitialChainState, TestBuilder}, + testing::{ + single_output_change_strategy, AddressType, FakeCompactOutput, InitialChainState, + TestBuilder, + }, wallet::{ - decrypt_and_store_transaction, - input_selection::{GreedyInputSelector, GreedyInputSelectorError}, + decrypt_and_store_transaction, input_selection::GreedyInputSelector, TransferErrT, }, Account as _, AccountBirthday, DecryptedTransaction, InputSource, Ratio, WalletCommitmentTrees, WalletRead, WalletSummary, WalletTest, WalletWrite, }, decrypt_transaction, - fees::{fixed, standard, DustOutputPolicy}, + fees::{ + fixed, + standard::{self, SingleOutputChangeStrategy}, + DustOutputPolicy, + }, scanning::ScanError, wallet::{Note, NoteId, OvkPolicy, ReceivedNote}, }; @@ -216,19 +220,21 @@ pub fn send_single_step_proposed_transfer( fee_rule, Some(change_memo.clone().into()), T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), ); - let input_selector = &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); + let input_selector = GreedyInputSelector::new(); let proposal = st .propose_transfer( account.id(), - input_selector, + &input_selector, + &change_strategy, request, NonZeroU32::new(1).unwrap(), ) .unwrap(); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal, @@ -399,7 +405,7 @@ pub fn send_multi_step_proposed_transfer( ); assert_eq!(steps[1].balance().proposed_change(), []); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal, @@ -499,7 +505,7 @@ pub fn send_multi_step_proposed_transfer( ) .unwrap(); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal, @@ -758,7 +764,7 @@ pub fn proposal_fails_if_not_all_ephemeral_outputs_consumed( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal, @@ -774,7 +780,7 @@ pub fn proposal_fails_if_not_all_ephemeral_outputs_consumed( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &frobbed_proposal, @@ -998,7 +1004,11 @@ pub fn spend_fails_on_unverified_notes( // Executing the proposal should succeed let txid = st - .create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal) + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) .unwrap()[0]; let (h, _) = st.generate_next_block_including(txid); @@ -1057,7 +1067,7 @@ pub fn spend_fails_on_locked_notes( // Executing the proposal should succeed assert_matches!( - st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal,), + st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal,), Ok(txids) if txids.len() == 1 ); @@ -1139,7 +1149,11 @@ pub fn spend_fails_on_locked_notes( .unwrap(); let txid2 = st - .create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal) + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) .unwrap()[0]; let (h, _) = st.generate_next_block_including(txid2); @@ -1187,7 +1201,11 @@ pub fn ovk_policy_prevents_recovery_from_chain( ovk_policy| -> Result< Option<(Note, Address, MemoBytes)>, - Error<_, _, GreedyInputSelectorError, Zip317FeeError>, + TransferErrT< + DSF::DataStore, + GreedyInputSelector, + SingleOutputChangeStrategy, + >, > { let min_confirmations = NonZeroU32::new(1).unwrap(); let proposal = st.propose_standard_transfer( @@ -1283,7 +1301,7 @@ pub fn spend_succeeds_to_t_addr_zero_change( // Executing the proposal should succeed assert_matches!( - st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal), + st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal), Ok(txids) if txids.len() == 1 ); } @@ -1346,7 +1364,7 @@ pub fn change_note_spends_succeed( // Executing the proposal should succeed assert_matches!( - st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal), + st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal), Ok(txids) if txids.len() == 1 ); } @@ -1396,14 +1414,18 @@ pub fn external_address_change_spends_detected_in_restore_from_seed( - ds_factory: impl DataStoreFactory, +pub fn zip317_spend( + ds_factory: DSF, cache: impl TestCache, ) { let mut st = TestBuilder::new() @@ -1484,7 +1506,9 @@ pub fn zip317_spend( assert_eq!(st.get_total_balance(account_id), total); assert_eq!(st.get_spendable_balance(account_id, 1), total); - let input_selector = input_selector(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::::new(); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); // This first request will fail due to insufficient non-dust funds let req = TransactionRequest::new(vec![Payment::without_memo( @@ -1496,6 +1520,7 @@ pub fn zip317_spend( assert_matches!( st.spend( &input_selector, + &change_strategy, account.usk(), req, OvkPolicy::Sender, @@ -1517,6 +1542,7 @@ pub fn zip317_spend( let txid = st .spend( &input_selector, + &change_strategy, account.usk(), req, OvkPolicy::Sender, @@ -1579,19 +1605,18 @@ where let res0 = st.wallet_mut().put_received_transparent_utxo(&utxo); assert_matches!(res0, Ok(_)); - let fee_rule = StandardFeeRule::Zip317; - - let input_selector = GreedyInputSelector::new( - standard::SingleOutputChangeStrategy::new(fee_rule, None, T::SHIELDED_PROTOCOL), - DustOutputPolicy::default(), - ); + let input_selector = GreedyInputSelector::new(); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); let txids = st .shield_transparent_funds( &input_selector, + &change_strategy, NonNegativeAmount::from_u64(10000).unwrap(), account.usk(), &[*taddr], + account.id(), 1, ) .unwrap(); @@ -1827,15 +1852,14 @@ pub fn pool_crossing_required( )]) .unwrap(); - let fee_rule = StandardFeeRule::Zip317; - let input_selector = GreedyInputSelector::new( - standard::SingleOutputChangeStrategy::new(fee_rule, None, P1::SHIELDED_PROTOCOL), - DustOutputPolicy::default(), - ); + let input_selector = GreedyInputSelector::new(); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); let proposal0 = st .propose_transfer( account.id(), &input_selector, + &change_strategy, p0_to_p1, NonZeroU32::new(1).unwrap(), ) @@ -1863,7 +1887,7 @@ pub fn pool_crossing_required( ); assert_eq!(change_output.value(), expected_change); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal0, @@ -1918,17 +1942,16 @@ pub fn fully_funded_fully_private( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal0, @@ -2009,17 +2032,16 @@ pub fn fully_funded_send_to_t( )]) .unwrap(); - let fee_rule = StandardFeeRule::Zip317; - let input_selector = GreedyInputSelector::new( - // We set the default change output pool to P0, because we want to verify later that - // change is actually sent to P1 (as the transaction is fully fundable from P1). - standard::SingleOutputChangeStrategy::new(fee_rule, None, P0::SHIELDED_PROTOCOL), - DustOutputPolicy::default(), - ); + let input_selector = GreedyInputSelector::new(); + // We set the default change output pool to P0, because we want to verify later that + // change is actually sent to P1 (as the transaction is fully fundable from P1). + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL); let proposal0 = st .propose_transfer( account.id(), &input_selector, + &change_strategy, p0_to_p1, NonZeroU32::new(1).unwrap(), ) @@ -2043,7 +2065,7 @@ pub fn fully_funded_send_to_t( assert_eq!(change_output.output_pool(), PoolType::SAPLING); assert_eq!(change_output.value(), expected_change); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal0, @@ -2110,11 +2132,9 @@ pub fn multi_pool_checkpoint( assert_eq!(st.get_spendable_balance(acct_id, 1), initial_balance); // Set up the fee rule and input selector we'll use for all the transfers. - let fee_rule = StandardFeeRule::Zip317; - let input_selector = GreedyInputSelector::new( - standard::SingleOutputChangeStrategy::new(fee_rule, None, P1::SHIELDED_PROTOCOL), - DustOutputPolicy::default(), - ); + let input_selector = GreedyInputSelector::new(); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); // First, send funds just to P0 let transfer_amount = NonNegativeAmount::const_from_u64(200000); @@ -2126,6 +2146,7 @@ pub fn multi_pool_checkpoint( let res = st .spend( &input_selector, + &change_strategy, account.usk(), p0_transfer, OvkPolicy::Sender, @@ -2157,6 +2178,7 @@ pub fn multi_pool_checkpoint( let res = st .spend( &input_selector, + &change_strategy, account.usk(), both_transfer, OvkPolicy::Sender, @@ -2597,17 +2619,14 @@ pub fn scan_cached_blocks_allows_blocks_out_of_order( .unwrap(); #[allow(deprecated)] - let input_selector = GreedyInputSelector::new( - standard::SingleOutputChangeStrategy::new( - StandardFeeRule::Zip317, - None, - T::SHIELDED_PROTOCOL, - ), - DustOutputPolicy::default(), - ); + let input_selector = GreedyInputSelector::new(); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + assert_matches!( st.spend( &input_selector, + &change_strategy, account.usk(), req, OvkPolicy::Sender, diff --git a/zcash_client_backend/src/data_api/testing/transparent.rs b/zcash_client_backend/src/data_api/testing/transparent.rs index 188d3061b5..9cfd6838a9 100644 --- a/zcash_client_backend/src/data_api/testing/transparent.rs +++ b/zcash_client_backend/src/data_api/testing/transparent.rs @@ -197,16 +197,23 @@ where check_balance(&st, 0, value); // Shield the output. - let input_selector = GreedyInputSelector::new( - fixed::SingleOutputChangeStrategy::new( - FixedFeeRule::non_standard(NonNegativeAmount::ZERO), - None, - ShieldedProtocol::Sapling, - ), + let input_selector = GreedyInputSelector::new(); + let change_strategy = fixed::SingleOutputChangeStrategy::new( + FixedFeeRule::non_standard(NonNegativeAmount::ZERO), + None, + ShieldedProtocol::Sapling, DustOutputPolicy::default(), ); let txid = st - .shield_transparent_funds(&input_selector, value, account.usk(), &[*taddr], 1) + .shield_transparent_funds( + &input_selector, + &change_strategy, + value, + account.usk(), + &[*taddr], + account.id(), + 1, + ) .unwrap()[0]; // The wallet should have zero transparent balance, because the shielding diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 7841b598fc..b0276f97e7 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -50,7 +50,7 @@ use crate::{ WalletRead, WalletWrite, }, decrypt_transaction, - fees::{self, DustOutputPolicy}, + fees::{standard::SingleOutputChangeStrategy, ChangeStrategy, DustOutputPolicy}, keys::UnifiedSpendingKey, proposal::{Proposal, ProposalError, Step, StepOutputIndex}, wallet::{Note, OvkPolicy, Recipient}, @@ -62,7 +62,7 @@ use zcash_primitives::{ transaction::{ builder::{BuildConfig, BuildResult, Builder}, components::{amount::NonNegativeAmount, sapling::zip212_enforcement, OutPoint}, - fees::{zip317::FeeError as Zip317FeeError, FeeRule, StandardFeeRule}, + fees::{FeeRule, StandardFeeRule}, Transaction, TxId, }, }; @@ -83,9 +83,7 @@ use { }; pub mod input_selection; -use input_selection::{ - GreedyInputSelector, GreedyInputSelectorError, InputSelector, InputSelectorError, -}; +use input_selection::{GreedyInputSelector, InputSelector, InputSelectorError}; /// Scans a [`Transaction`] for any information that can be decrypted by the accounts in /// the wallet, and saves it to the wallet. @@ -117,6 +115,53 @@ where Ok(()) } +pub type ProposeTransferErrT = Error< + ::Error, + CommitmentTreeErrT, + ::Error, + <::FeeRule as FeeRule>::Error, + ::Error, + <::InputSource as InputSource>::NoteRef, +>; + +#[cfg(feature = "transparent-inputs")] +pub type ProposeShieldingErrT = Error< + ::Error, + CommitmentTreeErrT, + ::Error, + <::FeeRule as FeeRule>::Error, + ::Error, + Infallible, +>; + +pub type CreateErrT = Error< + ::Error, + ::Error, + InputsErrT, + ::Error, + ChangeErrT, + N, +>; + +pub type TransferErrT = Error< + ::Error, + ::Error, + ::Error, + <::FeeRule as FeeRule>::Error, + ::Error, + <::InputSource as InputSource>::NoteRef, +>; + +#[cfg(feature = "transparent-inputs")] +pub type ShieldErrT = Error< + ::Error, + ::Error, + ::Error, + <::FeeRule as FeeRule>::Error, + ::Error, + Infallible, +>; + #[allow(clippy::needless_doctest_main)] /// Creates a transaction or series of transactions paying the specified address from /// the given account, and the [`TxId`] corresponding to each newly-created transaction. @@ -251,12 +296,7 @@ pub fn create_spend_to_address( fallback_change_pool: ShieldedProtocol, ) -> Result< NonEmpty, - Error< - ::Error, - ::Error, - GreedyInputSelectorError, - Zip317FeeError, - >, + TransferErrT, SingleOutputChangeStrategy>, > where ParamsT: consensus::Parameters + Clone, @@ -297,13 +337,6 @@ where ) } -type ErrorT = Error< - ::Error, - ::Error, - InputsErrT, - ::Error, ->; - /// Constructs a transaction or series of transactions that send funds as specified /// by the `request` argument, stores them to the wallet's "sent transactions" data /// store, and returns the [`TxId`] for each transaction constructed. @@ -358,17 +391,18 @@ type ErrorT = Error< #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] #[deprecated(note = "Use `propose_transfer` and `create_proposed_transactions` instead.")] -pub fn spend( +pub fn spend( wallet_db: &mut DbT, params: &ParamsT, spend_prover: &impl SpendProver, output_prover: &impl OutputProver, input_selector: &InputsT, + change_strategy: &ChangeT, usk: &UnifiedSpendingKey, request: zip321::TransactionRequest, ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, -) -> Result, ErrorT> +) -> Result, TransferErrT> where DbT: InputSource, DbT: WalletWrite< @@ -378,6 +412,7 @@ where DbT: WalletCommitmentTrees, ParamsT: consensus::Parameters + Clone, InputsT: InputSelector, + ChangeT: ChangeStrategy, { let account = wallet_db .get_account_for_ufvk(&usk.to_unified_full_viewing_key()) @@ -389,6 +424,7 @@ where params, account.id(), input_selector, + change_strategy, request, min_confirmations, )?; @@ -409,27 +445,24 @@ where /// [`create_proposed_transactions`]. #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -pub fn propose_transfer( +pub fn propose_transfer( wallet_db: &mut DbT, params: &ParamsT, spend_from_account: ::AccountId, input_selector: &InputsT, + change_strategy: &ChangeT, request: zip321::TransactionRequest, min_confirmations: NonZeroU32, ) -> Result< - Proposal::NoteRef>, - Error< - ::Error, - CommitmentTreeErrT, - InputsT::Error, - ::Error, - >, + Proposal::NoteRef>, + ProposeTransferErrT, > where DbT: WalletRead + InputSource::Error>, ::NoteRef: Copy + Eq + Ord, ParamsT: consensus::Parameters + Clone, InputsT: InputSelector, + ChangeT: ChangeStrategy, { let (target_height, anchor_height) = wallet_db .get_target_and_anchor_heights(min_confirmations) @@ -444,6 +477,7 @@ where anchor_height, spend_from_account, request, + change_strategy, ) .map_err(Error::from) } @@ -488,11 +522,11 @@ pub fn propose_standard_transfer_to_address( fallback_change_pool: ShieldedProtocol, ) -> Result< Proposal, - Error< - ::Error, + ProposeTransferErrT< + DbT, CommitmentTreeErrT, - GreedyInputSelectorError, - Zip317FeeError, + GreedyInputSelector, + SingleOutputChangeStrategy, >, > where @@ -517,19 +551,20 @@ where "It should not be possible for this to violate ZIP 321 request construction invariants.", ); - let change_strategy = fees::standard::SingleOutputChangeStrategy::new( + let input_selector = GreedyInputSelector::::new(); + let change_strategy = SingleOutputChangeStrategy::::new( fee_rule, change_memo, fallback_change_pool, + DustOutputPolicy::default(), ); - let input_selector = - GreedyInputSelector::::new(change_strategy, DustOutputPolicy::default()); propose_transfer( wallet_db, params, spend_from_account, &input_selector, + &change_strategy, request, min_confirmations, ) @@ -540,26 +575,24 @@ where #[cfg(feature = "transparent-inputs")] #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -pub fn propose_shielding( +pub fn propose_shielding( wallet_db: &mut DbT, params: &ParamsT, input_selector: &InputsT, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, from_addrs: &[TransparentAddress], + to_account: ::AccountId, min_confirmations: u32, ) -> Result< - Proposal, - Error< - ::Error, - CommitmentTreeErrT, - InputsT::Error, - ::Error, - >, + Proposal, + ProposeShieldingErrT, > where ParamsT: consensus::Parameters, DbT: WalletRead + InputSource::Error>, InputsT: ShieldingSelector, + ChangeT: ChangeStrategy, { let chain_tip_height = wallet_db .chain_height() @@ -570,8 +603,10 @@ where .propose_shielding( params, wallet_db, + change_strategy, shielding_threshold, from_addrs, + to_account, chain_tip_height + 1, min_confirmations, ) @@ -599,7 +634,7 @@ struct StepResult { /// and therefore the required spend proofs for such notes cannot be constructed. #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -pub fn create_proposed_transactions( +pub fn create_proposed_transactions( wallet_db: &mut DbT, params: &ParamsT, spend_prover: &impl SpendProver, @@ -607,7 +642,7 @@ pub fn create_proposed_transactions( usk: &UnifiedSpendingKey, ovk_policy: OvkPolicy, proposal: &Proposal, -) -> Result, ErrorT> +) -> Result, CreateErrT> where DbT: WalletWrite + WalletCommitmentTrees, ParamsT: consensus::Parameters + Clone, @@ -691,7 +726,7 @@ where // `TransparentAddress` and `Outpoint`. #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -fn create_proposed_transaction( +fn create_proposed_transaction( wallet_db: &mut DbT, params: &ParamsT, spend_prover: &impl SpendProver, @@ -707,7 +742,10 @@ fn create_proposed_transaction( StepOutput, (TransparentAddress, OutPoint), >, -) -> Result::AccountId>, ErrorT> +) -> Result< + StepResult<::AccountId>, + CreateErrT, +> where DbT: WalletWrite + WalletCommitmentTrees, ParamsT: consensus::Parameters + Clone, @@ -749,53 +787,54 @@ where return Err(Error::ProposalNotSupported); } - let (sapling_anchor, sapling_inputs) = - if proposal_step.involves(PoolType::Shielded(ShieldedProtocol::Sapling)) { - proposal_step.shielded_inputs().map_or_else( - || Ok((Some(sapling::Anchor::empty_tree()), vec![])), - |inputs| { - wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| { - let anchor = sapling_tree - .root_at_checkpoint_id(&inputs.anchor_height())? - .ok_or(ProposalError::AnchorNotFound(inputs.anchor_height()))? - .into(); - - let sapling_inputs = inputs - .notes() - .iter() - .filter_map(|selected| match selected.note() { - Note::Sapling(note) => { - let key = match selected.spending_key_scope() { - Scope::External => usk.sapling().clone(), - Scope::Internal => usk.sapling().derive_internal(), - }; - - sapling_tree - .witness_at_checkpoint_id_caching( - selected.note_commitment_tree_position(), - &inputs.anchor_height(), - ) - .and_then(|witness| { - witness.ok_or(ShardTreeError::Query( - QueryError::CheckpointPruned, - )) - }) - .map(|merkle_path| Some((key, note, merkle_path))) - .map_err(Error::from) - .transpose() - } - #[cfg(feature = "orchard")] - Note::Orchard(_) => None, - }) - .collect::, Error<_, _, _, _>>>()?; + let (sapling_anchor, sapling_inputs) = if proposal_step + .involves(PoolType::Shielded(ShieldedProtocol::Sapling)) + { + proposal_step.shielded_inputs().map_or_else( + || Ok((Some(sapling::Anchor::empty_tree()), vec![])), + |inputs| { + wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _, _, _>>(|sapling_tree| { + let anchor = sapling_tree + .root_at_checkpoint_id(&inputs.anchor_height())? + .ok_or(ProposalError::AnchorNotFound(inputs.anchor_height()))? + .into(); - Ok((Some(anchor), sapling_inputs)) - }) - }, - )? - } else { - (None, vec![]) - }; + let sapling_inputs = inputs + .notes() + .iter() + .filter_map(|selected| match selected.note() { + Note::Sapling(note) => { + let key = match selected.spending_key_scope() { + Scope::External => usk.sapling().clone(), + Scope::Internal => usk.sapling().derive_internal(), + }; + + sapling_tree + .witness_at_checkpoint_id_caching( + selected.note_commitment_tree_position(), + &inputs.anchor_height(), + ) + .and_then(|witness| { + witness.ok_or(ShardTreeError::Query( + QueryError::CheckpointPruned, + )) + }) + .map(|merkle_path| Some((key, note, merkle_path))) + .map_err(Error::from) + .transpose() + } + #[cfg(feature = "orchard")] + Note::Orchard(_) => None, + }) + .collect::, Error<_, _, _, _, _, _>>>()?; + + Ok((Some(anchor), sapling_inputs)) + }) + }, + )? + } else { + (None, vec![]) + }; #[cfg(feature = "orchard")] let (orchard_anchor, orchard_inputs) = if proposal_step @@ -804,7 +843,7 @@ where proposal_step.shielded_inputs().map_or_else( || Ok((Some(orchard::Anchor::empty_tree()), vec![])), |inputs| { - wallet_db.with_orchard_tree_mut::<_, _, Error<_, _, _, _>>(|orchard_tree| { + wallet_db.with_orchard_tree_mut::<_, _, Error<_, _, _, _, _, _>>(|orchard_tree| { let anchor = orchard_tree .root_at_checkpoint_id(&inputs.anchor_height())? .ok_or(ProposalError::AnchorNotFound(inputs.anchor_height()))? @@ -829,7 +868,7 @@ where .transpose(), Note::Sapling(_) => None, }) - .collect::, Error<_, _, _, _>>>()?; + .collect::, Error<_, _, _, _, _, _>>>()?; Ok((Some(anchor), orchard_inputs)) }) @@ -872,7 +911,7 @@ where #[cfg(feature = "transparent-inputs")] let mut metadata_from_address = |addr: TransparentAddress| -> Result< TransparentAddressMetadata, - ErrorT, + CreateErrT, > { match cache.get(&addr) { Some(result) => Ok(result.clone()), @@ -898,22 +937,23 @@ where #[cfg(feature = "transparent-inputs")] let utxos_spent = { let mut utxos_spent: Vec = vec![]; - let add_transparent_input = |builder: &mut Builder<_, _>, - utxos_spent: &mut Vec<_>, - address_metadata: &TransparentAddressMetadata, - outpoint: OutPoint, - txout: TxOut| - -> Result<(), ErrorT> { - let secret_key = usk - .transparent() - .derive_secret_key(address_metadata.scope(), address_metadata.address_index()) - .expect("spending key derivation should not fail"); - - utxos_spent.push(outpoint.clone()); - builder.add_transparent_input(secret_key, outpoint, txout)?; - - Ok(()) - }; + let add_transparent_input = + |builder: &mut Builder<_, _>, + utxos_spent: &mut Vec<_>, + address_metadata: &TransparentAddressMetadata, + outpoint: OutPoint, + txout: TxOut| + -> Result<(), CreateErrT> { + let secret_key = usk + .transparent() + .derive_secret_key(address_metadata.scope(), address_metadata.address_index()) + .expect("spending key derivation should not fail"); + + utxos_spent.push(outpoint.clone()); + builder.add_transparent_input(secret_key, outpoint, txout)?; + + Ok(()) + }; for utxo in proposal_step.transparent_inputs() { add_transparent_input( @@ -1031,7 +1071,10 @@ where let add_sapling_output = |builder: &mut Builder<_, _>, sapling_output_meta: &mut Vec<_>, to: sapling::PaymentAddress| - -> Result<(), ErrorT> { + -> Result< + (), + CreateErrT, + > { let memo = payment.memo().map_or_else(MemoBytes::empty, |m| m.clone()); builder.add_sapling_output(sapling_external_ovk, to, payment.amount(), memo.clone())?; sapling_output_meta.push(( @@ -1043,50 +1086,52 @@ where }; #[cfg(feature = "orchard")] - let add_orchard_output = |builder: &mut Builder<_, _>, - orchard_output_meta: &mut Vec<_>, - to: orchard::Address| - -> Result<(), ErrorT> { - let memo = payment.memo().map_or_else(MemoBytes::empty, |m| m.clone()); - builder.add_orchard_output( - orchard_external_ovk.clone(), - to, - payment.amount().into(), - memo.clone(), - )?; - orchard_output_meta.push(( - Recipient::External(recipient_address.clone(), PoolType::ORCHARD), - payment.amount(), - Some(memo), - )); - Ok(()) - }; - - let add_transparent_output = |builder: &mut Builder<_, _>, - transparent_output_meta: &mut Vec<_>, - to: TransparentAddress| - -> Result<(), ErrorT> { - // Always reject sending to one of our known ephemeral addresses. - #[cfg(feature = "transparent-inputs")] - if wallet_db - .find_account_for_ephemeral_address(&to) - .map_err(Error::DataSource)? - .is_some() - { - return Err(Error::PaysEphemeralTransparentAddress(to.encode(params))); - } - if payment.memo().is_some() { - return Err(Error::MemoForbidden); - } - builder.add_transparent_output(&to, payment.amount())?; - transparent_output_meta.push(( - Recipient::External(recipient_address.clone(), PoolType::TRANSPARENT), - to, - payment.amount(), - StepOutputIndex::Payment(payment_index), - )); - Ok(()) - }; + let add_orchard_output = + |builder: &mut Builder<_, _>, + orchard_output_meta: &mut Vec<_>, + to: orchard::Address| + -> Result<(), CreateErrT> { + let memo = payment.memo().map_or_else(MemoBytes::empty, |m| m.clone()); + builder.add_orchard_output( + orchard_external_ovk.clone(), + to, + payment.amount().into(), + memo.clone(), + )?; + orchard_output_meta.push(( + Recipient::External(recipient_address.clone(), PoolType::ORCHARD), + payment.amount(), + Some(memo), + )); + Ok(()) + }; + + let add_transparent_output = + |builder: &mut Builder<_, _>, + transparent_output_meta: &mut Vec<_>, + to: TransparentAddress| + -> Result<(), CreateErrT> { + // Always reject sending to one of our known ephemeral addresses. + #[cfg(feature = "transparent-inputs")] + if wallet_db + .find_account_for_ephemeral_address(&to) + .map_err(Error::DataSource)? + .is_some() + { + return Err(Error::PaysEphemeralTransparentAddress(to.encode(params))); + } + if payment.memo().is_some() { + return Err(Error::MemoForbidden); + } + builder.add_transparent_output(&to, payment.amount())?; + transparent_output_meta.push(( + Recipient::External(recipient_address.clone(), PoolType::TRANSPARENT), + to, + payment.amount(), + StepOutputIndex::Payment(payment_index), + )); + Ok(()) + }; match recipient_address .clone() @@ -1371,36 +1416,33 @@ where #[cfg(feature = "transparent-inputs")] #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -pub fn shield_transparent_funds( +pub fn shield_transparent_funds( wallet_db: &mut DbT, params: &ParamsT, spend_prover: &impl SpendProver, output_prover: &impl OutputProver, input_selector: &InputsT, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, usk: &UnifiedSpendingKey, from_addrs: &[TransparentAddress], + to_account: ::AccountId, min_confirmations: u32, -) -> Result< - NonEmpty, - Error< - ::Error, - ::Error, - InputsT::Error, - ::Error, - >, -> +) -> Result, ShieldErrT> where ParamsT: consensus::Parameters, DbT: WalletWrite + WalletCommitmentTrees + InputSource::Error>, InputsT: ShieldingSelector, + ChangeT: ChangeStrategy, { let proposal = propose_shielding( wallet_db, params, input_selector, + change_strategy, shielding_threshold, from_addrs, + to_account, min_confirmations, )?; 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 343905f738..5125059bcb 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -11,19 +11,16 @@ use nonempty::NonEmpty; use zcash_address::ConversionError; use zcash_primitives::{ consensus::{self, BlockHeight}, - transaction::{ - components::{ - amount::{BalanceError, NonNegativeAmount}, - TxOut, - }, - fees::FeeRule, + transaction::components::{ + amount::{BalanceError, NonNegativeAmount}, + TxOut, }, }; use crate::{ address::{Address, UnifiedAddress}, data_api::{InputSource, SimpleNoteRetention, SpendableNotes}, - fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy}, + fees::{sapling, ChangeError, ChangeStrategy}, proposal::{Proposal, ProposalError, ShieldedInputs}, wallet::WalletTransparentOutput, zip321::TransactionRequest, @@ -47,11 +44,13 @@ use crate::fees::orchard as orchard_fees; /// The type of errors that may be produced in input selection. #[derive(Debug)] -pub enum InputSelectorError { +pub enum InputSelectorError { /// An error occurred accessing the underlying data store. DataSource(DbErrT), /// An error occurred specific to the provided input selector's selection rules. Selection(SelectorErrT), + /// An error occurred in computing the change or fee for the proposed transfer. + Change(ChangeError), /// Input selection attempted to generate an invalid transaction proposal. Proposal(ProposalError), /// An error occurred parsing the address from a payment request. @@ -67,13 +66,9 @@ pub enum InputSelectorError { SyncRequired, } -impl From> for InputSelectorError { - fn from(value: ConversionError<&'static str>) -> Self { - InputSelectorError::Address(value) - } -} - -impl fmt::Display for InputSelectorError { +impl fmt::Display + for InputSelectorError +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { InputSelectorError::DataSource(e) => { @@ -86,6 +81,11 @@ impl fmt::Display for InputSelectorError { write!(f, "Note selection encountered the following error: {}", e) } + InputSelectorError::Change(e) => write!( + f, + "Proposal generation failed due to an error in computing change or transaction fees: {}", + e + ), InputSelectorError::Proposal(e) => { write!( f, @@ -116,21 +116,36 @@ impl fmt::Display for InputSelectorError error::Error for InputSelectorError +impl error::Error for InputSelectorError where DE: Debug + Display + error::Error + 'static, SE: Debug + Display + error::Error + 'static, + CE: Debug + Display + error::Error + 'static, + N: Debug + Display + 'static, { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match &self { Self::DataSource(e) => Some(e), Self::Selection(e) => Some(e), + Self::Change(e) => Some(e), Self::Proposal(e) => Some(e), _ => None, } } } +impl From> for InputSelectorError { + fn from(value: ConversionError<&'static str>) -> Self { + InputSelectorError::Address(value) + } +} + +impl From> for InputSelectorError { + fn from(err: ChangeError) -> Self { + InputSelectorError::Change(err) + } +} + /// A strategy for selecting transaction inputs and proposing transaction outputs. /// /// Proposals should include only economically useful inputs, as determined by `Self::FeeRule`; @@ -139,14 +154,13 @@ where pub trait InputSelector { /// The type of errors that may be generated in input selection type Error; - /// The type of data source that the input selector expects to access to obtain input Sapling - /// notes. This associated type permits input selectors that may use specialized knowledge of - /// the internals of a particular backing data store, if the generic API of - /// `InputSource` does not provide sufficiently fine-grained operations for a particular - /// backing store to optimally perform input selection. + + /// The type of data source that the input selector expects to access to obtain input notes. + /// This associated type permits input selectors that may use specialized knowledge of the + /// internals of a particular backing data store, if the generic API of `InputSource` does not + /// provide sufficiently fine-grained operations for a particular backing store to optimally + /// perform input selection. type InputSource: InputSource; - /// The type of the fee rule that this input selector uses when computing fees. - type FeeRule: FeeRule; /// Performs input selection and returns a proposal for transaction construction including /// change and fee outputs. @@ -163,7 +177,8 @@ pub trait InputSelector { /// If insufficient funds are available to satisfy the required outputs for the shielding /// request, this operation must fail and return [`InputSelectorError::InsufficientFunds`]. #[allow(clippy::type_complexity)] - fn propose_transaction( + #[allow(clippy::too_many_arguments)] + fn propose_transaction( &self, params: &ParamsT, wallet_db: &Self::InputSource, @@ -171,12 +186,19 @@ pub trait InputSelector { anchor_height: BlockHeight, account: ::AccountId, transaction_request: TransactionRequest, + change_strategy: &ChangeT, ) -> Result< - Proposal::NoteRef>, - InputSelectorError<::Error, Self::Error>, + Proposal<::FeeRule, ::NoteRef>, + InputSelectorError< + ::Error, + Self::Error, + ChangeT::Error, + ::NoteRef, + >, > where - ParamsT: consensus::Parameters; + ParamsT: consensus::Parameters, + ChangeT: ChangeStrategy; } /// A strategy for selecting transaction inputs and proposing transaction outputs @@ -192,8 +214,6 @@ pub trait ShieldingSelector { /// [`InputSource`] does not provide sufficiently fine-grained operations for a /// particular backing store to optimally perform input selection. type InputSource: InputSource; - /// The type of the fee rule that this input selector uses when computing fees. - type FeeRule: FeeRule; /// Performs input selection and returns a proposal for the construction of a shielding /// transaction. @@ -204,36 +224,43 @@ pub trait ShieldingSelector { /// outputs for the shielding request, this operation must fail and return /// [`InputSelectorError::InsufficientFunds`]. #[allow(clippy::type_complexity)] - fn propose_shielding( + #[allow(clippy::too_many_arguments)] + fn propose_shielding( &self, params: &ParamsT, wallet_db: &Self::InputSource, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, source_addrs: &[TransparentAddress], + to_account: ::AccountId, target_height: BlockHeight, min_confirmations: u32, ) -> Result< - Proposal, - InputSelectorError<::Error, Self::Error>, + Proposal<::FeeRule, Infallible>, + InputSelectorError< + ::Error, + Self::Error, + ChangeT::Error, + Infallible, + >, > where - ParamsT: consensus::Parameters; + ParamsT: consensus::Parameters, + ChangeT: ChangeStrategy; } /// Errors that can occur as a consequence of greedy input selection. #[derive(Debug, Clone, PartialEq, Eq)] -pub enum GreedyInputSelectorError { +pub enum GreedyInputSelectorError { /// An intermediate value overflowed or underflowed the valid monetary range. Balance(BalanceError), /// A unified address did not contain a supported receiver. UnsupportedAddress(Box), /// Support for transparent-source-only (TEX) addresses requires the transparent-inputs feature. UnsupportedTexAddress, - /// An error was encountered in change selection. - Change(ChangeError), } -impl fmt::Display for GreedyInputSelectorError { +impl fmt::Display for GreedyInputSelectorError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { GreedyInputSelectorError::Balance(e) => write!( @@ -249,32 +276,20 @@ impl fmt::Display for GreedyInputSelectorErro GreedyInputSelectorError::UnsupportedTexAddress => { write!(f, "Support for transparent-source-only (TEX) addresses requires the transparent-inputs feature.") } - GreedyInputSelectorError::Change(err) => { - write!(f, "An error occurred computing change and fees: {}", err) - } } } } -impl - From> - for InputSelectorError> +impl From + for InputSelectorError { - fn from(err: GreedyInputSelectorError) -> Self { + fn from(err: GreedyInputSelectorError) -> Self { InputSelectorError::Selection(err) } } -impl From> - for InputSelectorError> -{ - fn from(err: ChangeError) -> Self { - InputSelectorError::Selection(GreedyInputSelectorError::Change(err)) - } -} - -impl From - for InputSelectorError> +impl From + for InputSelectorError { fn from(err: BalanceError) -> Self { InputSelectorError::Selection(GreedyInputSelectorError::Balance(err)) @@ -319,13 +334,11 @@ impl orchard_fees::OutputView for OrchardPayment { /// /// This implementation performs input selection using methods available via the /// [`InputSource`] interface. -pub struct GreedyInputSelector { - change_strategy: ChangeT, - dust_output_policy: DustOutputPolicy, +pub struct GreedyInputSelector { _ds_type: PhantomData, } -impl GreedyInputSelector { +impl GreedyInputSelector { /// Constructs a new greedy input selector that uses the provided change strategy to determine /// change values and fee amounts. /// @@ -335,27 +348,25 @@ impl GreedyInputSelector { /// attempting to construct a transaction proposal that requires such an output. /// /// [`EphemeralBalance::Output`]: crate::fees::EphemeralBalance::Output - pub fn new(change_strategy: ChangeT, dust_output_policy: DustOutputPolicy) -> Self { + pub fn new() -> Self { GreedyInputSelector { - change_strategy, - dust_output_policy, _ds_type: PhantomData, } } } -impl InputSelector for GreedyInputSelector -where - DbT: InputSource, - ChangeT: ChangeStrategy, - ChangeT::FeeRule: Clone, -{ - type Error = GreedyInputSelectorError; +impl Default for GreedyInputSelector { + fn default() -> Self { + Self::new() + } +} + +impl InputSelector for GreedyInputSelector { + type Error = GreedyInputSelectorError; type InputSource = DbT; - type FeeRule = ChangeT::FeeRule; #[allow(clippy::type_complexity)] - fn propose_transaction( + fn propose_transaction( &self, params: &ParamsT, wallet_db: &Self::InputSource, @@ -363,13 +374,15 @@ where anchor_height: BlockHeight, account: ::AccountId, transaction_request: TransactionRequest, + change_strategy: &ChangeT, ) -> Result< - Proposal, - InputSelectorError<::Error, Self::Error>, + Proposal<::FeeRule, DbT::NoteRef>, + InputSelectorError<::Error, Self::Error, ChangeT::Error, DbT::NoteRef>, > where ParamsT: consensus::Parameters, Self::InputSource: InputSource, + ChangeT: ChangeStrategy, { let mut transparent_outputs = vec![]; let mut sapling_outputs = vec![]; @@ -484,8 +497,8 @@ where // catching the `InsufficientFunds` error to obtain the required amount // given the provided change strategy. Ignore the change memo in order // to avoid adding a change output. - let tr1_required_input_value = - match self.change_strategy.compute_balance::<_, DbT::NoteRef>( + let tr1_required_input_value = match change_strategy + .compute_balance::<_, DbT::NoteRef>( params, target_height, &[] as &[WalletTransparentOutput], @@ -493,17 +506,18 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)), + None, ) { - Err(ChangeError::InsufficientFunds { required, .. }) => required, - Ok(_) => NonNegativeAmount::ZERO, // shouldn't happen - Err(other) => return Err(other.into()), - }; + Err(ChangeError::InsufficientFunds { required, .. }) => required, + Err(ChangeError::DustInputs { .. }) => unreachable!("no inputs were supplied"), + Err(other) => return Err(InputSelectorError::Change(other)), + Ok(_) => NonNegativeAmount::ZERO, // shouldn't happen + }; // Now recompute to obtain the `TransactionBalance` and verify that it // fully accounts for the required fees. - let tr1_balance = self.change_strategy.compute_balance::<_, DbT::NoteRef>( + let tr1_balance = change_strategy.compute_balance::<_, DbT::NoteRef>( params, target_height, &[] as &[WalletTransparentOutput], @@ -511,8 +525,8 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, Some(&EphemeralBalance::Input(tr1_required_input_value)), + None, )?; assert_eq!(tr1_balance.total(), tr1_balance.fee_required()); @@ -573,9 +587,20 @@ where vec![] }; + let selected_input_ids = sapling_inputs.iter().map(|(id, _)| id); + #[cfg(feature = "orchard")] + let selected_input_ids = + selected_input_ids.chain(orchard_inputs.iter().map(|(id, _)| id)); + + let selected_input_ids = selected_input_ids.cloned().collect::>(); + + let wallet_meta = change_strategy + .fetch_wallet_meta(wallet_db, account, &selected_input_ids) + .map_err(InputSelectorError::DataSource)?; + // In the ZIP 320 case, this is the balance for transaction 0, taking into account // the ephemeral output. - let balance = self.change_strategy.compute_balance( + let balance = change_strategy.compute_balance( params, target_height, &[] as &[WalletTransparentOutput], @@ -591,8 +616,8 @@ where &orchard_inputs[..], &orchard_outputs[..], ), - &self.dust_output_policy, ephemeral_balance.as_ref(), + Some(&wallet_meta), ); match balance { @@ -681,7 +706,7 @@ where ); return Proposal::multi_step( - self.change_strategy.fee_rule().clone(), + change_strategy.fee_rule().clone(), target_height, NonEmpty::from_vec(steps).expect("steps is known to be nonempty"), ) @@ -694,7 +719,7 @@ where vec![], shielded_inputs, balance, - self.change_strategy.fee_rule().clone(), + (*change_strategy.fee_rule()).clone(), target_height, false, ) @@ -713,7 +738,7 @@ where Err(ChangeError::InsufficientFunds { required, .. }) => { amount_required = required; } - Err(other) => return Err(other.into()), + Err(other) => return Err(InputSelectorError::Change(other)), } #[cfg(not(feature = "orchard"))] @@ -747,31 +772,28 @@ where } #[cfg(feature = "transparent-inputs")] -impl ShieldingSelector for GreedyInputSelector -where - DbT: InputSource, - ChangeT: ChangeStrategy, - ChangeT::FeeRule: Clone, -{ - type Error = GreedyInputSelectorError; +impl ShieldingSelector for GreedyInputSelector { + type Error = GreedyInputSelectorError; type InputSource = DbT; - type FeeRule = ChangeT::FeeRule; #[allow(clippy::type_complexity)] - fn propose_shielding( + fn propose_shielding( &self, params: &ParamsT, wallet_db: &Self::InputSource, + change_strategy: &ChangeT, shielding_threshold: NonNegativeAmount, source_addrs: &[TransparentAddress], + to_account: ::AccountId, target_height: BlockHeight, min_confirmations: u32, ) -> Result< - Proposal, - InputSelectorError<::Error, Self::Error>, + Proposal<::FeeRule, Infallible>, + InputSelectorError<::Error, Self::Error, ChangeT::Error, Infallible>, > where ParamsT: consensus::Parameters, + ChangeT: ChangeStrategy, { let mut transparent_inputs: Vec = source_addrs .iter() @@ -784,7 +806,11 @@ where .flat_map(|v| v.into_iter()) .collect(); - let trial_balance = self.change_strategy.compute_balance( + let wallet_meta = change_strategy + .fetch_wallet_meta(wallet_db, to_account, &[]) + .map_err(InputSelectorError::DataSource)?; + + let trial_balance = change_strategy.compute_balance( params, target_height, &transparent_inputs, @@ -792,8 +818,8 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, None, + Some(&wallet_meta), ); let balance = match trial_balance { @@ -802,7 +828,7 @@ where let exclusions: BTreeSet = transparent.into_iter().collect(); transparent_inputs.retain(|i| !exclusions.contains(i.outpoint())); - self.change_strategy.compute_balance( + change_strategy.compute_balance( params, target_height, &transparent_inputs, @@ -810,13 +836,11 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, None, + Some(&wallet_meta), )? } - Err(other) => { - return Err(other.into()); - } + Err(other) => return Err(InputSelectorError::Change(other)), }; if balance.total() >= shielding_threshold { @@ -826,7 +850,7 @@ where transparent_inputs, None, balance, - (*self.change_strategy.fee_rule()).clone(), + (*change_strategy.fee_rule()).clone(), target_height, true, ) diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index fea9424243..89d2c1d7f7 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -1,18 +1,17 @@ -use std::fmt; +use std::fmt::{self, Debug, Display}; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, transaction::{ - components::{ - amount::{BalanceError, NonNegativeAmount}, - OutPoint, - }, + components::{amount::NonNegativeAmount, OutPoint}, fees::{transparent, FeeRule}, }, }; use zcash_protocol::{PoolType, ShieldedProtocol}; +use crate::data_api::InputSource; + pub(crate) mod common; pub mod fixed; #[cfg(feature = "orchard")] @@ -273,9 +272,16 @@ impl fmt::Display for ChangeError { } } -impl From for ChangeError { - fn from(err: BalanceError) -> ChangeError { - ChangeError::StrategyError(err) +impl std::error::Error for ChangeError +where + E: Debug + Display + std::error::Error + 'static, + N: Debug + Display + 'static, +{ + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self { + ChangeError::StrategyError(e) => Some(e), + _ => None, + } } } @@ -368,13 +374,30 @@ impl EphemeralBalance { /// A trait that represents the ability to compute the suggested change and fees that must be paid /// by a transaction having a specified set of inputs and outputs. pub trait ChangeStrategy { - type FeeRule: FeeRule; + type FeeRule: FeeRule + Clone; type Error; + /// The type of metadata source that this change strategy requires in order to be able to + /// retrieve required wallet metadata. If more capabilities are required of the backend than + /// are exposed in the [`InputSource`] trait, the implementer of this trait should define their + /// own trait that descends from [`InputSource`] and adds the required capabilities there, and + /// then implement that trait for their desired database backend. + type MetaSource: InputSource; + type WalletMeta; + /// Returns the fee rule that this change strategy will respect when performing /// balance computations. fn fee_rule(&self) -> &Self::FeeRule; + /// Uses the provided metadata source to obtain the wallet metadata required for change + /// creation determinations. + fn fetch_wallet_meta( + &self, + meta_source: &Self::MetaSource, + account: ::AccountId, + exclude: &[::NoteRef], + ) -> Result::Error>; + /// Computes the totals of inputs, suggested change amounts, and fees given the /// provided inputs and outputs being used to construct a transaction. /// @@ -393,7 +416,11 @@ pub trait ChangeStrategy { /// - `ephemeral_balance`: if the transaction is to be constructed with either an /// ephemeral transparent input or an ephemeral transparent output this argument /// may be used to provide the value of that input or output. The value of this - /// output should be `None` in the case that there are no such items. + /// argument should be `None` in the case that there are no such items. + /// - `wallet_meta`: Additional wallet metadata that the change strategy may use + /// in determining how to construct change outputs. This wallet metadata value + /// should be computed excluding the inputs provided in the `transparent_inputs`, + /// `sapling`, and `orchard` arguments. /// /// [ZIP 320]: https://zips.z.cash/zip-0320 #[allow(clippy::too_many_arguments)] @@ -405,8 +432,8 @@ pub trait ChangeStrategy { transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, - dust_output_policy: &DustOutputPolicy, ephemeral_balance: Option<&EphemeralBalance>, + wallet_meta: Option<&Self::WalletMeta>, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index d2033eba0b..48da67d53b 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -1,5 +1,7 @@ //! Change strategies designed for use with a fixed fee. +use std::marker::PhantomData; + use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, @@ -9,7 +11,7 @@ use zcash_primitives::{ }, }; -use crate::ShieldedProtocol; +use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, @@ -23,13 +25,15 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { +pub struct SingleOutputChangeStrategy { fee_rule: FixedFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + meta_source: PhantomData, } -impl SingleOutputChangeStrategy { +impl SingleOutputChangeStrategy { /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule /// and change memo. /// @@ -39,23 +43,37 @@ impl SingleOutputChangeStrategy { fee_rule: FixedFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, ) -> Self { Self { fee_rule, change_memo, fallback_change_pool, + dust_output_policy, + meta_source: PhantomData, } } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; + type MetaSource = I; + type WalletMeta = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule } + fn fetch_wallet_meta( + &self, + _meta_source: &Self::MetaSource, + _account: ::AccountId, + _exclude: &[::NoteRef], + ) -> Result::Error> { + Ok(()) + } + fn compute_balance( &self, params: &P, @@ -64,8 +82,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, - dust_output_policy: &DustOutputPolicy, ephemeral_balance: Option<&EphemeralBalance>, + _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { single_change_output_balance( params, @@ -76,7 +94,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, + &self.dust_output_policy, self.fee_rule.fixed_fee(), self.change_memo.as_ref(), self.fallback_change_pool, @@ -99,7 +117,7 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::wallet::input_selection::SaplingPayment, + data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, @@ -114,8 +132,12 @@ mod tests { fn change_without_dust() { #[allow(deprecated)] let fee_rule = FixedFeeRule::standard(); - let change_strategy = - SingleOutputChangeStrategy::new(fee_rule, None, ShieldedProtocol::Sapling); + let change_strategy = SingleOutputChangeStrategy::::new( + fee_rule, + None, + ShieldedProtocol::Sapling, + DustOutputPolicy::default(), + ); // spend a single Sapling note that is sufficient to pay the fee let result = change_strategy.compute_balance( @@ -137,7 +159,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); @@ -153,8 +175,12 @@ mod tests { fn dust_change() { #[allow(deprecated)] let fee_rule = FixedFeeRule::standard(); - let change_strategy = - SingleOutputChangeStrategy::new(fee_rule, None, ShieldedProtocol::Sapling); + let change_strategy = SingleOutputChangeStrategy::::new( + fee_rule, + None, + ShieldedProtocol::Sapling, + DustOutputPolicy::default(), + ); // spend a single Sapling note that is sufficient to pay the fee let result = change_strategy.compute_balance( @@ -183,7 +209,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index 4cad64c5d2..dbaf8ace1b 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -1,5 +1,7 @@ //! Change strategies designed for use with a standard fee. +use std::marker::PhantomData; + use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, @@ -14,7 +16,7 @@ use zcash_primitives::{ }, }; -use crate::ShieldedProtocol; +use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ fixed, sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, @@ -28,13 +30,15 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { +pub struct SingleOutputChangeStrategy { fee_rule: StandardFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + meta_source: PhantomData, } -impl SingleOutputChangeStrategy { +impl SingleOutputChangeStrategy { /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 /// fee parameters. /// @@ -44,23 +48,37 @@ impl SingleOutputChangeStrategy { fee_rule: StandardFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, ) -> Self { Self { fee_rule, change_memo, fallback_change_pool, + dust_output_policy, + meta_source: PhantomData, } } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = StandardFeeRule; type Error = Zip317FeeError; + type MetaSource = I; + type WalletMeta = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule } + fn fetch_wallet_meta( + &self, + _meta_source: &Self::MetaSource, + _account: ::AccountId, + _exclude: &[::NoteRef], + ) -> Result::Error> { + Ok(()) + } + fn compute_balance( &self, params: &P, @@ -69,15 +87,16 @@ impl ChangeStrategy for SingleOutputChangeStrategy { transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, - dust_output_policy: &DustOutputPolicy, ephemeral_balance: Option<&EphemeralBalance>, + wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { #[allow(deprecated)] match self.fee_rule() { - StandardFeeRule::PreZip313 => fixed::SingleOutputChangeStrategy::new( + StandardFeeRule::PreZip313 => fixed::SingleOutputChangeStrategy::::new( FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(10000)), self.change_memo.clone(), self.fallback_change_pool, + self.dust_output_policy, ) .compute_balance( params, @@ -87,14 +106,15 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, ephemeral_balance, + wallet_meta, ) .map_err(|e| e.map(Zip317FeeError::Balance)), - StandardFeeRule::Zip313 => fixed::SingleOutputChangeStrategy::new( + StandardFeeRule::Zip313 => fixed::SingleOutputChangeStrategy::::new( FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(1000)), self.change_memo.clone(), self.fallback_change_pool, + self.dust_output_policy, ) .compute_balance( params, @@ -104,14 +124,15 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, ephemeral_balance, + wallet_meta, ) .map_err(|e| e.map(Zip317FeeError::Balance)), - StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::new( + StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), self.change_memo.clone(), self.fallback_change_pool, + self.dust_output_policy, ) .compute_balance( params, @@ -121,8 +142,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, ephemeral_balance, + wallet_meta, ), } } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index d8813e3611..56f79cca6c 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -4,6 +4,8 @@ //! to ensure that inputs added to a transaction do not cause fees to rise by //! an amount greater than their value. +use std::marker::PhantomData; + use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, @@ -13,7 +15,7 @@ use zcash_primitives::{ }, }; -use crate::ShieldedProtocol; +use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, @@ -27,13 +29,15 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { +pub struct SingleOutputChangeStrategy { fee_rule: Zip317FeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + meta_source: PhantomData, } -impl SingleOutputChangeStrategy { +impl SingleOutputChangeStrategy { /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 /// fee parameters and change memo. /// @@ -43,23 +47,37 @@ impl SingleOutputChangeStrategy { fee_rule: Zip317FeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, ) -> Self { Self { fee_rule, change_memo, fallback_change_pool, + dust_output_policy, + meta_source: PhantomData, } } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; + type MetaSource = I; + type WalletMeta = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule } + fn fetch_wallet_meta( + &self, + _meta_source: &Self::MetaSource, + _account: ::AccountId, + _exclude: &[::NoteRef], + ) -> Result::Error> { + Ok(()) + } + fn compute_balance( &self, params: &P, @@ -68,8 +86,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { transparent_outputs: &[impl transparent::OutputView], sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, - dust_output_policy: &DustOutputPolicy, ephemeral_balance: Option<&EphemeralBalance>, + _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { single_change_output_balance( params, @@ -80,7 +98,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, + &self.dust_output_policy, self.fee_rule.marginal_fee(), self.change_memo.as_ref(), self.fallback_change_pool, @@ -106,7 +124,7 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::wallet::input_selection::SaplingPayment, + data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, @@ -122,10 +140,11 @@ mod tests { #[test] fn change_without_dust() { - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -148,7 +167,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); @@ -163,10 +182,11 @@ mod tests { #[test] #[cfg(feature = "orchard")] fn cross_pool_change_without_dust() { - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Orchard, + DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -192,7 +212,7 @@ mod tests { 30000, ))][..], ), - &DustOutputPolicy::default(), + None, None, ); @@ -206,22 +226,23 @@ mod tests { #[test] fn change_with_transparent_payments_implicitly_allowing_zero_change() { - change_with_transparent_payments(&DustOutputPolicy::default()) + change_with_transparent_payments(DustOutputPolicy::default()) } #[test] fn change_with_transparent_payments_explicitly_allowing_zero_change() { - change_with_transparent_payments(&DustOutputPolicy::new( + change_with_transparent_payments(DustOutputPolicy::new( DustAction::AllowDustChange, Some(NonNegativeAmount::ZERO), )) } - fn change_with_transparent_payments(dust_output_policy: &DustOutputPolicy) { - let change_strategy = SingleOutputChangeStrategy::new( + fn change_with_transparent_payments(dust_output_policy: DustOutputPolicy) { + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + dust_output_policy, ); // spend a single Sapling note that is sufficient to pay the fee @@ -245,7 +266,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - dust_output_policy, + None, None, ); @@ -263,10 +284,11 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + DustOutputPolicy::default(), ); // Spend a single transparent UTXO that is exactly sufficient to pay the fee. @@ -289,7 +311,7 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); @@ -307,10 +329,11 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + DustOutputPolicy::default(), ); // Spend a single transparent UTXO that is sufficient to pay the fee. @@ -333,7 +356,7 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); @@ -351,10 +374,14 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + DustOutputPolicy::new( + DustAction::AllowDustChange, + Some(NonNegativeAmount::const_from_u64(1000)), + ), ); // Spend a single transparent UTXO that is sufficient to pay the fee. @@ -380,10 +407,7 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::new( - DustAction::AllowDustChange, - Some(NonNegativeAmount::const_from_u64(1000)), - ), + None, None, ); @@ -397,22 +421,23 @@ mod tests { #[test] fn change_with_allowable_dust_implicitly_allowing_zero_change() { - change_with_allowable_dust(&DustOutputPolicy::default()) + change_with_allowable_dust(DustOutputPolicy::default()) } #[test] fn change_with_allowable_dust_explicitly_allowing_zero_change() { - change_with_allowable_dust(&DustOutputPolicy::new( + change_with_allowable_dust(DustOutputPolicy::new( DustAction::AllowDustChange, Some(NonNegativeAmount::ZERO), )) } - fn change_with_allowable_dust(dust_output_policy: &DustOutputPolicy) { - let change_strategy = SingleOutputChangeStrategy::new( + fn change_with_allowable_dust(dust_output_policy: DustOutputPolicy) { + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + dust_output_policy, ); // Spend two Sapling notes, one of them dust. There is sufficient to @@ -444,7 +469,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - dust_output_policy, + None, None, ); @@ -458,10 +483,11 @@ mod tests { #[test] fn change_with_disallowed_dust() { - let change_strategy = SingleOutputChangeStrategy::new( + let change_strategy = SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, + DustOutputPolicy::default(), ); // Attempt to spend three Sapling notes, one of them dust. Adding the third @@ -495,7 +521,7 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), + None, None, ); diff --git a/zcash_client_backend/src/proposal.rs b/zcash_client_backend/src/proposal.rs index 3c06be6c45..c7d8ff468a 100644 --- a/zcash_client_backend/src/proposal.rs +++ b/zcash_client_backend/src/proposal.rs @@ -126,7 +126,7 @@ impl Display for ProposalError { #[cfg(feature = "transparent-inputs")] ProposalError::EphemeralOutputsInvalid => write!( f, - "The change strategy provided to input selection failed to correctly generate an ephemeral change output when needed for sending to a TEX address." + "The proposal generator failed to correctly generate an ephemeral change output when needed for sending to a TEX address." ), } } diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 70879ab85e..80edb6c5ef 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -118,7 +118,7 @@ pub(crate) fn external_address_change_spends_detected_in_restore_from_seed< #[allow(dead_code)] pub(crate) fn zip317_spend() { - zcash_client_backend::data_api::testing::pool::zip317_spend::( + zcash_client_backend::data_api::testing::pool::zip317_spend::( TestDbFactory, BlockCache::new(), ) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 21977adad1..40ad625511 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -65,6 +65,7 @@ //! - `memo` the shielded memo associated with the output, if any. use incrementalmerkletree::{Marking, Retention}; + use rusqlite::{self, named_params, params, OptionalExtension}; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; @@ -78,6 +79,7 @@ use std::convert::TryFrom; use std::io::{self, Cursor}; use std::num::NonZeroU32; use std::ops::RangeInclusive; + use tracing::{debug, warn}; use zcash_address::ZcashAddress; diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 534656e3a8..9cb65e5717 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1779,20 +1779,21 @@ pub(crate) mod tests { fee_rule, Some(change_memo.into()), OrchardPoolTester::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), ); - let input_selector = - &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); + let input_selector = GreedyInputSelector::new(); let proposal = st .propose_transfer( account.id(), - input_selector, + &input_selector, + &change_strategy, request, NonZeroU32::new(10).unwrap(), ) .unwrap(); - let create_proposed_result = st.create_proposed_transactions::( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal, @@ -1867,13 +1868,14 @@ pub(crate) mod tests { fee_rule, Some(change_memo.into()), OrchardPoolTester::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), ); - let input_selector = - &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); + let input_selector = GreedyInputSelector::new(); let proposal = st.propose_transfer( account.id(), - input_selector, + &input_selector, + &change_strategy, request.clone(), NonZeroU32::new(10).unwrap(), ); @@ -1886,7 +1888,8 @@ pub(crate) mod tests { // Verify that it's now possible to create the proposal let proposal = st.propose_transfer( account.id(), - input_selector, + &input_selector, + &change_strategy, request, NonZeroU32::new(10).unwrap(), ); From 56f5c9444cbc8c9e0a9fd6f4a31040736e040eed Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 4/9] zcash_client_backend: Clean up arguments to `single_change_output_balance` --- zcash_client_backend/src/fees/common.rs | 66 ++++++++++++++++++------- zcash_client_backend/src/fees/fixed.rs | 21 +++++--- zcash_client_backend/src/fees/zip317.rs | 21 +++++--- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 99dcf046b2..59600cc9b2 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -164,6 +164,38 @@ impl OutputManifest { } } +pub(crate) struct SinglePoolBalanceConfig<'a, P, F> { + params: &'a P, + fee_rule: &'a F, + dust_output_policy: &'a DustOutputPolicy, + default_dust_threshold: NonNegativeAmount, + fallback_change_pool: ShieldedProtocol, + marginal_fee: NonNegativeAmount, + grace_actions: usize, +} + +impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { + pub(crate) fn new( + params: &'a P, + fee_rule: &'a F, + dust_output_policy: &'a DustOutputPolicy, + default_dust_threshold: NonNegativeAmount, + fallback_change_pool: ShieldedProtocol, + marginal_fee: NonNegativeAmount, + grace_actions: usize, + ) -> Self { + Self { + params, + fee_rule, + dust_output_policy, + default_dust_threshold, + fallback_change_pool, + marginal_fee, + grace_actions, + } + } +} + #[allow(clippy::too_many_arguments)] pub(crate) fn single_change_output_balance< P: consensus::Parameters, @@ -171,19 +203,13 @@ pub(crate) fn single_change_output_balance< F: FeeRule, E, >( - params: &P, - fee_rule: &F, + cfg: SinglePoolBalanceConfig, 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<&MemoBytes>, - fallback_change_pool: ShieldedProtocol, - marginal_fee: NonNegativeAmount, - grace_actions: usize, ephemeral_balance: Option<&EphemeralBalance>, ) -> Result> where @@ -208,7 +234,7 @@ where #[allow(unused_variables)] let (change_pool, sapling_change, orchard_change) = - single_change_output_policy(&net_flows, fallback_change_pool); + single_change_output_policy(&net_flows, cfg.fallback_change_pool); // We don't create a fully-transparent transaction if a change memo is used. let transparent = net_flows.is_transparent() && change_memo.is_none(); @@ -216,13 +242,13 @@ where // If we have a non-zero marginal fee, we need to check for uneconomic inputs. // This is basically assuming that fee rules with non-zero marginal fee are // "ZIP 317-like", but we can generalize later if needed. - if marginal_fee.is_positive() { + if cfg.marginal_fee.is_positive() { // Is it certain that there will be a change output? If it is not certain, // we should call `check_for_uneconomic_inputs` with `possible_change` // including both possibilities. let possible_change = // These are the situations where we might not have a change output. - if transparent || (dust_output_policy.action() == DustAction::AddDustToFee && change_memo.is_none()) { + if transparent || (cfg.dust_output_policy.action() == DustAction::AddDustToFee && change_memo.is_none()) { vec![ OutputManifest::ZERO, OutputManifest { @@ -241,8 +267,8 @@ where sapling, #[cfg(feature = "orchard")] orchard, - marginal_fee, - grace_actions, + cfg.marginal_fee, + cfg.grace_actions, &possible_change[..], ephemeral_balance, )?; @@ -331,9 +357,10 @@ where .map(|_| P2PKH_STANDARD_OUTPUT_SIZE), ); - let fee_without_change = fee_rule + let fee_without_change = cfg + .fee_rule .fee_required( - params, + cfg.params, target_height, transparent_input_sizes.clone(), transparent_output_sizes.clone(), @@ -345,9 +372,9 @@ where let fee_with_change = max( fee_without_change, - fee_rule + cfg.fee_rule .fee_required( - params, + cfg.params, target_height, transparent_input_sizes, transparent_output_sizes, @@ -394,12 +421,13 @@ where ) }; - let change_dust_threshold = dust_output_policy + let change_dust_threshold = cfg + .dust_output_policy .dust_threshold() - .unwrap_or(default_dust_threshold); + .unwrap_or(cfg.default_dust_threshold); if proposed_change < change_dust_threshold { - match dust_output_policy.action() { + match cfg.dust_output_policy.action() { DustAction::Reject => { // Always allow zero-valued change even for the `Reject` policy: // * it should be allowed in order to record change memos and to improve diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 48da67d53b..2d2938cfff 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -14,8 +14,9 @@ use zcash_primitives::{ use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ - common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, - DustOutputPolicy, EphemeralBalance, TransactionBalance, + common::{single_change_output_balance, SinglePoolBalanceConfig}, + sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, + TransactionBalance, }; #[cfg(feature = "orchard")] @@ -85,21 +86,25 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - single_change_output_balance( + let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, + &self.dust_output_policy, + self.fee_rule.fixed_fee(), + self.fallback_change_pool, + NonNegativeAmount::ZERO, + 0, + ); + + single_change_output_balance( + cfg, target_height, transparent_inputs, transparent_outputs, sapling, #[cfg(feature = "orchard")] orchard, - &self.dust_output_policy, - self.fee_rule.fixed_fee(), self.change_memo.as_ref(), - self.fallback_change_pool, - NonNegativeAmount::ZERO, - 0, ephemeral_balance, ) } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 56f79cca6c..673dc7cf74 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -18,8 +18,9 @@ use zcash_primitives::{ use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ - common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, - DustOutputPolicy, EphemeralBalance, TransactionBalance, + common::{single_change_output_balance, SinglePoolBalanceConfig}, + sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, + TransactionBalance, }; #[cfg(feature = "orchard")] @@ -89,21 +90,25 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - single_change_output_balance( + let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, + &self.dust_output_policy, + self.fee_rule.marginal_fee(), + self.fallback_change_pool, + self.fee_rule.marginal_fee(), + self.fee_rule.grace_actions(), + ); + + single_change_output_balance( + cfg, target_height, transparent_inputs, transparent_outputs, sapling, #[cfg(feature = "orchard")] orchard, - &self.dust_output_policy, - self.fee_rule.marginal_fee(), self.change_memo.as_ref(), - self.fallback_change_pool, - self.fee_rule.marginal_fee(), - self.fee_rule.grace_actions(), ephemeral_balance, ) } From ebca2ec65b4eb3d1042c35b77941d98fb10709e1 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 5/9] zcash_client_backend: Add `fees::zip317::MultiOutputChangeStrategy`. --- components/zcash_protocol/CHANGELOG.md | 4 + components/zcash_protocol/src/value.rs | 28 ++ zcash_client_backend/CHANGELOG.md | 2 + zcash_client_backend/src/data_api/testing.rs | 6 - .../src/data_api/testing/pool.rs | 179 ++++++++++++- zcash_client_backend/src/fees.rs | 71 ++++- zcash_client_backend/src/fees/common.rs | 253 ++++++++++++------ zcash_client_backend/src/fees/fixed.rs | 9 +- zcash_client_backend/src/fees/zip317.rs | 231 +++++++++++++++- zcash_client_sqlite/src/testing/pool.rs | 7 + zcash_client_sqlite/src/wallet/orchard.rs | 5 + zcash_client_sqlite/src/wallet/sapling.rs | 5 + zcash_keys/src/keys.rs | 1 + 13 files changed, 693 insertions(+), 108 deletions(-) diff --git a/components/zcash_protocol/CHANGELOG.md b/components/zcash_protocol/CHANGELOG.md index 505c8408a1..2b62e65a97 100644 --- a/components/zcash_protocol/CHANGELOG.md +++ b/components/zcash_protocol/CHANGELOG.md @@ -7,6 +7,10 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Added +- `zcash_protocol::value::DivRem` +- `zcash_protocol::value::Zatoshis::div_with_remainder` + ## [0.4.0] - 2024-10-02 ### Added - `impl Sub for BlockHeight` unlike the implementation that was diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index 395ac8edc2..7112013c20 100644 --- a/components/zcash_protocol/src/value.rs +++ b/components/zcash_protocol/src/value.rs @@ -1,6 +1,7 @@ use std::convert::{Infallible, TryFrom}; use std::error; use std::iter::Sum; +use std::num::NonZeroU64; use std::ops::{Add, Mul, Neg, Sub}; use memuse::DynamicUsage; @@ -229,6 +230,24 @@ impl Mul for ZatBalance { #[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Eq, Ord)] pub struct Zatoshis(u64); +/// A struct that provides both the quotient and remainder of a division operation. +pub struct QuotRem { + quotient: A, + remainder: A, +} + +impl QuotRem { + /// Returns the quotient portion of the value. + pub fn quotient(&self) -> &A { + &self.quotient + } + + /// Returns the remainder portion of the value. + pub fn remainder(&self) -> &A { + &self.remainder + } +} + impl Zatoshis { /// Returns the identity `Zatoshis` pub const ZERO: Self = Zatoshis(0); @@ -298,6 +317,15 @@ impl Zatoshis { pub fn is_positive(&self) -> bool { self > &Zatoshis::ZERO } + + /// Divides this `Zatoshis` value by the given divisor and returns the quotient and remainder. + pub fn div_with_remainder(&self, divisor: NonZeroU64) -> QuotRem { + let divisor = u64::from(divisor); + QuotRem { + quotient: Zatoshis(self.0 / divisor), + remainder: Zatoshis(self.0 % divisor), + } + } } impl From for ZatBalance { diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index a34a061009..c983d39f52 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -13,6 +13,8 @@ and this library adheres to Rust's notion of - `WalletSummary::progress` - `WalletMeta` - `impl Default for wallet::input_selection::GreedyInputSelector` +- `zcash_client_backend::fees::SplitPolicy` +- `zcash_client_backend::fees::zip317::MultiOutputChangeStrategy` ### Changed - `zcash_client_backend::data_api`: diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 2e095eb5e9..9e99355034 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -1221,12 +1221,6 @@ impl TestState { // } } -/// Helper method for constructing a [`GreedyInputSelector`] with a -/// [`standard::SingleOutputChangeStrategy`]. -pub fn input_selector() -> GreedyInputSelector { - GreedyInputSelector::::new() -} - pub fn single_output_change_strategy( fee_rule: StandardFeeRule, change_memo: Option<&str>, diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index dfba19e6fd..98d6efd869 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -2,7 +2,7 @@ use std::{ cmp::Eq, convert::Infallible, hash::Hash, - num::{NonZeroU32, NonZeroU8}, + num::{NonZeroU32, NonZeroU8, NonZeroUsize}, }; use assert_matches::assert_matches; @@ -17,7 +17,7 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ components::amount::NonNegativeAmount, - fees::{fixed::FeeRule as FixedFeeRule, StandardFeeRule}, + fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule, StandardFeeRule}, Transaction, }, }; @@ -48,9 +48,9 @@ use crate::{ }, decrypt_transaction, fees::{ - fixed, + self, standard::{self, SingleOutputChangeStrategy}, - DustOutputPolicy, + DustOutputPolicy, SplitPolicy, }, scanning::ScanError, wallet::{Note, NoteId, OvkPolicy, ReceivedNote}, @@ -316,6 +316,175 @@ pub fn send_single_step_proposed_transfer( ); } +pub fn send_with_multiple_change_outputs( + dsf: impl DataStoreFactory, + cache: impl TestCache, +) { + let mut st = TestBuilder::new() + .with_data_store_factory(dsf) + .with_block_cache(cache) + .with_account_from_sapling_activation(BlockHash([0; 32])) + .build(); + + let account = st.test_account().cloned().unwrap(); + let dfvk = T::test_account_fvk(&st); + + // Add funds to the wallet in a single note + let value = Zatoshis::const_from_u64(650_0000); + let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + st.scan_cached_blocks(h, 1); + + // Spendable balance matches total balance + assert_eq!(st.get_total_balance(account.id()), value); + assert_eq!(st.get_spendable_balance(account.id(), 1), value); + + assert_eq!( + st.wallet() + .block_max_scanned() + .unwrap() + .unwrap() + .block_height(), + h + ); + + let to_extsk = T::sk(&[0xf5; 32]); + let to: Address = T::sk_default_address(&to_extsk); + let request = zip321::TransactionRequest::new(vec![Payment::without_memo( + to.to_zcash_address(st.network()), + Zatoshis::const_from_u64(100_0000), + )]) + .unwrap(); + + let input_selector = GreedyInputSelector::new(); + let change_memo = "Test change memo".parse::().unwrap(); + let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + Zip317FeeRule::standard(), + Some(change_memo.clone().into()), + T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(2).unwrap(), + NonNegativeAmount::const_from_u64(100_0000), + ), + ); + + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request.clone(), + NonZeroU32::new(1).unwrap(), + ) + .unwrap(); + + let step = &proposal.steps().head; + assert_eq!(step.balance().proposed_change().len(), 2); + + let create_proposed_result = st.create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ); + assert_matches!(&create_proposed_result, Ok(txids) if txids.len() == 1); + + let sent_tx_id = create_proposed_result.unwrap()[0]; + + // Verify that the sent transaction was stored and that we can decrypt the memos + let tx = st + .wallet() + .get_transaction(sent_tx_id) + .unwrap() + .expect("Created transaction was stored."); + let ufvks = [(account.id(), account.usk().to_unified_full_viewing_key())] + .into_iter() + .collect(); + let d_tx = decrypt_transaction(st.network(), h + 1, &tx, &ufvks); + assert_eq!(T::decrypted_pool_outputs_count(&d_tx), 3); + + let mut found_tx_change_memo = false; + let mut found_tx_empty_memo = false; + T::with_decrypted_pool_memos(&d_tx, |memo| { + if Memo::try_from(memo).unwrap() == change_memo { + found_tx_change_memo = true + } + if Memo::try_from(memo).unwrap() == Memo::Empty { + found_tx_empty_memo = true + } + }); + assert!(found_tx_change_memo); + assert!(found_tx_empty_memo); + + // Verify that the stored sent notes match what we're expecting + let sent_note_ids = st + .wallet() + .get_sent_note_ids(&sent_tx_id, T::SHIELDED_PROTOCOL) + .unwrap(); + assert_eq!(sent_note_ids.len(), 3); + + // The sent memo should be the empty memo for the sent output, and the + // change output's memo should be as specified. + let mut change_memo_count = 0; + let mut found_sent_empty_memo = false; + for sent_note_id in sent_note_ids { + match st + .wallet() + .get_memo(sent_note_id) + .expect("Note id is valid") + .as_ref() + { + Some(m) if m == &change_memo => { + change_memo_count += 1; + } + Some(m) if m == &Memo::Empty => { + found_sent_empty_memo = true; + } + Some(other) => panic!("Unexpected memo value: {:?}", other), + None => panic!("Memo should not be stored as NULL"), + } + } + assert_eq!(change_memo_count, 2); + assert!(found_sent_empty_memo); + + let tx_history = st.wallet().get_tx_history().unwrap(); + assert_eq!(tx_history.len(), 2); + + let network = *st.network(); + assert_matches!( + decrypt_and_store_transaction(&network, st.wallet_mut(), &tx, None), + Ok(_) + ); + + let (h, _) = st.generate_next_block_including(sent_tx_id); + st.scan_cached_blocks(h, 1); + + // Now, create another proposal with more outputs requested. We have two change notes; + // we'll spend one of them, and then we'll generate 7 splits. + let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + Zip317FeeRule::standard(), + Some(change_memo.into()), + T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(8).unwrap(), + NonNegativeAmount::const_from_u64(10_0000), + ), + ); + + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + NonZeroU32::new(1).unwrap(), + ) + .unwrap(); + + let step = &proposal.steps().head; + assert_eq!(step.balance().proposed_change().len(), 7); +} + #[cfg(feature = "transparent-inputs")] pub fn send_multi_step_proposed_transfer( ds_factory: DSF, @@ -1414,7 +1583,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed Self { + Self { + target_output_count, + min_split_output_size, + } + } + + /// Constructs a [`SplitPolicy`] that prescribes a single output (no splitting). + pub fn single_output() -> Self { + Self { + target_output_count: NonZeroUsize::MIN, + min_split_output_size: NonNegativeAmount::ZERO, + } + } + + /// Returns the minimum value for a note resulting from splitting of change. + /// + /// If splitting change would result in notes of value less than the minimum split output size, + /// a smaller number of splits should be chosen. + pub fn min_split_output_size(&self) -> NonNegativeAmount { + self.min_split_output_size + } + + /// Returns the number of output notes to produce from the given total change value, given the + /// number of existing unspent notes in the account and this policy. + pub fn split_count( + &self, + existing_notes: usize, + total_change: NonNegativeAmount, + ) -> NonZeroUsize { + let mut split_count = + NonZeroUsize::new(usize::from(self.target_output_count).saturating_sub(existing_notes)) + .unwrap_or(NonZeroUsize::MIN); + + loop { + let per_output_change = total_change.div_with_remainder( + NonZeroU64::new( + u64::try_from(usize::from(split_count)).expect("usize fits into u64"), + ) + .unwrap(), + ); + if split_count > NonZeroUsize::MIN + && *per_output_change.quotient() < self.min_split_output_size + { + split_count = NonZeroUsize::new(usize::from(split_count) - 1) + .expect("split_count checked to be > 1"); + } else { + return split_count; + } + } + } +} + /// `EphemeralBalance` describes the ephemeral input or output value for a transaction. It is used /// in fee computation for series of transactions that use an ephemeral transparent output in an /// intermediate step, such as when sending from a shielded pool to a [ZIP 320] "TEX" address. diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 59600cc9b2..9c221bbd59 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -1,4 +1,5 @@ use core::cmp::{max, min}; +use std::num::{NonZeroU64, NonZeroUsize}; use zcash_primitives::{ consensus::{self, BlockHeight}, @@ -10,9 +11,11 @@ use zcash_primitives::{ }; use zcash_protocol::ShieldedProtocol; +use crate::data_api::WalletMeta; + use super::{ sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, - EphemeralBalance, TransactionBalance, + EphemeralBalance, SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -112,33 +115,26 @@ where } /// Decide which shielded pool change should go to if there is any. -pub(crate) fn single_change_output_policy( +pub(crate) fn select_change_pool( _net_flows: &NetFlows, _fallback_change_pool: ShieldedProtocol, -) -> (ShieldedProtocol, usize, usize) { +) -> ShieldedProtocol { // TODO: implement a less naive strategy for selecting the pool to which change will be sent. - let change_pool = { - #[cfg(feature = "orchard")] - 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 - } 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 - } else { - // The flows are transparent, so there may not be change. If there is, the caller - // gets to decide where to shield it. - _fallback_change_pool - } - #[cfg(not(feature = "orchard"))] + #[cfg(feature = "orchard")] + 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 + } 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 - }; - ( - change_pool, - (change_pool == ShieldedProtocol::Sapling).into(), - (change_pool == ShieldedProtocol::Orchard).into(), - ) + } else { + // The flows are transparent, so there may not be change. If there is, the caller + // gets to decide where to shield it. + _fallback_change_pool + } + #[cfg(not(feature = "orchard"))] + ShieldedProtocol::Sapling } #[derive(Clone, Copy, Debug)] @@ -162,6 +158,10 @@ impl OutputManifest { pub(crate) fn orchard(&self) -> usize { self.orchard } + + pub(crate) fn total_shielded(&self) -> usize { + self.sapling + self.orchard + } } pub(crate) struct SinglePoolBalanceConfig<'a, P, F> { @@ -169,17 +169,20 @@ pub(crate) struct SinglePoolBalanceConfig<'a, P, F> { fee_rule: &'a F, dust_output_policy: &'a DustOutputPolicy, default_dust_threshold: NonNegativeAmount, + split_policy: &'a SplitPolicy, fallback_change_pool: ShieldedProtocol, marginal_fee: NonNegativeAmount, grace_actions: usize, } impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { + #[allow(clippy::too_many_arguments)] pub(crate) fn new( params: &'a P, fee_rule: &'a F, dust_output_policy: &'a DustOutputPolicy, default_dust_threshold: NonNegativeAmount, + split_policy: &'a SplitPolicy, fallback_change_pool: ShieldedProtocol, marginal_fee: NonNegativeAmount, grace_actions: usize, @@ -189,6 +192,7 @@ impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { fee_rule, dust_output_policy, default_dust_threshold, + split_policy, fallback_change_pool, marginal_fee, grace_actions, @@ -197,13 +201,9 @@ impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { } #[allow(clippy::too_many_arguments)] -pub(crate) fn single_change_output_balance< - P: consensus::Parameters, - NoteRefT: Clone, - F: FeeRule, - E, ->( +pub(crate) fn single_pool_output_balance( cfg: SinglePoolBalanceConfig, + wallet_meta: Option<&WalletMeta>, target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], @@ -232,9 +232,32 @@ where ephemeral_balance, )?; - #[allow(unused_variables)] - let (change_pool, sapling_change, orchard_change) = - single_change_output_policy(&net_flows, cfg.fallback_change_pool); + let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool); + let target_change_counts = OutputManifest { + transparent: 0, + sapling: if change_pool == ShieldedProtocol::Sapling { + wallet_meta.map_or(1, |m| { + std::cmp::max( + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()), + 1, + ) + }) + } else { + 0 + }, + orchard: if change_pool == ShieldedProtocol::Orchard { + wallet_meta.map_or(1, |m| { + std::cmp::max( + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()), + 1, + ) + }) + } else { + 0 + }, + }; // We don't create a fully-transparent transaction if a change memo is used. let transparent = net_flows.is_transparent() && change_memo.is_none(); @@ -246,20 +269,17 @@ where // Is it certain that there will be a change output? If it is not certain, // we should call `check_for_uneconomic_inputs` with `possible_change` // including both possibilities. - let possible_change = + let possible_change = { // These are the situations where we might not have a change output. - if transparent || (cfg.dust_output_policy.action() == DustAction::AddDustToFee && change_memo.is_none()) { - vec![ - OutputManifest::ZERO, - OutputManifest { - transparent: 0, - sapling: sapling_change, - orchard: orchard_change - } - ] + if transparent + || (cfg.dust_output_policy.action() == DustAction::AddDustToFee + && change_memo.is_none()) + { + vec![OutputManifest::ZERO, target_change_counts] } else { - vec![OutputManifest { transparent: 0, sapling: sapling_change, orchard: orchard_change}] - }; + vec![target_change_counts] + } + }; check_for_uneconomic_inputs( transparent_inputs, @@ -285,35 +305,36 @@ where .bundle_type() .num_spends(sapling.inputs().len()) .map_err(ChangeError::BundleError)?; - let sapling_output_count = sapling - .bundle_type() - .num_outputs(sapling.inputs().len(), sapling.outputs().len()) - .map_err(ChangeError::BundleError)?; - let sapling_output_count_with_change = sapling - .bundle_type() - .num_outputs( - sapling.inputs().len(), - sapling.outputs().len() + sapling_change, - ) - .map_err(ChangeError::BundleError)?; + let sapling_output_count = |change_count| { + sapling + .bundle_type() + .num_outputs( + sapling.inputs().len(), + sapling.outputs().len() + change_count, + ) + .map_err(ChangeError::BundleError) + }; #[cfg(feature = "orchard")] - let orchard_action_count = orchard - .bundle_type() - .num_actions(orchard.inputs().len(), orchard.outputs().len()) - .map_err(ChangeError::BundleError)?; - #[cfg(feature = "orchard")] - let orchard_action_count_with_change = orchard - .bundle_type() - .num_actions( - orchard.inputs().len(), - orchard.outputs().len() + orchard_change, - ) - .map_err(ChangeError::BundleError)?; - #[cfg(not(feature = "orchard"))] - let orchard_action_count = 0; + let orchard_action_count = |change_count| { + orchard + .bundle_type() + .num_actions( + orchard.inputs().len(), + orchard.outputs().len() + change_count, + ) + .map_err(ChangeError::BundleError) + }; #[cfg(not(feature = "orchard"))] - let orchard_action_count_with_change = 0; + let orchard_action_count = |change_count: usize| -> Result> { + if change_count != 0 { + Err(ChangeError::BundleError( + "Nonzero Orchard change requested but the `orchard` feature is not enabled.", + )) + } else { + Ok(0) + } + }; // Once we calculate the balance with and without change, there are five cases: // @@ -365,8 +386,8 @@ where transparent_input_sizes.clone(), transparent_output_sizes.clone(), sapling_input_count, - sapling_output_count, - orchard_action_count, + sapling_output_count(0)?, + orchard_action_count(0)?, ) .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?; @@ -376,11 +397,11 @@ where .fee_required( cfg.params, target_height, - transparent_input_sizes, - transparent_output_sizes, + transparent_input_sizes.clone(), + transparent_output_sizes.clone(), sapling_input_count, - sapling_output_count_with_change, - orchard_action_count_with_change, + sapling_output_count(target_change_counts.sapling())?, + orchard_action_count(target_change_counts.orchard())?, ) .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?, ); @@ -410,13 +431,73 @@ where // Case 3b or 3c. let proposed_change = (total_in - total_out_plus_fee_with_change).expect("checked above"); + + // We obtain a split count based on the total number of notes of sufficient size + // available in the wallet, irrespective of pool. If we don't have any wallet metadata + // available, we fall back to generating a single change output. + let split_count = wallet_meta.map_or(NonZeroUsize::MIN, |wm| { + cfg.split_policy + .split_count(wm.total_note_count(), proposed_change) + }); + let per_output_change = proposed_change.div_with_remainder( + NonZeroU64::new( + u64::try_from(usize::from(split_count)).expect("usize fits into u64"), + ) + .unwrap(), + ); + + // If we don't have as many change outputs as we expected, recompute the fee. + let (fee_with_change, excess_fee) = + if usize::from(split_count) < target_change_counts.total_shielded() { + let new_fee_with_change = cfg + .fee_rule + .fee_required( + cfg.params, + target_height, + transparent_input_sizes, + transparent_output_sizes, + sapling_input_count, + sapling_output_count(if change_pool == ShieldedProtocol::Sapling { + usize::from(split_count) + } else { + 0 + })?, + orchard_action_count(if change_pool == ShieldedProtocol::Orchard { + usize::from(split_count) + } else { + 0 + })?, + ) + .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?; + ( + new_fee_with_change, + (fee_with_change - new_fee_with_change).unwrap_or(NonNegativeAmount::ZERO), + ) + } else { + (fee_with_change, NonNegativeAmount::ZERO) + }; + let simple_case = || { ( - vec![ChangeValue::shielded( - change_pool, - proposed_change, - change_memo.cloned(), - )], + (0usize..split_count.into()) + .map(|i| { + ChangeValue::shielded( + change_pool, + if i == 0 { + // Add any remainder to the first output only + (*per_output_change.quotient() + + *per_output_change.remainder() + + excess_fee) + .unwrap() + } else { + // For any other output, the change value will just be the + // quotient. + *per_output_change.quotient() + }, + change_memo.cloned(), + ) + }) + .collect(), fee_with_change, ) }; @@ -426,7 +507,7 @@ where .dust_threshold() .unwrap_or(cfg.default_dust_threshold); - if proposed_change < change_dust_threshold { + if per_output_change.quotient() < &change_dust_threshold { match cfg.dust_output_policy.action() { DustAction::Reject => { // Always allow zero-valued change even for the `Reject` policy: @@ -435,11 +516,11 @@ where // * this case occurs in practice when sending all funds from an account; // * zero-valued notes do not require witness tracking; // * the effect on trial decryption overhead is small. - if proposed_change.is_zero() { + if per_output_change.quotient().is_zero() { simple_case() } else { - let shortfall = - (change_dust_threshold - proposed_change).ok_or_else(underflow)?; + let shortfall = (change_dust_threshold - *per_output_change.quotient()) + .ok_or_else(underflow)?; return Err(ChangeError::InsufficientFunds { available: total_in, diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 2d2938cfff..311c806bb4 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -14,9 +14,9 @@ use zcash_primitives::{ use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ - common::{single_change_output_balance, SinglePoolBalanceConfig}, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - TransactionBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -86,18 +86,21 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { + let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, &self.dust_output_policy, self.fee_rule.fixed_fee(), + &split_policy, self.fallback_change_pool, NonNegativeAmount::ZERO, 0, ); - single_change_output_balance( + single_pool_output_balance( cfg, + None, target_height, transparent_inputs, transparent_outputs, diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 673dc7cf74..862e613029 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -15,12 +15,15 @@ use zcash_primitives::{ }, }; -use crate::{data_api::InputSource, ShieldedProtocol}; +use crate::{ + data_api::{InputSource, WalletMeta}, + ShieldedProtocol, +}; use super::{ - common::{single_change_output_balance, SinglePoolBalanceConfig}, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - TransactionBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -90,18 +93,118 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { + let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, &self.dust_output_policy, self.fee_rule.marginal_fee(), + &split_policy, self.fallback_change_pool, self.fee_rule.marginal_fee(), self.fee_rule.grace_actions(), ); - single_change_output_balance( + single_pool_output_balance( cfg, + None, + target_height, + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + self.change_memo.as_ref(), + ephemeral_balance, + ) + } +} + +/// A change strategy that attempts to split the change value into some number of equal-sized notes +/// as dictated by the included [`SplitPolicy`] value. +pub struct MultiOutputChangeStrategy { + fee_rule: Zip317FeeRule, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + meta_source: PhantomData, +} + +impl MultiOutputChangeStrategy { + /// Constructs a new [`MultiOutputChangeStrategy`] with the specified ZIP 317 + /// fee parameters, change memo, and change splitting policy. + /// + /// This change strategy will fall back to creating a single change output if insufficient + /// change value is available to create notes with at least the minimum value dictated by the + /// split policy. + /// + /// `fallback_change_pool`: the pool to which change will be sent if when more than one + /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. + /// `split_policy`: A policy value describing how the change value should be returned as + /// multiple notes. + pub fn new( + fee_rule: Zip317FeeRule, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + ) -> Self { + Self { + fee_rule, + change_memo, + fallback_change_pool, + dust_output_policy, + split_policy, + meta_source: PhantomData, + } + } +} + +impl ChangeStrategy for MultiOutputChangeStrategy { + type FeeRule = Zip317FeeRule; + type Error = Zip317FeeError; + type MetaSource = I; + type WalletMeta = WalletMeta; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn fetch_wallet_meta( + &self, + meta_source: &Self::MetaSource, + account: ::AccountId, + exclude: &[::NoteRef], + ) -> Result::Error> { + meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) + } + + fn compute_balance( + &self, + params: &P, + 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, + ephemeral_balance: Option<&EphemeralBalance>, + wallet_meta: Option<&Self::WalletMeta>, + ) -> Result> { + let cfg = SinglePoolBalanceConfig::new( + params, + &self.fee_rule, + &self.dust_output_policy, + self.fee_rule.marginal_fee(), + &self.split_policy, + self.fallback_change_pool, + self.fee_rule.marginal_fee(), + self.fee_rule.grace_actions(), + ); + + single_pool_output_balance( + cfg, + wallet_meta, target_height, transparent_inputs, transparent_outputs, @@ -116,7 +219,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(test)] mod tests { - use std::convert::Infallible; + use std::{convert::Infallible, num::NonZeroUsize}; use zcash_primitives::{ consensus::{Network, NetworkUpgrade, Parameters}, @@ -129,10 +232,11 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment}, + data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment, WalletMeta}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, - ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, + zip317::MultiOutputChangeStrategy, + ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, SplitPolicy, }, ShieldedProtocol, }; @@ -184,6 +288,119 @@ mod tests { ); } + #[test] + fn change_without_dust_multi() { + let change_strategy = MultiOutputChangeStrategy::::new( + Zip317FeeRule::standard(), + None, + ShieldedProtocol::Sapling, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(5).unwrap(), + NonNegativeAmount::const_from_u64(100_0000), + ), + ); + + { + // spend a single Sapling note and produce 5 outputs + let balance = |existing_notes| { + change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &[] as &[TestTransparentInput], + &[] as &[TxOut], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(750_0000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 100_0000, + ))][..], + ), + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + None, + Some(&WalletMeta::new( + existing_notes, + #[cfg(feature = "orchard")] + 0, + )), + ) + }; + + assert_matches!( + balance(0), + Ok(balance) if + balance.proposed_change() == [ + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(30000) + ); + + assert_matches!( + balance(2), + Ok(balance) if + balance.proposed_change() == [ + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(20000) + ); + } + + { + // spend a single Sapling note and produce 4 outputs, as the value of the note isn't + // sufficient to produce 5 + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &[] as &[TestTransparentInput], + &[] as &[TxOut], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(600_0000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 100_0000, + ))][..], + ), + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + None, + Some(&WalletMeta::new( + 0, + #[cfg(feature = "orchard")] + 0, + )), + ); + + assert_matches!( + result, + Ok(balance) if + balance.proposed_change() == [ + ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_7500), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_2500), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_2500), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_2500), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(25000) + ); + } + } + #[test] #[cfg(feature = "orchard")] fn cross_pool_change_without_dust() { diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 80edb6c5ef..35f3d9a064 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -36,6 +36,13 @@ pub(crate) fn send_single_step_proposed_transfer() { ) } +pub(crate) fn send_with_multiple_change_outputs() { + zcash_client_backend::data_api::testing::pool::send_with_multiple_change_outputs::( + TestDbFactory, + BlockCache::new(), + ) +} + #[cfg(feature = "transparent-inputs")] pub(crate) fn send_multi_step_proposed_transfer() { zcash_client_backend::data_api::testing::pool::send_multi_step_proposed_transfer::( diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index ec3b5f468b..890bfa7bbf 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -399,6 +399,11 @@ pub(crate) mod tests { testing::pool::send_single_step_proposed_transfer::() } + #[test] + fn send_with_multiple_change_outputs() { + testing::pool::send_with_multiple_change_outputs::() + } + #[test] #[cfg(feature = "transparent-inputs")] fn send_multi_step_proposed_transfer() { diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 67ed843d7c..27b4fde3ca 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -412,6 +412,11 @@ pub(crate) mod tests { testing::pool::send_single_step_proposed_transfer::() } + #[test] + fn send_with_multiple_change_outputs() { + testing::pool::send_with_multiple_change_outputs::() + } + #[test] #[cfg(feature = "transparent-inputs")] fn send_multi_step_proposed_transfer() { diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index dbd8f79346..0ac93fd217 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -1723,6 +1723,7 @@ mod tests { fn prop_usk_roundtrip(usk in arb_unified_spending_key(zcash_protocol::consensus::Network::MainNetwork)) { let encoded = usk.to_bytes(Era::Orchard); + #[allow(clippy::let_and_return)] let encoded_len = { let len = 4; From 5afea2e3b1aa65248496c2142d72b0bf3c3ea48c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sun, 20 Oct 2024 11:52:07 -0600 Subject: [PATCH 6/9] Apply suggestions from code review Co-authored-by: Jack Grigg Co-authored-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 38 +++++++++---------- zcash_client_backend/src/data_api.rs | 13 +++---- .../src/data_api/testing/pool.rs | 2 +- zcash_client_backend/src/data_api/wallet.rs | 7 ++++ zcash_client_backend/src/fees.rs | 12 +++--- zcash_client_backend/src/fees/common.rs | 21 ++++------ zcash_client_backend/src/fees/zip317.rs | 8 ++-- zcash_client_sqlite/src/lib.rs | 5 +++ zcash_client_sqlite/src/wallet/common.rs | 6 ++- 9 files changed, 57 insertions(+), 55 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c983d39f52..32f6cd71ae 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -24,12 +24,12 @@ and this library adheres to Rust's notion of `ChangeErrT` and `NoteRefT`. - The following methods each now take an additional `change_strategy` argument, along with an associated `ChangeT` type parameter: - - `zcash_client_backend::data_api::wallet::spend` - - `zcash_client_backend::data_api::wallet::propose_transfer` - - `zcash_client_backend::data_api::wallet::propose_shielding`. This method - also now takes an additional `to_account` argument. - - `zcash_client_backend::data_api::wallet::shield_transparent_funds`. This - method also now takes an additional `to_account` argument. + - `wallet::spend` + - `wallet::propose_transfer` + - `wallet::propose_shielding`. This method also now takes an additional + `to_account` argument. + - `wallet::shield_transparent_funds`. This method also now takes an + additional `to_account` argument. - `wallet::input_selection::InputSelectionError` now has an additional `Change` variant. This necessitates the addition of two type parameters. - `wallet::input_selection::InputSelector::propose_transaction` takes an @@ -49,20 +49,16 @@ and this library adheres to Rust's notion of has been removed, along with the additional type parameters it necessitated. - The arguments to `wallet::input_selection::GreedyInputSelector::new` have changed. -- `zcash_client_backend::fees::ChangeStrategy` has changed. It has two new - associated types, `MetaSource` and `WalletMeta`, and its `FeeRule` associated - type now has an additional `Clone` bound. In addition, it defines a new - `fetch_wallet_meta` method, and the arguments to `compute_balance` have - changed. -- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. -- `zcash_client_backend::fees::standard::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. -- `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. +- `zcash_client_backend::fees`: + - `ChangeStrategy` has changed. It has two new associated types, `MetaSource` + and `WalletMeta`, and its `FeeRule` associated type now has an additional + `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and + the arguments to `compute_balance` have changed. + - The following methods now take an additional `DustOutputPolicy` argument, + and carry an additional type parameter: + - `fixed::SingleOutputChangeStrategy::new` + - `standard::SingleOutputChangeStrategy::new` + - `zip317::SingleOutputChangeStrategy::new` ### Changed - Migrated to `arti-client 0.22`. @@ -71,6 +67,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have been removed. Use `WalletSummary::progress` instead. + - `testing::input_selector` use explicit `InputSelector` constructors + directly instead. - `zcash_client_backend::fees`: - `impl From for ChangeError<...>` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 03083a5cce..d56465413c 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -848,12 +848,7 @@ impl WalletMeta { /// Returns the total number of unspent shielded notes belonging to the account for which this /// was generated. pub fn total_note_count(&self) -> usize { - #[cfg(feature = "orchard")] - let orchard_note_count = self.orchard_note_count; - #[cfg(not(feature = "orchard"))] - let orchard_note_count = 0; - - self.sapling_note_count + orchard_note_count + self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard) } } @@ -903,8 +898,10 @@ pub trait InputSource { /// Returns metadata describing the structure of the wallet for the specified account. /// - /// The returned metadata value must exclude spent notes and unspent notes having value less - /// than the specified minimum value or identified in the given exclude list. + /// The returned metadata value must exclude: + /// - spent notes; + /// - unspent notes having value less than the specified minimum value; + /// - unspent notes identified in the given `exclude` list. fn get_wallet_metadata( &self, account: Self::AccountId, diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 98d6efd869..3bef99a040 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -422,7 +422,7 @@ pub fn send_with_multiple_change_outputs( .unwrap(); assert_eq!(sent_note_ids.len(), 3); - // The sent memo should be the empty memo for the sent output, and the + // The sent memo should be the empty memo for the sent output, and each // change output's memo should be as specified. let mut change_memo_count = 0; let mut found_sent_empty_memo = false; diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index b0276f97e7..6d9bf6f3a8 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -115,6 +115,8 @@ where Ok(()) } +/// Errors that may be generated in construction of proposals for shielded->shielded or +/// shielded->transparent transfers. pub type ProposeTransferErrT = Error< ::Error, CommitmentTreeErrT, @@ -124,6 +126,8 @@ pub type ProposeTransferErrT = Error< <::InputSource as InputSource>::NoteRef, >; +/// Errors that may be generated in construction of proposals for transparent->shielded +/// wallet-internal transfers. #[cfg(feature = "transparent-inputs")] pub type ProposeShieldingErrT = Error< ::Error, @@ -134,6 +138,7 @@ pub type ProposeShieldingErrT = Error Infallible, >; +/// Errors that may be generated in combined creation and execution of transaction proposals. pub type CreateErrT = Error< ::Error, ::Error, @@ -143,6 +148,7 @@ pub type CreateErrT = Error< N, >; +/// Errors that may be generated in the execution of proposals that may send shielded inputs. pub type TransferErrT = Error< ::Error, ::Error, @@ -152,6 +158,7 @@ pub type TransferErrT = Error< <::InputSource as InputSource>::NoteRef, >; +/// Errors that may be generated in the execution of shielding proposals. #[cfg(feature = "transparent-inputs")] pub type ShieldErrT = Error< ::Error, diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index af816d7984..921d4ea7a1 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -393,13 +393,13 @@ impl SplitPolicy { ) .unwrap(), ); - if split_count > NonZeroUsize::MIN - && *per_output_change.quotient() < self.min_split_output_size - { - split_count = NonZeroUsize::new(usize::from(split_count) - 1) - .expect("split_count checked to be > 1"); - } else { + if *per_output_change.quotient() >= self.min_split_output_size { return split_count; + } else if let Some(new_count) = NonZeroUsize::new(usize::from(split_count) - 1) { + split_count = new_count; + } else { + // We always create at least one change output. + return NonZeroUsize::MIN; } } } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 9c221bbd59..45b77e5dfa 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -233,27 +233,20 @@ where )?; let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool); + let target_change_count = wallet_meta.map_or(1, |m| { + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()) + .max(1) + }); let target_change_counts = OutputManifest { transparent: 0, sapling: if change_pool == ShieldedProtocol::Sapling { - wallet_meta.map_or(1, |m| { - std::cmp::max( - usize::from(cfg.split_policy.target_output_count) - .saturating_sub(m.total_note_count()), - 1, - ) - }) + target_change_count } else { 0 }, orchard: if change_pool == ShieldedProtocol::Orchard { - wallet_meta.map_or(1, |m| { - std::cmp::max( - usize::from(cfg.split_policy.target_output_count) - .saturating_sub(m.total_note_count()), - 1, - ) - }) + target_change_count } else { 0 }, diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 862e613029..8b05656b6e 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -139,10 +139,10 @@ impl MultiOutputChangeStrategy { /// change value is available to create notes with at least the minimum value dictated by the /// split policy. /// - /// `fallback_change_pool`: the pool to which change will be sent if when more than one - /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. - /// `split_policy`: A policy value describing how the change value should be returned as - /// multiple notes. + /// - `fallback_change_pool`: the pool to which change will be sent if when more than one + /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. + /// - `split_policy`: A policy value describing how the change value should be returned as + /// multiple notes. pub fn new( fee_rule: Zip317FeeRule, change_memo: Option, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index d8ab67b5e1..30e6f30542 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -354,12 +354,16 @@ impl, P: consensus::Parameters> InputSource for min_value: NonNegativeAmount, exclude: &[Self::NoteRef], ) -> Result { + let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())? + .ok_or(SqliteClientError::ChainHeightUnknown)?; + let sapling_note_count = count_outputs( self.conn.borrow(), account_id, min_value, exclude, ShieldedProtocol::Sapling, + chain_tip_height, )?; #[cfg(feature = "orchard")] @@ -369,6 +373,7 @@ impl, P: consensus::Parameters> InputSource for min_value, exclude, ShieldedProtocol::Orchard, + chain_tip_height, )?; Ok(WalletMeta::new( diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 707a759143..378abbe1a1 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -232,6 +232,7 @@ pub(crate) fn count_outputs( min_value: NonNegativeAmount, exclude: &[ReceivedNoteId], protocol: ShieldedProtocol, + chain_tip_height: BlockHeight, ) -> Result { let (table_prefix, _, _) = per_protocol_names(protocol); @@ -265,13 +266,14 @@ pub(crate) fn count_outputs( JOIN transactions stx ON stx.id_tx = transaction_id WHERE stx.block IS NOT NULL -- the spending tx is mined OR stx.expiry_height IS NULL -- the spending tx will not expire - OR stx.expiry_height > :anchor_height -- the spending tx is unexpired + OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired )" ), named_params![ ":account_id": account.0, ":min_value": u64::from(min_value), - ":exclude": &excluded_ptr + ":exclude": &excluded_ptr, + ":chain_tip_height": u32::from(chain_tip_height) ], |row| row.get(0), ) From f58988a315e81af7428327a3dcdf88a3e9e68b97 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sun, 20 Oct 2024 22:43:19 +0100 Subject: [PATCH 7/9] Remove `fixed::FeeRule::standard` (which was misleadingly named because fixed fees are not standard), and deprecate `fixed::FeeRule::non_standard`. Signed-off-by: Daira-Emma Hopwood --- .../src/data_api/testing/pool.rs | 8 ++++-- .../src/data_api/testing/transparent.rs | 1 + zcash_client_backend/src/fees/fixed.rs | 8 +++--- .../init/migrations/receiving_key_scopes.rs | 1 + zcash_extensions/src/transparent/demo.rs | 4 +-- zcash_primitives/CHANGELOG.md | 14 +++++++++++ zcash_primitives/src/transaction/builder.rs | 6 ++--- .../src/transaction/fees/fixed.rs | 25 ++++--------------- 8 files changed, 36 insertions(+), 31 deletions(-) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 3bef99a040..70d1af0145 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -17,7 +17,11 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ components::amount::NonNegativeAmount, - fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule, StandardFeeRule}, + fees::{ + fixed::FeeRule as FixedFeeRule, + zip317::{FeeRule as Zip317FeeRule, MINIMUM_FEE}, + StandardFeeRule, + }, Transaction, }, }; @@ -1582,7 +1586,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed::new( fee_rule, None, @@ -175,14 +175,14 @@ mod tests { result, Ok(balance) if balance.proposed_change() == [ChangeValue::sapling(NonNegativeAmount::const_from_u64(10000), None)] && - balance.fee_required() == NonNegativeAmount::const_from_u64(10000) + balance.fee_required() == MINIMUM_FEE ); } #[test] fn dust_change() { #[allow(deprecated)] - let fee_rule = FixedFeeRule::standard(); + let fee_rule = FixedFeeRule::non_standard(MINIMUM_FEE); let change_strategy = SingleOutputChangeStrategy::::new( fee_rule, None, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index 987f50f6a9..cfae9d438d 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -405,6 +405,7 @@ mod tests { OsRng, &prover, &prover, + #[allow(deprecated)] &fixed::FeeRule::non_standard(NonNegativeAmount::ZERO), ) .unwrap(); diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 597bda6a5b..42b29d2b4b 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -493,7 +493,7 @@ mod tests { amount::{Amount, NonNegativeAmount}, tze::{Authorized, Bundle, OutPoint, TzeIn, TzeOut}, }, - fees::fixed, + fees::{fixed, zip317::MINIMUM_FEE}, Transaction, TransactionData, TxVersion, }, }; @@ -794,7 +794,7 @@ mod tests { // FIXME: implement zcash_primitives::transaction::fees::FutureFeeRule for zip317::FeeRule. #[allow(deprecated)] - let fee_rule = fixed::FeeRule::standard(); + let fee_rule = fixed::FeeRule::non_standard(MINIMUM_FEE); // create some inputs to spend let extsk = ExtendedSpendingKey::master(&[]); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index d746e2b88d..500305d2cb 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -7,6 +7,20 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Deprecated +- `zcash_primitives::transaction::fees`: + - `fixed::FeeRule::non_standard`. Using a fixed fee may result in a transaction + that cannot be mined on the current Zcash network. To calculate the ZIP 317 fee, + use `zip317::FeeRule::standard()`. + +### Removed +- `zcash_primitives::transaction::fees`: + - `fixed::FeeRule::standard`. This constructor was misleadingly named: using a + fixed fee does not conform to any current Zcash standard. To calculate the + ZIP 317 fee, use `zip317::FeeRule::standard()`. To preserve the current + (deprecated) behaviour, use `fixed::FeeRule::non_standard(zip317::MINIMUM_FEE)`, + but note that this is likely to result in transactions that cannot be mined. + ## [0.19.0] - 2024-10-02 ### Changed diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index b40655dc55..ffa374d538 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -883,7 +883,7 @@ mod testing { self, prover::mock::{MockOutputProver, MockSpendProver}, }, - transaction::fees::fixed, + transaction::fees::{fixed, zip317::MINIMUM_FEE}, }; impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder<'a, P, U> { @@ -910,12 +910,12 @@ mod testing { } } - #[allow(deprecated)] self.build( FakeCryptoRng(rng), &MockSpendProver, &MockOutputProver, - &fixed::FeeRule::standard(), + #[allow(deprecated)] + &fixed::FeeRule::non_standard(MINIMUM_FEE), ) } } diff --git a/zcash_primitives/src/transaction/fees/fixed.rs b/zcash_primitives/src/transaction/fees/fixed.rs index 82e8301ee0..de3206fbee 100644 --- a/zcash_primitives/src/transaction/fees/fixed.rs +++ b/zcash_primitives/src/transaction/fees/fixed.rs @@ -1,7 +1,7 @@ use crate::{ consensus::{self, BlockHeight}, transaction::components::amount::NonNegativeAmount, - transaction::fees::{transparent, zip317}, + transaction::fees::transparent, }; #[cfg(zcash_unstable = "zfuture")] @@ -16,27 +16,12 @@ pub struct FeeRule { impl FeeRule { /// Creates a new nonstandard fixed fee rule with the specified fixed fee. - pub fn non_standard(fixed_fee: NonNegativeAmount) -> Self { - Self { fixed_fee } - } - - /// Creates a new fixed fee rule with the minimum possible [ZIP 317] fee, - /// i.e. 10000 zatoshis. - /// - /// Note that using a fixed fee is not compliant with [ZIP 317]; consider - /// using [`zcash_primitives::transaction::fees::zip317::FeeRule::standard()`] - /// instead. - /// - /// [`zcash_primitives::transaction::fees::zip317::FeeRule::standard()`]: crate::transaction::fees::zip317::FeeRule::standard - /// [ZIP 317]: https://zips.z.cash/zip-0317 #[deprecated( - since = "0.12.0", - note = "To calculate the ZIP 317 fee, use `transaction::fees::zip317::FeeRule::standard()`. For a fixed fee, use the `non_standard` constructor." + note = "Using a fixed fee may result in a transaction that cannot be mined. \ + To calculate the ZIP 317 fee, use `transaction::fees::zip317::FeeRule::standard()`." )] - pub fn standard() -> Self { - Self { - fixed_fee: zip317::MINIMUM_FEE, - } + pub fn non_standard(fixed_fee: NonNegativeAmount) -> Self { + Self { fixed_fee } } /// Returns the fixed fee amount which this rule was configured. From 4ecd8308f04b90b9ddbc1a2237ca6999ea448ed4 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sun, 20 Oct 2024 14:25:10 +0100 Subject: [PATCH 8/9] Remove `zcash_primitives::transaction::fees::StandardFeeRule::{PreZip313,Zip313}`. Rename `zcash_client_backend::proto::ProposalDecodingError::FeeRuleNotSpecified` to `FeeRuleNotSupported` and use it to report attempted use of the removed rules. Signed-off-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 4 ++ .../src/data_api/testing/pool.rs | 2 +- zcash_client_backend/src/data_api/wallet.rs | 2 +- zcash_client_backend/src/fees.rs | 27 ---------- zcash_client_backend/src/fees/standard.rs | 51 ++----------------- zcash_client_backend/src/proto.rs | 19 +++---- zcash_primitives/CHANGELOG.md | 2 + zcash_primitives/src/transaction/fees.rs | 11 ---- 8 files changed, 20 insertions(+), 98 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 32f6cd71ae..8f6b8a7672 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -59,6 +59,10 @@ and this library adheres to Rust's notion of - `fixed::SingleOutputChangeStrategy::new` - `standard::SingleOutputChangeStrategy::new` - `zip317::SingleOutputChangeStrategy::new` +- `zcash_client_backend::proto`: + - The `FeeRuleNotSpecified` variant of `FeeProposalDecodingError` has been + renamed to `FeeRuleNotSupported`. It is now used to report attempted use + of unsupported fee rules, as well as of an unspecified fee rule. ### Changed - Migrated to `arti-client 0.22`. diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 70d1af0145..3007ecf21b 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -1018,7 +1018,7 @@ where assert_matches!( st.propose_standard_transfer::( account_id, - StandardFeeRule::PreZip313, + StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), &to, NonNegativeAmount::const_from_u64(1), diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 6d9bf6f3a8..2133c507eb 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -323,7 +323,7 @@ where let proposal = propose_standard_transfer_to_address( wallet_db, params, - StandardFeeRule::PreZip313, + StandardFeeRule::Zip317, account.id(), min_confirmations, to, diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 921d4ea7a1..bf005d9396 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -203,33 +203,6 @@ pub enum ChangeError { BundleError(&'static str), } -impl ChangeError { - pub(crate) fn map E0>(self, f: F) -> ChangeError { - match self { - ChangeError::InsufficientFunds { - available, - required, - } => ChangeError::InsufficientFunds { - available, - required, - }, - 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), - } - } -} - impl fmt::Display for ChangeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index dbaf8ace1b..cbc7440e91 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -5,21 +5,17 @@ use std::marker::PhantomData; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, - transaction::{ - components::amount::NonNegativeAmount, - fees::{ - fixed::FeeRule as FixedFeeRule, - transparent, - zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, - StandardFeeRule, - }, + transaction::fees::{ + transparent, + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + StandardFeeRule, }, }; use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ - fixed, sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, + sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, TransactionBalance, }; @@ -90,44 +86,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - #[allow(deprecated)] match self.fee_rule() { - StandardFeeRule::PreZip313 => fixed::SingleOutputChangeStrategy::::new( - FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(10000)), - self.change_memo.clone(), - self.fallback_change_pool, - self.dust_output_policy, - ) - .compute_balance( - params, - target_height, - transparent_inputs, - transparent_outputs, - sapling, - #[cfg(feature = "orchard")] - orchard, - ephemeral_balance, - wallet_meta, - ) - .map_err(|e| e.map(Zip317FeeError::Balance)), - StandardFeeRule::Zip313 => fixed::SingleOutputChangeStrategy::::new( - FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(1000)), - self.change_memo.clone(), - self.fallback_change_pool, - self.dust_output_policy, - ) - .compute_balance( - params, - target_height, - transparent_inputs, - transparent_outputs, - sapling, - #[cfg(feature = "orchard")] - orchard, - ephemeral_balance, - wallet_meta, - ) - .map_err(|e| e.map(Zip317FeeError::Balance)), StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::::new( Zip317FeeRule::standard(), self.change_memo.clone(), diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index c832fb44f6..47f51ec3d9 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -356,8 +356,8 @@ pub enum ProposalDecodingError { MemoInvalid(memo::Error), /// The serialization version returned by the protobuf was not recognized. VersionInvalid(u32), - /// The proposal did not correctly specify a standard fee rule. - FeeRuleNotSpecified, + /// The proposal specified a fee rule that is not supported, or did not specify a fee rule. + FeeRuleNotSupported(String), /// The proposal violated balance or structural constraints. ProposalInvalid(ProposalError), /// An inputs field for the given protocol was present, but contained no input note references. @@ -409,8 +409,8 @@ impl Display for ProposalDecodingError { ProposalDecodingError::VersionInvalid(v) => { write!(f, "Unrecognized proposal version {}", v) } - ProposalDecodingError::FeeRuleNotSpecified => { - write!(f, "Proposal did not specify a known fee rule.") + ProposalDecodingError::FeeRuleNotSupported(rule) => { + write!(f, "Proposal fee rule '{}' is not supported", rule) } ProposalDecodingError::ProposalInvalid(err) => write!(f, "{}", err), ProposalDecodingError::EmptyShieldedInputs(protocol) => write!( @@ -596,12 +596,9 @@ impl proposal::Proposal { }) .collect(); - #[allow(deprecated)] proposal::Proposal { proto_version: PROPOSAL_SER_V1, fee_rule: match value.fee_rule() { - StandardFeeRule::PreZip313 => proposal::FeeRule::PreZip313, - StandardFeeRule::Zip313 => proposal::FeeRule::Zip313, StandardFeeRule::Zip317 => proposal::FeeRule::Zip317, } .into(), @@ -622,13 +619,11 @@ impl proposal::Proposal { use self::proposal::proposed_input::Value::*; match self.proto_version { PROPOSAL_SER_V1 => { - #[allow(deprecated)] let fee_rule = match self.fee_rule() { - proposal::FeeRule::PreZip313 => StandardFeeRule::PreZip313, - proposal::FeeRule::Zip313 => StandardFeeRule::Zip313, proposal::FeeRule::Zip317 => StandardFeeRule::Zip317, - proposal::FeeRule::NotSpecified => { - return Err(ProposalDecodingError::FeeRuleNotSpecified); + _ => { + let rule = format!("{:?}", self.fee_rule()); + return Err(ProposalDecodingError::FeeRuleNotSupported(rule)); } }; diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 500305d2cb..9772baf67b 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -20,6 +20,8 @@ and this library adheres to Rust's notion of ZIP 317 fee, use `zip317::FeeRule::standard()`. To preserve the current (deprecated) behaviour, use `fixed::FeeRule::non_standard(zip317::MINIMUM_FEE)`, but note that this is likely to result in transactions that cannot be mined. + - `StandardFeeRule::{PreZip313,Zip313}`. These also are likely to result in + transactions that cannot be mined. ## [0.19.0] - 2024-10-02 diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index ae996de6ce..e6b73ce744 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -62,14 +62,6 @@ pub trait FutureFeeRule: FeeRule { /// An enumeration of the standard fee rules supported by the wallet. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum StandardFeeRule { - #[deprecated( - note = "Using this fee rule violates ZIP 317, and might cause transactions built with it to fail. Use `StandardFeeRule::Zip317` instead." - )] - PreZip313, - #[deprecated( - note = "Using this fee rule violates ZIP 317, and might cause transactions built with it to fail. Use `StandardFeeRule::Zip317` instead." - )] - Zip313, Zip317, } @@ -86,10 +78,7 @@ impl FeeRule for StandardFeeRule { sapling_output_count: usize, orchard_action_count: usize, ) -> Result { - #[allow(deprecated)] match self { - Self::PreZip313 => Ok(zip317::MINIMUM_FEE), - Self::Zip313 => Ok(NonNegativeAmount::const_from_u64(1000)), Self::Zip317 => zip317::FeeRule::standard().fee_required( params, target_height, From e5ea08fcd568788a9b6423884f05874379ef7441 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Mon, 21 Oct 2024 00:29:05 +0100 Subject: [PATCH 9/9] Refactor change strategies so that we can use a `CommonChangeStrategy` that is generic in the fee rule. This should simplify future generalizations of the change strategy. We also undo the changes to `{fixed,standard,zip317}::SingleOutputChangeStrategy` in this commit, and deprecate those types' `new` constructors. Signed-off-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 20 +- zcash_client_backend/src/data_api.rs | 2 +- zcash_client_backend/src/data_api/testing.rs | 31 +-- .../src/data_api/testing/pool.rs | 44 ++-- .../src/data_api/testing/transparent.rs | 10 +- zcash_client_backend/src/data_api/wallet.rs | 15 +- .../src/data_api/wallet/input_selection.rs | 6 +- zcash_client_backend/src/fees.rs | 58 +++++- zcash_client_backend/src/fees/common.rs | 126 ++++++++++- zcash_client_backend/src/fees/fixed.rs | 72 ++----- zcash_client_backend/src/fees/standard.rs | 83 +++----- zcash_client_backend/src/fees/zip317.rs | 196 +++--------------- zcash_client_sqlite/src/lib.rs | 6 +- zcash_client_sqlite/src/wallet/scanning.rs | 11 +- zcash_primitives/CHANGELOG.md | 5 + zcash_primitives/src/transaction/fees.rs | 26 +++ .../src/transaction/fees/fixed.rs | 19 +- .../src/transaction/fees/zip317.rs | 23 +- 18 files changed, 373 insertions(+), 380 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 8f6b8a7672..00d50874dc 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -13,8 +13,10 @@ and this library adheres to Rust's notion of - `WalletSummary::progress` - `WalletMeta` - `impl Default for wallet::input_selection::GreedyInputSelector` -- `zcash_client_backend::fees::SplitPolicy` -- `zcash_client_backend::fees::zip317::MultiOutputChangeStrategy` +- `zcash_client_backend::fees::{SplitPolicy, CommonChangeStrategy}`. These + allow specifying a policy for splitting change into more than one output, + which can help to make enough notes available to perform multiple spends + without waiting for change. ### Changed - `zcash_client_backend::data_api`: @@ -54,11 +56,6 @@ and this library adheres to Rust's notion of and `WalletMeta`, and its `FeeRule` associated type now has an additional `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and the arguments to `compute_balance` have changed. - - The following methods now take an additional `DustOutputPolicy` argument, - and carry an additional type parameter: - - `fixed::SingleOutputChangeStrategy::new` - - `standard::SingleOutputChangeStrategy::new` - - `zip317::SingleOutputChangeStrategy::new` - `zcash_client_backend::proto`: - The `FeeRuleNotSpecified` variant of `FeeProposalDecodingError` has been renamed to `FeeRuleNotSupported`. It is now used to report attempted use @@ -67,12 +64,19 @@ and this library adheres to Rust's notion of ### Changed - Migrated to `arti-client 0.22`. +### Deprecated +- `zcash_client_backend::fees::{fixed,standard,zip317}::SingleOutputChangeStrategy::new` + have been deprecated: instead use `CommonChangeStrategy::simple`. + (For most uses, the type parameter of the fee rule will be inferred.) + ### Removed - `zcash_client_backend::data_api`: - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have been removed. Use `WalletSummary::progress` instead. - - `testing::input_selector` use explicit `InputSelector` constructors + - `testing::input_selector`. Use explicit `InputSelector` constructors directly instead. + - `testing::single_output_change_strategy`. Use `fees::CommonChangeStrategy::simple` + instead. - `zcash_client_backend::fees`: - `impl From for ChangeError<...>` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index d56465413c..6afab96b27 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -907,7 +907,7 @@ pub trait InputSource { account: Self::AccountId, min_value: NonNegativeAmount, exclude: &[Self::NoteRef], - ) -> Result; + ) -> Result, Self::Error>; /// Fetches the transparent output corresponding to the provided `outpoint`. /// diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 9e99355034..3db14bcab3 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -46,10 +46,7 @@ use zip32::{fingerprint::SeedFingerprint, DiversifierIndex}; use crate::{ address::UnifiedAddress, - fees::{ - standard::{self, SingleOutputChangeStrategy}, - ChangeStrategy, DustOutputPolicy, - }, + fees::{standard::SingleOutputChangeStrategy, ChangeStrategy}, keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey}, proposal::Proposal, proto::compact_formats::{ @@ -60,13 +57,15 @@ use crate::{ }; #[allow(deprecated)] +use super::wallet::{create_spend_to_address, spend}; + use super::{ chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary}, scanning::ScanRange, wallet::{ - create_proposed_transactions, create_spend_to_address, + create_proposed_transactions, input_selection::{GreedyInputSelector, InputSelector}, - propose_standard_transfer_to_address, propose_transfer, spend, + propose_standard_transfer_to_address, propose_transfer, }, Account, AccountBalance, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, @@ -877,7 +876,7 @@ where fallback_change_pool: ShieldedProtocol, ) -> Result< NonEmpty, - super::wallet::TransferErrT, SingleOutputChangeStrategy>, + super::wallet::TransferErrT, SingleOutputChangeStrategy>, > { let prover = LocalTxProver::bundled(); let network = self.network().clone(); @@ -977,7 +976,7 @@ where DbT, CommitmentTreeErrT, GreedyInputSelector, - SingleOutputChangeStrategy, + SingleOutputChangeStrategy, >, > { let network = self.network().clone(); @@ -1221,20 +1220,6 @@ impl TestState { // } } -pub fn single_output_change_strategy( - fee_rule: StandardFeeRule, - change_memo: Option<&str>, - fallback_change_pool: ShieldedProtocol, -) -> standard::SingleOutputChangeStrategy { - let change_memo = change_memo.map(|m| MemoBytes::from(m.parse::().unwrap())); - standard::SingleOutputChangeStrategy::new( - fee_rule, - change_memo, - fallback_change_pool, - DustOutputPolicy::default(), - ) -} - // Checks that a protobuf proposal serialized from the provided proposal value correctly parses to // the same proposal value. fn check_proposal_serialization_roundtrip( @@ -2379,7 +2364,7 @@ impl InputSource for MockWalletDb { _account: Self::AccountId, _min_value: NonNegativeAmount, _exclude: &[Self::NoteRef], - ) -> Result { + ) -> Result, Self::Error> { Err(()) } } diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 3007ecf21b..8f57d9f5f0 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -40,10 +40,7 @@ use crate::{ self, chain::{self, ChainState, CommitmentTreeRoot, ScanSummary}, error::Error, - testing::{ - single_output_change_strategy, AddressType, FakeCompactOutput, InitialChainState, - TestBuilder, - }, + testing::{AddressType, FakeCompactOutput, InitialChainState, TestBuilder}, wallet::{ decrypt_and_store_transaction, input_selection::GreedyInputSelector, TransferErrT, }, @@ -52,9 +49,7 @@ use crate::{ }, decrypt_transaction, fees::{ - self, - standard::{self, SingleOutputChangeStrategy}, - DustOutputPolicy, SplitPolicy, + standard::SingleOutputChangeStrategy, CommonChangeStrategy, DustOutputPolicy, SplitPolicy, }, scanning::ScanError, wallet::{Note, NoteId, OvkPolicy, ReceivedNote}, @@ -217,14 +212,11 @@ pub fn send_single_step_proposed_transfer( )]) .unwrap(); - let fee_rule = StandardFeeRule::Zip317; - let change_memo = "Test change memo".parse::().unwrap(); - let change_strategy = standard::SingleOutputChangeStrategy::new( - fee_rule, + let change_strategy = CommonChangeStrategy::simple( + StandardFeeRule::Zip317, Some(change_memo.clone().into()), T::SHIELDED_PROTOCOL, - DustOutputPolicy::default(), ); let input_selector = GreedyInputSelector::new(); @@ -361,7 +353,7 @@ pub fn send_with_multiple_change_outputs( let input_selector = GreedyInputSelector::new(); let change_memo = "Test change memo".parse::().unwrap(); - let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + let change_strategy = CommonChangeStrategy::new( Zip317FeeRule::standard(), Some(change_memo.clone().into()), T::SHIELDED_PROTOCOL, @@ -464,7 +456,7 @@ pub fn send_with_multiple_change_outputs( // Now, create another proposal with more outputs requested. We have two change notes; // we'll spend one of them, and then we'll generate 7 splits. - let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + let change_strategy = CommonChangeStrategy::new( Zip317FeeRule::standard(), Some(change_memo.into()), T::SHIELDED_PROTOCOL, @@ -1377,7 +1369,7 @@ pub fn ovk_policy_prevents_recovery_from_chain( TransferErrT< DSF::DataStore, GreedyInputSelector, - SingleOutputChangeStrategy, + SingleOutputChangeStrategy, >, > { let min_confirmations = NonZeroU32::new(1).unwrap(); @@ -1587,12 +1579,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed( let input_selector = GreedyInputSelector::::new(); let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); // This first request will fail due to insufficient non-dust funds let req = TransactionRequest::new(vec![Payment::without_memo( @@ -1780,7 +1767,7 @@ where let input_selector = GreedyInputSelector::new(); let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); let txids = st .shield_transparent_funds( @@ -2027,7 +2014,7 @@ pub fn pool_crossing_required( let input_selector = GreedyInputSelector::new(); let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); let proposal0 = st .propose_transfer( account.id(), @@ -2119,7 +2106,7 @@ pub fn fully_funded_fully_private( // We set the default change output pool to P0, because we want to verify later that // change is actually sent to P1 (as the transaction is fully fundable from P1). let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL); let proposal0 = st .propose_transfer( account.id(), @@ -2307,7 +2294,7 @@ pub fn multi_pool_checkpoint( // Set up the fee rule and input selector we'll use for all the transfers. let input_selector = GreedyInputSelector::new(); let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL); // First, send funds just to P0 let transfer_amount = NonNegativeAmount::const_from_u64(200000); @@ -2791,10 +2778,9 @@ pub fn scan_cached_blocks_allows_blocks_out_of_order( )]) .unwrap(); - #[allow(deprecated)] let input_selector = GreedyInputSelector::new(); let change_strategy = - single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); assert_matches!( st.spend( diff --git a/zcash_client_backend/src/data_api/testing/transparent.rs b/zcash_client_backend/src/data_api/testing/transparent.rs index 0c58068d7d..c46e5c6a5c 100644 --- a/zcash_client_backend/src/data_api/testing/transparent.rs +++ b/zcash_client_backend/src/data_api/testing/transparent.rs @@ -1,11 +1,12 @@ use crate::{ data_api::{ - testing::{AddressType, TestBuilder, TestState}, - testing::{DataStoreFactory, ShieldedProtocol, TestCache}, + testing::{ + AddressType, DataStoreFactory, ShieldedProtocol, TestBuilder, TestCache, TestState, + }, wallet::input_selection::GreedyInputSelector, Account as _, InputSource, WalletRead, WalletWrite, }, - fees::{fixed, DustOutputPolicy}, + fees::CommonChangeStrategy, wallet::WalletTransparentOutput, }; use assert_matches::assert_matches; @@ -198,12 +199,11 @@ where // Shield the output. let input_selector = GreedyInputSelector::new(); - let change_strategy = fixed::SingleOutputChangeStrategy::new( + let change_strategy = CommonChangeStrategy::simple( #[allow(deprecated)] FixedFeeRule::non_standard(NonNegativeAmount::ZERO), None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); let txid = st .shield_transparent_funds( diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 2133c507eb..2d2240eae2 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -50,7 +50,10 @@ use crate::{ WalletRead, WalletWrite, }, decrypt_transaction, - fees::{standard::SingleOutputChangeStrategy, ChangeStrategy, DustOutputPolicy}, + fees::{ + standard::SingleOutputChangeStrategy, ChangeStrategy, CommonChangeStrategy, + DustOutputPolicy, SplitPolicy, + }, keys::UnifiedSpendingKey, proposal::{Proposal, ProposalError, Step, StepOutputIndex}, wallet::{Note, OvkPolicy, Recipient}, @@ -301,10 +304,7 @@ pub fn create_spend_to_address( min_confirmations: NonZeroU32, change_memo: Option, fallback_change_pool: ShieldedProtocol, -) -> Result< - NonEmpty, - TransferErrT, SingleOutputChangeStrategy>, -> +) -> Result, TransferErrT, SingleOutputChangeStrategy>> where ParamsT: consensus::Parameters + Clone, DbT: InputSource, @@ -533,7 +533,7 @@ pub fn propose_standard_transfer_to_address( DbT, CommitmentTreeErrT, GreedyInputSelector, - SingleOutputChangeStrategy, + SingleOutputChangeStrategy, >, > where @@ -559,11 +559,12 @@ where ); let input_selector = GreedyInputSelector::::new(); - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::new( fee_rule, change_memo, fallback_change_pool, DustOutputPolicy::default(), + SplitPolicy::single_output(), // TODO: consider changing ); propose_transfer( 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 5125059bcb..b012a6fe93 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -617,7 +617,7 @@ impl InputSelector for GreedyInputSelector { &orchard_outputs[..], ), ephemeral_balance.as_ref(), - Some(&wallet_meta), + wallet_meta.as_ref(), ); match balance { @@ -819,7 +819,7 @@ impl ShieldingSelector for GreedyInputSelector { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&wallet_meta), + wallet_meta.as_ref(), ); let balance = match trial_balance { @@ -837,7 +837,7 @@ impl ShieldingSelector for GreedyInputSelector { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&wallet_meta), + wallet_meta.as_ref(), )? } Err(other) => return Err(InputSelectorError::Change(other)), diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index bf005d9396..1220aa1a68 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -9,13 +9,19 @@ use zcash_primitives::{ transaction::{ components::{amount::NonNegativeAmount, OutPoint}, fees::{transparent, FeeRule}, + TxId, }, }; use zcash_protocol::{PoolType, ShieldedProtocol}; -use crate::data_api::InputSource; +use crate::{ + data_api::{InputSource, SpendableNotes, WalletMeta}, + wallet::{Note, ReceivedNote}, +}; pub(crate) mod common; +pub use common::CommonChangeStrategy; + pub mod fixed; #[cfg(feature = "orchard")] pub mod orchard; @@ -432,13 +438,16 @@ pub trait ChangeStrategy { fn fee_rule(&self) -> &Self::FeeRule; /// Uses the provided metadata source to obtain the wallet metadata required for change - /// creation determinations. + /// creation determinations. If no wallet metadata is needed for this change strategy, + /// it is sufficient to use the default implementation which returns `Ok(None)`. fn fetch_wallet_meta( &self, - meta_source: &Self::MetaSource, - account: ::AccountId, - exclude: &[::NoteRef], - ) -> Result::Error>; + _meta_source: &Self::MetaSource, + _account: ::AccountId, + _exclude: &[::NoteRef], + ) -> Result, ::Error> { + Ok(None) + } /// Computes the totals of inputs, suggested change amounts, and fees given the /// provided inputs and outputs being used to construct a transaction. @@ -479,6 +488,43 @@ pub trait ChangeStrategy { ) -> Result>; } +pub struct DummyMetaSource; + +impl InputSource for DummyMetaSource { + type Error = (); + type AccountId = (); + type NoteRef = (); + + fn get_spendable_note( + &self, + _txid: &TxId, + _protocol: ShieldedProtocol, + _index: u32, + ) -> Result>, Self::Error> { + Err(()) + } + + fn select_spendable_notes( + &self, + _account: Self::AccountId, + _target_value: NonNegativeAmount, + _sources: &[ShieldedProtocol], + _anchor_height: BlockHeight, + _exclude: &[Self::NoteRef], + ) -> Result, Self::Error> { + Err(()) + } + + fn get_wallet_metadata( + &self, + _account: Self::AccountId, + _min_value: NonNegativeAmount, + _exclude: &[Self::NoteRef], + ) -> Result, Self::Error> { + Err(()) + } +} + #[cfg(test)] pub(crate) mod tests { use zcash_primitives::transaction::{ diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 45b77e5dfa..5b31b92075 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -1,5 +1,8 @@ use core::cmp::{max, min}; -use std::num::{NonZeroU64, NonZeroUsize}; +use std::{ + marker::PhantomData, + num::{NonZeroU64, NonZeroUsize}, +}; use zcash_primitives::{ consensus::{self, BlockHeight}, @@ -11,16 +14,131 @@ use zcash_primitives::{ }; use zcash_protocol::ShieldedProtocol; -use crate::data_api::WalletMeta; +use crate::data_api::{InputSource, WalletMeta}; use super::{ - sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, - EphemeralBalance, SplitPolicy, TransactionBalance, + sapling as sapling_fees, ChangeError, ChangeStrategy, ChangeValue, DustAction, + DustOutputPolicy, EphemeralBalance, SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] use super::orchard as orchard_fees; +/// A change strategy that attempts to split the change value into some number of equal-sized notes +/// as dictated by the included [`SplitPolicy`] value. +pub struct CommonChangeStrategy { + fee_rule: FR, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + _meta_source: PhantomData, +} + +impl CommonChangeStrategy { + /// Constructs a new [`CommonChangeStrategy`] with the specified fee rule, change memo, + /// dust output policy, and change splitting policy. + /// + /// This change strategy will fall back to creating a single change output if insufficient + /// change value is available to create notes with at least the minimum value dictated by the + /// split policy. + /// + /// * `fallback_change_pool`: the pool to which change will be sent if when more than one + /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. + /// * `split_policy`: A policy value describing how the change value should be returned as + /// multiple notes. + pub fn new( + fee_rule: FR, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + ) -> Self { + Self { + fee_rule, + change_memo, + fallback_change_pool, + dust_output_policy, + split_policy, + _meta_source: PhantomData, + } + } + + /// Constructs a new [`CommonChangeStrategy`] with the specified fee rule, change memo, + /// and a single change output. This is equivalent to [`CommonChangeStrategy::new`] with + /// `dust_output_policy == DustOutputPolicy::default()` and + /// `split_policy == SplitPolicy::single_output()`. + pub fn simple( + fee_rule: FR, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + ) -> Self { + Self::new( + fee_rule, + change_memo, + fallback_change_pool, + DustOutputPolicy::default(), + SplitPolicy::single_output(), + ) + } +} + +impl ChangeStrategy for CommonChangeStrategy { + type FeeRule = FR; + type Error = FR::FeeRuleOrBalanceError; + type MetaSource = I; + type WalletMeta = WalletMeta; + + fn fee_rule(&self) -> &FR { + &self.fee_rule + } + + fn fetch_wallet_meta( + &self, + meta_source: &Self::MetaSource, + account: ::AccountId, + exclude: &[::NoteRef], + ) -> Result, ::Error> { + meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) + } + + fn compute_balance( + &self, + params: &P, + 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, + ephemeral_balance: Option<&EphemeralBalance>, + wallet_meta: Option<&Self::WalletMeta>, + ) -> Result> { + let cfg = SinglePoolBalanceConfig::new( + params, + &self.fee_rule, + &self.dust_output_policy, + self.fee_rule.default_dust_threshold(), + &self.split_policy, + self.fallback_change_pool, + self.fee_rule.marginal_fee(), + self.fee_rule.grace_actions(), + ); + + single_pool_output_balance( + cfg, + wallet_meta, + target_height, + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + self.change_memo.as_ref(), + ephemeral_balance, + ) + } +} + pub(crate) struct NetFlows { t_in: NonNegativeAmount, t_out: NonNegativeAmount, diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 9d7eef498d..ef93f0764a 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -1,22 +1,21 @@ //! Change strategies designed for use with a fixed fee. -use std::marker::PhantomData; +use std::convert::Infallible; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, transaction::{ - components::amount::{BalanceError, NonNegativeAmount}, + components::amount::BalanceError, fees::{fixed::FeeRule as FixedFeeRule, transparent}, }, }; -use crate::{data_api::InputSource, ShieldedProtocol}; +use crate::ShieldedProtocol; use super::{ - common::{single_pool_output_balance, SinglePoolBalanceConfig}, - sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - SplitPolicy, TransactionBalance, + sapling as sapling_fees, ChangeError, ChangeStrategy, CommonChangeStrategy, DummyMetaSource, + EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -26,53 +25,36 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { - fee_rule: FixedFeeRule, - change_memo: Option, - fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, - meta_source: PhantomData, -} +pub struct SingleOutputChangeStrategy(CommonChangeStrategy); -impl SingleOutputChangeStrategy { +impl SingleOutputChangeStrategy { /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule /// and change memo. /// /// `fallback_change_pool` is used when more than one shielded pool is enabled via /// feature flags, and the transaction has no shielded inputs. + #[deprecated(note = "Use [`CommonChangeStrategy::simple`] instead.")] pub fn new( fee_rule: FixedFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, ) -> Self { - Self { + Self(CommonChangeStrategy::simple( fee_rule, change_memo, fallback_change_pool, - dust_output_policy, - meta_source: PhantomData, - } + )) } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; - type MetaSource = I; - type WalletMeta = (); + type MetaSource = DummyMetaSource; + type WalletMeta = Infallible; fn fee_rule(&self) -> &Self::FeeRule { - &self.fee_rule - } - - fn fetch_wallet_meta( - &self, - _meta_source: &Self::MetaSource, - _account: ::AccountId, - _exclude: &[::NoteRef], - ) -> Result::Error> { - Ok(()) + self.0.fee_rule() } fn compute_balance( @@ -86,29 +68,16 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - let split_policy = SplitPolicy::single_output(); - let cfg = SinglePoolBalanceConfig::new( + self.0.compute_balance( params, - &self.fee_rule, - &self.dust_output_policy, - self.fee_rule.fixed_fee(), - &split_policy, - self.fallback_change_pool, - NonNegativeAmount::ZERO, - 0, - ); - - single_pool_output_balance( - cfg, - None, target_height, transparent_inputs, transparent_outputs, sapling, #[cfg(feature = "orchard")] orchard, - self.change_memo.as_ref(), ephemeral_balance, + None, ) } } @@ -123,12 +92,11 @@ mod tests { }, }; - use super::SingleOutputChangeStrategy; use crate::{ data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, - ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, + ChangeError, ChangeStrategy, ChangeValue, CommonChangeStrategy, }, ShieldedProtocol, }; @@ -140,11 +108,10 @@ mod tests { fn change_without_dust() { #[allow(deprecated)] let fee_rule = FixedFeeRule::non_standard(MINIMUM_FEE); - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( fee_rule, None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -183,11 +150,10 @@ mod tests { fn dust_change() { #[allow(deprecated)] let fee_rule = FixedFeeRule::non_standard(MINIMUM_FEE); - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( fee_rule, None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index cbc7440e91..71e330f7e6 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -1,21 +1,17 @@ //! Change strategies designed for use with a standard fee. -use std::marker::PhantomData; +use std::convert::Infallible; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, - transaction::fees::{ - transparent, - zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, - StandardFeeRule, - }, + transaction::fees::{transparent, zip317::FeeError as Zip317FeeError, StandardFeeRule}, }; -use crate::{data_api::InputSource, ShieldedProtocol}; +use crate::ShieldedProtocol; use super::{ - sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, + sapling as sapling_fees, ChangeError, ChangeStrategy, CommonChangeStrategy, DummyMetaSource, EphemeralBalance, TransactionBalance, }; @@ -26,53 +22,36 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { - fee_rule: StandardFeeRule, - change_memo: Option, - fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, - meta_source: PhantomData, -} +pub struct SingleOutputChangeStrategy(CommonChangeStrategy); -impl SingleOutputChangeStrategy { - /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 - /// fee parameters. +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule + /// and change memo. /// /// `fallback_change_pool` is used when more than one shielded pool is enabled via /// feature flags, and the transaction has no shielded inputs. + #[deprecated(note = "Use [`CommonChangeStrategy::simple`] instead.")] pub fn new( fee_rule: StandardFeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, ) -> Self { - Self { + Self(CommonChangeStrategy::simple( fee_rule, change_memo, fallback_change_pool, - dust_output_policy, - meta_source: PhantomData, - } + )) } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = StandardFeeRule; type Error = Zip317FeeError; - type MetaSource = I; - type WalletMeta = (); + type MetaSource = DummyMetaSource; + type WalletMeta = Infallible; fn fee_rule(&self) -> &Self::FeeRule { - &self.fee_rule - } - - fn fetch_wallet_meta( - &self, - _meta_source: &Self::MetaSource, - _account: ::AccountId, - _exclude: &[::NoteRef], - ) -> Result::Error> { - Ok(()) + self.0.fee_rule() } fn compute_balance( @@ -84,26 +63,18 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, + _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - match self.fee_rule() { - StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::::new( - Zip317FeeRule::standard(), - self.change_memo.clone(), - self.fallback_change_pool, - self.dust_output_policy, - ) - .compute_balance( - params, - target_height, - transparent_inputs, - transparent_outputs, - sapling, - #[cfg(feature = "orchard")] - orchard, - ephemeral_balance, - wallet_meta, - ), - } + self.0.compute_balance( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + ephemeral_balance, + None, + ) } } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 8b05656b6e..74f7d43e19 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -4,7 +4,7 @@ //! to ensure that inputs added to a transaction do not cause fees to rise by //! an amount greater than their value. -use std::marker::PhantomData; +use std::convert::Infallible; use zcash_primitives::{ consensus::{self, BlockHeight}, @@ -15,15 +15,11 @@ use zcash_primitives::{ }, }; -use crate::{ - data_api::{InputSource, WalletMeta}, - ShieldedProtocol, -}; +use crate::ShieldedProtocol; use super::{ - common::{single_pool_output_balance, SinglePoolBalanceConfig}, - sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - SplitPolicy, TransactionBalance, + sapling as sapling_fees, ChangeError, ChangeStrategy, CommonChangeStrategy, DummyMetaSource, + EphemeralBalance, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -33,53 +29,36 @@ use super::orchard as orchard_fees; /// as the most current pool that avoids unnecessary pool-crossing (with a specified /// fallback when the transaction has no shielded inputs). Fee calculation is delegated /// to the provided fee rule. -pub struct SingleOutputChangeStrategy { - fee_rule: Zip317FeeRule, - change_memo: Option, - fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, - meta_source: PhantomData, -} +pub struct SingleOutputChangeStrategy(CommonChangeStrategy); -impl SingleOutputChangeStrategy { - /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 - /// fee parameters and change memo. +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule + /// and change memo. /// /// `fallback_change_pool` is used when more than one shielded pool is enabled via /// feature flags, and the transaction has no shielded inputs. + #[deprecated(note = "Use [`CommonChangeStrategy::simple`] instead.")] pub fn new( fee_rule: Zip317FeeRule, change_memo: Option, fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, ) -> Self { - Self { + Self(CommonChangeStrategy::simple( fee_rule, change_memo, fallback_change_pool, - dust_output_policy, - meta_source: PhantomData, - } + )) } } -impl ChangeStrategy for SingleOutputChangeStrategy { +impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; - type MetaSource = I; - type WalletMeta = (); + type MetaSource = DummyMetaSource; + type WalletMeta = Infallible; fn fee_rule(&self) -> &Self::FeeRule { - &self.fee_rule - } - - fn fetch_wallet_meta( - &self, - _meta_source: &Self::MetaSource, - _account: ::AccountId, - _exclude: &[::NoteRef], - ) -> Result::Error> { - Ok(()) + self.0.fee_rule() } fn compute_balance( @@ -93,126 +72,16 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { - let split_policy = SplitPolicy::single_output(); - let cfg = SinglePoolBalanceConfig::new( + self.0.compute_balance( params, - &self.fee_rule, - &self.dust_output_policy, - self.fee_rule.marginal_fee(), - &split_policy, - self.fallback_change_pool, - self.fee_rule.marginal_fee(), - self.fee_rule.grace_actions(), - ); - - single_pool_output_balance( - cfg, - None, target_height, transparent_inputs, transparent_outputs, sapling, #[cfg(feature = "orchard")] orchard, - self.change_memo.as_ref(), - ephemeral_balance, - ) - } -} - -/// A change strategy that attempts to split the change value into some number of equal-sized notes -/// as dictated by the included [`SplitPolicy`] value. -pub struct MultiOutputChangeStrategy { - fee_rule: Zip317FeeRule, - change_memo: Option, - fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, - split_policy: SplitPolicy, - meta_source: PhantomData, -} - -impl MultiOutputChangeStrategy { - /// Constructs a new [`MultiOutputChangeStrategy`] with the specified ZIP 317 - /// fee parameters, change memo, and change splitting policy. - /// - /// This change strategy will fall back to creating a single change output if insufficient - /// change value is available to create notes with at least the minimum value dictated by the - /// split policy. - /// - /// - `fallback_change_pool`: the pool to which change will be sent if when more than one - /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. - /// - `split_policy`: A policy value describing how the change value should be returned as - /// multiple notes. - pub fn new( - fee_rule: Zip317FeeRule, - change_memo: Option, - fallback_change_pool: ShieldedProtocol, - dust_output_policy: DustOutputPolicy, - split_policy: SplitPolicy, - ) -> Self { - Self { - fee_rule, - change_memo, - fallback_change_pool, - dust_output_policy, - split_policy, - meta_source: PhantomData, - } - } -} - -impl ChangeStrategy for MultiOutputChangeStrategy { - type FeeRule = Zip317FeeRule; - type Error = Zip317FeeError; - type MetaSource = I; - type WalletMeta = WalletMeta; - - fn fee_rule(&self) -> &Self::FeeRule { - &self.fee_rule - } - - fn fetch_wallet_meta( - &self, - meta_source: &Self::MetaSource, - account: ::AccountId, - exclude: &[::NoteRef], - ) -> Result::Error> { - meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) - } - - fn compute_balance( - &self, - params: &P, - 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, - ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, - ) -> Result> { - let cfg = SinglePoolBalanceConfig::new( - params, - &self.fee_rule, - &self.dust_output_policy, - self.fee_rule.marginal_fee(), - &self.split_policy, - self.fallback_change_pool, - self.fee_rule.marginal_fee(), - self.fee_rule.grace_actions(), - ); - - single_pool_output_balance( - cfg, - wallet_meta, - target_height, - transparent_inputs, - transparent_outputs, - sapling, - #[cfg(feature = "orchard")] - orchard, - self.change_memo.as_ref(), ephemeral_balance, + None, ) } } @@ -230,13 +99,12 @@ mod tests { }, }; - use super::SingleOutputChangeStrategy; use crate::{ data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment, WalletMeta}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, - zip317::MultiOutputChangeStrategy, - ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, SplitPolicy, + ChangeError, ChangeStrategy, ChangeValue, CommonChangeStrategy, DustAction, + DustOutputPolicy, SplitPolicy, }, ShieldedProtocol, }; @@ -249,11 +117,10 @@ mod tests { #[test] fn change_without_dust() { - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -290,7 +157,7 @@ mod tests { #[test] fn change_without_dust_multi() { - let change_strategy = MultiOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, @@ -404,11 +271,10 @@ mod tests { #[test] #[cfg(feature = "orchard")] fn cross_pool_change_without_dust() { - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( Zip317FeeRule::standard(), None, ShieldedProtocol::Orchard, - DustOutputPolicy::default(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -460,11 +326,12 @@ mod tests { } fn change_with_transparent_payments(dust_output_policy: DustOutputPolicy) { - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, dust_output_policy, + SplitPolicy::single_output(), ); // spend a single Sapling note that is sufficient to pay the fee @@ -506,11 +373,10 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // Spend a single transparent UTXO that is exactly sufficient to pay the fee. @@ -551,11 +417,10 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // Spend a single transparent UTXO that is sufficient to pay the fee. @@ -596,7 +461,7 @@ mod tests { use crate::fees::sapling as sapling_fees; use zcash_primitives::{legacy::TransparentAddress, transaction::components::OutPoint}; - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, @@ -604,6 +469,7 @@ mod tests { DustAction::AllowDustChange, Some(NonNegativeAmount::const_from_u64(1000)), ), + SplitPolicy::single_output(), ); // Spend a single transparent UTXO that is sufficient to pay the fee. @@ -655,11 +521,12 @@ mod tests { } fn change_with_allowable_dust(dust_output_policy: DustOutputPolicy) { - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::new( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, dust_output_policy, + SplitPolicy::single_output(), ); // Spend two Sapling notes, one of them dust. There is sufficient to @@ -705,11 +572,10 @@ mod tests { #[test] fn change_with_disallowed_dust() { - let change_strategy = SingleOutputChangeStrategy::::new( + let change_strategy = CommonChangeStrategy::::simple( Zip317FeeRule::standard(), None, ShieldedProtocol::Sapling, - DustOutputPolicy::default(), ); // Attempt to spend three Sapling notes, one of them dust. Adding the third diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 30e6f30542..3e1794dc3d 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -353,7 +353,7 @@ impl, P: consensus::Parameters> InputSource for account_id: Self::AccountId, min_value: NonNegativeAmount, exclude: &[Self::NoteRef], - ) -> Result { + ) -> Result, Self::Error> { let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())? .ok_or(SqliteClientError::ChainHeightUnknown)?; @@ -376,11 +376,11 @@ impl, P: consensus::Parameters> InputSource for chain_tip_height, )?; - Ok(WalletMeta::new( + Ok(Some(WalletMeta::new( sapling_note_count, #[cfg(feature = "orchard")] orchard_note_count, - )) + ))) } } diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 9cb65e5717..1a426a246a 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -599,7 +599,6 @@ pub(crate) mod tests { testing::orchard::OrchardPoolTester, wallet::input_selection::GreedyInputSelector, WalletCommitmentTrees, }, - fees::{standard, DustOutputPolicy}, wallet::OvkPolicy, }, zcash_primitives::{memo::Memo, transaction::fees::StandardFeeRule}, @@ -1713,7 +1712,7 @@ pub(crate) mod tests { #[test] #[cfg(feature = "orchard")] fn orchard_block_spanning_tip_boundary_complete() { - use zcash_client_backend::data_api::Account as _; + use zcash_client_backend::{data_api::Account as _, fees::CommonChangeStrategy}; let mut st = prepare_orchard_block_spanning_test(true); let account = st.test_account().cloned().unwrap(); @@ -1775,11 +1774,10 @@ pub(crate) mod tests { let fee_rule = StandardFeeRule::Zip317; let change_memo = "Test change memo".parse::().unwrap(); - let change_strategy = standard::SingleOutputChangeStrategy::new( + let change_strategy = CommonChangeStrategy::simple( fee_rule, Some(change_memo.into()), OrchardPoolTester::SHIELDED_PROTOCOL, - DustOutputPolicy::default(), ); let input_selector = GreedyInputSelector::new(); @@ -1806,7 +1804,7 @@ pub(crate) mod tests { #[test] #[cfg(feature = "orchard")] fn orchard_block_spanning_tip_boundary_incomplete() { - use zcash_client_backend::data_api::Account as _; + use zcash_client_backend::{data_api::Account as _, fees::CommonChangeStrategy}; let mut st = prepare_orchard_block_spanning_test(false); let account = st.test_account().cloned().unwrap(); @@ -1864,11 +1862,10 @@ pub(crate) mod tests { let fee_rule = StandardFeeRule::Zip317; let change_memo = "Test change memo".parse::().unwrap(); - let change_strategy = standard::SingleOutputChangeStrategy::new( + let change_strategy = CommonChangeStrategy::simple( fee_rule, Some(change_memo.into()), OrchardPoolTester::SHIELDED_PROTOCOL, - DustOutputPolicy::default(), ); let input_selector = GreedyInputSelector::new(); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 9772baf67b..cedb07bde8 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -7,6 +7,11 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Changed +- The `zcash_primitives::transaction::fees::FeeRule` trait has additional + `default_dust_threshold`, `marginal_fee`, and `grace_actions` methods, and + a `FeeRuleOrBalanceError` associated type. + ### Deprecated - `zcash_primitives::transaction::fees`: - `fixed::FeeRule::non_standard`. Using a fixed fee may result in a transaction diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index e6b73ce744..2816f636be 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -1,5 +1,7 @@ //! Abstractions and types related to fee calculations. +use zcash_protocol::value::BalanceError; + use crate::{ consensus::{self, BlockHeight}, transaction::{components::amount::NonNegativeAmount, fees::transparent::InputSize}, @@ -16,6 +18,7 @@ pub mod tze; /// by a transaction having a specified set of inputs and outputs. pub trait FeeRule { type Error; + type FeeRuleOrBalanceError: From + From; /// Computes the total fee required for a transaction given the provided inputs and outputs. /// @@ -33,6 +36,16 @@ pub trait FeeRule { sapling_output_count: usize, orchard_action_count: usize, ) -> Result; + + /// Returns the default dust threshold. + fn default_dust_threshold(&self) -> NonNegativeAmount; + + /// Returns the marginal fee, i.e. the amount by which the fee increases for each + /// logical action as defined in ZIP 317. + fn marginal_fee(&self) -> NonNegativeAmount; + + /// Returns the number of grace actions, or 0 if this rule does not use grace actions. + fn grace_actions(&self) -> usize; } /// A trait that represents the ability to compute the fees that must be paid by a transaction @@ -67,6 +80,7 @@ pub enum StandardFeeRule { impl FeeRule for StandardFeeRule { type Error = zip317::FeeError; + type FeeRuleOrBalanceError = zip317::FeeError; fn fee_required( &self, @@ -90,4 +104,16 @@ impl FeeRule for StandardFeeRule { ), } } + + fn default_dust_threshold(&self) -> NonNegativeAmount { + zip317::MARGINAL_FEE + } + + fn marginal_fee(&self) -> NonNegativeAmount { + zip317::MARGINAL_FEE + } + + fn grace_actions(&self) -> usize { + zip317::GRACE_ACTIONS + } } diff --git a/zcash_primitives/src/transaction/fees/fixed.rs b/zcash_primitives/src/transaction/fees/fixed.rs index de3206fbee..f29020cb5a 100644 --- a/zcash_primitives/src/transaction/fees/fixed.rs +++ b/zcash_primitives/src/transaction/fees/fixed.rs @@ -1,3 +1,7 @@ +use std::convert::Infallible; + +use zcash_protocol::value::BalanceError; + use crate::{ consensus::{self, BlockHeight}, transaction::components::amount::NonNegativeAmount, @@ -31,7 +35,8 @@ impl FeeRule { } impl super::FeeRule for FeeRule { - type Error = std::convert::Infallible; + type Error = Infallible; + type FeeRuleOrBalanceError = BalanceError; fn fee_required( &self, @@ -45,6 +50,18 @@ impl super::FeeRule for FeeRule { ) -> Result { Ok(self.fixed_fee) } + + fn default_dust_threshold(&self) -> NonNegativeAmount { + self.fixed_fee + } + + fn marginal_fee(&self) -> NonNegativeAmount { + NonNegativeAmount::ZERO + } + + fn grace_actions(&self) -> usize { + 0 + } } #[cfg(zcash_unstable = "zfuture")] diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs index 48457b8007..ddb100aa14 100644 --- a/zcash_primitives/src/transaction/fees/zip317.rs +++ b/zcash_primitives/src/transaction/fees/zip317.rs @@ -83,7 +83,7 @@ impl FeeRule { || p2pkh_standard_input_size > P2PKH_STANDARD_INPUT_SIZE \ || p2pkh_standard_output_size > P2PKH_STANDARD_OUTPUT_SIZE` \ violates ZIP 317, and might cause transactions built with it to fail. \ - This API is likely to be removed. Use `[FeeRule::standard]` instead." + This API is likely to be removed. Use [`FeeRule::standard`] instead." )] pub fn non_standard( marginal_fee: NonNegativeAmount, @@ -103,14 +103,6 @@ impl FeeRule { } } - /// Returns the ZIP 317 marginal fee. - pub fn marginal_fee(&self) -> NonNegativeAmount { - 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 @@ -152,6 +144,7 @@ impl std::fmt::Display for FeeError { impl super::FeeRule for FeeRule { type Error = FeeError; + type FeeRuleOrBalanceError = FeeError; fn fee_required( &self, @@ -193,4 +186,16 @@ impl super::FeeRule for FeeRule { (self.marginal_fee * max(self.grace_actions, logical_actions)) .ok_or_else(|| BalanceError::Overflow.into()) } + + fn default_dust_threshold(&self) -> NonNegativeAmount { + self.marginal_fee + } + + fn marginal_fee(&self) -> NonNegativeAmount { + self.marginal_fee + } + + fn grace_actions(&self) -> usize { + self.grace_actions + } }