From 6d1f78a06677dcbe77f49d455bdb710def18bec8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Oct 2024 12:32:25 -0600 Subject: [PATCH 1/7] 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 e21fce4411596fcbec06f73de1f4480bc7ce6b75 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 15 Oct 2024 16:39:14 -0600 Subject: [PATCH 2/7] 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 0f5a39acc8..7bdaf6b231 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 - MSRV is now 1.77.0. @@ -28,7 +33,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 19b66ff147..32ee1e2fdc 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 6d5a6ac7ac1a23836d4fc1684775e37b7bb32688 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 3/7] 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 7bdaf6b231..e05617d246 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 - MSRV is now 1.77.0. @@ -25,6 +70,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 4418910970..443b220ab2 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 { use fmt::Write; @@ -122,6 +129,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) } @@ -178,12 +188,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 { @@ -197,35 +209,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, @@ -240,20 +255,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 328e99bae3..5ea28edab1 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 1787b3540a..45391cb2b7 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 2c2101aad1..3b51953de0 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 9a58a1dafe541f91504db492ea0d0fd9abc67840 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 4/7] 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 5bf0f16dd7f5b7f14194f4386855be7fc63ea555 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Oct 2024 14:58:50 -0600 Subject: [PATCH 5/7] zcash_client_backend: Add `fees::zip317::MultiOutputChangeStrategy`. --- components/zcash_protocol/CHANGELOG.md | 8 +- 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, 695 insertions(+), 110 deletions(-) diff --git a/components/zcash_protocol/CHANGELOG.md b/components/zcash_protocol/CHANGELOG.md index 2b1d1d8c1c..9636526625 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::QuotRem` +- `zcash_protocol::value::Zatoshis::div_with_remainder` + ### Changed - MSRV is now 1.77.0. @@ -80,9 +84,9 @@ The entries below are relative to the `zcash_primitives` crate as of the tag - `zcash_protocol::consensus::Parameters` has been split into two traits, with the newly added `NetworkConstants` trait providing all network constant accessors. Also, the `address_network` method has been replaced with a new - `network_type` method that serves the same purpose. A blanket impl of + `network_type` method that serves the same purpose. A blanket impl of `NetworkConstants` is provided for all types that implement `Parameters`, - so call sites for methods that have moved to `NetworkConstants` should + so call sites for methods that have moved to `NetworkConstants` should remain unchanged (though they may require an additional `use` statement.) ### Removed diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index 317c15918b..94f2b7ba64 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 e05617d246..189bda455d 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 5ea28edab1..7a88941b2f 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 c2912b07c7..fc78facfe7 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 fde0fc9730cc91ab2b9294c835288eaf590e6f82 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sun, 20 Oct 2024 11:52:07 -0600 Subject: [PATCH 6/7] 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 | 21 +++++----- zcash_client_backend/src/fees/common.rs | 21 ++++------ zcash_client_backend/src/fees/fixed.rs | 6 +-- zcash_client_backend/src/fees/standard.rs | 6 +-- zcash_client_backend/src/fees/zip317.rs | 20 +++++----- zcash_client_sqlite/src/lib.rs | 5 +++ zcash_client_sqlite/src/wallet/common.rs | 6 ++- 11 files changed, 75 insertions(+), 70 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 189bda455d..7a7ef321fb 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 - MSRV is now 1.77.0. @@ -72,6 +68,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 7a88941b2f..e818c3e005 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..e109d58478 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; } } } @@ -452,7 +452,10 @@ pub trait ChangeStrategy { /// 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; + + /// Tye type of wallet metadata that this change strategy relies upon in order to compute + /// change. + type WalletMetaT; /// Returns the fee rule that this change strategy will respect when performing /// balance computations. @@ -465,7 +468,7 @@ pub trait ChangeStrategy { meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error>; + ) -> Result::Error>; /// Computes the totals of inputs, suggested change amounts, and fees given the /// provided inputs and outputs being used to construct a transaction. @@ -502,7 +505,7 @@ pub trait ChangeStrategy { sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, + wallet_meta: Option<&Self::WalletMetaT>, ) -> Result>; } 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/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 311c806bb4..529e9cdfcd 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -60,7 +60,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -71,7 +71,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -84,7 +84,7 @@ 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::WalletMetaT>, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index dbaf8ace1b..330ec6de36 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -64,7 +64,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = StandardFeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -75,7 +75,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -88,7 +88,7 @@ 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::WalletMetaT>, ) -> Result> { #[allow(deprecated)] match self.fee_rule() { diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 862e613029..c342c46657 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -67,7 +67,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -78,7 +78,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -91,7 +91,7 @@ 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::WalletMetaT>, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( @@ -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, @@ -165,7 +165,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = WalletMeta; + type WalletMetaT = WalletMeta; fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -176,7 +176,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) } @@ -189,7 +189,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { 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::WalletMetaT>, ) -> Result> { let cfg = SinglePoolBalanceConfig::new( params, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 32ee1e2fdc..de70c5955b 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 47b1065db901e9e423e3b0cdb35fc94d9bd4b24c Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 21 Oct 2024 14:56:44 -0600 Subject: [PATCH 7/7] zcash_client_backend: Require wallet metadata for balance calculation --- .../src/data_api/wallet/input_selection.rs | 118 +++++++++--------- zcash_client_backend/src/fees.rs | 2 +- zcash_client_backend/src/fees/fixed.rs | 6 +- zcash_client_backend/src/fees/standard.rs | 2 +- zcash_client_backend/src/fees/zip317.rs | 30 ++--- 5 files changed, 80 insertions(+), 78 deletions(-) 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..09cbf7e99d 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -482,61 +482,6 @@ impl InputSelector for GreedyInputSelector { } } - #[cfg(not(feature = "transparent-inputs"))] - let ephemeral_balance = None; - - #[cfg(feature = "transparent-inputs")] - let (ephemeral_balance, tr1_balance_opt) = { - if tr1_transparent_outputs.is_empty() { - (None, None) - } else { - // The ephemeral input going into transaction 1 must be able to pay that - // transaction's fee, as well as the TEX address payments. - - // First compute the required total with an additional zero input, - // 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 change_strategy - .compute_balance::<_, DbT::NoteRef>( - params, - target_height, - &[] as &[WalletTransparentOutput], - &tr1_transparent_outputs, - &sapling::EmptyBundleView, - #[cfg(feature = "orchard")] - &orchard_fees::EmptyBundleView, - Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)), - None, - ) { - 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 = change_strategy.compute_balance::<_, DbT::NoteRef>( - params, - target_height, - &[] as &[WalletTransparentOutput], - &tr1_transparent_outputs, - &sapling::EmptyBundleView, - #[cfg(feature = "orchard")] - &orchard_fees::EmptyBundleView, - Some(&EphemeralBalance::Input(tr1_required_input_value)), - None, - )?; - assert_eq!(tr1_balance.total(), tr1_balance.fee_required()); - - ( - Some(EphemeralBalance::Output(tr1_required_input_value)), - Some(tr1_balance), - ) - } - }; - let mut shielded_inputs = SpendableNotes::empty(); let mut prior_available = NonNegativeAmount::ZERO; let mut amount_required = NonNegativeAmount::ZERO; @@ -598,6 +543,63 @@ impl InputSelector for GreedyInputSelector { .fetch_wallet_meta(wallet_db, account, &selected_input_ids) .map_err(InputSelectorError::DataSource)?; + #[cfg(not(feature = "transparent-inputs"))] + let ephemeral_balance = None; + + #[cfg(feature = "transparent-inputs")] + let (ephemeral_balance, tr1_balance_opt) = { + if tr1_transparent_outputs.is_empty() { + (None, None) + } else { + // The ephemeral input going into transaction 1 must be able to pay that + // transaction's fee, as well as the TEX address payments. + + // First compute the required total with an additional zero input, + // 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 change_strategy + .compute_balance::<_, DbT::NoteRef>( + params, + target_height, + &[] as &[WalletTransparentOutput], + &tr1_transparent_outputs, + &sapling::EmptyBundleView, + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)), + &wallet_meta, + ) { + 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 = change_strategy.compute_balance::<_, DbT::NoteRef>( + params, + target_height, + &[] as &[WalletTransparentOutput], + &tr1_transparent_outputs, + &sapling::EmptyBundleView, + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + Some(&EphemeralBalance::Input(tr1_required_input_value)), + &wallet_meta, + )?; + assert_eq!(tr1_balance.total(), tr1_balance.fee_required()); + + ( + Some(EphemeralBalance::Output(tr1_required_input_value)), + Some(tr1_balance), + ) + } + }; + // In the ZIP 320 case, this is the balance for transaction 0, taking into account // the ephemeral output. let balance = change_strategy.compute_balance( @@ -617,7 +619,7 @@ impl InputSelector for GreedyInputSelector { &orchard_outputs[..], ), ephemeral_balance.as_ref(), - Some(&wallet_meta), + &wallet_meta, ); match balance { @@ -819,7 +821,7 @@ impl ShieldingSelector for GreedyInputSelector { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&wallet_meta), + &wallet_meta, ); let balance = match trial_balance { @@ -837,7 +839,7 @@ impl ShieldingSelector for GreedyInputSelector { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&wallet_meta), + &wallet_meta, )? } Err(other) => return Err(InputSelectorError::Change(other)), diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index e109d58478..e855a0462a 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -505,7 +505,7 @@ pub trait ChangeStrategy { sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMetaT>, + wallet_meta: &Self::WalletMetaT, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 529e9cdfcd..452135fbba 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -84,7 +84,7 @@ 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::WalletMetaT>, + _wallet_meta: &Self::WalletMetaT, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( @@ -168,7 +168,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -218,7 +218,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index 330ec6de36..c3e46d3875 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -88,7 +88,7 @@ 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::WalletMetaT>, + wallet_meta: &Self::WalletMetaT, ) -> Result> { #[allow(deprecated)] match self.fee_rule() { diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index c342c46657..0706bfaf81 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -91,7 +91,7 @@ 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::WalletMetaT>, + _wallet_meta: &Self::WalletMetaT, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( @@ -189,7 +189,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMetaT>, + wallet_meta: &Self::WalletMetaT, ) -> Result> { let cfg = SinglePoolBalanceConfig::new( params, @@ -204,7 +204,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { single_pool_output_balance( cfg, - wallet_meta, + Some(wallet_meta), target_height, transparent_inputs, transparent_outputs, @@ -277,7 +277,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -324,11 +324,11 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&WalletMeta::new( + &WalletMeta::new( existing_notes, #[cfg(feature = "orchard")] 0, - )), + ), ) }; @@ -380,11 +380,11 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - Some(&WalletMeta::new( + &WalletMeta::new( 0, #[cfg(feature = "orchard")] 0, - )), + ), ); assert_matches!( @@ -435,7 +435,7 @@ mod tests { ))][..], ), None, - None, + &(), ); assert_matches!( @@ -489,7 +489,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -534,7 +534,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -579,7 +579,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -630,7 +630,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -692,7 +692,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); assert_matches!( @@ -744,7 +744,7 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - None, + &(), ); // We will get an error here, because the dust input isn't free to add