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 0f5a39acc8..7a7ef321fb 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -11,6 +11,54 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `Progress` - `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`: + - `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: + - `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 + 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. + - 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. @@ -20,6 +68,10 @@ 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<...>` ## [0.14.0] - 2024-10-04 @@ -28,7 +80,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..d56465413c 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -794,6 +794,64 @@ 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 { + self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard) + } +} + /// 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 +896,19 @@ 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; + /// - 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, + 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/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 9e5a401d01..9e99355034 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::{ @@ -56,20 +59,20 @@ use crate::{ ShieldedProtocol, }; -use super::WalletTest; #[allow(deprecated)] use super::{ chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary}, 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, 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")] @@ -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, ) } @@ -1227,17 +1221,18 @@ impl TestState { // } } -/// Helper method for constructing a [`GreedyInputSelector`] with a -/// [`standard::SingleOutputChangeStrategy`]. -pub fn input_selector( +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 @@ -2378,6 +2373,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_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 328e99bae3..e818c3e005 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,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, zip317::FeeRule as Zip317FeeRule, 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::{ + self, + standard::{self, SingleOutputChangeStrategy}, + DustOutputPolicy, SplitPolicy, + }, 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, @@ -310,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 each + // 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, @@ -399,7 +574,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 +674,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 +933,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 +949,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 +1173,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 +1236,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 +1318,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 +1370,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 +1470,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 +1533,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 +1583,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 +1675,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 +1689,7 @@ pub fn zip317_spend( assert_matches!( st.spend( &input_selector, + &change_strategy, account.usk(), req, OvkPolicy::Sender, @@ -1517,6 +1711,7 @@ pub fn zip317_spend( let txid = st .spend( &input_selector, + &change_strategy, account.usk(), req, OvkPolicy::Sender, @@ -1579,19 +1774,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 +2021,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 +2056,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 +2111,16 @@ pub fn fully_funded_fully_private( + let create_proposed_result = st.create_proposed_transactions::( account.usk(), OvkPolicy::Sender, &proposal0, @@ -2009,17 +2201,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 +2234,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 +2301,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 +2315,7 @@ pub fn multi_pool_checkpoint( let res = st .spend( &input_selector, + &change_strategy, account.usk(), p0_transfer, OvkPolicy::Sender, @@ -2157,6 +2347,7 @@ pub fn multi_pool_checkpoint( let res = st .spend( &input_selector, + &change_strategy, account.usk(), both_transfer, OvkPolicy::Sender, @@ -2597,17 +2788,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..6d9bf6f3a8 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,60 @@ where Ok(()) } +/// Errors that may be generated in construction of proposals for shielded->shielded or +/// shielded->transparent transfers. +pub type ProposeTransferErrT = Error< + ::Error, + CommitmentTreeErrT, + ::Error, + <::FeeRule as FeeRule>::Error, + ::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, + CommitmentTreeErrT, + ::Error, + <::FeeRule as FeeRule>::Error, + ::Error, + Infallible, +>; + +/// Errors that may be generated in combined creation and execution of transaction proposals. +pub type CreateErrT = Error< + ::Error, + ::Error, + InputsErrT, + ::Error, + ChangeErrT, + N, +>; + +/// Errors that may be generated in the execution of proposals that may send shielded inputs. +pub type TransferErrT = Error< + ::Error, + ::Error, + ::Error, + <::FeeRule as FeeRule>::Error, + ::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, + ::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 +303,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 +344,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 +398,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 +419,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 +431,7 @@ where params, account.id(), input_selector, + change_strategy, request, min_confirmations, )?; @@ -409,27 +452,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 +484,7 @@ where anchor_height, spend_from_account, request, + change_strategy, ) .map_err(Error::from) } @@ -488,11 +529,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 +558,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 +582,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 +610,10 @@ where .propose_shielding( params, wallet_db, + change_strategy, shielding_threshold, from_addrs, + to_account, chain_tip_height + 1, min_confirmations, ) @@ -599,7 +641,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 +649,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 +733,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 +749,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 +794,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 +850,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 +875,7 @@ where .transpose(), Note::Sapling(_) => None, }) - .collect::, Error<_, _, _, _>>>()?; + .collect::, Error<_, _, _, _, _, _>>>()?; Ok((Some(anchor), orchard_inputs)) }) @@ -872,7 +918,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 +944,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 +1078,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 +1093,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 +1423,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..09cbf7e99d 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![]; @@ -469,60 +482,6 @@ where } } - #[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 self.change_strategy.compute_balance::<_, DbT::NoteRef>( - params, - target_height, - &[] as &[WalletTransparentOutput], - &tr1_transparent_outputs, - &sapling::EmptyBundleView, - #[cfg(feature = "orchard")] - &orchard_fees::EmptyBundleView, - &self.dust_output_policy, - Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)), - ) { - Err(ChangeError::InsufficientFunds { required, .. }) => required, - Ok(_) => NonNegativeAmount::ZERO, // shouldn't happen - Err(other) => return Err(other.into()), - }; - - // 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>( - params, - target_height, - &[] as &[WalletTransparentOutput], - &tr1_transparent_outputs, - &sapling::EmptyBundleView, - #[cfg(feature = "orchard")] - &orchard_fees::EmptyBundleView, - &self.dust_output_policy, - Some(&EphemeralBalance::Input(tr1_required_input_value)), - )?; - 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; @@ -573,9 +532,77 @@ 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)?; + + #[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 = self.change_strategy.compute_balance( + let balance = change_strategy.compute_balance( params, target_height, &[] as &[WalletTransparentOutput], @@ -591,8 +618,8 @@ where &orchard_inputs[..], &orchard_outputs[..], ), - &self.dust_output_policy, ephemeral_balance.as_ref(), + &wallet_meta, ); match balance { @@ -681,7 +708,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 +721,7 @@ where vec![], shielded_inputs, balance, - self.change_strategy.fee_rule().clone(), + (*change_strategy.fee_rule()).clone(), target_height, false, ) @@ -713,7 +740,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 +774,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 +808,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 +820,8 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, None, + &wallet_meta, ); let balance = match trial_balance { @@ -802,7 +830,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 +838,11 @@ where &sapling::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &self.dust_output_policy, None, + &wallet_meta, )? } - Err(other) => { - return Err(other.into()); - } + Err(other) => return Err(InputSelectorError::Change(other)), }; if balance.total() >= shielding_threshold { @@ -826,7 +852,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..e855a0462a 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -1,18 +1,20 @@ -use std::fmt; +use std::{ + fmt::{self, Debug, Display}, + num::{NonZeroU64, NonZeroUsize}, +}; 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 +275,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, + } } } @@ -330,6 +339,72 @@ impl Default for DustOutputPolicy { } } +/// A policy that describes how change output should be split into multiple notes for the purpose +/// of note management. +#[derive(Clone, Copy, Debug)] +pub struct SplitPolicy { + target_output_count: NonZeroUsize, + min_split_output_size: NonNegativeAmount, +} + +impl SplitPolicy { + /// Constructs a new [`SplitPolicy`] from its constituent parts. + pub fn new( + target_output_count: NonZeroUsize, + min_split_output_size: NonNegativeAmount, + ) -> 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 *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; + } + } + } +} + /// `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. @@ -368,13 +443,33 @@ 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; + + /// 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. 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 +488,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 +504,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: &Self::WalletMetaT, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index f1b0b7319e..45b77e5dfa 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,55 +115,101 @@ 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 + } 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)] +pub(crate) struct OutputManifest { + transparent: usize, + sapling: usize, + orchard: usize, +} + +impl OutputManifest { + const ZERO: OutputManifest = OutputManifest { + transparent: 0, + sapling: 0, + orchard: 0, }; - ( - change_pool, - (change_pool == ShieldedProtocol::Sapling).into(), - (change_pool == ShieldedProtocol::Orchard).into(), - ) + + pub(crate) fn sapling(&self) -> usize { + self.sapling + } + + 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> { + 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, +} + +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, + ) -> Self { + Self { + params, + fee_rule, + dust_output_policy, + default_dust_threshold, + split_policy, + fallback_change_pool, + marginal_fee, + grace_actions, + } + } } #[allow(clippy::too_many_arguments)] -pub(crate) fn single_change_output_balance< - P: consensus::Parameters, - NoteRefT: Clone, - F: FeeRule, - E, ->( - params: &P, - fee_rule: &F, +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], 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 @@ -183,9 +232,25 @@ where ephemeral_balance, )?; - #[allow(unused_variables)] - let (change_pool, sapling_change, orchard_change) = - single_change_output_policy(&net_flows, fallback_change_pool); + 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 { + target_change_count + } else { + 0 + }, + orchard: if change_pool == ShieldedProtocol::Orchard { + target_change_count + } 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(); @@ -193,17 +258,21 @@ 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 = + 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)] + if transparent + || (cfg.dust_output_policy.action() == DustAction::AddDustToFee + && change_memo.is_none()) + { + vec![OutputManifest::ZERO, target_change_counts] } else { - vec![(0, sapling_change, orchard_change)] - }; + vec![target_change_counts] + } + }; check_for_uneconomic_inputs( transparent_inputs, @@ -211,8 +280,8 @@ where sapling, #[cfg(feature = "orchard")] orchard, - marginal_fee, - grace_actions, + cfg.marginal_fee, + cfg.grace_actions, &possible_change[..], ephemeral_balance, )?; @@ -229,35 +298,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: // @@ -301,29 +371,30 @@ 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(), 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)))?; 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, + 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)))?, ); @@ -353,23 +424,84 @@ 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, ) }; - 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() { + 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: // * it should be allowed in order to record change memos and to improve @@ -377,11 +509,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, @@ -456,7 +588,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 +651,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 +666,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 +698,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 +740,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(()) diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index d2033eba0b..452135fbba 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,11 +11,12 @@ 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, - DustOutputPolicy, EphemeralBalance, TransactionBalance, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, + sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -23,13 +26,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 +44,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 WalletMetaT = (); 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,24 +83,31 @@ 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: &Self::WalletMetaT, ) -> Result> { - single_change_output_balance( + 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_pool_output_balance( + cfg, + None, target_height, transparent_inputs, transparent_outputs, sapling, #[cfg(feature = "orchard")] orchard, - dust_output_policy, - self.fee_rule.fixed_fee(), self.change_memo.as_ref(), - self.fallback_change_pool, - NonNegativeAmount::ZERO, - 0, ephemeral_balance, ) } @@ -99,7 +125,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 +140,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,8 +167,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( @@ -153,8 +183,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,8 +217,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index 4cad64c5d2..c3e46d3875 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 WalletMetaT = (); 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: &Self::WalletMetaT, ) -> 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..0706bfaf81 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,11 +15,15 @@ use zcash_primitives::{ }, }; -use crate::ShieldedProtocol; +use crate::{ + data_api::{InputSource, WalletMeta}, + ShieldedProtocol, +}; use super::{ - common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, - DustOutputPolicy, EphemeralBalance, TransactionBalance, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, + sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -27,13 +33,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 +51,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 WalletMetaT = (); 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,24 +90,128 @@ 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: &Self::WalletMetaT, ) -> Result> { - single_change_output_balance( + 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_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 WalletMetaT = 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: &Self::WalletMetaT, + ) -> Result> { + let cfg = SinglePoolBalanceConfig::new( + params, + &self.fee_rule, + &self.dust_output_policy, self.fee_rule.marginal_fee(), - self.change_memo.as_ref(), + &self.split_policy, self.fallback_change_pool, self.fee_rule.marginal_fee(), self.fee_rule.grace_actions(), + ); + + single_pool_output_balance( + cfg, + Some(wallet_meta), + target_height, + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + self.change_memo.as_ref(), ephemeral_balance, ) } @@ -93,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}, @@ -106,10 +232,11 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::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, }; @@ -122,10 +249,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,8 +276,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( @@ -160,13 +288,127 @@ 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, + &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, + &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() { - 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,8 +434,8 @@ mod tests { 30000, ))][..], ), - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( @@ -206,22 +448,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,8 +488,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - dust_output_policy, None, + &(), ); assert_matches!( @@ -263,10 +506,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,8 +533,8 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( @@ -307,10 +551,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,8 +578,8 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); assert_matches!( @@ -351,10 +596,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,11 +629,8 @@ mod tests { &sapling_fees::EmptyBundleView, #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::new( - DustAction::AllowDustChange, - Some(NonNegativeAmount::const_from_u64(1000)), - ), None, + &(), ); assert_matches!( @@ -397,22 +643,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,8 +691,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - dust_output_policy, None, + &(), ); assert_matches!( @@ -458,10 +705,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,8 +743,8 @@ mod tests { ), #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, - &DustOutputPolicy::default(), None, + &(), ); // We will get an error here, because the dust input isn't free to add 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/lib.rs b/zcash_client_sqlite/src/lib.rs index 19b66ff147..de70c5955b 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,41 @@ 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 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")] + let orchard_note_count = count_outputs( + self.conn.borrow(), + account_id, + min_value, + exclude, + ShieldedProtocol::Orchard, + chain_tip_height, + )?; + + 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/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 70879ab85e..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::( @@ -118,7 +125,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/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 893a6a7dc1..378abbe1a1 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -225,3 +225,56 @@ where .filter_map(|r| r.transpose()) .collect::>() } + +pub(crate) fn count_outputs( + conn: &rusqlite::Connection, + account: AccountId, + min_value: NonNegativeAmount, + exclude: &[ReceivedNoteId], + protocol: ShieldedProtocol, + chain_tip_height: BlockHeight, +) -> 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 > :chain_tip_height -- the spending tx is unexpired + )" + ), + named_params![ + ":account_id": account.0, + ":min_value": u64::from(min_value), + ":exclude": &excluded_ptr, + ":chain_tip_height": u32::from(chain_tip_height) + ], + |row| row.get(0), + ) +} 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_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(), ); 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;