diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index d56465413..2d459bb09 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -804,20 +804,28 @@ impl SpendableNotes { /// the wallet. pub struct WalletMeta { sapling_note_count: usize, + sapling_total_value: NonNegativeAmount, #[cfg(feature = "orchard")] orchard_note_count: usize, + #[cfg(feature = "orchard")] + orchard_total_value: NonNegativeAmount, } impl WalletMeta { /// Constructs a new [`WalletMeta`] value from its constituent parts. pub fn new( sapling_note_count: usize, + sapling_total_value: NonNegativeAmount, #[cfg(feature = "orchard")] orchard_note_count: usize, + #[cfg(feature = "orchard")] orchard_total_value: NonNegativeAmount, ) -> Self { Self { sapling_note_count, + sapling_total_value, #[cfg(feature = "orchard")] orchard_note_count, + #[cfg(feature = "orchard")] + orchard_total_value, } } @@ -838,6 +846,11 @@ impl WalletMeta { self.sapling_note_count } + /// Returns the total value of Sapling notes represented by [`Self::sapling_note_count`]. + pub fn sapling_total_value(&self) -> NonNegativeAmount { + self.sapling_total_value + } + /// Returns the number of unspent Orchard notes belonging to the account for which this was /// generated. #[cfg(feature = "orchard")] @@ -845,11 +858,93 @@ impl WalletMeta { self.orchard_note_count } + /// Returns the total value of Orchard notes represented by [`Self::orchard_note_count`]. + #[cfg(feature = "orchard")] + pub fn orchard_total_value(&self) -> NonNegativeAmount { + self.orchard_total_value + } + /// 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) } + + /// Returns the total value of shielded notes represented by [`Self::total_note_count`] + pub fn total_value(&self) -> NonNegativeAmount { + #[cfg(feature = "orchard")] + let orchard_value = self.orchard_total_value; + #[cfg(not(feature = "orchard"))] + let orchard_value = NonNegativeAmount::ZERO; + + (self.sapling_total_value + orchard_value).expect("Does not overflow Zcash maximum value.") + } +} + +/// A `u8` value in the range 0..=MAX +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub struct BoundedU8(u8); + +impl BoundedU8 { + /// Creates a constant `BoundedU8` from a [`u8`] value. + /// + /// Panics: if the value is outside the range `0..=100`. + pub const fn new_const(value: u8) -> Self { + assert!(value <= MAX); + Self(value) + } + + /// Creates a `BoundedU8` from a [`u8`] value. + /// + /// Returns `None` if the provided value is outside the range `0..=100`. + pub fn new(value: u8) -> Option { + if value <= MAX { + Some(Self(value)) + } else { + None + } + } + + /// Returns the wrapped [`u8`] value. + pub fn value(&self) -> u8 { + self.0 + } +} + +/// A small query language for filtering notes belonging to an account. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum NoteSelector { + /// Selects notes having value greater than or equal to the provided value. + ExceedsMinValue(NonNegativeAmount), + /// Selects notes having value greater than or equal to the n'th percentile of previously sent + /// notes in the wallet. The wrapped value must be in the range `1..=100` + ExceedsPriorSendPercentile(BoundedU8<100>), + /// Selects notes having value greater than or equal to the specified percentage of the wallet + /// balance. The wrapped value must be in the range `1..=100` + ExceedsBalancePercentage(BoundedU8<100>), + /// A note will be selected if it satisfies both of the specified conditions. + /// + /// If it is not possible to evaluate one of the conditions (for example, + /// [`NoteSelector::ExceedsPriorSendPercentile`] cannot be evaluated if no sends have been + /// performed) then that condition will be ignored. + And(Box, Box), + /// A note will be selected if it satisfies the first condition; if it is not possible to + /// evaluate that condition (for example, [`NoteSelector::ExceedsPriorSendPercentile`] cannot + /// be evaluated if no sends have been performed) then the second condition will be used for + /// evaluation. + Attempt { + condition: Box, + fallback: Box, + }, +} + +impl NoteSelector { + pub fn attempt(condition: NoteSelector, fallback: NoteSelector) -> Self { + Self::Attempt { + condition: Box::new(condition), + fallback: Box::new(fallback), + } + } } /// A trait representing the capability to query a data store for unspent transaction outputs @@ -900,12 +995,15 @@ pub trait InputSource { /// /// The returned metadata value must exclude: /// - spent notes; - /// - unspent notes having value less than the specified minimum value; + /// - unspent notes excluded by the provided selector; /// - unspent notes identified in the given `exclude` list. + /// + /// Implementations of this method may limit the complexity of supported queries. Such + /// limitations should be clearly documented for the implementing type. fn get_wallet_metadata( &self, account: Self::AccountId, - min_value: NonNegativeAmount, + selector: &NoteSelector, exclude: &[Self::NoteRef], ) -> Result; diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 0c117c7c8..8fc2416d8 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -59,7 +59,6 @@ use crate::{ ShieldedProtocol, }; -use super::error::Error; use super::{ chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary}, scanning::ScanRange, @@ -74,6 +73,7 @@ use super::{ WalletCommitmentTrees, WalletMeta, WalletRead, WalletSummary, WalletTest, WalletWrite, SAPLING_SHARD_HEIGHT, }; +use super::{error::Error, NoteSelector}; #[cfg(feature = "transparent-inputs")] use { @@ -2354,7 +2354,7 @@ impl InputSource for MockWalletDb { fn get_wallet_metadata( &self, _account: Self::AccountId, - _min_value: NonNegativeAmount, + _selector: &NoteSelector, _exclude: &[Self::NoteRef], ) -> Result { Err(()) diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 846a68113..3e612b948 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -364,7 +364,7 @@ pub fn send_with_multiple_change_outputs( DustOutputPolicy::default(), SplitPolicy::new( NonZeroUsize::new(2).unwrap(), - NonNegativeAmount::const_from_u64(100_0000), + Some(NonNegativeAmount::const_from_u64(100_0000)), ), ); @@ -467,7 +467,7 @@ pub fn send_with_multiple_change_outputs( DustOutputPolicy::default(), SplitPolicy::new( NonZeroUsize::new(8).unwrap(), - NonNegativeAmount::const_from_u64(10_0000), + Some(NonNegativeAmount::const_from_u64(10_0000)), ), ); diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index a10136db5..6ef58f648 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -16,7 +16,7 @@ use zcash_primitives::{ }; use zcash_protocol::{PoolType, ShieldedProtocol}; -use crate::data_api::InputSource; +use crate::data_api::{BoundedU8, InputSource, Ratio}; pub mod common; #[cfg(feature = "non-standard-fees")] @@ -355,18 +355,24 @@ impl Default for DustOutputPolicy { #[derive(Clone, Copy, Debug)] pub struct SplitPolicy { target_output_count: NonZeroUsize, - min_split_output_size: NonNegativeAmount, + min_split_output_size: Option, + notes_must_exceed_prior_send_percentile: Option>, + notes_must_exceed_balance_percentage: Option>, } impl SplitPolicy { /// Constructs a new [`SplitPolicy`] from its constituent parts. pub fn new( target_output_count: NonZeroUsize, - min_split_output_size: NonNegativeAmount, + min_split_output_size: Option, + notes_must_exceed_prior_send_percentile: Option>, + notes_must_exceed_balance_percentage: Option>, ) -> Self { Self { target_output_count, min_split_output_size, + notes_must_exceed_prior_send_percentile, + notes_must_exceed_balance_percentage, } } @@ -374,7 +380,9 @@ impl SplitPolicy { pub fn single_output() -> Self { Self { target_output_count: NonZeroUsize::MIN, - min_split_output_size: NonNegativeAmount::ZERO, + min_split_output_size: None, + notes_must_exceed_prior_send_percentile: None, + notes_must_exceed_balance_percentage: None, } } @@ -382,36 +390,74 @@ impl SplitPolicy { /// /// 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 { + pub fn min_split_output_size(&self) -> Option { self.min_split_output_size } + /// Returns the bound on output size that is used to evaluate against prior send behavior. + /// + /// If splitting change would result in notes of value less than the `n`'th percentile of prior + /// send values, a smaller number of splits should be chosen. + pub fn notes_must_exceed_prior_send_percentile(&self) -> Option> { + self.notes_must_exceed_prior_send_percentile + } + + /// Returns the bound on output size that is used to evaluate against wallet balance. + /// + /// If splitting change would result in notes of value less than `n` percent of the wallet + /// balance, a smaller number of splits should be chosen. + pub fn notes_must_exceed_balance_percentage(&self) -> Option> { + self.notes_must_exceed_balance_percentage + } + /// 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. + /// total value and number of existing unspent notes in the account and this policy. pub fn split_count( &self, existing_notes: usize, + existing_notes_total: NonNegativeAmount, total_change: NonNegativeAmount, ) -> NonZeroUsize { + fn to_nonzero_u64(value: usize) -> NonZeroU64 { + NonZeroU64::new(u64::try_from(value).expect("usize fits into u64")) + .expect("NonZeroU64 input derived from 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; + let min_split_output_size = self.min_split_output_size.or_else(|| { + // If no minimum split output size is set, we choose the minimum split size to be a + // quarter of the average value of notes in the wallet after the transaction. + (existing_notes_total + total_change).map(|total| { + *total + .div_with_remainder(to_nonzero_u64( + usize::from(self.target_output_count).saturating_mul(4), + )) + .quotient() + }) + }); + + if let Some(min_split_output_size) = min_split_output_size { + loop { + let per_output_change = + total_change.div_with_remainder(to_nonzero_u64(usize::from(split_count))); + if *per_output_change.quotient() >= 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; + } } + } else { + // This is purely defensive; this case would only arise in the case that the addition + // of the existing notes with the total change overflows the maximum monetary amount. + // Since it's always safe to fall back to a single change value, this is better than a + // panic. + return NonZeroUsize::MIN; } } } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 2a1a4a4e2..ba4be4fa9 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -429,8 +429,11 @@ where // 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) + cfg.split_policy.split_count( + wm.total_note_count(), + wm.total_value(), + proposed_change, + ) }); let per_output_change = proposed_change.div_with_remainder( NonZeroU64::new( diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 7d89897be..9e4b4114e 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -14,7 +14,7 @@ use zcash_primitives::{ use zcash_protocol::value::{BalanceError, Zatoshis}; use crate::{ - data_api::{InputSource, WalletMeta}, + data_api::{InputSource, NoteSelector, WalletMeta}, fees::StandardFeeRule, ShieldedProtocol, }; @@ -342,7 +342,7 @@ mod tests { { // spend a single Sapling note and produce 5 outputs - let balance = |existing_notes| { + let balance = |existing_notes, total| { change_strategy.compute_balance( &Network::TestNetwork, Network::TestNetwork @@ -365,14 +365,17 @@ mod tests { None, &WalletMeta::new( existing_notes, + total, #[cfg(feature = "orchard")] 0, + #[cfg(feature = "orchard")] + NonNegativeAmount::ZERO, ), ) }; assert_matches!( - balance(0), + balance(0, NonNegativeAmount::ZERO), Ok(balance) if balance.proposed_change() == [ ChangeValue::sapling(NonNegativeAmount::const_from_u64(129_4000), None), @@ -385,7 +388,7 @@ mod tests { ); assert_matches!( - balance(2), + balance(2, NonNegativeAmount::const_from_u64(100_0000)), Ok(balance) if balance.proposed_change() == [ ChangeValue::sapling(NonNegativeAmount::const_from_u64(216_0000), None), @@ -421,8 +424,11 @@ mod tests { None, &WalletMeta::new( 0, + NonNegativeAmount::ZERO, #[cfg(feature = "orchard")] 0, + #[cfg(feature = "orchard")] + NonNegativeAmount::ZERO, ), ); diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 741003b6e..f08af7979 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -121,6 +121,9 @@ pub enum SqliteClientError { /// An error occurred in computing wallet balance BalanceError(BalanceError), + /// A note selection query contained an invalid constant or was otherwise not supported. + NoteSelectorInvalid(NoteSelector), + /// The proposal cannot be constructed until transactions with previously reserved /// ephemeral address outputs have been mined. The parameters are the account id and /// the index that could not safely be reserved. @@ -187,6 +190,7 @@ impl fmt::Display for SqliteClientError { SqliteClientError::ChainHeightUnknown => write!(f, "Chain height unknown; please call `update_chain_tip`"), SqliteClientError::UnsupportedPoolType(t) => write!(f, "Pool type is not currently supported: {}", t), SqliteClientError::BalanceError(e) => write!(f, "Balance error: {}", e), + SqliteClientError::NoteSelectorInvalid(s) => write!(f, "Could not evaluate selection query: {:?}", s), #[cfg(feature = "transparent-inputs")] SqliteClientError::ReachedGapLimit(account_id, bad_index) => write!(f, "The proposal cannot be constructed until transactions with previously reserved ephemeral address outputs have been mined. \ diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index f11022927..226d03822 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -51,9 +51,10 @@ use zcash_client_backend::{ chain::{BlockSource, ChainState, CommitmentTreeRoot}, scanning::{ScanPriority, ScanRange}, Account, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, - DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, - SentTransaction, SpendableNotes, TransactionDataRequest, WalletCommitmentTrees, WalletMeta, - WalletRead, WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, + DecryptedTransaction, InputSource, NoteSelector, NullifierQuery, ScannedBlock, + SeedRelevance, SentTransaction, SpendableNotes, TransactionDataRequest, + WalletCommitmentTrees, WalletMeta, WalletRead, WalletSummary, WalletWrite, + SAPLING_SHARD_HEIGHT, }, keys::{ AddressGenerationError, UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey, @@ -128,7 +129,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, - common::count_outputs, + common::spendable_notes_meta, SubtreeProgressEstimator, }; @@ -351,35 +352,39 @@ impl, P: consensus::Parameters> InputSource for fn get_wallet_metadata( &self, account_id: Self::AccountId, - min_value: NonNegativeAmount, + selector: &NoteSelector, + exclude: &[Self::NoteRef], 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( + let sapling_pool_meta = spendable_notes_meta( self.conn.borrow(), - account_id, - min_value, - exclude, ShieldedProtocol::Sapling, chain_tip_height, + account_id, + selector, + exclude, )?; #[cfg(feature = "orchard")] - let orchard_note_count = count_outputs( + let orchard_pool_meta = spendable_notes_meta( self.conn.borrow(), - account_id, - min_value, - exclude, ShieldedProtocol::Orchard, chain_tip_height, + account_id, + selector, + exclude, )?; Ok(WalletMeta::new( - sapling_note_count, + sapling_pool_meta.note_count, + sapling_pool_meta.total_value, + #[cfg(feature = "orchard")] + orchard_pool_meta.note_count, #[cfg(feature = "orchard")] - orchard_note_count, + orchard_pool_meta.total_value, )) } } diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 378abbe1a..b0ca75018 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -3,8 +3,8 @@ use rusqlite::{named_params, types::Value, Connection, Row}; use std::rc::Rc; -use zcash_client_backend::{wallet::ReceivedNote, ShieldedProtocol}; -use zcash_primitives::transaction::{components::amount::NonNegativeAmount, TxId}; +use zcash_client_backend::{data_api::NoteSelector, wallet::ReceivedNote, ShieldedProtocol}; +use zcash_primitives::transaction::{components::amount::NonNegativeAmount, fees::zip317, TxId}; use zcash_protocol::consensus::{self, BlockHeight}; use super::wallet_birthday; @@ -226,16 +226,83 @@ where .collect::>() } -pub(crate) fn count_outputs( +pub(crate) struct PoolMeta { + pub(crate) note_count: usize, + pub(crate) total_value: NonNegativeAmount, +} + +pub(crate) fn spendable_notes_meta( conn: &rusqlite::Connection, - account: AccountId, - min_value: NonNegativeAmount, - exclude: &[ReceivedNoteId], protocol: ShieldedProtocol, chain_tip_height: BlockHeight, -) -> Result { + account: AccountId, + selector: &NoteSelector, + exclude: &[ReceivedNoteId], +) -> Result { let (table_prefix, _, _) = per_protocol_names(protocol); + let run_selection = |min_value| { + conn.query_row( + &format!( + "SELECT COUNT(*), SUM(rn.value) + 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 rn.value >= :min_value + AND accounts.id = :account_id + AND accounts.ufvk IS NOT NULL + AND rn.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 rns + JOIN transactions stx ON stx.id_tx = rns.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| Ok((row.get::<_, usize>(0)?, row.get::<_, Option>(1)?)), + ) + }; + + fn min_note_value( + selector: &NoteSelector, + ) -> Result, SqliteClientError> { + match selector { + NoteSelector::MinValue(v) => Ok(Some(v)), + NoteSelector::PriorSendPercentile(n) => { + if *n < 1 || *n > 100 { + return Err(SqliteClientError::NoteSelectorInvalid(selector.clone())); + } + + todo!() + } + NoteSelector::BalancePercentage(n) => { + if *n < 1 || *n > 100 { + return Err(SqliteClientError::NoteSelectorInvalid(selector.clone())); + } + + Ok(run_selection(zip317::MARGINAL_FEE)? + .and_then(|(_, total)| (total * 100).checked_div(n))) + } + NoteSelector::Try { + condition, + fallback, + } => match min_value(condition)? { + Some(n) => Ok(Some(n)), + None => min_value(fallback), + }, + } + } + let excluded: Vec = exclude .iter() .filter_map(|ReceivedNoteId(p, n)| { @@ -248,33 +315,13 @@ pub(crate) fn count_outputs( .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), - ) + let min_value = min_note_value(selector)?.unwrap_or(zip317::MARGINAL_FEE * 10); + let (note_count, total_value) = run_selection(min_value)?; + + Ok(PoolMeta { + note_count, + total_value: total_value.map_or(Ok(NonNegativeAmount::ZERO), |v| { + NonNegativeAmount::from_nonnegative_i64(v).map_err(SqliteClientError::BalanceError) + })?, + }) }