Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions

components/zcash_protocol/CHANGELOG.md

@@ -7,6 +7,10 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 
+### Added
+- `zcash_protocol::value::DivRem`
+- `zcash_protocol::value::Zatoshis::div_with_remainder`
+
 ## [0.4.0] - 2024-10-02
 ### Added
 - `impl Sub<BlockHeight> for BlockHeight` unlike the implementation that was

components/zcash_protocol/src/value.rs

@@ -1,6 +1,7 @@
 use std::convert::{Infallible, TryFrom};
 use std::error;
 use std::iter::Sum;
+use std::num::NonZeroU64;
 use std::ops::{Add, Mul, Neg, Sub};
 
 use memuse::DynamicUsage;
@@ -229,6 +230,24 @@ impl Mul<usize> for ZatBalance {
 #[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Eq, Ord)]
 pub struct Zatoshis(u64);
 
+/// A struct that provides both the quotient and remainder of a division operation.
+pub struct QuotRem<A> {
+    quotient: A,
+    remainder: A,
+}
+
+impl<A> QuotRem<A> {
+    /// Returns the quotient portion of the value.
+    pub fn quotient(&self) -> &A {
+        &self.quotient
+    }
+
+    /// Returns the remainder portion of the value.
+    pub fn remainder(&self) -> &A {
+        &self.remainder
+    }
+}
+
 impl Zatoshis {
     /// Returns the identity `Zatoshis`
     pub const ZERO: Self = Zatoshis(0);
@@ -298,6 +317,15 @@ impl Zatoshis {
     pub fn is_positive(&self) -> bool {
         self > &Zatoshis::ZERO
     }
+
+    /// Divides this `Zatoshis` value by the given divisor and returns the quotient and remainder.
+    pub fn div_with_remainder(&self, divisor: NonZeroU64) -> QuotRem<Zatoshis> {
+        let divisor = u64::from(divisor);
+        QuotRem {
+            quotient: Zatoshis(self.0 / divisor),
+            remainder: Zatoshis(self.0 % divisor),
+        }
+    }
 }
 
 impl From<Zatoshis> for ZatBalance {

zcash_client_backend/CHANGELOG.md

@@ -11,14 +11,74 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api`:
   - `Progress`
   - `WalletSummary::progress`
+  - `WalletMeta`
+  - `impl Default for wallet::input_selection::GreedyInputSelector`
+- `zcash_client_backend::fees::{SplitPolicy, CommonChangeStrategy}`. These
+  allow specifying a policy for splitting change into more than one output,
+  which can help to make enough notes available to perform multiple spends
+  without waiting for change.
+
+### Changed
+- `zcash_client_backend::data_api`:
+  - `InputSource` has an added method `get_wallet_metadata`
+  - `error::Error` has additional variant `Error::Change`. This necessitates
+    the addition of two type parameters to the `Error` type,
+    `ChangeErrT` and `NoteRefT`.
+  - The following methods each now take an additional `change_strategy`
+    argument, along with an associated `ChangeT` type parameter:
+    - `wallet::spend`
+    - `wallet::propose_transfer`
+    - `wallet::propose_shielding`. This method also now takes an additional
+      `to_account` argument.
+    - `wallet::shield_transparent_funds`. This method also now takes an
+      additional `to_account` argument.
+  - `wallet::input_selection::InputSelectionError` now has an additional `Change`
+    variant. This necessitates the addition of two type parameters.
+  - `wallet::input_selection::InputSelector::propose_transaction` takes an
+    additional `change_strategy` argument, along with an associated `ChangeT`
+    type parameter.
+  - The `wallet::input_selection::InputSelector::FeeRule` associated type has
+    been removed. The fee rule is now part of the change strategy passed to
+    `propose_transaction`.
+  - `wallet::input_selection::ShieldingSelector::propose_shielding` takes an
+    additional `change_strategy` argument, along with an associated `ChangeT`
+    type parameter. In addition, it also takes a new `to_account` argument
+    that identifies the destination account for the shielded notes.
+  - The `wallet::input_selection::ShieldingSelector::FeeRule` associated type
+    has been removed. The fee rule is now part of the change strategy passed to
+    `propose_shielding`.
+  - The `Change` variant of `wallet::input_selection::GreedyInputSelectorError`
+    has been removed, along with the additional type parameters it necessitated.
+  - The arguments to `wallet::input_selection::GreedyInputSelector::new` have
+    changed.
+- `zcash_client_backend::fees`:
+  - `ChangeStrategy` has changed. It has two new associated types, `MetaSource`
+    and `WalletMeta`, and its `FeeRule` associated type now has an additional
+    `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and
+    the arguments to `compute_balance` have changed.
+- `zcash_client_backend::proto`:
+  - The `FeeRuleNotSpecified` variant of `FeeProposalDecodingError` has been
+    renamed to `FeeRuleNotSupported`. It is now used to report attempted use
+    of unsupported fee rules, as well as of an unspecified fee rule.
 
 ### Changed
 - Migrated to `arti-client 0.22`.
 
+### Deprecated
+- `zcash_client_backend::fees::{fixed,standard,zip317}::SingleOutputChangeStrategy::new`
+  have been deprecated: instead use `CommonChangeStrategy::simple`.
+  (For most uses, the type parameter of the fee rule will be inferred.)
+
 ### Removed
 - `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.
+  - `testing::single_output_change_strategy`. Use `fees::CommonChangeStrategy::simple`
+    instead.
+- `zcash_client_backend::fees`:
+  - `impl From<BalanceError> for ChangeError<...>`
 
 ## [0.14.0] - 2024-10-04
 
@@ -27,7 +87,7 @@ and this library adheres to Rust's notion of
   - `GAP_LIMIT`
   - `WalletSummary::recovery_progress`
   - `SpendableNotes::{take_sapling, take_orchard}`
-  - Tests and testing infrastructure have been migrated from the 
+  - Tests and testing infrastructure have been migrated from the
     `zcash_client_sqlite` internal tests to the `testing` module, and have been
     generalized so that they may be used for testing arbitrary implementations
     of the `zcash_client_backend::data_api` interfaces. The following have been

zcash_client_backend/src/data_api.rs

@@ -794,6 +794,64 @@ impl<NoteRef> SpendableNotes<NoteRef> {
     }
 }
 
+/// Metadata about the structure of the wallet for a particular account.
+///
+/// At present this just contains counts of unspent outputs in each pool, but it may be extended in
+/// the future to contain note values or other more detailed information about wallet structure.
+///
+/// Values of this type are intended to be used in selection of change output values. A value of
+/// this type may represent filtered data, and may therefore not count all of the unspent notes in
+/// the wallet.
+pub struct WalletMeta {
+    sapling_note_count: usize,
+    #[cfg(feature = "orchard")]
+    orchard_note_count: usize,
+}
+
+impl WalletMeta {
+    /// Constructs a new [`WalletMeta`] value from its constituent parts.
+    pub fn new(
+        sapling_note_count: usize,
+        #[cfg(feature = "orchard")] orchard_note_count: usize,
+    ) -> Self {
+        Self {
+            sapling_note_count,
+            #[cfg(feature = "orchard")]
+            orchard_note_count,
+        }
+    }
+
+    /// Returns the number of unspent notes in the wallet for the given shielded protocol.
+    pub fn note_count(&self, protocol: ShieldedProtocol) -> usize {
+        match protocol {
+            ShieldedProtocol::Sapling => self.sapling_note_count,
+            #[cfg(feature = "orchard")]
+            ShieldedProtocol::Orchard => self.orchard_note_count,
+            #[cfg(not(feature = "orchard"))]
+            ShieldedProtocol::Orchard => 0,
+        }
+    }
+
+    /// Returns the number of unspent Sapling notes belonging to the account for which this was
+    /// generated.
+    pub fn sapling_note_count(&self) -> usize {
+        self.sapling_note_count
+    }
+
+    /// Returns the number of unspent Orchard notes belonging to the account for which this was
+    /// generated.
+    #[cfg(feature = "orchard")]
+    pub fn orchard_note_count(&self) -> usize {
+        self.orchard_note_count
+    }
+
+    /// Returns the total number of unspent shielded notes belonging to the account for which this
+    /// was generated.
+    pub fn total_note_count(&self) -> usize {
+        self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard)
+    }
+}
+
 /// A trait representing the capability to query a data store for unspent transaction outputs
 /// belonging to a wallet.
 #[cfg_attr(feature = "test-dependencies", delegatable_trait)]
@@ -838,6 +896,19 @@ pub trait InputSource {
         exclude: &[Self::NoteRef],
     ) -> Result<SpendableNotes<Self::NoteRef>, Self::Error>;
 
+    /// Returns metadata describing the structure of the wallet for the specified account.
+    ///
+    /// The returned metadata value must exclude:
+    /// - spent notes;
+    /// - unspent notes having value less than the specified minimum value;
+    /// - unspent notes identified in the given `exclude` list.
+    fn get_wallet_metadata(
+        &self,
+        account: Self::AccountId,
+        min_value: NonNegativeAmount,
+        exclude: &[Self::NoteRef],
+    ) -> Result<Option<WalletMeta>, Self::Error>;
+
     /// Fetches the transparent output corresponding to the provided `outpoint`.
     ///
     /// Returns `Ok(None)` if the UTXO is not known to belong to the wallet or is not

zcash_client_backend/src/data_api/error.rs

@@ -13,6 +13,7 @@ use zcash_primitives::transaction::{
 
 use crate::address::UnifiedAddress;
 use crate::data_api::wallet::input_selection::InputSelectorError;
+use crate::fees::ChangeError;
 use crate::proposal::ProposalError;
 use crate::PoolType;
 
@@ -23,7 +24,8 @@ use crate::wallet::NoteId;
 
 /// Errors that can occur as a consequence of wallet operations.
 #[derive(Debug)]
-pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
+pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError, ChangeErrT, NoteRefT>
+{
     /// An error occurred retrieving data from the underlying data source
     DataSource(DataSourceError),
 
@@ -33,6 +35,9 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     /// An error in note selection
     NoteSelection(SelectionError),
 
+    /// An error in change selection during transaction proposal construction
+    Change(ChangeError<ChangeErrT, NoteRefT>),
+
     /// An error in transaction proposal construction
     Proposal(ProposalError),
 
@@ -98,12 +103,14 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     PaysEphemeralTransparentAddress(String),
 }
 
-impl<DE, CE, SE, FE> fmt::Display for Error<DE, CE, SE, FE>
+impl<DE, TE, SE, FE, CE, N> fmt::Display for Error<DE, TE, SE, FE, CE, N>
 where
     DE: fmt::Display,
-    CE: fmt::Display,
+    TE: fmt::Display,
     SE: fmt::Display,
     FE: fmt::Display,
+    CE: fmt::Display,
+    N: fmt::Display,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
@@ -120,6 +127,9 @@ where
             Error::NoteSelection(e) => {
                 write!(f, "Note selection encountered the following error: {}", e)
             }
+            Error::Change(e) => {
+                write!(f, "Change output generation failed: {}", e)
+            }
             Error::Proposal(e) => {
                 write!(f, "Input selection attempted to construct an invalid proposal: {}", e)
             }
@@ -173,12 +183,14 @@ where
     }
 }
 
-impl<DE, CE, SE, FE> error::Error for Error<DE, CE, SE, FE>
+impl<DE, TE, SE, FE, CE, N> error::Error for Error<DE, TE, SE, FE, CE, N>
 where
     DE: Debug + Display + error::Error + 'static,
-    CE: Debug + Display + error::Error + 'static,
+    TE: Debug + Display + error::Error + 'static,
     SE: Debug + Display + error::Error + 'static,
     FE: Debug + Display + 'static,
+    CE: Debug + Display + error::Error + 'static,
+    N: Debug + Display + 'static,
 {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match &self {
@@ -192,35 +204,38 @@ where
     }
 }
 
-impl<DE, CE, SE, FE> From<builder::Error<FE>> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<builder::Error<FE>> for Error<DE, TE, SE, FE, CE, N> {
     fn from(e: builder::Error<FE>) -> Self {
         Error::Builder(e)
     }
 }
 
-impl<DE, CE, SE, FE> From<ProposalError> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<ProposalError> for Error<DE, TE, SE, FE, CE, N> {
     fn from(e: ProposalError) -> Self {
         Error::Proposal(e)
     }
 }
 
-impl<DE, CE, SE, FE> From<BalanceError> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<BalanceError> for Error<DE, TE, SE, FE, CE, N> {
     fn from(e: BalanceError) -> Self {
         Error::BalanceError(e)
     }
 }
 
-impl<DE, CE, SE, FE> From<ConversionError<&'static str>> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<ConversionError<&'static str>> for Error<DE, TE, SE, FE, CE, N> {
     fn from(value: ConversionError<&'static str>) -> Self {
         Error::Address(value)
     }
 }
 
-impl<DE, CE, SE, FE> From<InputSelectorError<DE, SE>> for Error<DE, CE, SE, FE> {
-    fn from(e: InputSelectorError<DE, SE>) -> Self {
+impl<DE, TE, SE, FE, CE, N> From<InputSelectorError<DE, SE, CE, N>>
+    for Error<DE, TE, SE, FE, CE, N>
+{
+    fn from(e: InputSelectorError<DE, SE, CE, N>) -> Self {
         match e {
             InputSelectorError::DataSource(e) => Error::DataSource(e),
             InputSelectorError::Selection(e) => Error::NoteSelection(e),
+            InputSelectorError::Change(e) => Error::Change(e),
             InputSelectorError::Proposal(e) => Error::Proposal(e),
             InputSelectorError::InsufficientFunds {
                 available,
@@ -235,20 +250,20 @@ impl<DE, CE, SE, FE> From<InputSelectorError<DE, SE>> for Error<DE, CE, SE, FE>
     }
 }
 
-impl<DE, CE, SE, FE> From<sapling::builder::Error> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<sapling::builder::Error> for Error<DE, TE, SE, FE, CE, N> {
     fn from(e: sapling::builder::Error) -> Self {
         Error::Builder(builder::Error::SaplingBuild(e))
     }
 }
 
-impl<DE, CE, SE, FE> From<transparent::builder::Error> for Error<DE, CE, SE, FE> {
+impl<DE, TE, SE, FE, CE, N> From<transparent::builder::Error> for Error<DE, TE, SE, FE, CE, N> {
     fn from(e: transparent::builder::Error) -> Self {
         Error::Builder(builder::Error::TransparentBuild(e))
     }
 }
 
-impl<DE, CE, SE, FE> From<ShardTreeError<CE>> for Error<DE, CE, SE, FE> {
-    fn from(e: ShardTreeError<CE>) -> Self {
+impl<DE, TE, SE, FE, CE, N> From<ShardTreeError<TE>> for Error<DE, TE, SE, FE, CE, N> {
+    fn from(e: ShardTreeError<TE>) -> Self {
         Error::CommitmentTree(e)
     }
 }