diff --git a/components/zcash_protocol/CHANGELOG.md b/components/zcash_protocol/CHANGELOG.md index 505c8408a1..2b62e65a97 100644 --- a/components/zcash_protocol/CHANGELOG.md +++ b/components/zcash_protocol/CHANGELOG.md @@ -7,6 +7,10 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Added +- `zcash_protocol::value::DivRem` +- `zcash_protocol::value::Zatoshis::div_with_remainder` + ## [0.4.0] - 2024-10-02 ### Added - `impl Sub for BlockHeight` unlike the implementation that was diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index 395ac8edc2..fcae170418 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::NonZeroUsize; 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 dividend and remainder of a division operation. +pub struct DivRem { + dividend: A, + remainder: A, +} + +impl DivRem { + /// Returns the dividend portion of the value. + pub fn dividend(&self) -> &A { + &self.dividend + } + + /// 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 dividend and remainder. + pub fn div_with_remainder(&self, divisor: NonZeroUsize) -> DivRem { + let divisor = u64::try_from(usize::from(divisor)).expect("divisor fits into a u64"); + DivRem { + dividend: 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 5157255a34..4ade66af93 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -13,6 +13,8 @@ and this library adheres to Rust's notion of - `WalletSummary::progress` - `WalletMeta` - `impl Default for wallet::input_selection::GreedyInputSelector` +- `zcash_client_backend::fees::SplitPolicy` +- `zcash_client_backend::fees::zip317::MultiOutputChangeStrategy` ### Changed - `zcash_client_backend::data_api`: diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index bc2dad95c2..03083a5cce 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -794,7 +794,7 @@ impl SpendableNotes { } } -/// Metadata about the structure of the wallet for a particular account. +/// Metadata about the structure of the wallet for a particular account. /// /// At present this just contains counts of unspent outputs in each pool, but it may be extended in /// the future to contain note values or other more detailed information about wallet structure. diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index dfba19e6fd..98d6efd869 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -2,7 +2,7 @@ use std::{ cmp::Eq, convert::Infallible, hash::Hash, - num::{NonZeroU32, NonZeroU8}, + num::{NonZeroU32, NonZeroU8, NonZeroUsize}, }; use assert_matches::assert_matches; @@ -17,7 +17,7 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ components::amount::NonNegativeAmount, - fees::{fixed::FeeRule as FixedFeeRule, StandardFeeRule}, + fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule, StandardFeeRule}, Transaction, }, }; @@ -48,9 +48,9 @@ use crate::{ }, decrypt_transaction, fees::{ - fixed, + self, standard::{self, SingleOutputChangeStrategy}, - DustOutputPolicy, + DustOutputPolicy, SplitPolicy, }, scanning::ScanError, wallet::{Note, NoteId, OvkPolicy, ReceivedNote}, @@ -316,6 +316,175 @@ pub fn send_single_step_proposed_transfer( ); } +pub fn send_with_multiple_change_outputs( + dsf: impl DataStoreFactory, + cache: impl TestCache, +) { + let mut st = TestBuilder::new() + .with_data_store_factory(dsf) + .with_block_cache(cache) + .with_account_from_sapling_activation(BlockHash([0; 32])) + .build(); + + let account = st.test_account().cloned().unwrap(); + let dfvk = T::test_account_fvk(&st); + + // Add funds to the wallet in a single note + let value = Zatoshis::const_from_u64(650_0000); + let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + st.scan_cached_blocks(h, 1); + + // Spendable balance matches total balance + assert_eq!(st.get_total_balance(account.id()), value); + assert_eq!(st.get_spendable_balance(account.id(), 1), value); + + assert_eq!( + st.wallet() + .block_max_scanned() + .unwrap() + .unwrap() + .block_height(), + h + ); + + let to_extsk = T::sk(&[0xf5; 32]); + let to: Address = T::sk_default_address(&to_extsk); + let request = zip321::TransactionRequest::new(vec![Payment::without_memo( + to.to_zcash_address(st.network()), + Zatoshis::const_from_u64(100_0000), + )]) + .unwrap(); + + let input_selector = GreedyInputSelector::new(); + let change_memo = "Test change memo".parse::().unwrap(); + let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + Zip317FeeRule::standard(), + Some(change_memo.clone().into()), + T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(2).unwrap(), + NonNegativeAmount::const_from_u64(100_0000), + ), + ); + + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request.clone(), + NonZeroU32::new(1).unwrap(), + ) + .unwrap(); + + let step = &proposal.steps().head; + assert_eq!(step.balance().proposed_change().len(), 2); + + let create_proposed_result = st.create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ); + assert_matches!(&create_proposed_result, Ok(txids) if txids.len() == 1); + + let sent_tx_id = create_proposed_result.unwrap()[0]; + + // Verify that the sent transaction was stored and that we can decrypt the memos + let tx = st + .wallet() + .get_transaction(sent_tx_id) + .unwrap() + .expect("Created transaction was stored."); + let ufvks = [(account.id(), account.usk().to_unified_full_viewing_key())] + .into_iter() + .collect(); + let d_tx = decrypt_transaction(st.network(), h + 1, &tx, &ufvks); + assert_eq!(T::decrypted_pool_outputs_count(&d_tx), 3); + + let mut found_tx_change_memo = false; + let mut found_tx_empty_memo = false; + T::with_decrypted_pool_memos(&d_tx, |memo| { + if Memo::try_from(memo).unwrap() == change_memo { + found_tx_change_memo = true + } + if Memo::try_from(memo).unwrap() == Memo::Empty { + found_tx_empty_memo = true + } + }); + assert!(found_tx_change_memo); + assert!(found_tx_empty_memo); + + // Verify that the stored sent notes match what we're expecting + let sent_note_ids = st + .wallet() + .get_sent_note_ids(&sent_tx_id, T::SHIELDED_PROTOCOL) + .unwrap(); + assert_eq!(sent_note_ids.len(), 3); + + // The sent memo should be the empty memo for the sent output, and the + // change output's memo should be as specified. + let mut change_memo_count = 0; + let mut found_sent_empty_memo = false; + for sent_note_id in sent_note_ids { + match st + .wallet() + .get_memo(sent_note_id) + .expect("Note id is valid") + .as_ref() + { + Some(m) if m == &change_memo => { + change_memo_count += 1; + } + Some(m) if m == &Memo::Empty => { + found_sent_empty_memo = true; + } + Some(other) => panic!("Unexpected memo value: {:?}", other), + None => panic!("Memo should not be stored as NULL"), + } + } + assert_eq!(change_memo_count, 2); + assert!(found_sent_empty_memo); + + let tx_history = st.wallet().get_tx_history().unwrap(); + assert_eq!(tx_history.len(), 2); + + let network = *st.network(); + assert_matches!( + decrypt_and_store_transaction(&network, st.wallet_mut(), &tx, None), + Ok(_) + ); + + let (h, _) = st.generate_next_block_including(sent_tx_id); + st.scan_cached_blocks(h, 1); + + // Now, create another proposal with more outputs requested. We have two change notes; + // we'll spend one of them, and then we'll generate 7 splits. + let change_strategy = fees::zip317::MultiOutputChangeStrategy::new( + Zip317FeeRule::standard(), + Some(change_memo.into()), + T::SHIELDED_PROTOCOL, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(8).unwrap(), + NonNegativeAmount::const_from_u64(10_0000), + ), + ); + + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + NonZeroU32::new(1).unwrap(), + ) + .unwrap(); + + let step = &proposal.steps().head; + assert_eq!(step.balance().proposed_change().len(), 7); +} + #[cfg(feature = "transparent-inputs")] pub fn send_multi_step_proposed_transfer( ds_factory: DSF, @@ -1414,7 +1583,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed Self { + Self { + target_output_count, + min_split_output_size, + } + } + + /// Constructs a [`SplitPolicy`] that prescribes a single output (no splitting). + pub fn single_output() -> Self { + Self { + target_output_count: NonZeroUsize::MIN, + min_split_output_size: NonNegativeAmount::ZERO, + } + } + + /// Returns the minimum value for a note resulting from splitting of change. + /// + /// If splitting change would result in notes of value less than the minimum split output size, + /// a smaller number of splits should be chosen. + pub fn min_split_output_size(&self) -> NonNegativeAmount { + self.min_split_output_size + } + + /// Returns the number of output notes to produce from the given total change value, given the + /// number of existing unspent notes in the account and this policy. + pub fn split_count( + &self, + existing_notes: usize, + total_change: NonNegativeAmount, + ) -> NonZeroUsize { + let mut split_count = + NonZeroUsize::new(usize::from(self.target_output_count).saturating_sub(existing_notes)) + .unwrap_or(NonZeroUsize::MIN); + + loop { + let per_output_change = total_change.div_with_remainder(split_count); + if split_count > NonZeroUsize::MIN + && *per_output_change.dividend() < self.min_split_output_size + { + // safety: `split_count` has just been verified to be > 1 + split_count = unsafe { NonZeroUsize::new_unchecked(usize::from(split_count) - 1) }; + } else { + return split_count; + } + } + } +} + /// `EphemeralBalance` describes the ephemeral input or output value for a transaction. It is used /// in fee computation for series of transactions that use an ephemeral transparent output in an /// intermediate step, such as when sending from a shielded pool to a [ZIP 320] "TEX" address. diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 59600cc9b2..5a889850df 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::NonZeroUsize; use zcash_primitives::{ consensus::{self, BlockHeight}, @@ -10,9 +11,11 @@ use zcash_primitives::{ }; use zcash_protocol::ShieldedProtocol; +use crate::data_api::WalletMeta; + use super::{ sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, - EphemeralBalance, TransactionBalance, + EphemeralBalance, SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -112,33 +115,26 @@ where } /// Decide which shielded pool change should go to if there is any. -pub(crate) fn single_change_output_policy( +pub(crate) fn select_change_pool( _net_flows: &NetFlows, _fallback_change_pool: ShieldedProtocol, -) -> (ShieldedProtocol, usize, usize) { +) -> ShieldedProtocol { // TODO: implement a less naive strategy for selecting the pool to which change will be sent. - let change_pool = { - #[cfg(feature = "orchard")] - if _net_flows.orchard_in.is_positive() || _net_flows.orchard_out.is_positive() { - // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs. - ShieldedProtocol::Orchard - } else if _net_flows.sapling_in.is_positive() || _net_flows.sapling_out.is_positive() { - // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any - // Sapling outputs, so that we avoid pool-crossing. - ShieldedProtocol::Sapling - } else { - // The flows are transparent, so there may not be change. If there is, the caller - // gets to decide where to shield it. - _fallback_change_pool - } - #[cfg(not(feature = "orchard"))] + #[cfg(feature = "orchard")] + if _net_flows.orchard_in.is_positive() || _net_flows.orchard_out.is_positive() { + // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs. + ShieldedProtocol::Orchard + } else if _net_flows.sapling_in.is_positive() || _net_flows.sapling_out.is_positive() { + // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any + // Sapling outputs, so that we avoid pool-crossing. ShieldedProtocol::Sapling - }; - ( - change_pool, - (change_pool == ShieldedProtocol::Sapling).into(), - (change_pool == ShieldedProtocol::Orchard).into(), - ) + } else { + // The flows are transparent, so there may not be change. If there is, the caller + // gets to decide where to shield it. + _fallback_change_pool + } + #[cfg(not(feature = "orchard"))] + ShieldedProtocol::Sapling } #[derive(Clone, Copy, Debug)] @@ -159,6 +155,7 @@ impl OutputManifest { self.sapling } + #[cfg(feature = "orchard")] pub(crate) fn orchard(&self) -> usize { self.orchard } @@ -169,17 +166,20 @@ pub(crate) struct SinglePoolBalanceConfig<'a, P, F> { fee_rule: &'a F, dust_output_policy: &'a DustOutputPolicy, default_dust_threshold: NonNegativeAmount, + split_policy: &'a SplitPolicy, fallback_change_pool: ShieldedProtocol, marginal_fee: NonNegativeAmount, grace_actions: usize, } impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { + #[allow(clippy::too_many_arguments)] pub(crate) fn new( params: &'a P, fee_rule: &'a F, dust_output_policy: &'a DustOutputPolicy, default_dust_threshold: NonNegativeAmount, + split_policy: &'a SplitPolicy, fallback_change_pool: ShieldedProtocol, marginal_fee: NonNegativeAmount, grace_actions: usize, @@ -189,6 +189,7 @@ impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { fee_rule, dust_output_policy, default_dust_threshold, + split_policy, fallback_change_pool, marginal_fee, grace_actions, @@ -197,13 +198,9 @@ impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { } #[allow(clippy::too_many_arguments)] -pub(crate) fn single_change_output_balance< - P: consensus::Parameters, - NoteRefT: Clone, - F: FeeRule, - E, ->( +pub(crate) fn single_pool_output_balance( cfg: SinglePoolBalanceConfig, + wallet_meta: Option<&WalletMeta>, target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], @@ -232,9 +229,32 @@ where ephemeral_balance, )?; - #[allow(unused_variables)] - let (change_pool, sapling_change, orchard_change) = - single_change_output_policy(&net_flows, cfg.fallback_change_pool); + let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool); + let target_change_counts = OutputManifest { + transparent: 0, + sapling: if change_pool == ShieldedProtocol::Sapling { + wallet_meta.map_or(1, |m| { + std::cmp::max( + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()), + 1, + ) + }) + } else { + 0 + }, + orchard: if change_pool == ShieldedProtocol::Orchard { + wallet_meta.map_or(1, |m| { + std::cmp::max( + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()), + 1, + ) + }) + } else { + 0 + }, + }; // We don't create a fully-transparent transaction if a change memo is used. let transparent = net_flows.is_transparent() && change_memo.is_none(); @@ -246,20 +266,17 @@ where // Is it certain that there will be a change output? If it is not certain, // we should call `check_for_uneconomic_inputs` with `possible_change` // including both possibilities. - let possible_change = + let possible_change = { // These are the situations where we might not have a change output. - if transparent || (cfg.dust_output_policy.action() == DustAction::AddDustToFee && change_memo.is_none()) { - vec![ - OutputManifest::ZERO, - OutputManifest { - transparent: 0, - sapling: sapling_change, - orchard: orchard_change - } - ] + if transparent + || (cfg.dust_output_policy.action() == DustAction::AddDustToFee + && change_memo.is_none()) + { + vec![OutputManifest::ZERO, target_change_counts] } else { - vec![OutputManifest { transparent: 0, sapling: sapling_change, orchard: orchard_change}] - }; + vec![target_change_counts] + } + }; check_for_uneconomic_inputs( transparent_inputs, @@ -293,7 +310,7 @@ where .bundle_type() .num_outputs( sapling.inputs().len(), - sapling.outputs().len() + sapling_change, + sapling.outputs().len() + target_change_counts.sapling(), ) .map_err(ChangeError::BundleError)?; @@ -307,7 +324,7 @@ where .bundle_type() .num_actions( orchard.inputs().len(), - orchard.outputs().len() + orchard_change, + orchard.outputs().len() + target_change_counts.orchard(), ) .map_err(ChangeError::BundleError)?; #[cfg(not(feature = "orchard"))] @@ -410,13 +427,35 @@ 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(split_count); + 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.dividend() + *per_output_change.remainder()) + .unwrap() + } else { + // For any other output, the change value will just be the + // dividend. + *per_output_change.dividend() + }, + change_memo.cloned(), + ) + }) + .collect(), fee_with_change, ) }; @@ -426,7 +465,7 @@ where .dust_threshold() .unwrap_or(cfg.default_dust_threshold); - if proposed_change < change_dust_threshold { + if per_output_change.dividend() < &change_dust_threshold { match cfg.dust_output_policy.action() { DustAction::Reject => { // Always allow zero-valued change even for the `Reject` policy: @@ -435,11 +474,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.dividend().is_zero() { simple_case() } else { - let shortfall = - (change_dust_threshold - proposed_change).ok_or_else(underflow)?; + let shortfall = (change_dust_threshold - *per_output_change.dividend()) + .ok_or_else(underflow)?; return Err(ChangeError::InsufficientFunds { available: total_in, diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 2d2938cfff..311c806bb4 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -14,9 +14,9 @@ use zcash_primitives::{ use crate::{data_api::InputSource, ShieldedProtocol}; use super::{ - common::{single_change_output_balance, SinglePoolBalanceConfig}, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - TransactionBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -86,18 +86,21 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { + let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, &self.dust_output_policy, self.fee_rule.fixed_fee(), + &split_policy, self.fallback_change_pool, NonNegativeAmount::ZERO, 0, ); - single_change_output_balance( + single_pool_output_balance( cfg, + None, target_height, transparent_inputs, transparent_outputs, diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 673dc7cf74..d7fc2d2423 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -15,12 +15,15 @@ use zcash_primitives::{ }, }; -use crate::{data_api::InputSource, ShieldedProtocol}; +use crate::{ + data_api::{InputSource, WalletMeta}, + ShieldedProtocol, +}; use super::{ - common::{single_change_output_balance, SinglePoolBalanceConfig}, + common::{single_pool_output_balance, SinglePoolBalanceConfig}, sapling as sapling_fees, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralBalance, - TransactionBalance, + SplitPolicy, TransactionBalance, }; #[cfg(feature = "orchard")] @@ -90,18 +93,118 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ephemeral_balance: Option<&EphemeralBalance>, _wallet_meta: Option<&Self::WalletMeta>, ) -> Result> { + let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( params, &self.fee_rule, &self.dust_output_policy, self.fee_rule.marginal_fee(), + &split_policy, self.fallback_change_pool, self.fee_rule.marginal_fee(), self.fee_rule.grace_actions(), ); - single_change_output_balance( + single_pool_output_balance( cfg, + None, + target_height, + transparent_inputs, + transparent_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, + self.change_memo.as_ref(), + ephemeral_balance, + ) + } +} + +/// A change strategy that attempts to split the change value into some number of equal-sized notes +/// as dictated by the included [`SplitPolicy`] value. +pub struct MultiOutputChangeStrategy { + fee_rule: Zip317FeeRule, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + meta_source: PhantomData, +} + +impl MultiOutputChangeStrategy { + /// Constructs a new [`MultiOutputChangeStrategy`] with the specified ZIP 317 + /// fee parameters, change memo, and change splitting policy. + /// + /// This change strategy will fall back to creating a single change output if insufficient + /// change value is available to create notes with at least the minimum value dictated by the + /// split policy. + /// + /// `fallback_change_pool`: the pool to which change will be sent if when more than one + /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. + /// `split_policy`: A policy value describing how the change value should be returned as + /// multiple notes. + pub fn new( + fee_rule: Zip317FeeRule, + change_memo: Option, + fallback_change_pool: ShieldedProtocol, + dust_output_policy: DustOutputPolicy, + split_policy: SplitPolicy, + ) -> Self { + Self { + fee_rule, + change_memo, + fallback_change_pool, + dust_output_policy, + split_policy, + meta_source: PhantomData, + } + } +} + +impl ChangeStrategy for MultiOutputChangeStrategy { + type FeeRule = Zip317FeeRule; + type Error = Zip317FeeError; + type MetaSource = I; + type WalletMeta = WalletMeta; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn fetch_wallet_meta( + &self, + meta_source: &Self::MetaSource, + account: ::AccountId, + exclude: &[::NoteRef], + ) -> Result::Error> { + meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) + } + + fn compute_balance( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling: &impl sapling_fees::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, + ephemeral_balance: Option<&EphemeralBalance>, + wallet_meta: Option<&Self::WalletMeta>, + ) -> Result> { + let cfg = SinglePoolBalanceConfig::new( + params, + &self.fee_rule, + &self.dust_output_policy, + self.fee_rule.marginal_fee(), + &self.split_policy, + self.fallback_change_pool, + self.fee_rule.marginal_fee(), + self.fee_rule.grace_actions(), + ); + + single_pool_output_balance( + cfg, + wallet_meta, target_height, transparent_inputs, transparent_outputs, @@ -116,7 +219,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(test)] mod tests { - use std::convert::Infallible; + use std::{convert::Infallible, num::NonZeroUsize}; use zcash_primitives::{ consensus::{Network, NetworkUpgrade, Parameters}, @@ -129,10 +232,11 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment}, + data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment, WalletMeta}, fees::{ tests::{TestSaplingInput, TestTransparentInput}, - ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, + zip317::MultiOutputChangeStrategy, + ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, SplitPolicy, }, ShieldedProtocol, }; @@ -184,6 +288,132 @@ mod tests { ); } + #[test] + fn change_without_dust_multi() { + let change_strategy = MultiOutputChangeStrategy::::new( + Zip317FeeRule::standard(), + None, + ShieldedProtocol::Sapling, + DustOutputPolicy::default(), + SplitPolicy::new( + NonZeroUsize::new(5).unwrap(), + NonNegativeAmount::const_from_u64(100_0000), + ), + ); + + { + // spend a single Sapling note and produce 5 outputs + let balance = |existing_notes| { + change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &[] as &[TestTransparentInput], + &[] as &[TxOut], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(750_0000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 100_0000, + ))][..], + ), + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + None, + Some(&WalletMeta::new( + existing_notes, + #[cfg(feature = "orchard")] + 0, + )), + ) + }; + + assert_matches!( + balance(0), + Ok(balance) if + balance.proposed_change() == [ + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(30000) + ); + + assert_matches!( + balance(2), + Ok(balance) if + balance.proposed_change() == [ + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(20000) + ); + } + + { + // spend a single Sapling note and produce 4 outputs, as the value of the note isn't + // sufficient to produce 5 + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &[] as &[TestTransparentInput], + &[] as &[TxOut], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(600_0000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 100_0000, + ))][..], + ), + #[cfg(feature = "orchard")] + &orchard_fees::EmptyBundleView, + None, + Some(&WalletMeta::new( + 0, + #[cfg(feature = "orchard")] + 0, + )), + ); + + // FIXME: The current splitting strategy may result in overpaying fees by a small amount. + // This may be acceptable for an initial implementation. + //assert_matches!( + // result, + // Ok(balance) if + // balance.proposed_change() == [ + // ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_3750), None), + // ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_3750), None), + // ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_3750), None), + // ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_3750), None), + // ] && + // balance.fee_required() == NonNegativeAmount::const_from_u64(25000) + //); + assert_matches!( + result, + Ok(balance) if + balance.proposed_change() == [ + 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), + ChangeValue::sapling(NonNegativeAmount::const_from_u64(124_2500), None), + ] && + balance.fee_required() == NonNegativeAmount::const_from_u64(30000) + ); + } + } + #[test] #[cfg(feature = "orchard")] fn cross_pool_change_without_dust() { diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 80edb6c5ef..35f3d9a064 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -36,6 +36,13 @@ pub(crate) fn send_single_step_proposed_transfer() { ) } +pub(crate) fn send_with_multiple_change_outputs() { + zcash_client_backend::data_api::testing::pool::send_with_multiple_change_outputs::( + TestDbFactory, + BlockCache::new(), + ) +} + #[cfg(feature = "transparent-inputs")] pub(crate) fn send_multi_step_proposed_transfer() { zcash_client_backend::data_api::testing::pool::send_multi_step_proposed_transfer::( diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index ec3b5f468b..890bfa7bbf 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -399,6 +399,11 @@ pub(crate) mod tests { testing::pool::send_single_step_proposed_transfer::() } + #[test] + fn send_with_multiple_change_outputs() { + testing::pool::send_with_multiple_change_outputs::() + } + #[test] #[cfg(feature = "transparent-inputs")] fn send_multi_step_proposed_transfer() { diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 67ed843d7c..27b4fde3ca 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -412,6 +412,11 @@ pub(crate) mod tests { testing::pool::send_single_step_proposed_transfer::() } + #[test] + fn send_with_multiple_change_outputs() { + testing::pool::send_with_multiple_change_outputs::() + } + #[test] #[cfg(feature = "transparent-inputs")] fn send_multi_step_proposed_transfer() { diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index dbd8f79346..0ac93fd217 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -1723,6 +1723,7 @@ mod tests { fn prop_usk_roundtrip(usk in arb_unified_spending_key(zcash_protocol::consensus::Network::MainNetwork)) { let encoded = usk.to_bytes(Era::Orchard); + #[allow(clippy::let_and_return)] let encoded_len = { let len = 4;