diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c983d39f5..32f6cd71a 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -24,12 +24,12 @@ and this library adheres to Rust's notion of `ChangeErrT` and `NoteRefT`. - The following methods each now take an additional `change_strategy` argument, along with an associated `ChangeT` type parameter: - - `zcash_client_backend::data_api::wallet::spend` - - `zcash_client_backend::data_api::wallet::propose_transfer` - - `zcash_client_backend::data_api::wallet::propose_shielding`. This method - also now takes an additional `to_account` argument. - - `zcash_client_backend::data_api::wallet::shield_transparent_funds`. This - method also now takes an additional `to_account` argument. + - `wallet::spend` + - `wallet::propose_transfer` + - `wallet::propose_shielding`. This method also now takes an additional + `to_account` argument. + - `wallet::shield_transparent_funds`. This method also now takes an + additional `to_account` argument. - `wallet::input_selection::InputSelectionError` now has an additional `Change` variant. This necessitates the addition of two type parameters. - `wallet::input_selection::InputSelector::propose_transaction` takes an @@ -49,20 +49,16 @@ and this library adheres to Rust's notion of has been removed, along with the additional type parameters it necessitated. - The arguments to `wallet::input_selection::GreedyInputSelector::new` have changed. -- `zcash_client_backend::fees::ChangeStrategy` has changed. It has two new - associated types, `MetaSource` and `WalletMeta`, and its `FeeRule` associated - type now has an additional `Clone` bound. In addition, it defines a new - `fetch_wallet_meta` method, and the arguments to `compute_balance` have - changed. -- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. -- `zcash_client_backend::fees::standard::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. -- `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new` - now takes an additional `DustOutputPolicy` argument. It also now carries - an additional type parameter. +- `zcash_client_backend::fees`: + - `ChangeStrategy` has changed. It has two new associated types, `MetaSource` + and `WalletMeta`, and its `FeeRule` associated type now has an additional + `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and + the arguments to `compute_balance` have changed. + - The following methods now take an additional `DustOutputPolicy` argument, + and carry an additional type parameter: + - `fixed::SingleOutputChangeStrategy::new` + - `standard::SingleOutputChangeStrategy::new` + - `zip317::SingleOutputChangeStrategy::new` ### Changed - Migrated to `arti-client 0.22`. @@ -71,6 +67,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api`: - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have been removed. Use `WalletSummary::progress` instead. + - `testing::input_selector` use explicit `InputSelector` constructors + directly instead. - `zcash_client_backend::fees`: - `impl From for ChangeError<...>` diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 03083a5cc..d56465413 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -848,12 +848,7 @@ impl WalletMeta { /// Returns the total number of unspent shielded notes belonging to the account for which this /// was generated. pub fn total_note_count(&self) -> usize { - #[cfg(feature = "orchard")] - let orchard_note_count = self.orchard_note_count; - #[cfg(not(feature = "orchard"))] - let orchard_note_count = 0; - - self.sapling_note_count + orchard_note_count + self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard) } } @@ -903,8 +898,10 @@ pub trait InputSource { /// Returns metadata describing the structure of the wallet for the specified account. /// - /// The returned metadata value must exclude spent notes and unspent notes having value less - /// than the specified minimum value or identified in the given exclude list. + /// The returned metadata value must exclude: + /// - spent notes; + /// - unspent notes having value less than the specified minimum value; + /// - unspent notes identified in the given `exclude` list. fn get_wallet_metadata( &self, account: Self::AccountId, diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 98d6efd86..3bef99a04 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -422,7 +422,7 @@ pub fn send_with_multiple_change_outputs( .unwrap(); assert_eq!(sent_note_ids.len(), 3); - // The sent memo should be the empty memo for the sent output, and the + // The sent memo should be the empty memo for the sent output, and each // change output's memo should be as specified. let mut change_memo_count = 0; let mut found_sent_empty_memo = false; diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index b0276f97e..6d9bf6f3a 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -115,6 +115,8 @@ where Ok(()) } +/// Errors that may be generated in construction of proposals for shielded->shielded or +/// shielded->transparent transfers. pub type ProposeTransferErrT = Error< ::Error, CommitmentTreeErrT, @@ -124,6 +126,8 @@ pub type ProposeTransferErrT = Error< <::InputSource as InputSource>::NoteRef, >; +/// Errors that may be generated in construction of proposals for transparent->shielded +/// wallet-internal transfers. #[cfg(feature = "transparent-inputs")] pub type ProposeShieldingErrT = Error< ::Error, @@ -134,6 +138,7 @@ pub type ProposeShieldingErrT = Error Infallible, >; +/// Errors that may be generated in combined creation and execution of transaction proposals. pub type CreateErrT = Error< ::Error, ::Error, @@ -143,6 +148,7 @@ pub type CreateErrT = Error< N, >; +/// Errors that may be generated in the execution of proposals that may send shielded inputs. pub type TransferErrT = Error< ::Error, ::Error, @@ -152,6 +158,7 @@ pub type TransferErrT = Error< <::InputSource as InputSource>::NoteRef, >; +/// Errors that may be generated in the execution of shielding proposals. #[cfg(feature = "transparent-inputs")] pub type ShieldErrT = Error< ::Error, diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index af816d798..e109d5847 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -393,13 +393,13 @@ impl SplitPolicy { ) .unwrap(), ); - if split_count > NonZeroUsize::MIN - && *per_output_change.quotient() < self.min_split_output_size - { - split_count = NonZeroUsize::new(usize::from(split_count) - 1) - .expect("split_count checked to be > 1"); - } else { + if *per_output_change.quotient() >= self.min_split_output_size { return split_count; + } else if let Some(new_count) = NonZeroUsize::new(usize::from(split_count) - 1) { + split_count = new_count; + } else { + // We always create at least one change output. + return NonZeroUsize::MIN; } } } @@ -452,7 +452,10 @@ pub trait ChangeStrategy { /// own trait that descends from [`InputSource`] and adds the required capabilities there, and /// then implement that trait for their desired database backend. type MetaSource: InputSource; - type WalletMeta; + + /// Tye type of wallet metadata that this change strategy relies upon in order to compute + /// change. + type WalletMetaT; /// Returns the fee rule that this change strategy will respect when performing /// balance computations. @@ -465,7 +468,7 @@ pub trait ChangeStrategy { meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error>; + ) -> Result::Error>; /// Computes the totals of inputs, suggested change amounts, and fees given the /// provided inputs and outputs being used to construct a transaction. @@ -502,7 +505,7 @@ pub trait ChangeStrategy { sapling: &impl sapling::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, + wallet_meta: Option<&Self::WalletMetaT>, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 9c221bbd5..45b77e5df 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -233,27 +233,20 @@ where )?; let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool); + let target_change_count = wallet_meta.map_or(1, |m| { + usize::from(cfg.split_policy.target_output_count) + .saturating_sub(m.total_note_count()) + .max(1) + }); let target_change_counts = OutputManifest { transparent: 0, sapling: if change_pool == ShieldedProtocol::Sapling { - wallet_meta.map_or(1, |m| { - std::cmp::max( - usize::from(cfg.split_policy.target_output_count) - .saturating_sub(m.total_note_count()), - 1, - ) - }) + target_change_count } else { 0 }, orchard: if change_pool == ShieldedProtocol::Orchard { - wallet_meta.map_or(1, |m| { - std::cmp::max( - usize::from(cfg.split_policy.target_output_count) - .saturating_sub(m.total_note_count()), - 1, - ) - }) + target_change_count } else { 0 }, diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 311c806bb..529e9cdfc 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -60,7 +60,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -71,7 +71,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -84,7 +84,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - _wallet_meta: Option<&Self::WalletMeta>, + _wallet_meta: Option<&Self::WalletMetaT>, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index dbaf8ace1..330ec6de3 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -64,7 +64,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = StandardFeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -75,7 +75,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -88,7 +88,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, + wallet_meta: Option<&Self::WalletMetaT>, ) -> Result> { #[allow(deprecated)] match self.fee_rule() { diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 862e61302..c342c4665 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -67,7 +67,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = (); + type WalletMetaT = (); fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -78,7 +78,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { _meta_source: &Self::MetaSource, _account: ::AccountId, _exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { Ok(()) } @@ -91,7 +91,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - _wallet_meta: Option<&Self::WalletMeta>, + _wallet_meta: Option<&Self::WalletMetaT>, ) -> Result> { let split_policy = SplitPolicy::single_output(); let cfg = SinglePoolBalanceConfig::new( @@ -139,10 +139,10 @@ impl MultiOutputChangeStrategy { /// change value is available to create notes with at least the minimum value dictated by the /// split policy. /// - /// `fallback_change_pool`: the pool to which change will be sent if when more than one - /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. - /// `split_policy`: A policy value describing how the change value should be returned as - /// multiple notes. + /// - `fallback_change_pool`: the pool to which change will be sent if when more than one + /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs. + /// - `split_policy`: A policy value describing how the change value should be returned as + /// multiple notes. pub fn new( fee_rule: Zip317FeeRule, change_memo: Option, @@ -165,7 +165,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { type FeeRule = Zip317FeeRule; type Error = Zip317FeeError; type MetaSource = I; - type WalletMeta = WalletMeta; + type WalletMetaT = WalletMeta; fn fee_rule(&self) -> &Self::FeeRule { &self.fee_rule @@ -176,7 +176,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { meta_source: &Self::MetaSource, account: ::AccountId, exclude: &[::NoteRef], - ) -> Result::Error> { + ) -> Result::Error> { meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude) } @@ -189,7 +189,7 @@ impl ChangeStrategy for MultiOutputChangeStrategy { sapling: &impl sapling_fees::BundleView, #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, ephemeral_balance: Option<&EphemeralBalance>, - wallet_meta: Option<&Self::WalletMeta>, + wallet_meta: Option<&Self::WalletMetaT>, ) -> Result> { let cfg = SinglePoolBalanceConfig::new( params, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index d8ab67b5e..30e6f3054 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -354,12 +354,16 @@ impl, P: consensus::Parameters> InputSource for min_value: NonNegativeAmount, exclude: &[Self::NoteRef], ) -> Result { + let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())? + .ok_or(SqliteClientError::ChainHeightUnknown)?; + let sapling_note_count = count_outputs( self.conn.borrow(), account_id, min_value, exclude, ShieldedProtocol::Sapling, + chain_tip_height, )?; #[cfg(feature = "orchard")] @@ -369,6 +373,7 @@ impl, P: consensus::Parameters> InputSource for min_value, exclude, ShieldedProtocol::Orchard, + chain_tip_height, )?; Ok(WalletMeta::new( diff --git a/zcash_client_sqlite/src/wallet/common.rs b/zcash_client_sqlite/src/wallet/common.rs index 707a75914..378abbe1a 100644 --- a/zcash_client_sqlite/src/wallet/common.rs +++ b/zcash_client_sqlite/src/wallet/common.rs @@ -232,6 +232,7 @@ pub(crate) fn count_outputs( min_value: NonNegativeAmount, exclude: &[ReceivedNoteId], protocol: ShieldedProtocol, + chain_tip_height: BlockHeight, ) -> Result { let (table_prefix, _, _) = per_protocol_names(protocol); @@ -265,13 +266,14 @@ pub(crate) fn count_outputs( JOIN transactions stx ON stx.id_tx = transaction_id WHERE stx.block IS NOT NULL -- the spending tx is mined OR stx.expiry_height IS NULL -- the spending tx will not expire - OR stx.expiry_height > :anchor_height -- the spending tx is unexpired + OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired )" ), named_params![ ":account_id": account.0, ":min_value": u64::from(min_value), - ":exclude": &excluded_ptr + ":exclude": &excluded_ptr, + ":chain_tip_height": u32::from(chain_tip_height) ], |row| row.get(0), )