From 3352671e1f1ac0e03cfda2192907d8b55c26cc5e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 17 Oct 2024 11:55:47 -0600 Subject: [PATCH 1/2] zcash_client_backend: Generalize & extend account metadata query API This generalizes the previous account metadata query API to be able to represent more complex queries, and also to return note totals in addition to note counts. --- components/zcash_protocol/CHANGELOG.md | 2 + components/zcash_protocol/src/value.rs | 22 +- zcash_client_backend/CHANGELOG.md | 9 +- zcash_client_backend/src/data_api.rs | 237 +++++++++++++++--- zcash_client_backend/src/data_api/testing.rs | 59 ++++- .../src/data_api/testing/pool.rs | 120 +++++++-- zcash_client_backend/src/fees.rs | 109 +++++--- zcash_client_backend/src/fees/common.rs | 18 +- zcash_client_backend/src/fees/fixed.rs | 6 +- zcash_client_backend/src/fees/zip317.rs | 47 ++-- zcash_client_sqlite/CHANGELOG.md | 1 + zcash_client_sqlite/src/error.rs | 5 + zcash_client_sqlite/src/lib.rs | 41 ++- zcash_client_sqlite/src/testing/pool.rs | 7 + zcash_client_sqlite/src/wallet/common.rs | 234 ++++++++++++++--- zcash_client_sqlite/src/wallet/init.rs | 3 + zcash_client_sqlite/src/wallet/orchard.rs | 5 + zcash_client_sqlite/src/wallet/sapling.rs | 5 + .../src/transaction/fees/zip317.rs | 4 +- 19 files changed, 738 insertions(+), 196 deletions(-) diff --git a/components/zcash_protocol/CHANGELOG.md b/components/zcash_protocol/CHANGELOG.md index 9636526625..3291d5f9d6 100644 --- a/components/zcash_protocol/CHANGELOG.md +++ b/components/zcash_protocol/CHANGELOG.md @@ -10,6 +10,8 @@ and this library adheres to Rust's notion of ### Added - `zcash_protocol::value::QuotRem` - `zcash_protocol::value::Zatoshis::div_with_remainder` +- `impl Mul for zcash_protocol::value::Zatoshis` +- `impl Div for zcash_protocol::value::Zatoshis` ### Changed - MSRV is now 1.77.0. diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index 94f2b7ba64..aa4d1709bc 100644 --- a/components/zcash_protocol/src/value.rs +++ b/components/zcash_protocol/src/value.rs @@ -2,7 +2,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 std::ops::{Add, Div, Mul, Neg, Sub}; use memuse::DynamicUsage; @@ -321,6 +321,7 @@ impl Zatoshis { /// 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); + // `self` is already bounds-checked, so we don't need to re-check it in division QuotRem { quotient: Zatoshis(self.0 / divisor), remainder: Zatoshis(self.0 % divisor), @@ -394,11 +395,19 @@ impl Sub for Option { } } +impl Mul for Zatoshis { + type Output = Option; + + fn mul(self, rhs: u64) -> Option { + Zatoshis::from_u64(self.0.checked_mul(rhs)?).ok() + } +} + impl Mul for Zatoshis { type Output = Option; fn mul(self, rhs: usize) -> Option { - Zatoshis::from_u64(self.0.checked_mul(u64::try_from(rhs).ok()?)?).ok() + self * u64::try_from(rhs).ok()? } } @@ -414,6 +423,15 @@ impl<'a> Sum<&'a Zatoshis> for Option { } } +impl Div for Zatoshis { + type Output = Zatoshis; + + fn div(self, rhs: NonZeroU64) -> Zatoshis { + // `self` is already bounds-checked, so we don't need to re-check it + Zatoshis(self.0 / u64::from(rhs)) + } +} + /// A type for balance violations in amount addition and subtraction /// (overflow and underflow of allowed ranges) #[derive(Copy, Clone, Debug, PartialEq, Eq)] diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index aebf460767..a915fd5695 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -11,8 +11,11 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `Progress` - `WalletSummary::progress` - - `WalletMeta` + - `PoolMeta` + - `AccountMeta` - `impl Default for wallet::input_selection::GreedyInputSelector` + - `BoundedU8` + - `NoteFilter` - `zcash_client_backend::fees` - `SplitPolicy` - `StandardFeeRule` has been moved here from `zcash_primitives::fees`. Relative @@ -32,7 +35,7 @@ and this library adheres to Rust's notion of - MSRV is now 1.77.0. - Migrated to `arti-client 0.23`. - `zcash_client_backend::data_api`: - - `InputSource` has an added method `get_wallet_metadata` + - `InputSource` has an added method `get_account_metadata` - `error::Error` has additional variant `Error::Change`. This necessitates the addition of two type parameters to the `Error` type, `ChangeErrT` and `NoteRefT`. @@ -65,7 +68,7 @@ and this library adheres to Rust's notion of 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 + and `AccountMetaT`, 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. - `zip317::SingleOutputChangeStrategy` has been made polymorphic in the fee diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index ab827f70a6..a2c9a635ad 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -794,6 +794,35 @@ impl SpendableNotes { } } +/// Metadata about the structure of unspent outputs in a single pool within a wallet account. +/// +/// This type is often used to represent a filtered view of outputs in the account that were +/// selected according to the conditions imposed by a [`NoteFilter`]. +#[derive(Debug, Clone)] +pub struct PoolMeta { + note_count: usize, + value: NonNegativeAmount, +} + +impl PoolMeta { + /// Constructs a new [`PoolMeta`] value from its constituent parts. + pub fn new(note_count: usize, value: NonNegativeAmount) -> Self { + Self { note_count, value } + } + + /// Returns the number of unspent outputs in the account, potentially selected in accordance + /// with some [`NoteFilter`]. + pub fn note_count(&self) -> usize { + self.note_count + } + + /// Returns the total value of unspent outputs in the account that are accounted for in + /// [`Self::note_count`]. + pub fn value(&self) -> NonNegativeAmount { + self.value + } +} + /// 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 @@ -802,58 +831,185 @@ impl SpendableNotes { /// 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, +/// +/// A [`AccountMeta`] value is normally produced by querying the wallet database via passing a +/// [`NoteFilter`] to [`InputSource::get_account_metadata`]. +#[derive(Debug, Clone)] +pub struct AccountMeta { + sapling: Option, + orchard: Option, } -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, - } +impl AccountMeta { + /// Constructs a new [`AccountMeta`] value from its constituent parts. + pub fn new(sapling: Option, orchard: Option) -> Self { + Self { sapling, orchard } + } + + /// Returns metadata about Sapling notes belonging to the account for which this was generated. + /// + /// Returns [`None`] if no metadata is available or it was not possible to evaluate the query + /// described by a [`NoteFilter`] given the available wallet data. + pub fn sapling(&self) -> Option<&PoolMeta> { + self.sapling.as_ref() + } + + /// Returns metadata about Orchard notes belonging to the account for which this was generated. + /// + /// Returns [`None`] if no metadata is available or it was not possible to evaluate the query + /// described by a [`NoteFilter`] given the available wallet data. + pub fn orchard(&self) -> Option<&PoolMeta> { + self.orchard.as_ref() + } + + fn sapling_note_count(&self) -> Option { + self.sapling.as_ref().map(|m| m.note_count) + } + + fn orchard_note_count(&self) -> Option { + self.orchard.as_ref().map(|m| m.note_count) } /// Returns the number of unspent notes in the wallet for the given shielded protocol. - pub fn note_count(&self, protocol: ShieldedProtocol) -> usize { + pub fn note_count(&self, protocol: ShieldedProtocol) -> Option { match protocol { - ShieldedProtocol::Sapling => self.sapling_note_count, - #[cfg(feature = "orchard")] - ShieldedProtocol::Orchard => self.orchard_note_count, - #[cfg(not(feature = "orchard"))] - ShieldedProtocol::Orchard => 0, + ShieldedProtocol::Sapling => self.sapling_note_count(), + ShieldedProtocol::Orchard => self.orchard_note_count(), + } + } + + /// Returns the total number of unspent shielded notes belonging to the account for which this + /// was generated. + /// + /// Returns [`None`] if no metadata is available or it was not possible to evaluate the query + /// described by a [`NoteFilter`] given the available wallet data. If metadata is available + /// only for a single pool, the metadata for that pool will be returned. + pub fn total_note_count(&self) -> Option { + let s = self.sapling_note_count(); + let o = self.orchard_note_count(); + s.zip(o).map(|(s, o)| s + o).or(s).or(o) + } + + fn sapling_value(&self) -> Option { + self.sapling.as_ref().map(|m| m.value) + } + + fn orchard_value(&self) -> Option { + self.orchard.as_ref().map(|m| m.value) + } + + /// Returns the total value of shielded notes represented by [`Self::total_note_count`] + /// + /// Returns [`None`] if no metadata is available or it was not possible to evaluate the query + /// described by a [`NoteFilter`] given the available wallet data. If metadata is available + /// only for a single pool, the metadata for that pool will be returned. + pub fn total_value(&self) -> Option { + let s = self.sapling_value(); + let o = self.orchard_value(); + s.zip(o) + .map(|(s, o)| (s + o).expect("Does not overflow Zcash maximum value.")) + .or(s) + .or(o) + } +} + +/// 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..=MAX`. + 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..=MAX`. + pub fn new(value: u8) -> Option { + if value <= MAX { + Some(Self(value)) + } else { + None } } - /// 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 wrapped [`u8`] value. + pub fn value(&self) -> u8 { + self.0 } +} - /// 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 +impl From> for u8 { + fn from(value: BoundedU8) -> Self { + value.0 } +} - /// 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) +impl From> for usize { + fn from(value: BoundedU8) -> Self { + usize::from(value.0) + } +} + +/// A small query language for filtering notes belonging to an account. +/// +/// A filter described using this language is applied to notes individually. It is primarily +/// intended for retrieval of account metadata in service of making determinations for how to +/// allocate change notes, and is not currently intended for use in broader note selection +/// contexts. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum NoteFilter { + /// Selects notes having value greater than or equal to the provided value. + ExceedsMinValue(NonNegativeAmount), + /// Selects notes having value greater than or equal to approximately the n'th percentile of + /// previously sent notes in the account, irrespective of pool. The wrapped value must be in + /// the range `1..=99`. The value `n` is respected in a best-effort fashion; results are likely + /// to be inaccurate if the account has not yet completed scanning or if insufficient send data + /// is available to establish a distribution. + // TODO: it might be worthwhile to add an optional parameter here that can be used to ignore + // low-valued (test/memo-only) sends when constructing the distribution to be drawn from. + ExceedsPriorSendPercentile(BoundedU8<99>), + /// Selects notes having value greater than or equal to the specified percentage of the account + /// balance across all shielded pools. The wrapped value must be in the range `1..=99` + ExceedsBalancePercentage(BoundedU8<99>), + /// 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, + /// [`NoteFilter::ExceedsPriorSendPercentile`] cannot be evaluated if no sends have been + /// performed) then that condition will be ignored. If neither condition can be evaluated, + /// then the entire condition cannot be evaluated. + Combine(Box, Box), + /// A note will be selected if it satisfies the first condition; if it is not possible to + /// evaluate that condition (for example, [`NoteFilter::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 NoteFilter { + /// Constructs a [`NoteFilter::Combine`] query node. + pub fn combine(l: NoteFilter, r: NoteFilter) -> Self { + Self::Combine(Box::new(l), Box::new(r)) + } + + /// Constructs a [`NoteFilter::Attempt`] query node. + pub fn attempt(condition: NoteFilter, fallback: NoteFilter) -> 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 -/// belonging to a wallet. +/// belonging to a account. #[cfg_attr(feature = "test-dependencies", delegatable_trait)] pub trait InputSource { /// The type of errors produced by a wallet backend. @@ -900,14 +1056,17 @@ 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. - fn get_wallet_metadata( + /// + /// Implementations of this method may limit the complexity of supported queries. Such + /// limitations should be clearly documented for the implementing type. + fn get_account_metadata( &self, account: Self::AccountId, - min_value: NonNegativeAmount, + selector: &NoteFilter, exclude: &[Self::NoteRef], - ) -> Result; + ) -> Result; /// Fetches the transparent output corresponding to the provided `outpoint`. /// diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 0c117c7c8e..0041c24bc0 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -23,6 +23,7 @@ use secrecy::{ExposeSecret, Secret, SecretVec}; use shardtree::{error::ShardTreeError, store::memory::MemoryShardStore, ShardTree}; use subtle::ConditionallySelectable; +use zcash_address::ZcashAddress; use zcash_keys::address::Address; use zcash_note_encryption::Domain; use zcash_primitives::{ @@ -43,6 +44,7 @@ use zcash_protocol::{ value::{ZatBalance, Zatoshis}, }; use zip32::{fingerprint::SeedFingerprint, DiversifierIndex}; +use zip321::Payment; use crate::{ address::UnifiedAddress, @@ -59,7 +61,6 @@ use crate::{ ShieldedProtocol, }; -use super::error::Error; use super::{ chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary}, scanning::ScanRange, @@ -68,12 +69,13 @@ use super::{ input_selection::{GreedyInputSelector, InputSelector}, propose_standard_transfer_to_address, propose_transfer, }, - Account, AccountBalance, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata, - DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, + Account, AccountBalance, AccountBirthday, AccountMeta, AccountPurpose, AccountSource, + BlockMetadata, DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance, SentTransaction, SpendableNotes, TransactionDataRequest, TransactionStatus, - WalletCommitmentTrees, WalletMeta, WalletRead, WalletSummary, WalletTest, WalletWrite, + WalletCommitmentTrees, WalletRead, WalletSummary, WalletTest, WalletWrite, SAPLING_SHARD_HEIGHT, }; +use super::{error::Error, NoteFilter}; #[cfg(feature = "transparent-inputs")] use { @@ -861,6 +863,49 @@ where + WalletCommitmentTrees, ::AccountId: ConditionallySelectable + Default + Send + 'static, { + // Creates a transaction that sends the specified value from the given account to + // the provided recipient address, using a greedy input selector and the default + // mutli-output change strategy. + pub fn create_standard_transaction( + &mut self, + from_account: &TestAccount, + to: ZcashAddress, + value: NonNegativeAmount, + ) -> Result< + NonEmpty, + super::wallet::TransferErrT< + DbT, + GreedyInputSelector, + standard::MultiOutputChangeStrategy, + >, + > { + let input_selector = GreedyInputSelector::new(); + + #[cfg(not(feature = "orchard"))] + let fallback_change_pool = ShieldedProtocol::Sapling; + #[cfg(feature = "orchard")] + let fallback_change_pool = ShieldedProtocol::Orchard; + + let change_strategy = standard::SingleOutputChangeStrategy::new( + StandardFeeRule::Zip317, + None, + fallback_change_pool, + DustOutputPolicy::default(), + ); + + let request = + zip321::TransactionRequest::new(vec![Payment::without_memo(to, value)]).unwrap(); + + self.spend( + &input_selector, + &change_strategy, + from_account.usk(), + request, + OvkPolicy::Sender, + NonZeroU32::MIN, + ) + } + /// Prepares and executes the given [`zip321::TransactionRequest`] in a single step. #[allow(clippy::type_complexity)] pub fn spend( @@ -2351,12 +2396,12 @@ impl InputSource for MockWalletDb { Ok(SpendableNotes::empty()) } - fn get_wallet_metadata( + fn get_account_metadata( &self, _account: Self::AccountId, - _min_value: NonNegativeAmount, + _selector: &NoteFilter, _exclude: &[Self::NoteRef], - ) -> Result { + ) -> 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 846a681136..756be223aa 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, NonZeroUsize}, + num::{NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize}, }; use assert_matches::assert_matches; @@ -43,8 +43,8 @@ use crate::{ wallet::{ decrypt_and_store_transaction, input_selection::GreedyInputSelector, TransferErrT, }, - Account as _, AccountBirthday, DecryptedTransaction, InputSource, Ratio, - WalletCommitmentTrees, WalletRead, WalletSummary, WalletTest, WalletWrite, + Account as _, AccountBirthday, BoundedU8, DecryptedTransaction, InputSource, NoteFilter, + Ratio, WalletCommitmentTrees, WalletRead, WalletSummary, WalletTest, WalletWrite, }, decrypt_transaction, fees::{ @@ -362,7 +362,7 @@ pub fn send_with_multiple_change_outputs( Some(change_memo.clone().into()), T::SHIELDED_PROTOCOL, DustOutputPolicy::default(), - SplitPolicy::new( + SplitPolicy::with_min_output_value( NonZeroUsize::new(2).unwrap(), NonNegativeAmount::const_from_u64(100_0000), ), @@ -465,7 +465,7 @@ pub fn send_with_multiple_change_outputs( Some(change_memo.into()), T::SHIELDED_PROTOCOL, DustOutputPolicy::default(), - SplitPolicy::new( + SplitPolicy::with_min_output_value( NonZeroUsize::new(8).unwrap(), NonNegativeAmount::const_from_u64(10_0000), ), @@ -530,7 +530,7 @@ pub fn send_multi_step_proposed_transfer( // Add funds to the wallet. add_funds(st, value); - let expected_step0_fee = (zip317::MARGINAL_FEE * 3).unwrap(); + let expected_step0_fee = (zip317::MARGINAL_FEE * 3u64).unwrap(); let expected_step1_fee = zip317::MINIMUM_FEE; let expected_ephemeral = (transfer_amount + expected_step1_fee).unwrap(); let expected_step0_change = @@ -1123,7 +1123,7 @@ pub fn spend_fails_on_unverified_notes( st.scan_cached_blocks(h2 + 1, 8); // Total balance is value * number of blocks scanned (10). - assert_eq!(st.get_total_balance(account_id), (value * 10).unwrap()); + assert_eq!(st.get_total_balance(account_id), (value * 10u64).unwrap()); // Spend still fails assert_matches!( @@ -1150,15 +1150,15 @@ pub fn spend_fails_on_unverified_notes( st.scan_cached_blocks(h11, 1); // Total balance is value * number of blocks scanned (11). - assert_eq!(st.get_total_balance(account_id), (value * 11).unwrap()); + assert_eq!(st.get_total_balance(account_id), (value * 11u64).unwrap()); // Spendable balance at 10 confirmations is value * 2. assert_eq!( st.get_spendable_balance(account_id, 10), - (value * 2).unwrap() + (value * 2u64).unwrap() ); assert_eq!( st.get_pending_shielded_balance(account_id, 10), - (value * 9).unwrap() + (value * 9u64).unwrap() ); // Should now be able to generate a proposal @@ -1192,7 +1192,7 @@ pub fn spend_fails_on_unverified_notes( // TODO: send to an account so that we can check its balance. assert_eq!( st.get_total_balance(account_id), - ((value * 11).unwrap() + ((value * 11u64).unwrap() - (amount_sent + NonNegativeAmount::from_u64(10000).unwrap()).unwrap()) .unwrap() ); @@ -2124,7 +2124,7 @@ pub fn fully_funded_fully_private( st.generate_next_block(&p1_fvk, AddressType::DefaultExternal, note_value); st.scan_cached_blocks(account.birthday().height(), 2); - let initial_balance = (note_value * 2).unwrap(); + let initial_balance = (note_value * 2u64).unwrap(); assert_eq!(st.get_total_balance(account.id()), initial_balance); assert_eq!(st.get_spendable_balance(account.id(), 1), initial_balance); @@ -2307,7 +2307,7 @@ pub fn multi_pool_checkpoint( let next_to_scan = scanned.scanned_range().end; - let initial_balance = (note_value * 3).unwrap(); + let initial_balance = (note_value * 3u64).unwrap(); assert_eq!(st.get_total_balance(acct_id), initial_balance); assert_eq!(st.get_spendable_balance(acct_id, 1), initial_balance); @@ -2352,7 +2352,7 @@ pub fn multi_pool_checkpoint( let expected_change = (note_value - transfer_amount - expected_fee).unwrap(); assert_eq!( st.get_total_balance(acct_id), - ((note_value * 2).unwrap() + expected_change).unwrap() + ((note_value * 2u64).unwrap() + expected_change).unwrap() ); assert_eq!(st.get_pending_change(acct_id, 1), expected_change); @@ -2396,8 +2396,8 @@ pub fn multi_pool_checkpoint( ); let expected_final = (initial_balance + note_value - - (transfer_amount * 3).unwrap() - - (expected_fee * 3).unwrap()) + - (transfer_amount * 3u64).unwrap() + - (expected_fee * 3u64).unwrap()) .unwrap(); assert_eq!(st.get_total_balance(acct_id), expected_final); @@ -2972,3 +2972,89 @@ pub fn scan_cached_blocks_detects_spends_out_of_order( + ds_factory: DSF, + cache: TC, +) where + DSF: DataStoreFactory, + ::AccountId: std::fmt::Debug, + TC: TestCache, +{ + let mut st = TestBuilder::new() + .with_data_store_factory(ds_factory) + .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); + + // Create 10 blocks with successively increasing value + let value = NonNegativeAmount::const_from_u64(100_0000); + let (h0, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let mut note_values = vec![value]; + for i in 2..=10 { + let value = NonNegativeAmount::const_from_u64(i * 100_0000); + st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + note_values.push(value); + } + st.scan_cached_blocks(h0, 10); + + let test_meta = |st: &TestState, query, expected_count| { + let metadata = st + .wallet() + .get_account_metadata(account.id(), &query, &[]) + .unwrap(); + + assert_eq!(metadata.note_count(T::SHIELDED_PROTOCOL), expected_count); + }; + + test_meta( + &st, + NoteFilter::ExceedsMinValue(NonNegativeAmount::const_from_u64(1000_0000)), + Some(1), + ); + test_meta( + &st, + NoteFilter::ExceedsMinValue(NonNegativeAmount::const_from_u64(500_0000)), + Some(6), + ); + test_meta( + &st, + NoteFilter::ExceedsBalancePercentage(BoundedU8::new_const(10)), + Some(5), + ); + + // We haven't sent any funds yet, so we can't evaluate this query + test_meta( + &st, + NoteFilter::ExceedsPriorSendPercentile(BoundedU8::new_const(50)), + None, + ); + + // Spend half of each one of our notes, so that we can get a distribution of sent note values. + // FIXME: This test is currently excessively specialized to the `zcash_client_sqlite::WalletDb` + // implmentation of the `InputSource` trait. A better approach would be to create a test input + // source that can select a set of notes directly based upon their nullifiers. + let not_our_key = T::sk_to_fvk(&T::sk(&[0xf5; 32])); + let to = T::fvk_default_address(¬_our_key).to_zcash_address(st.network()); + let nz2 = NonZeroU64::new(2).unwrap(); + + for value in ¬e_values { + let txids = st + .create_standard_transaction(&account, to.clone(), *value / nz2) + .unwrap(); + st.generate_next_block_including(txids.head); + } + st.scan_cached_blocks(h0 + 10, 10); + + // Since we've spent half our notes, our remaining notes each have approximately half their + // original value. The 50th percentile of our spends should be 250_0000 ZAT, and half of our + // remaining notes should have value greater than that. + test_meta( + &st, + NoteFilter::ExceedsPriorSendPercentile(BoundedU8::new_const(50)), + Some(5), + ); +} diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index a10136db51..eba11c7e6f 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -10,7 +10,8 @@ use zcash_primitives::{ components::{amount::NonNegativeAmount, OutPoint}, fees::{ transparent::{self, InputSize}, - zip317 as prim_zip317, FeeRule, + zip317::{self as prim_zip317}, + FeeRule, }, }, }; @@ -352,21 +353,32 @@ impl Default for DustOutputPolicy { /// A policy that describes how change output should be split into multiple notes for the purpose /// of note management. +/// +/// If an account contains at least [`Self::target_output_count`] notes having at least value +/// [`Self::min_split_output_value`], this policy will recommend a single output; if the account +/// contains fewer such notes, this policy will recommend that multiple outputs be produced in +/// order to achieve the target. #[derive(Clone, Copy, Debug)] pub struct SplitPolicy { target_output_count: NonZeroUsize, - min_split_output_size: NonNegativeAmount, + min_split_output_value: Option, } impl SplitPolicy { - /// Constructs a new [`SplitPolicy`] from its constituent parts. - pub fn new( + /// In the case that no other conditions provided by the user are available to fall back on, + /// a default value of [`MARGINAL_FEE`] * 100 will be used as the "minimum usable note value" + /// when retrieving wallet metadata. + pub(crate) const MIN_NOTE_VALUE: NonNegativeAmount = NonNegativeAmount::const_from_u64(500000); + + /// Constructs a new [`SplitPolicy`] that splits change to ensure the given number of spendable + /// outputs exists within an account, each having at least the specified minimum note value. + pub fn with_min_output_value( target_output_count: NonZeroUsize, - min_split_output_size: NonNegativeAmount, + min_split_output_value: NonNegativeAmount, ) -> Self { Self { target_output_count, - min_split_output_size, + min_split_output_value: Some(min_split_output_value), } } @@ -374,44 +386,71 @@ impl SplitPolicy { pub fn single_output() -> Self { Self { target_output_count: NonZeroUsize::MIN, - min_split_output_size: NonNegativeAmount::ZERO, + min_split_output_value: None, } } + /// Returns the number of outputs that this policy will attempt to ensure that the wallet has + /// available for spending. + pub fn target_output_count(&self) -> NonZeroUsize { + self.target_output_count + } + /// 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 + pub fn min_split_output_value(&self) -> Option { + self.min_split_output_value } /// 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. + /// + /// If splitting change to produce [`Self::target_output_count`] would result in notes of value less than + /// [`Self::min_split_output_value`], then this will produce + /// pub fn split_count( &self, - existing_notes: usize, + existing_notes: Option, + existing_notes_total: Option, 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; + 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(usize::MAX)), + ) + .unwrap_or(NonZeroUsize::MIN); + + let min_split_output_value = self.min_split_output_value.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_value) = min_split_output_value { + 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_value { + 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 { + NonZeroUsize::MIN } } } @@ -466,7 +505,7 @@ pub trait ChangeStrategy { /// Tye type of wallet metadata that this change strategy relies upon in order to compute /// change. - type WalletMetaT; + type AccountMetaT; /// Returns the fee rule that this change strategy will respect when performing /// balance computations. @@ -479,7 +518,7 @@ pub trait ChangeStrategy { meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error>; + ) -> Result::Error>; /// Computes the totals of inputs, suggested change amounts, and fees given the /// provided inputs and outputs being used to construct a transaction. @@ -516,7 +555,7 @@ pub trait ChangeStrategy { sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: &Self::WalletMetaT, + wallet_meta: &Self::AccountMetaT, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 2a1a4a4e2d..8aa7f38f40 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -11,7 +11,7 @@ use zcash_primitives::{ }; use zcash_protocol::ShieldedProtocol; -use crate::data_api::WalletMeta; +use crate::data_api::AccountMeta; use super::{ sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, @@ -203,7 +203,7 @@ impl<'a, P, F> SinglePoolBalanceConfig<'a, P, F> { #[allow(clippy::too_many_arguments)] pub(crate) fn single_pool_output_balance( cfg: SinglePoolBalanceConfig, - wallet_meta: Option<&WalletMeta>, + wallet_meta: Option<&AccountMeta>, target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], @@ -235,7 +235,8 @@ where let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool); let target_change_count = wallet_meta.map_or(1, |m| { usize::from(cfg.split_policy.target_output_count) - .saturating_sub(m.total_note_count()) + // If we cannot determine a total note count, fall back to a single output + .saturating_sub(m.total_note_count().unwrap_or(usize::MAX)) .max(1) }); let target_change_counts = OutputManifest { @@ -429,8 +430,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( @@ -531,8 +535,8 @@ where // We can add a change output if necessary. assert!(fee_with_change <= fee_with_dust); - let reasonable_fee = - (fee_with_change + (MINIMUM_FEE * 10).unwrap()).ok_or_else(overflow)?; + let reasonable_fee = (fee_with_change + (MINIMUM_FEE * 10u64).unwrap()) + .ok_or_else(overflow)?; if fee_with_dust > reasonable_fee { // Defend against losing money by using AddDustToFee with a too-high diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 49cd861148..b755d653b3 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -60,7 +60,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; type MetaSource = I; - type WalletMetaT = (); + type AccountMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -71,7 +71,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -84,7 +84,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - _wallet_meta: &Self::WalletMetaT, + _wallet_meta: &Self::AccountMetaT, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 7d89897be6..e654bb56d7 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::{AccountMeta, InputSource, NoteFilter}, fees::StandardFeeRule, ShieldedProtocol, }; @@ -101,7 +101,7 @@ where type FeeRule = R; type Error = ::Error; type MetaSource = I; - type WalletMetaT = (); + type AccountMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -112,7 +112,7 @@ where _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -125,7 +125,7 @@ where sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - _wallet_meta: &Self::WalletMetaT, + _wallet_meta: &Self::AccountMetaT, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( @@ -204,7 +204,7 @@ where type FeeRule = R; type Error = ::Error; type MetaSource = I; - type WalletMetaT = WalletMeta; + type AccountMetaT = AccountMeta; fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -215,8 +215,14 @@ where meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error> { - meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) + ) -> Result::Error> { + let note_selector = NoteFilter::ExceedsMinValue( + self.split_policy + .min_split_output_value() + .unwrap_or(SplitPolicy::MIN_NOTE_VALUE), + ); + + meta_source.get_account_metadata(account, ¬e_selector, exclude) } fn compute_balance( @@ -228,7 +234,7 @@ where sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: &Self::WalletMetaT, + wallet_meta: &Self::AccountMetaT, ) -> Result> { let cfg = SinglePoolBalanceConfig::new( params, @@ -271,7 +277,9 @@ mod tests { use super::SingleOutputChangeStrategy; use crate::{ - data_api::{testing::MockWalletDb, wallet::input_selection::SaplingPayment, WalletMeta}, + data_api::{ + testing::MockWalletDb, wallet::input_selection::SaplingPayment, AccountMeta, PoolMeta, + }, fees::{ tests::{TestSaplingInput, TestTransparentInput}, zip317::MultiOutputChangeStrategy, @@ -334,7 +342,7 @@ mod tests { None, ShieldedProtocol::Sapling, DustOutputPolicy::default(), - SplitPolicy::new( + SplitPolicy::with_min_output_value( NonZeroUsize::new(5).unwrap(), NonNegativeAmount::const_from_u64(100_0000), ), @@ -342,7 +350,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 @@ -363,16 +371,12 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - &WalletMeta::new( - existing_notes, - #[cfg(feature = "orchard")] - 0, - ), + &AccountMeta::new(Some(PoolMeta::new(existing_notes, total)), None), ) }; 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 +389,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), @@ -419,10 +423,9 @@ mod tests { #[cfg(feature = "orchard")] &orchard_fees::EmptyBundleView, None, - &WalletMeta::new( - 0, - #[cfg(feature = "orchard")] - 0, + &AccountMeta::new( + Some(PoolMeta::new(0, NonNegativeAmount::ZERO)), + Some(PoolMeta::new(0, NonNegativeAmount::ZERO)), ), ); diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 035313f541..0851eb471e 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -15,6 +15,7 @@ and this library adheres to Rust's notion of - MSRV is now 1.77.0. - Migrated from `schemer` to our fork `schemerz`. - Migrated to `rusqlite 0.32`. +- `error::SqliteClientError` has additional variant `NoteSelectorInvalid` ## [0.12.2] - 2024-10-21 diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 741003b6ea..43be989395 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -5,6 +5,7 @@ use std::fmt; use shardtree::error::ShardTreeError; use zcash_address::ParseError; +use zcash_client_backend::data_api::NoteFilter; use zcash_client_backend::PoolType; use zcash_keys::keys::AddressGenerationError; use zcash_primitives::zip32; @@ -121,6 +122,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. + NoteFilterInvalid(NoteFilter), + /// 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 +191,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::NoteFilterInvalid(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 f110229273..74e2d23a4b 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -50,10 +50,10 @@ use zcash_client_backend::{ self, 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, + Account, AccountBirthday, AccountMeta, AccountPurpose, AccountSource, BlockMetadata, + DecryptedTransaction, InputSource, NoteFilter, NullifierQuery, ScannedBlock, SeedRelevance, + SentTransaction, SpendableNotes, TransactionDataRequest, WalletCommitmentTrees, WalletRead, + WalletSummary, WalletWrite, SAPLING_SHARD_HEIGHT, }, keys::{ AddressGenerationError, UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey, @@ -128,7 +128,7 @@ pub mod error; pub mod wallet; use wallet::{ commitment_tree::{self, put_shard_roots}, - common::count_outputs, + common::spendable_notes_meta, SubtreeProgressEstimator, }; @@ -348,39 +348,38 @@ impl, P: consensus::Parameters> InputSource for ) } - fn get_wallet_metadata( + /// Returns metadata for the spendable notes in the wallet. + fn get_account_metadata( &self, account_id: Self::AccountId, - min_value: NonNegativeAmount, + selector: &NoteFilter, exclude: &[Self::NoteRef], - ) -> Result { + ) -> 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, )?; + #[cfg(not(feature = "orchard"))] + let orchard_pool_meta = None; - Ok(WalletMeta::new( - sapling_note_count, - #[cfg(feature = "orchard")] - orchard_note_count, - )) + Ok(AccountMeta::new(sapling_pool_meta, orchard_pool_meta)) } } diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 8465a582a4..726a377034 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -254,3 +254,10 @@ pub(crate) fn scan_cached_blocks_detects_spends_out_of_order(TestDbFactory::default(), BlockCache::new()) } + +pub(crate) fn metadata_queries_exclude_unwanted_notes() { + zcash_client_backend::data_api::testing::pool::metadata_queries_exclude_unwanted_notes::( + TestDbFactory::default(), + BlockCache::new(), + ) +} diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 378abbe1a1..b76b1a2bf0 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -1,14 +1,24 @@ //! Functions common to Sapling and Orchard support in the wallet. use rusqlite::{named_params, types::Value, Connection, Row}; -use std::rc::Rc; +use std::{num::NonZeroU64, rc::Rc}; -use zcash_client_backend::{wallet::ReceivedNote, ShieldedProtocol}; +use zcash_client_backend::{ + data_api::{NoteFilter, PoolMeta}, + wallet::ReceivedNote, + ShieldedProtocol, +}; use zcash_primitives::transaction::{components::amount::NonNegativeAmount, TxId}; -use zcash_protocol::consensus::{self, BlockHeight}; +use zcash_protocol::{ + consensus::{self, BlockHeight}, + value::BalanceError, + PoolType, +}; use super::wallet_birthday; -use crate::{error::SqliteClientError, AccountId, ReceivedNoteId, SAPLING_TABLES_PREFIX}; +use crate::{ + error::SqliteClientError, wallet::pool_code, AccountId, ReceivedNoteId, SAPLING_TABLES_PREFIX, +}; #[cfg(feature = "orchard")] use crate::ORCHARD_TABLES_PREFIX; @@ -226,14 +236,14 @@ where .collect::>() } -pub(crate) fn count_outputs( +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: &NoteFilter, + exclude: &[ReceivedNoteId], +) -> Result, SqliteClientError> { let (table_prefix, _, _) = per_protocol_names(protocol); let excluded: Vec = exclude @@ -248,33 +258,181 @@ 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), - ) + fn zatoshis(value: i64) -> Result { + NonNegativeAmount::from_nonnegative_i64(value).map_err(|_| { + SqliteClientError::CorruptedData(format!("Negative received note value: {}", value)) + }) + } + + let run_selection = |min_value| { + conn.query_row_and_then::<_, SqliteClientError, _, _>( + &format!( + "SELECT COUNT(*), SUM(rn.value) + FROM {table_prefix}_received_notes rn + INNER JOIN accounts a ON a.id = rn.account_id + INNER JOIN transactions ON transactions.id_tx = rn.tx + WHERE rn.account_id = :account_id + AND a.ufvk IS NOT NULL + AND rn.value >= :min_value + 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)?.map(zatoshis).transpose()?, + )) + }, + ) + }; + + // Evaluates the provided note filter conditions against the wallet database in order to + // determine the minimum value of notes to be produced by note splitting. + fn min_note_value( + conn: &rusqlite::Connection, + table_prefix: &str, + account: AccountId, + selector: &NoteFilter, + chain_tip_height: BlockHeight, + ) -> Result, SqliteClientError> { + match selector { + NoteFilter::ExceedsMinValue(v) => Ok(Some(*v)), + NoteFilter::ExceedsPriorSendPercentile(n) => { + let mut bucket_query = conn.prepare( + "WITH bucketed AS ( + SELECT s.value, NTILE(10) OVER (ORDER BY s.value) AS bucket_index + FROM sent_notes s + JOIN transactions t ON s.tx = t.id_tx + WHERE s.from_account_id = :account_id + -- only count mined transactions + AND t.mined_height IS NOT NULL + -- exclude change and account-internal sends + AND (s.to_account_id IS NULL OR s.from_account_id != s.to_account_id) + ) + SELECT MAX(value) as value + FROM bucketed + GROUP BY bucket_index + ORDER BY bucket_index", + )?; + + let bucket_maxima = bucket_query + .query_and_then::<_, SqliteClientError, _, _>( + named_params![":account_id": account.0], + |row| { + NonNegativeAmount::from_nonnegative_i64(row.get::<_, i64>(0)?).map_err( + |_| { + SqliteClientError::CorruptedData(format!( + "Negative received note value: {}", + n.value() + )) + }, + ) + }, + )? + .collect::, _>>()?; + + // Pick a bucket index by scaling the requested percentile to the number of buckets + let i = (bucket_maxima.len() * usize::from(*n) / 100).saturating_sub(1); + Ok(bucket_maxima.get(i).copied()) + } + NoteFilter::ExceedsBalancePercentage(p) => { + let balance = conn.query_row_and_then::<_, SqliteClientError, _, _>( + &format!( + "SELECT SUM(rn.value) + FROM v_received_outputs rn + INNER JOIN accounts a ON a.id = rn.account_id + INNER JOIN transactions ON transactions.id_tx = rn.transaction_id + WHERE rn.account_id = :account_id + AND a.ufvk IS NOT NULL + AND transactions.mined_height IS NOT NULL + AND rn.pool != :transparent_pool + AND rn.id_within_pool_table NOT IN ( + SELECT rns.received_output_id + FROM v_received_output_spends rns + JOIN transactions stx ON stx.id_tx = rns.transaction_id + WHERE rns.pool == rn.pool + AND ( + 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, + ":chain_tip_height": u32::from(chain_tip_height), + ":transparent_pool": pool_code(PoolType::Transparent) + ], + |row| row.get::<_, Option>(0)?.map(zatoshis).transpose(), + )?; + + Ok(match balance { + None => None, + Some(b) => { + let numerator = (b * u64::from(p.value())).ok_or(BalanceError::Overflow)?; + Some(numerator / NonZeroU64::new(100).expect("Constant is nonzero.")) + } + }) + } + NoteFilter::Combine(a, b) => { + // All the existing note selectors set lower bounds on note value, so the "and" + // operation is just taking the maximum of the two lower bounds. + let a_min_value = + min_note_value(conn, table_prefix, account, a.as_ref(), chain_tip_height)?; + let b_min_value = + min_note_value(conn, table_prefix, account, b.as_ref(), chain_tip_height)?; + Ok(a_min_value + .zip(b_min_value) + .map(|(av, bv)| std::cmp::max(av, bv)) + .or(a_min_value) + .or(b_min_value)) + } + NoteFilter::Attempt { + condition, + fallback, + } => { + let cond = min_note_value( + conn, + table_prefix, + account, + condition.as_ref(), + chain_tip_height, + )?; + if cond.is_none() { + min_note_value(conn, table_prefix, account, fallback, chain_tip_height) + } else { + Ok(cond) + } + } + } + } + + // TODO: Simplify the query before executing it. Not worrying about this now because queries + // will be developer-configured, not end-user defined. + if let Some(min_value) = + min_note_value(conn, table_prefix, account, selector, chain_tip_height)? + { + let (note_count, total_value) = run_selection(min_value)?; + + Ok(Some(PoolMeta::new( + note_count, + total_value.unwrap_or(NonNegativeAmount::ZERO), + ))) + } else { + Ok(None) + } } diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index baed0a1297..b5fbc95397 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -230,6 +230,9 @@ fn sqlite_client_error_to_wallet_migration_error(e: SqliteClientError) -> Wallet SqliteClientError::EphemeralAddressReuse(_, _) => { unreachable!("we don't do ephemeral address tracking") } + SqliteClientError::NoteFilterInvalid(_) => { + unreachable!("we don't do note selection in migrations") + } } } diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 046b81f6b2..1b0e0cacb0 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -486,6 +486,11 @@ pub(crate) mod tests { testing::pool::scan_cached_blocks_detects_spends_out_of_order::() } + #[test] + fn metadata_queries_exclude_unwanted_notes() { + testing::pool::metadata_queries_exclude_unwanted_notes::() + } + #[test] fn pool_crossing_required() { testing::pool::pool_crossing_required::() diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index c9cf157a4a..9652761b1d 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -499,6 +499,11 @@ pub(crate) mod tests { testing::pool::scan_cached_blocks_detects_spends_out_of_order::() } + #[test] + fn metadata_queries_exclude_unwanted_notes() { + testing::pool::metadata_queries_exclude_unwanted_notes::() + } + #[test] #[cfg(feature = "orchard")] fn pool_crossing_required() { diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs index 63c8089d07..7ea41415c4 100644 --- a/zcash_primitives/src/transaction/fees/zip317.rs +++ b/zcash_primitives/src/transaction/fees/zip317.rs @@ -78,8 +78,8 @@ impl FeeRule { /// /// Using this fee rule with /// ```compile_fail - /// marginal_fee < 5000 || grace_actions < 2 \ - /// || p2pkh_standard_input_size > P2PKH_STANDARD_INPUT_SIZE \ + /// marginal_fee < 5000 || grace_actions < 2 + /// || p2pkh_standard_input_size > P2PKH_STANDARD_INPUT_SIZE /// || p2pkh_standard_output_size > P2PKH_STANDARD_OUTPUT_SIZE /// ``` /// violates ZIP 317, and might cause transactions built with it to fail. From 00cafa3b9e8e060faf37816c523aea54709d7f45 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 13 Nov 2024 13:45:45 -0700 Subject: [PATCH 2/2] Apply suggestions from code review & Clippy fixes. Co-authored by: Jack Grig Co-authored-by: Daira-Emma Hopwood --- components/zcash_protocol/src/value.rs | 6 ++- zcash_client_backend/src/fees.rs | 8 +-- zcash_client_sqlite/CHANGELOG.md | 2 +- zcash_client_sqlite/src/error.rs | 2 +- zcash_client_sqlite/src/wallet/common.rs | 66 ++++++++++-------------- 5 files changed, 37 insertions(+), 47 deletions(-) diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index aa4d1709bc..e7e5001dbc 100644 --- a/components/zcash_protocol/src/value.rs +++ b/components/zcash_protocol/src/value.rs @@ -321,7 +321,8 @@ impl Zatoshis { /// 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); - // `self` is already bounds-checked, so we don't need to re-check it in division + // `self` is already bounds-checked, and both the quotient and remainder + // are <= self, so we don't need to re-check them in division. QuotRem { quotient: Zatoshis(self.0 / divisor), remainder: Zatoshis(self.0 % divisor), @@ -427,7 +428,8 @@ impl Div for Zatoshis { type Output = Zatoshis; fn div(self, rhs: NonZeroU64) -> Zatoshis { - // `self` is already bounds-checked, so we don't need to re-check it + // `self` is already bounds-checked and the quotient is <= self, so + // we don't need to re-check it Zatoshis(self.0 / u64::from(rhs)) } } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index eba11c7e6f..1950c05092 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -368,6 +368,8 @@ impl SplitPolicy { /// In the case that no other conditions provided by the user are available to fall back on, /// a default value of [`MARGINAL_FEE`] * 100 will be used as the "minimum usable note value" /// when retrieving wallet metadata. + /// + /// [`MARGINAL_FEE`]: zcash_primitives::transaction::fees::zip317::MARGINAL_FEE pub(crate) const MIN_NOTE_VALUE: NonNegativeAmount = NonNegativeAmount::const_from_u64(500000); /// Constructs a new [`SplitPolicy`] that splits change to ensure the given number of spendable @@ -404,9 +406,9 @@ impl SplitPolicy { /// Returns the number of output notes to produce from the given total change value, given the /// total value and number of existing unspent notes in the account and this policy. /// - /// If splitting change to produce [`Self::target_output_count`] would result in notes of value less than - /// [`Self::min_split_output_value`], then this will produce - /// + /// If splitting change to produce [`Self::target_output_count`] would result in notes of value + /// less than [`Self::min_split_output_value`], then this will suggest a smaller number of + /// splits so that each resulting change note has sufficient value. pub fn split_count( &self, existing_notes: Option, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 0851eb471e..fcf0fa286e 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -15,7 +15,7 @@ and this library adheres to Rust's notion of - MSRV is now 1.77.0. - Migrated from `schemer` to our fork `schemerz`. - Migrated to `rusqlite 0.32`. -- `error::SqliteClientError` has additional variant `NoteSelectorInvalid` +- `error::SqliteClientError` has additional variant `NoteFilterInvalid` ## [0.12.2] - 2024-10-21 diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 43be989395..f8b2602bb1 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -191,7 +191,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::NoteFilterInvalid(s) => write!(f, "Could not evaluate selection query: {:?}", s), + SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate filter 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/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index b76b1a2bf0..9e94ea63af 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -241,7 +241,7 @@ pub(crate) fn spendable_notes_meta( protocol: ShieldedProtocol, chain_tip_height: BlockHeight, account: AccountId, - selector: &NoteFilter, + filter: &NoteFilter, exclude: &[ReceivedNoteId], ) -> Result, SqliteClientError> { let (table_prefix, _, _) = per_protocol_names(protocol); @@ -304,12 +304,11 @@ pub(crate) fn spendable_notes_meta( // determine the minimum value of notes to be produced by note splitting. fn min_note_value( conn: &rusqlite::Connection, - table_prefix: &str, account: AccountId, - selector: &NoteFilter, + filter: &NoteFilter, chain_tip_height: BlockHeight, ) -> Result, SqliteClientError> { - match selector { + match filter { NoteFilter::ExceedsMinValue(v) => Ok(Some(*v)), NoteFilter::ExceedsPriorSendPercentile(n) => { let mut bucket_query = conn.prepare( @@ -351,27 +350,24 @@ pub(crate) fn spendable_notes_meta( } NoteFilter::ExceedsBalancePercentage(p) => { let balance = conn.query_row_and_then::<_, SqliteClientError, _, _>( - &format!( - "SELECT SUM(rn.value) - FROM v_received_outputs rn - INNER JOIN accounts a ON a.id = rn.account_id - INNER JOIN transactions ON transactions.id_tx = rn.transaction_id - WHERE rn.account_id = :account_id - AND a.ufvk IS NOT NULL - AND transactions.mined_height IS NOT NULL - AND rn.pool != :transparent_pool - AND rn.id_within_pool_table NOT IN ( - SELECT rns.received_output_id - FROM v_received_output_spends rns - JOIN transactions stx ON stx.id_tx = rns.transaction_id - WHERE rns.pool == rn.pool - AND ( - 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 - ) - )" - ), + "SELECT SUM(rn.value) + FROM v_received_outputs rn + INNER JOIN accounts a ON a.id = rn.account_id + INNER JOIN transactions ON transactions.id_tx = rn.transaction_id + WHERE rn.account_id = :account_id + AND a.ufvk IS NOT NULL + AND transactions.mined_height IS NOT NULL + AND rn.pool != :transparent_pool + AND (rn.pool, rn.id_within_pool_table) NOT IN ( + SELECT rns.pool, rns.received_output_id + FROM v_received_output_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, ":chain_tip_height": u32::from(chain_tip_height), @@ -391,10 +387,8 @@ pub(crate) fn spendable_notes_meta( NoteFilter::Combine(a, b) => { // All the existing note selectors set lower bounds on note value, so the "and" // operation is just taking the maximum of the two lower bounds. - let a_min_value = - min_note_value(conn, table_prefix, account, a.as_ref(), chain_tip_height)?; - let b_min_value = - min_note_value(conn, table_prefix, account, b.as_ref(), chain_tip_height)?; + let a_min_value = min_note_value(conn, account, a.as_ref(), chain_tip_height)?; + let b_min_value = min_note_value(conn, account, b.as_ref(), chain_tip_height)?; Ok(a_min_value .zip(b_min_value) .map(|(av, bv)| std::cmp::max(av, bv)) @@ -405,15 +399,9 @@ pub(crate) fn spendable_notes_meta( condition, fallback, } => { - let cond = min_note_value( - conn, - table_prefix, - account, - condition.as_ref(), - chain_tip_height, - )?; + let cond = min_note_value(conn, account, condition.as_ref(), chain_tip_height)?; if cond.is_none() { - min_note_value(conn, table_prefix, account, fallback, chain_tip_height) + min_note_value(conn, account, fallback, chain_tip_height) } else { Ok(cond) } @@ -423,9 +411,7 @@ pub(crate) fn spendable_notes_meta( // TODO: Simplify the query before executing it. Not worrying about this now because queries // will be developer-configured, not end-user defined. - if let Some(min_value) = - min_note_value(conn, table_prefix, account, selector, chain_tip_height)? - { + if let Some(min_value) = min_note_value(conn, account, filter, chain_tip_height)? { let (note_count, total_value) = run_selection(min_value)?; Ok(Some(PoolMeta::new(