Add traits for fee estimation and input selection

This adds a set of abstractions that allow wallets to provide
independent strategies for fee estimation and note selection, and
implementations of these strategies that perform these operations in the
same fashion as the existing `spend` and `shield_transparent_funds`
functions.

This required a somewhat hefty rework of the error handling in
zcash_client_backend. It fixes an issue with the error types whereby
callees needed to have a bit too much information about the error
types produced by their callers.

Reflect the updated note selection and error handling in zcash_client_sqlite.

zcash_client_backend/CHANGELOG.md

@@ -28,6 +28,7 @@ and this library adheres to Rust's notion of
   - `AddressMetadata`
 - `zcash_client_backend::data_api`:
   - `PoolType`
+  - `ShieldedPool`
   - `Recipient`
   - `SentTransactionOutput`
   - `WalletRead::get_unified_full_viewing_keys`
@@ -42,6 +43,11 @@ and this library adheres to Rust's notion of
   - `WalletWrite::get_next_available_address`
   - `WalletWrite::put_received_transparent_utxo`
   - `impl From<prost::DecodeError> for error::Error`
+  - `chain::error`: a module containing error types type that that can occur only
+    in chain validation and sync have been separated out from errors related to
+    other wallet operations.
+  - `input_selection`: a module containing types related to the process
+    of selecting inputs to be spent, given a transaction request.
 - `zcash_client_backend::decrypt`:
   - `TransferType`
 - `zcash_client_backend::proto`:
@@ -50,6 +56,7 @@ and this library adheres to Rust's notion of
   - gRPC bindings for the `lightwalletd` server, behind a `lightwalletd-tonic`
     feature flag.
 - `zcash_client_backend::zip321::TransactionRequest` methods:
+  - `TransactionRequest::empty` for constructing a new empty request.
   - `TransactionRequest::new` for constructing a request from `Vec<Payment>`.
   - `TransactionRequest::payments` for accessing the `Payments` that make up a
     request.
@@ -72,6 +79,7 @@ and this library adheres to Rust's notion of
   - `zcash_client_backend::encoding::AddressCodec`
   - `zcash_client_backend::encoding::encode_payment_address`
   - `zcash_client_backend::encoding::encode_transparent_address`
+- `impl Eq for zcash_client_backend::address::UnifiedAddress`
 
 ### Changed
 - MSRV is now 1.56.1.
@@ -90,13 +98,8 @@ and this library adheres to Rust's notion of
     been replaced by `ephemeral_key`.
   - `zcash_client_backend::proto::compact_formats::CompactSaplingOutput`: the
     `epk` method has been replaced by `ephemeral_key`.
-- `data_api::wallet::spend_to_address` now takes a `min_confirmations`
-  parameter, which the caller can provide to specify the minimum number of
-  confirmations required for notes being selected. A default value of 10
-  confirmations is recommended.
 - Renamed the following in `zcash_client_backend::data_api` to use lower-case
   abbreviations (matching Rust naming conventions):
-  - `error::Error::InvalidExtSK` to `Error::InvalidExtSk`
   - `testing::MockWalletDB` to `testing::MockWalletDb`
 - Changes to the `data_api::WalletRead` trait:
   - `WalletRead::get_target_and_anchor_heights` now takes
@@ -107,6 +110,8 @@ and this library adheres to Rust's notion of
     `get_spendable_sapling_notes`
   - `WalletRead::select_spendable_notes` has been renamed to
     `select_spendable_sapling_notes`
+  - The `WalletRead::NoteRef` and `WalletRead::TxRef` associated types are now
+    required to implement `Eq` and `Ord`
 - The `zcash_client_backend::data_api::SentTransaction` type has been
   substantially modified to accommodate handling of transparent inputs.
   Per-output data has been split out into a new struct `SentTransactionOutput`
@@ -116,24 +121,28 @@ and this library adheres to Rust's notion of
   `store_decrypted_tx`.
 - `data_api::ReceivedTransaction` has been renamed to `DecryptedTransaction`,
   and its `outputs` field has been renamed to `sapling_outputs`.
-- `data_api::error::Error::Protobuf` now wraps `prost::DecodeError` instead of
+- `data_api::error::Error` has been substantially modified. It now wraps database,
-  `protobuf::ProtobufError`.
+  note selection, builder, and other errors
-- `data_api::error::Error` has the following additional cases:
+  - `Error::DataSource` has been added.
-  - `Error::BalanceError` in the case of amount addition overflow
+  - `Error::NoteSelection` has been added.
-    or subtraction underflow.
+  - `Error::BalanceError` has been added.
-  - `Error::MemoForbidden` to report the condition where a memo was
+  - `Error::MemoForbidden` has been added.
-    specified to be sent to a transparent recipient.
+  - `Error::AddressNotRecognized` has been added.
-  - `Error::TransparentInputsNotSupported` to represent the condition
+  - `Error::ChildIndexOutOfRange` has been added.
-    where a transparent spend has been requested of a wallet compiled without
+  - `Error::NoteMismatch` has been added.
-    the `transparent-inputs` feature.
+  - `Error::InsufficientBalance` has been renamed `InsufficientFunds` and
-  - `Error::AddressNotRecognized` to indicate that a transparent address from
+    restructured to have named fields.
-    which funds are being requested to be spent does not appear to be associated
+  - `Error::Protobuf` has been removed; these decoding errors are now
-    with this wallet.
+    produced as data source and/or block-source implementation-specific
-  - `Error::ChildIndexOutOfRange` to indicate that a diversifier index for an
+    errors.
-    address is out of range for valid transparent child indices.
+  - `Error::InvalidChain` has been removed; its former purpose is now served
-  - `Error::NoteMismatch` to indicate that a note being spent is not associated
+    by `zcash_client_backend::data_api::chain::ChainError`.
-    with either the internal or external full viewing keys corresponding to the
+  - `Error::InvalidNewWitnessAnchor` and `Error::InvalidWitnessAnchor` have
-    provided spending key.
+    been moved to `zcash_client_backend::data_api::chain::error::ContinuityError`
+  - `Error::InvalidExtSk` (now unused) has been removed.
+  - `Error::KeyNotFound` (now unused) has been removed.
+  - `Error::KeyDerivationError` (now unused) has been removed.
+  - `Error::SaplingNotActive` (now unused) has been removed.
 - `zcash_client_backend::decrypt`:
   - `decrypt_transaction` now takes a `HashMap<_, UnifiedFullViewingKey>`
     instead of `HashMap<_, ExtendedFullViewingKey>`.
@@ -160,8 +169,38 @@ and this library adheres to Rust's notion of
   - `decode_extended_spending_key`
   - `decode_extended_full_viewing_key`
   - `decode_payment_address`
-- `data_api::wallet::create_spend_to_address` has been modified to use a unified
+- `zcash_client_backend::wallet::SpendableNote` is now parameterized by a note
-  spending key rather than a Sapling extended spending key.
+  identifier type and has an additional `note_id` field that is used to hold the
+  identifier used to refer to the note in the wallet database.
+- `zcash_client_backend::data_api::BlockSource` has been moved to the
+  `zcash_client_backend::data_api::chain` module.
+- The types of the `with_row` callback argument to `BlockSource::with_blocks`
+  and the return type of this method have been modified to return
+  `zcash_client_backend::data_api::chain::error::Error`.
+- `zcash_client_backend::data_api::testing::MockBlockSource` has been moved to
+  `zcash_client_backend::data_api::chain::testing::MockBlockSource` module.
+- `zcash_client_backend::data_api::chain::{validate_chain, scan_cached_blocks}`
+  have altered parameters and result types. The latter have been modified to
+  return`zcash_client_backend::data_api::chain::error::Error` instead of
+  abstract error types. This new error type now wraps the errors of the block
+  source and wallet database to which these methods delegate IO operations
+  directly, which simplifies error handling in cases where callback functions
+  are involved.
+- `zcash_client_backend::data_api::error::ChainInvalid` has been moved to
+  `zcash_client_backend::data_api::chain::error`.
+- The error type of `zcash_client_backend::data_api::wallet::decrypt_and_store_transaction`
+  has been changed; the error type produced by the provided `WalletWrite` instance is
+  returned directly.
+
+### Deprecated
+- `zcash_client_backend::data_api::wallet::create_spend_to_address` has been
+  deprecated. Use `zcash_client_backend::data_api::wallet::spend` instead. If
+  you wish to continue using `create_spend_to_address`, note that the arguments
+  to the function has been modified to take a unified spending key instead of a
+  Sapling extended spending key, and now also requires a `min_confirmations`
+  argument that the caller can provide to specify a minimum number of
+  confirmations required for notes being selected. A minimum of 10
+  confirmations is recommended.
 
 ### Removed
 - `zcash_client_backend::data_api`:

zcash_client_backend/src/address.rs

@@ -36,7 +36,7 @@ impl AddressMetadata {
 }
 
 /// A Unified Address.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub struct UnifiedAddress {
     orchard: Option<orchard::Address>,
     sapling: Option<PaymentAddress>,

zcash_client_backend/src/data_api.rs

@@ -20,7 +20,6 @@ use crate::{
     address::{AddressMetadata, UnifiedAddress},
     decrypt::DecryptedOutput,
     keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
-    proto::compact_formats::CompactBlock,
     wallet::{SpendableNote, WalletTransparentOutput, WalletTx},
 };
 
@@ -44,14 +43,14 @@ pub trait WalletRead {
     ///
     /// For example, this might be a database identifier type
     /// or a UUID.
-    type NoteRef: Copy + Debug;
+    type NoteRef: Copy + Debug + Eq + Ord;
 
     /// Backend-specific transaction identifier.
     ///
     /// For example, this might be a database identifier type
     /// or a TxId if the backend is able to support that type
     /// directly.
-    type TxRef: Copy + Debug;
+    type TxRef: Copy + Debug + Eq + Ord;
 
     /// Returns the minimum and maximum block heights for stored blocks.
     ///
@@ -188,7 +187,7 @@ pub trait WalletRead {
         &self,
         account: AccountId,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error>;
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error>;
 
     /// Returns a list of spendable Sapling notes sufficient to cover the specified
     /// target value, if possible.
@@ -197,7 +196,7 @@ pub trait WalletRead {
         account: AccountId,
         target_value: Amount,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error>;
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error>;
 
     /// Returns the set of all transparent receivers associated with the given account.
     ///
@@ -227,7 +226,9 @@ pub trait WalletRead {
 }
 
 /// The subset of information that is relevant to this wallet that has been
-/// decrypted and extracted from a [CompactBlock].
+/// decrypted and extracted from a [`CompactBlock`].
+///
+/// [`CompactBlock`]: crate::proto::compact_formats::CompactBlock
 pub struct PrunedBlock<'a> {
     pub block_height: BlockHeight,
     pub block_hash: BlockHash,
@@ -268,6 +269,7 @@ pub enum PoolType {
     Transparent,
     /// The Sapling value pool
     Sapling,
+    // TODO: Orchard
 }
 
 /// A type that represents the recipient of a transaction output; a recipient address (and, for
@@ -386,23 +388,6 @@ pub trait WalletWrite: WalletRead {
     ) -> Result<Self::UtxoRef, Self::Error>;
 }
 
-/// This trait provides sequential access to raw blockchain data via a callback-oriented
-/// API.
-pub trait BlockSource {
-    type Error;
-
-    /// Scan the specified `limit` number of blocks from the blockchain, starting at
-    /// `from_height`, applying the provided callback to each block.
-    fn with_blocks<F>(
-        &self,
-        from_height: BlockHeight,
-        limit: Option<u32>,
-        with_row: F,
-    ) -> Result<(), Self::Error>
-    where
-        F: FnMut(CompactBlock) -> Result<(), Self::Error>;
-}
-
 #[cfg(feature = "test-dependencies")]
 pub mod testing {
     use secrecy::{ExposeSecret, SecretVec};
@@ -422,39 +407,17 @@ pub mod testing {
     use crate::{
         address::{AddressMetadata, UnifiedAddress},
         keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
-        proto::compact_formats::CompactBlock,
         wallet::{SpendableNote, WalletTransparentOutput},
     };
 
-    use super::{
+    use super::{DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead, WalletWrite};
-        error::Error, BlockSource, DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead,
-        WalletWrite,
-    };
-
-    pub struct MockBlockSource {}
-
-    impl BlockSource for MockBlockSource {
-        type Error = Error<u32>;
-
-        fn with_blocks<F>(
-            &self,
-            _from_height: BlockHeight,
-            _limit: Option<u32>,
-            _with_row: F,
-        ) -> Result<(), Self::Error>
-        where
-            F: FnMut(CompactBlock) -> Result<(), Self::Error>,
-        {
-            Ok(())
-        }
-    }
 
     pub struct MockWalletDb {
         pub network: Network,
     }
 
     impl WalletRead for MockWalletDb {
-        type Error = Error<u32>;
+        type Error = ();
         type NoteRef = u32;
         type TxRef = TxId;
 
@@ -514,7 +477,7 @@ pub mod testing {
         }
 
         fn get_transaction(&self, _id_tx: Self::TxRef) -> Result<Transaction, Self::Error> {
-            Err(Error::ScanRequired) // wrong error but we'll fix it later.
+            Err(())
         }
 
         fn get_commitment_tree(
@@ -544,7 +507,7 @@ pub mod testing {
             &self,
             _account: AccountId,
             _anchor_height: BlockHeight,
-        ) -> Result<Vec<SpendableNote>, Self::Error> {
+        ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
             Ok(Vec::new())
         }
 
@@ -553,7 +516,7 @@ pub mod testing {
             _account: AccountId,
             _target_value: Amount,
             _anchor_height: BlockHeight,
-        ) -> Result<Vec<SpendableNote>, Self::Error> {
+        ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
             Ok(Vec::new())
         }
 
@@ -591,7 +554,7 @@ pub mod testing {
             let account = AccountId::from(0);
             UnifiedSpendingKey::from_seed(&self.network, seed.expose_secret(), account)
                 .map(|k| (account, k))
-                .map_err(|_| Error::KeyDerivationError(account))
+                .map_err(|_| ())
         }
 
         fn get_next_available_address(

zcash_client_backend/src/data_api/chain.rs

@@ -12,42 +12,47 @@
 //!
 //! use zcash_client_backend::{
 //!     data_api::{
-//!         BlockSource, WalletRead, WalletWrite,
+//!         WalletRead, WalletWrite,
 //!         chain::{
-//!             validate_chain,
+//!             BlockSource,
+//!             error::Error,
 //!             scan_cached_blocks,
+//!             validate_chain,
+//!             testing as chain_testing,
 //!         },
-//!         error::Error,
 //!         testing,
 //!     },
 //! };
 //!
+//! # use std::convert::Infallible;
+//!
 //! # fn main() {
 //! #   test();
 //! # }
 //! #
-//! # fn test() -> Result<(), Error<u32>> {
+//! # fn test() -> Result<(), Error<(), Infallible, u32>> {
 //! let network = Network::TestNetwork;
-//! let db_cache = testing::MockBlockSource {};
+//! let block_source = chain_testing::MockBlockSource;
 //! let mut db_data = testing::MockWalletDb {
 //!     network: Network::TestNetwork
 //! };
 //!
-//! // 1) Download new CompactBlocks into db_cache.
+//! // 1) Download new CompactBlocks into block_source.
 //!
 //! // 2) Run the chain validator on the received blocks.
 //! //
 //! // Given that we assume the server always gives us correct-at-the-time blocks, any
 //! // errors are in the blocks we have previously cached or scanned.
-//! if let Err(e) = validate_chain(&network, &db_cache, db_data.get_max_height_hash()?) {
+//! let max_height_hash = db_data.get_max_height_hash().map_err(Error::Wallet)?;
+//! if let Err(e) = validate_chain(&network, &block_source, max_height_hash) {
 //!     match e {
-//!         Error::InvalidChain(lower_bound, _) => {
+//!         Error::Chain(e) => {
 //!             // a) Pick a height to rewind to.
 //!             //
 //!             // This might be informed by some external chain reorg information, or
 //!             // heuristics such as the platform, available bandwidth, size of recent
 //!             // CompactBlocks, etc.
-//!             let rewind_height = lower_bound - 10;
+//!             let rewind_height = e.at_height() - 10;
 //!
 //!             // b) Rewind scanned block information.
 //!             db_data.rewind_to_height(rewind_height);
@@ -74,12 +79,12 @@
 //! // At this point, the cache and scanned data are locally consistent (though not
 //! // necessarily consistent with the latest chain tip - this would be discovered the
 //! // next time this codepath is executed after new blocks are received).
-//! scan_cached_blocks(&network, &db_cache, &mut db_data, None)
+//! scan_cached_blocks(&network, &block_source, &mut db_data, None)
 //! # }
 //! # }
 //! ```
 
-use std::fmt::Debug;
+use std::convert::Infallible;
 
 use zcash_primitives::{
     block::BlockHash,
@@ -90,56 +95,75 @@ use zcash_primitives::{
 };
 
 use crate::{
-    data_api::{
+    data_api::{PrunedBlock, WalletWrite},
-        error::{ChainInvalid, Error},
-        BlockSource, PrunedBlock, WalletWrite,
-    },
     proto::compact_formats::CompactBlock,
     scan::BatchRunner,
     wallet::WalletTx,
     welding_rig::{add_block_to_runner, scan_block_with_runner},
 };
 
+pub mod error;
+use error::{ChainError, Error};
+
+/// This trait provides sequential access to raw blockchain data via a callback-oriented
+/// API.
+pub trait BlockSource {
+    type Error;
+
+    /// Scan the specified `limit` number of blocks from the blockchain, starting at
+    /// `from_height`, applying the provided callback to each block.
+    ///
+    /// * `WalletErrT`: the types of errors produced by the wallet operations performed
+    ///   as part of processing each row.
+    /// * `NoteRefT`: the type of note identifiers in the wallet data store, for use in
+    ///   reporting errors related to specific notes.
+    fn with_blocks<F, WalletErrT, NoteRefT>(
+        &self,
+        from_height: BlockHeight,
+        limit: Option<u32>,
+        with_row: F,
+    ) -> Result<(), error::Error<WalletErrT, Self::Error, NoteRefT>>
+    where
+        F: FnMut(CompactBlock) -> Result<(), error::Error<WalletErrT, Self::Error, NoteRefT>>;
+}
+
 /// Checks that the scanned blocks in the data database, when combined with the recent
-/// `CompactBlock`s in the cache database, form a valid chain.
+/// `CompactBlock`s in the block_source database, form a valid chain.
 ///
 /// This function is built on the core assumption that the information provided in the
-/// cache database is more likely to be accurate than the previously-scanned information.
+/// block source is more likely to be accurate than the previously-scanned information.
 /// This follows from the design (and trust) assumption that the `lightwalletd` server
 /// provides accurate block information as of the time it was requested.
 ///
 /// Arguments:
 /// - `parameters` Network parameters
-/// - `cache` Source of compact blocks
+/// - `block_source` Source of compact blocks
 /// - `from_tip` Height & hash of last validated block; if no validation has previously
 ///    been performed, this will begin scanning from `sapling_activation_height - 1`
 ///
 /// Returns:
 /// - `Ok(())` if the combined chain is valid.
-/// - `Err(ErrorKind::InvalidChain(upper_bound, cause))` if the combined chain is invalid.
+/// - `Err(Error::Chain(cause))` if the combined chain is invalid.
-///   `upper_bound` is the height of the highest invalid block (on the assumption that the
-///   highest block in the cache database is correct).
 /// - `Err(e)` if there was an error during validation unrelated to chain validity.
 ///
 /// This function does not mutate either of the databases.
-pub fn validate_chain<N, E, P, C>(
+pub fn validate_chain<ParamsT, BlockSourceT>(
-    parameters: &P,
+    parameters: &ParamsT,
-    cache: &C,
+    block_source: &BlockSourceT,
     validate_from: Option<(BlockHeight, BlockHash)>,
-) -> Result<(), E>
+) -> Result<(), Error<Infallible, BlockSourceT::Error, Infallible>>
 where
-    E: From<Error<N>>,
+    ParamsT: consensus::Parameters,
-    P: consensus::Parameters,
+    BlockSourceT: BlockSource,
-    C: BlockSource<Error = E>,
 {
     let sapling_activation_height = parameters
         .activation_height(NetworkUpgrade::Sapling)
-        .ok_or(Error::SaplingNotActive)?;
+        .expect("Sapling activation height must be known.");
 
-    // The cache will contain blocks above the `validate_from` height.  Validate from that maximum
+    // The block source will contain blocks above the `validate_from` height.  Validate from that
-    // height up to the chain tip, returning the hash of the block found in the cache at the
+    // maximum height up to the chain tip, returning the hash of the block found in the block
-    // `validate_from` height, which can then be used to verify chain integrity by comparing
+    // source at the `validate_from` height, which can then be used to verify chain integrity by
-    // against the `validate_from` hash.
+    // comparing against the `validate_from` hash.
     let from_height = validate_from
         .map(|(height, _)| height)
         .unwrap_or(sapling_activation_height - 1);
@@ -147,74 +171,74 @@ where
     let mut prev_height = from_height;
     let mut prev_hash: Option<BlockHash> = validate_from.map(|(_, hash)| hash);
 
-    cache.with_blocks(from_height, None, move |block| {
+    block_source.with_blocks::<_, Infallible, Infallible>(from_height, None, move |block| {
         let current_height = block.height();
         let result = if current_height != prev_height + 1 {
-            Err(ChainInvalid::block_height_discontinuity(
+            Err(ChainError::block_height_discontinuity(prev_height + 1, current_height).into())
-                prev_height + 1,
-                current_height,
-            ))
         } else {
             match prev_hash {
                 None => Ok(()),
                 Some(h) if h == block.prev_hash() => Ok(()),
-                Some(_) => Err(ChainInvalid::prev_hash_mismatch(current_height)),
+                Some(_) => Err(ChainError::prev_hash_mismatch(current_height).into()),
             }
         };
 
         prev_height = current_height;
         prev_hash = Some(block.hash());
-        result.map_err(E::from)
+        result
     })
 }
 
-#[allow(clippy::needless_doctest_main)]
+/// Scans at most `limit` new blocks added to the block source for any transactions received by the
-/// Scans at most `limit` new blocks added to the cache for any transactions received by
+/// tracked accounts.
-/// the tracked accounts.
 ///
-/// This function will return without error after scanning at most `limit` new blocks, to
+/// This function will return without error after scanning at most `limit` new blocks, to enable
-/// enable the caller to update their UI with scanning progress. Repeatedly calling this
+/// the caller to update their UI with scanning progress. Repeatedly calling this function will
-/// function will process sequential ranges of blocks, and is equivalent to calling
+/// process sequential ranges of blocks, and is equivalent to calling `scan_cached_blocks` and
-/// `scan_cached_blocks` and passing `None` for the optional `limit` value.
+/// passing `None` for the optional `limit` value.
 ///
-/// This function pays attention only to cached blocks with heights greater than the
+/// This function pays attention only to cached blocks with heights greater than the highest
-/// highest scanned block in `data`. Cached blocks with lower heights are not verified
+/// scanned block in `data`. Cached blocks with lower heights are not verified against
-/// against previously-scanned blocks. In particular, this function **assumes** that the
+/// previously-scanned blocks. In particular, this function **assumes** that the caller is handling
-/// caller is handling rollbacks.
+/// rollbacks.
 ///
-/// For brand-new light client databases, this function starts scanning from the Sapling
+/// For brand-new light client databases, this function starts scanning from the Sapling activation
-/// activation height. This height can be fast-forwarded to a more recent block by
+/// height. This height can be fast-forwarded to a more recent block by initializing the client
-/// initializing the client database with a starting block (for example, calling
+/// database with a starting block (for example, calling `init_blocks_table` before this function
-/// `init_blocks_table` before this function if using `zcash_client_sqlite`).
+/// if using `zcash_client_sqlite`).
 ///
-/// Scanned blocks are required to be height-sequential. If a block is missing from the
+/// Scanned blocks are required to be height-sequential. If a block is missing from the block
-/// cache, an error will be returned with kind [`ChainInvalid::BlockHeightDiscontinuity`].
+/// source, an error will be returned with cause [`error::Cause::BlockHeightDiscontinuity`].
-pub fn scan_cached_blocks<E, N, P, C, D>(
+#[allow(clippy::type_complexity)]
-    params: &P,
+pub fn scan_cached_blocks<ParamsT, DbT, BlockSourceT>(
-    cache: &C,
+    params: &ParamsT,
-    data: &mut D,
+    block_source: &BlockSourceT,
+    data_db: &mut DbT,
     limit: Option<u32>,
-) -> Result<(), E>
+) -> Result<(), Error<DbT::Error, BlockSourceT::Error, DbT::NoteRef>>
 where
-    P: consensus::Parameters + Send + 'static,
+    ParamsT: consensus::Parameters + Send + 'static,
-    C: BlockSource<Error = E>,
+    BlockSourceT: BlockSource,
-    D: WalletWrite<Error = E, NoteRef = N>,
+    DbT: WalletWrite,
-    N: Copy + Debug,
-    E: From<Error<N>>,
 {
     let sapling_activation_height = params
         .activation_height(NetworkUpgrade::Sapling)
-        .ok_or(Error::SaplingNotActive)?;
+        .expect("Sapling activation height is known.");
 
     // Recall where we synced up to previously.
     // If we have never synced, use sapling activation height to select all cached CompactBlocks.
-    let mut last_height = data.block_height_extrema().map(|opt| {
+    let mut last_height = data_db
-        opt.map(|(_, max)| max)
+        .block_height_extrema()
-            .unwrap_or(sapling_activation_height - 1)
+        .map(|opt| {
-    })?;
+            opt.map(|(_, max)| max)
+                .unwrap_or(sapling_activation_height - 1)
+        })
+        .map_err(Error::Wallet)?;
 
     // Fetch the UnifiedFullViewingKeys we are tracking
-    let ufvks = data.get_unified_full_viewing_keys()?;
+    let ufvks = data_db
+        .get_unified_full_viewing_keys()
+        .map_err(Error::Wallet)?;
     // TODO: Change `scan_block` to also scan Orchard.
     // https://github.com/zcash/librustzcash/issues/403
     let dfvks: Vec<_> = ufvks
@@ -223,15 +247,16 @@ where
         .collect();
 
     // Get the most recent CommitmentTree
-    let mut tree = data
+    let mut tree = data_db
         .get_commitment_tree(last_height)
-        .map(|t| t.unwrap_or_else(CommitmentTree::empty))?;
+        .map(|t| t.unwrap_or_else(CommitmentTree::empty))
+        .map_err(Error::Wallet)?;
 
     // Get most recent incremental witnesses for the notes we are tracking
-    let mut witnesses = data.get_witnesses(last_height)?;
+    let mut witnesses = data_db.get_witnesses(last_height).map_err(Error::Wallet)?;
 
     // Get the nullifiers for the notes we are tracking
-    let mut nullifiers = data.get_nullifiers()?;
+    let mut nullifiers = data_db.get_nullifiers().map_err(Error::Wallet)?;
 
     let mut batch_runner = BatchRunner::<_, _, _, ()>::new(
         100,
@@ -246,91 +271,133 @@ where
             .map(|(tag, ivk)| (tag, PreparedIncomingViewingKey::new(&ivk))),
     );
 
-    cache.with_blocks(last_height, limit, |block: CompactBlock| {
+    block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>(
-        add_block_to_runner(params, block, &mut batch_runner);
+        last_height,
-        Ok(())
+        limit,
-    })?;
+        |block: CompactBlock| {
+            add_block_to_runner(params, block, &mut batch_runner);
+            Ok(())
+        },
+    )?;
 
     batch_runner.flush();
 
-    cache.with_blocks(last_height, limit, |block: CompactBlock| {
+    block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>(
-        let current_height = block.height();
+        last_height,
+        limit,
+        |block: CompactBlock| {
+            let current_height = block.height();
 
-        // Scanned blocks MUST be height-sequential.
+            // Scanned blocks MUST be height-sequential.
-        if current_height != (last_height + 1) {
+            if current_height != (last_height + 1) {
-            return Err(
+                return Err(ChainError::block_height_discontinuity(
-                ChainInvalid::block_height_discontinuity(last_height + 1, current_height).into(),
+                    last_height + 1,
-            );
+                    current_height,
-        }
+                )
-
+                .into());
-        let block_hash = BlockHash::from_slice(&block.hash);
-        let block_time = block.time;
-
-        let txs: Vec<WalletTx<Nullifier>> = {
-            let mut witness_refs: Vec<_> = witnesses.iter_mut().map(|w| &mut w.1).collect();
-
-            scan_block_with_runner(
-                params,
-                block,
-                &dfvks,
-                &nullifiers,
-                &mut tree,
-                &mut witness_refs[..],
-                Some(&mut batch_runner),
-            )
-        };
-
-        // Enforce that all roots match. This is slow, so only include in debug builds.
-        #[cfg(debug_assertions)]
-        {
-            let cur_root = tree.root();
-            for row in &witnesses {
-                if row.1.root() != cur_root {
-                    return Err(Error::InvalidWitnessAnchor(row.0, current_height).into());
-                }
             }
-            for tx in &txs {
+
-                for output in tx.shielded_outputs.iter() {
+            let block_hash = BlockHash::from_slice(&block.hash);
-                    if output.witness.root() != cur_root {
+            let block_time = block.time;
-                        return Err(Error::InvalidNewWitnessAnchor(
+
-                            output.index,
+            let txs: Vec<WalletTx<Nullifier>> = {
-                            tx.txid,
+                let mut witness_refs: Vec<_> = witnesses.iter_mut().map(|w| &mut w.1).collect();
-                            current_height,
+
-                            output.witness.root(),
+                scan_block_with_runner(
-                        )
+                    params,
-                        .into());
+                    block,
+                    &dfvks,
+                    &nullifiers,
+                    &mut tree,
+                    &mut witness_refs[..],
+                    Some(&mut batch_runner),
+                )
+            };
+
+            // Enforce that all roots match. This is slow, so only include in debug builds.
+            #[cfg(debug_assertions)]
+            {
+                let cur_root = tree.root();
+                for row in &witnesses {
+                    if row.1.root() != cur_root {
+                        return Err(
+                            ChainError::invalid_witness_anchor(current_height, row.0).into()
+                        );
+                    }
+                }
+                for tx in &txs {
+                    for output in tx.shielded_outputs.iter() {
+                        if output.witness.root() != cur_root {
+                            return Err(ChainError::invalid_new_witness_anchor(
+                                current_height,
+                                tx.txid,
+                                output.index,
+                                output.witness.root(),
+                            )
+                            .into());
+                        }
                     }
                 }
             }
-        }
 
-        let new_witnesses = data.advance_by_block(
+            let new_witnesses = data_db
-            &(PrunedBlock {
+                .advance_by_block(
-                block_height: current_height,
+                    &(PrunedBlock {
-                block_hash,
+                        block_height: current_height,
-                block_time,
+                        block_hash,
-                commitment_tree: &tree,
+                        block_time,
-                transactions: &txs,
+                        commitment_tree: &tree,
-            }),
+                        transactions: &txs,
-            &witnesses,
+                    }),
-        )?;
+                    &witnesses,
+                )
+                .map_err(Error::Wallet)?;
 
-        let spent_nf: Vec<Nullifier> = txs
+            let spent_nf: Vec<Nullifier> = txs
-            .iter()
+                .iter()
-            .flat_map(|tx| tx.shielded_spends.iter().map(|spend| spend.nf))
+                .flat_map(|tx| tx.shielded_spends.iter().map(|spend| spend.nf))
-            .collect();
+                .collect();
-        nullifiers.retain(|(_, nf)| !spent_nf.contains(nf));
+            nullifiers.retain(|(_, nf)| !spent_nf.contains(nf));
-        nullifiers.extend(
+            nullifiers.extend(
-            txs.iter()
+                txs.iter()
-                .flat_map(|tx| tx.shielded_outputs.iter().map(|out| (out.account, out.nf))),
+                    .flat_map(|tx| tx.shielded_outputs.iter().map(|out| (out.account, out.nf))),
-        );
+            );
 
-        witnesses.extend(new_witnesses);
+            witnesses.extend(new_witnesses);
 
-        last_height = current_height;
+            last_height = current_height;
 
-        Ok(())
+            Ok(())
-    })?;
+        },
+    )?;
 
     Ok(())
 }
+
+#[cfg(feature = "test-dependencies")]
+pub mod testing {
+    use std::convert::Infallible;
+    use zcash_primitives::consensus::BlockHeight;
+
+    use crate::proto::compact_formats::CompactBlock;
+
+    use super::{error::Error, BlockSource};
+
+    pub struct MockBlockSource;
+
+    impl BlockSource for MockBlockSource {
+        type Error = Infallible;
+
+        fn with_blocks<F, DbErrT, NoteRef>(
+            &self,
+            _from_height: BlockHeight,
+            _limit: Option<u32>,
+            _with_row: F,
+        ) -> Result<(), Error<DbErrT, Infallible, NoteRef>>
+        where
+            F: FnMut(CompactBlock) -> Result<(), Error<DbErrT, Infallible, NoteRef>>,
+        {
+            Ok(())
+        }
+    }
+}

zcash_client_backend/src/data_api/chain/error.rs

@@ -0,0 +1,190 @@
+//! Types for chain scanning error handling.
+
+use std::error;
+use std::fmt::{self, Debug, Display};
+
+use zcash_primitives::{consensus::BlockHeight, sapling, transaction::TxId};
+
+/// The underlying cause of a [`ChainError`].
+#[derive(Copy, Clone, Debug)]
+pub enum Cause<NoteRef> {
+    /// The hash of the parent block given by a proposed new chain tip does not match the hash of
+    /// the current chain tip.
+    PrevHashMismatch,
+
+    /// The block height field of the proposed new chain tip is not equal to the height of the
+    /// previous chain tip + 1. This variant stores a copy of the incorrect height value for
+    /// reporting purposes.
+    BlockHeightDiscontinuity(BlockHeight),
+
+    /// The root of an output's witness tree in a newly arrived transaction does not correspond to
+    /// root of the stored commitment tree at the recorded height.
+    ///
+    /// This error is currently only produced when performing the slow checks that are enabled by
+    /// compiling with `-C debug-assertions`.
+    InvalidNewWitnessAnchor {
+        /// The id of the transaction containing the mismatched witness.
+        txid: TxId,
+        /// The index of the shielded output within the transaction where the witness root does not
+        /// match.
+        index: usize,
+        /// The root of the witness that failed to match the root of the current note commitment
+        /// tree.
+        node: sapling::Node,
+    },
+
+    /// The root of an output's witness tree in a previously stored transaction does not correspond
+    /// to root of the current commitment tree.
+    ///
+    /// This error is currently only produced when performing the slow checks that are enabled by
+    /// compiling with `-C debug-assertions`.
+    InvalidWitnessAnchor(NoteRef),
+}
+
+/// Errors that may occur in chain scanning or validation.
+#[derive(Copy, Clone, Debug)]
+pub struct ChainError<NoteRef> {
+    at_height: BlockHeight,
+    cause: Cause<NoteRef>,
+}
+
+impl<N: Display> fmt::Display for ChainError<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match &self.cause {
+            Cause::PrevHashMismatch => write!(
+                f,
+                "The parent hash of proposed block does not correspond to the block hash at height {}.",
+                self.at_height
+            ),
+            Cause::BlockHeightDiscontinuity(h) => {
+                write!(f, "Block height discontinuity at height {}; next height is : {}", self.at_height, h)
+            }
+            Cause::InvalidNewWitnessAnchor { txid, index, node } => write!(
+                f,
+                "New witness for output {} in tx {} at height {} has incorrect anchor: {:?}",
+                index, txid, self.at_height, node,
+            ),
+            Cause::InvalidWitnessAnchor(id_note) => {
+                write!(f, "Witness for note {} has incorrect anchor for height {}", id_note, self.at_height)
+            }
+        }
+    }
+}
+
+impl<NoteRef> ChainError<NoteRef> {
+    /// Constructs an error that indicates block hashes failed to chain.
+    ///
+    /// * `at_height` the height of the block whose parent hash does not match the hash of the
+    ///   previous block
+    pub fn prev_hash_mismatch(at_height: BlockHeight) -> Self {
+        ChainError {
+            at_height,
+            cause: Cause::PrevHashMismatch,
+        }
+    }
+
+    /// Constructs an error that indicates a gap in block heights.
+    ///
+    /// * `at_height` the height of the block being added to the chain.
+    /// * `prev_chain_tip` the height of the previous chain tip.
+    pub fn block_height_discontinuity(at_height: BlockHeight, prev_chain_tip: BlockHeight) -> Self {
+        ChainError {
+            at_height,
+            cause: Cause::BlockHeightDiscontinuity(prev_chain_tip),
+        }
+    }
+
+    /// Constructs an error that indicates a mismatch between an updated note's witness and the
+    /// root of the current note commitment tree.
+    pub fn invalid_witness_anchor(at_height: BlockHeight, note_ref: NoteRef) -> Self {
+        ChainError {
+            at_height,
+            cause: Cause::InvalidWitnessAnchor(note_ref),
+        }
+    }
+
+    /// Constructs an error that indicates a mismatch between a new note's witness and the root of
+    /// the current note commitment tree.
+    pub fn invalid_new_witness_anchor(
+        at_height: BlockHeight,
+        txid: TxId,
+        index: usize,
+        node: sapling::Node,
+    ) -> Self {
+        ChainError {
+            at_height,
+            cause: Cause::InvalidNewWitnessAnchor { txid, index, node },
+        }
+    }
+
+    /// Returns the block height at which this error was discovered.
+    pub fn at_height(&self) -> BlockHeight {
+        self.at_height
+    }
+
+    /// Returns the cause of this error.
+    pub fn cause(&self) -> &Cause<NoteRef> {
+        &self.cause
+    }
+}
+
+/// Errors related to chain validation and scanning.
+#[derive(Debug)]
+pub enum Error<WalletError, BlockSourceError, NoteRef> {
+    /// An error that was produced by wallet operations in the course of scanning the chain.
+    Wallet(WalletError),
+
+    /// An error that was produced by the underlying block data store in the process of validation
+    /// or scanning.
+    BlockSource(BlockSourceError),
+
+    /// A block that was received violated rules related to chain continuity or contained note
+    /// commitments that could not be reconciled with the note commitment tree(s) maintained by the
+    /// wallet.
+    Chain(ChainError<NoteRef>),
+}
+
+impl<WE: fmt::Display, BE: fmt::Display, N: Display> fmt::Display for Error<WE, BE, N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match &self {
+            Error::Wallet(e) => {
+                write!(
+                    f,
+                    "The underlying datasource produced the following error: {}",
+                    e
+                )
+            }
+            Error::BlockSource(e) => {
+                write!(
+                    f,
+                    "The underlying block store produced the following error: {}",
+                    e
+                )
+            }
+            Error::Chain(err) => {
+                write!(f, "{}", err)
+            }
+        }
+    }
+}
+
+impl<WE, BE, N> error::Error for Error<WE, BE, N>
+where
+    WE: Debug + Display + error::Error + 'static,
+    BE: Debug + Display + error::Error + 'static,
+    N: Debug + Display,
+{
+    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
+        match &self {
+            Error::Wallet(e) => Some(e),
+            Error::BlockSource(e) => Some(e),
+            _ => None,
+        }
+    }
+}
+
+impl<WE, BSE, N> From<ChainError<N>> for Error<WE, BSE, N> {
+    fn from(e: ChainError<N>) -> Self {
+        Error::Chain(e)
+    }
+}

zcash_client_backend/src/data_api/error.rs

@@ -1,36 +1,32 @@
 //! Types for wallet error handling.
 
 use std::error;
-use std::fmt;
+use std::fmt::{self, Debug, Display};
-use zcash_address::unified::Typecode;
 use zcash_primitives::{
-    consensus::BlockHeight,
-    sapling::Node,
     transaction::{
         builder,
-        components::amount::{Amount, BalanceError},
+        components::{
-        TxId,
+            amount::{Amount, BalanceError},
+            sapling, transparent,
+        },
     },
     zip32::AccountId,
 };
 
+use crate::data_api::wallet::input_selection::InputSelectorError;
+
 #[cfg(feature = "transparent-inputs")]
 use zcash_primitives::{legacy::TransparentAddress, zip32::DiversifierIndex};
 
+/// Errors that can occur as a consequence of wallet operations.
 #[derive(Debug)]
-pub enum ChainInvalid {
+pub enum Error<DataSourceError, SelectionError, FeeError, NoteRef> {
-    /// The hash of the parent block given by a proposed new chain tip does
+    /// An error occurred retrieving data from the underlying data source
-    /// not match the hash of the current chain tip.
+    DataSource(DataSourceError),
-    PrevHashMismatch,
 
-    /// The block height field of the proposed new chain tip is not equal
+    /// An error in note selection
-    /// to the height of the previous chain tip + 1. This variant stores
+    NoteSelection(SelectionError),
-    /// a copy of the incorrect height value for reporting purposes.
-    BlockHeightDiscontinuity(BlockHeight),
-}
 
-#[derive(Debug)]
-pub enum Error<NoteId> {
     /// No account could be found corresponding to a provided spending key.
     KeyNotRecognized,
 
@@ -41,60 +37,21 @@ pub enum Error<NoteId> {
     BalanceError(BalanceError),
 
     /// Unable to create a new spend because the wallet balance is not sufficient.
-    /// The first argument is the amount available, the second is the amount needed
+    InsufficientFunds { available: Amount, required: Amount },
-    /// to construct a valid transaction.
-    InsufficientBalance(Amount, Amount),
-
-    /// Chain validation detected an error in the block at the specified block height.
-    InvalidChain(BlockHeight, ChainInvalid),
-
-    /// A provided extsk is not associated with the specified account.
-    InvalidExtSk(AccountId),
-
-    /// The root of an output's witness tree in a newly arrived transaction does
-    /// not correspond to root of the stored commitment tree at the recorded height.
-    ///
-    /// The `usize` member of this struct is the index of the shielded output within
-    /// the transaction where the witness root does not match.
-    InvalidNewWitnessAnchor(usize, TxId, BlockHeight, Node),
-
-    /// The root of an output's witness tree in a previously stored transaction
-    /// does not correspond to root of the current commitment tree.
-    InvalidWitnessAnchor(NoteId, BlockHeight),
-
-    /// No key of the given type was associated with the specified account.
-    KeyNotFound(AccountId, Typecode),
 
     /// The wallet must first perform a scan of the blockchain before other
     /// operations can be performed.
     ScanRequired,
 
     /// An error occurred building a new transaction.
-    Builder(builder::Error),
+    Builder(builder::Error<FeeError>),
-
-    /// An error occurred decoding a protobuf message.
-    Protobuf(prost::DecodeError),
-
-    /// The wallet attempted a sapling-only operation at a block
-    /// height when Sapling was not yet active.
-    SaplingNotActive,
 
     /// It is forbidden to provide a memo when constructing a transparent output.
     MemoForbidden,
 
-    /// An error occurred deriving a spending key from a seed and an account
-    /// identifier.
-    KeyDerivationError(AccountId),
-
     /// A note being spent does not correspond to either the internal or external
     /// full viewing key for an account.
-    // TODO: Return the note id for the note that caused the failure
+    NoteMismatch(NoteRef),
-    NoteMismatch,
-
-    /// An error indicating that a call was attempted to a method providing
-    /// support
-    #[cfg(not(feature = "transparent-inputs"))]
-    TransparentInputsNotSupported,
 
     #[cfg(feature = "transparent-inputs")]
     AddressNotRecognized(TransparentAddress),
@@ -103,95 +60,119 @@ pub enum Error<NoteId> {
     ChildIndexOutOfRange(DiversifierIndex),
 }
 
-impl ChainInvalid {
+impl<DE, SE, FE, N> fmt::Display for Error<DE, SE, FE, N>
-    pub fn prev_hash_mismatch<N>(at_height: BlockHeight) -> Error<N> {
+where
-        Error::InvalidChain(at_height, ChainInvalid::PrevHashMismatch)
+    DE: fmt::Display,
-    }
+    SE: fmt::Display,
-
+    FE: fmt::Display,
-    pub fn block_height_discontinuity<N>(at_height: BlockHeight, found: BlockHeight) -> Error<N> {
+    N: fmt::Display,
-        Error::InvalidChain(at_height, ChainInvalid::BlockHeightDiscontinuity(found))
+{
-    }
-}
-
-impl<N: fmt::Display> fmt::Display for Error<N> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self {
+            Error::DataSource(e) => {
+                write!(
+                    f,
+                    "The underlying datasource produced the following error: {}",
+                    e
+                )
+            }
+            Error::NoteSelection(e) => {
+                write!(f, "Note selection encountered the following error: {}", e)
+            }
             Error::KeyNotRecognized => {
-                write!(f, "Wallet does not contain an account corresponding to the provided spending key")
+                write!(
+                    f,
+                    "Wallet does not contain an account corresponding to the provided spending key"
+                )
             }
             Error::AccountNotFound(account) => {
                 write!(f, "Wallet does not contain account {}", u32::from(*account))
             }
             Error::BalanceError(e) => write!(
                 f,
-                "The value lies outside the valid range of Zcash amounts: {:?}.", e
+                "The value lies outside the valid range of Zcash amounts: {:?}.",
+                e
             ),
-            Error::InsufficientBalance(have, need) => write!(
+            Error::InsufficientFunds { available, required } => write!(
                 f,
                 "Insufficient balance (have {}, need {} including fee)",
-                i64::from(*have), i64::from(*need)
+                i64::from(*available),
+                i64::from(*required)
             ),
-            Error::InvalidChain(upper_bound, cause) => {
-                write!(f, "Invalid chain (upper bound: {}): {:?}", u32::from(*upper_bound), cause)
-            }
-            Error::InvalidExtSk(account) => {
-                write!(f, "Incorrect ExtendedSpendingKey for account {}", u32::from(*account))
-            }
-            Error::InvalidNewWitnessAnchor(output, txid, last_height, anchor) => write!(
-                f,
-                "New witness for output {} in tx {} has incorrect anchor after scanning block {}: {:?}",
-                output, txid, last_height, anchor,
-            ),
-            Error::InvalidWitnessAnchor(id_note, last_height) => write!(
-                f,
-                "Witness for note {} has incorrect anchor after scanning block {}",
-                id_note, last_height
-            ),
-            Error::KeyNotFound(account, typecode) => {
-                write!(f, "No {:?} key was available for account {}", typecode, u32::from(*account))
-            }
             Error::ScanRequired => write!(f, "Must scan blocks first"),
-            Error::Builder(e) => write!(f, "{:?}", e),
+            Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e),
-            Error::Protobuf(e) => write!(f, "{}", e),
-            Error::SaplingNotActive => write!(f, "Could not determine Sapling upgrade activation height."),
             Error::MemoForbidden => write!(f, "It is not possible to send a memo to a transparent address."),
-            Error::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id),
+            Error::NoteMismatch(n) => write!(f, "A note being spent ({}) does not correspond to either the internal or external full viewing key for the provided spending key.", n),
-            Error::NoteMismatch => write!(f, "A note being spent does not correspond to either the internal or external full viewing key for the provided spending key."),
 
-            #[cfg(not(feature = "transparent-inputs"))]
-            Error::TransparentInputsNotSupported => {
-                write!(f, "This wallet does not support spending or manipulating transparent UTXOs.")
-            }
             #[cfg(feature = "transparent-inputs")]
             Error::AddressNotRecognized(_) => {
                 write!(f, "The specified transparent address was not recognized as belonging to the wallet.")
             }
             #[cfg(feature = "transparent-inputs")]
             Error::ChildIndexOutOfRange(i) => {
-                write!(f, "The diversifier index {:?} is out of range for transparent addresses.", i)
+                write!(
+                    f,
+                    "The diversifier index {:?} is out of range for transparent addresses.",
+                    i
+                )
             }
         }
     }
 }
 
-impl<N: error::Error + 'static> error::Error for Error<N> {
+impl<DE, SE, FE, N> error::Error for Error<DE, SE, FE, N>
+where
+    DE: Debug + Display + error::Error + 'static,
+    SE: Debug + Display + error::Error + 'static,
+    FE: Debug + Display + 'static,
+    N: Debug + Display,
+{
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match &self {
+            Error::DataSource(e) => Some(e),
+            Error::NoteSelection(e) => Some(e),
             Error::Builder(e) => Some(e),
-            Error::Protobuf(e) => Some(e),
             _ => None,
         }
     }
 }
 
-impl<N> From<builder::Error> for Error<N> {
+impl<DE, SE, FE, N> From<builder::Error<FE>> for Error<DE, SE, FE, N> {
-    fn from(e: builder::Error) -> Self {
+    fn from(e: builder::Error<FE>) -> Self {
         Error::Builder(e)
     }
 }
 
-impl<N> From<prost::DecodeError> for Error<N> {
+impl<DE, SE, FE, N> From<BalanceError> for Error<DE, SE, FE, N> {
-    fn from(e: prost::DecodeError) -> Self {
+    fn from(e: BalanceError) -> Self {
-        Error::Protobuf(e)
+        Error::BalanceError(e)
+    }
+}
+
+impl<DE, SE, FE, N> From<InputSelectorError<DE, SE>> for Error<DE, SE, FE, N> {
+    fn from(e: InputSelectorError<DE, SE>) -> Self {
+        match e {
+            InputSelectorError::DataSource(e) => Error::DataSource(e),
+            InputSelectorError::Selection(e) => Error::NoteSelection(e),
+            InputSelectorError::InsufficientFunds {
+                available,
+                required,
+            } => Error::InsufficientFunds {
+                available,
+                required,
+            },
+        }
+    }
+}
+
+impl<DE, SE, FE, N> From<sapling::builder::Error> for Error<DE, SE, FE, N> {
+    fn from(e: sapling::builder::Error) -> Self {
+        Error::Builder(builder::Error::SaplingBuild(e))
+    }
+}
+
+impl<DE, SE, FE, N> From<transparent::builder::Error> for Error<DE, SE, FE, N> {
+    fn from(e: transparent::builder::Error) -> Self {
+        Error::Builder(builder::Error::TransparentBuild(e))
     }
 }

zcash_client_backend/src/data_api/wallet.rs

@@ -1,14 +1,17 @@
 use std::fmt::Debug;
+
 use zcash_primitives::{
     consensus::{self, NetworkUpgrade},
     memo::MemoBytes,
-    sapling::prover::TxProver,
+    merkle_tree::MerklePath,
+    sapling::{self, prover::TxProver as SaplingProver},
     transaction::{
         builder::Builder,
         components::amount::{Amount, BalanceError, DEFAULT_FEE},
+        fees::{FeeRule, FixedFeeRule},
         Transaction,
     },
-    zip32::Scope,
+    zip32::{sapling::DiversifiableFullViewingKey, sapling::ExtendedSpendingKey, Scope},
 };
 
 use crate::{
@@ -18,29 +21,31 @@ use crate::{
         SentTransactionOutput, WalletWrite,
     },
     decrypt_transaction,
-    fees::{BasicFixedFeeChangeStrategy, ChangeError, ChangeStrategy, ChangeValue},
+    fees::{ChangeValue, SingleOutputFixedFeeChangeStrategy},
     keys::UnifiedSpendingKey,
-    wallet::OvkPolicy,
+    wallet::{OvkPolicy, SpendableNote},
-    zip321::{Payment, TransactionRequest},
+    zip321::{self, Payment},
 };
 
+pub mod input_selection;
+use input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector};
+
 #[cfg(feature = "transparent-inputs")]
-use {
+use zcash_primitives::{
-    crate::wallet::WalletTransparentOutput,
+    legacy::TransparentAddress, sapling::keys::OutgoingViewingKey,
-    zcash_primitives::{legacy::TransparentAddress, sapling::keys::OutgoingViewingKey},
+    transaction::components::amount::NonNegativeAmount,
 };
 
 /// Scans a [`Transaction`] for any information that can be decrypted by the accounts in
 /// the wallet, and saves it to the wallet.
-pub fn decrypt_and_store_transaction<N, E, P, D>(
+pub fn decrypt_and_store_transaction<ParamsT, DbT>(
-    params: &P,
+    params: &ParamsT,
-    data: &mut D,
+    data: &mut DbT,
     tx: &Transaction,
-) -> Result<(), E>
+) -> Result<(), DbT::Error>
 where
-    E: From<Error<N>>,
+    ParamsT: consensus::Parameters,
-    P: consensus::Parameters,
+    DbT: WalletWrite,
-    D: WalletWrite<Error = E>,
 {
     // Fetch the UnifiedFullViewingKeys we are tracking
     let ufvks = data.get_unified_full_viewing_keys()?;
@@ -53,7 +58,7 @@ where
             .block_height_extrema()?
             .map(|(_, max_height)| max_height + 1))
         .or_else(|| params.activation_height(NetworkUpgrade::Sapling))
-        .ok_or(Error::SaplingNotActive)?;
+        .expect("Sapling activation height must be known.");
 
     data.store_decrypted_tx(&DecryptedTransaction {
         tx,
@@ -93,7 +98,7 @@ where
 /// Parameters:
 /// * `wallet_db`: A read/write reference to the wallet database
 /// * `params`: Consensus parameters
-/// * `prover`: The TxProver to use in constructing the shielded transaction.
+/// * `prover`: The [`sapling::TxProver`] to use in constructing the shielded transaction.
 /// * `usk`: The unified spending key that controls the funds that will be spent
 ///   in the resulting transaction. This procedure will return an error if the
 ///   USK does not correspond to an account known to the wallet.
@@ -125,11 +130,18 @@ where
 ///     wallet::OvkPolicy,
 /// };
 ///
+/// # use std::convert::Infallible;
+/// # use zcash_primitives::transaction::components::amount::BalanceError;
+/// # use zcash_client_backend::{
+/// #     data_api::wallet::input_selection::GreedyInputSelectorError,
+/// # };
+/// #
 /// # fn main() {
 /// #   test();
 /// # }
 /// #
-/// # fn test() -> Result<TxId, Error<u32>> {
+/// # #[allow(deprecated)]
+/// # fn test() -> Result<TxId, Error<(), GreedyInputSelectorError<BalanceError, u32>, Infallible, u32>> {
 ///
 /// let tx_prover = match LocalTxProver::with_default_location() {
 ///     Some(tx_prover) => tx_prover,
@@ -161,25 +173,35 @@ where
 /// # }
 /// # }
 /// ```
+/// [`sapling::TxProver`]: zcash_primitives::sapling::prover::TxProver
 #[allow(clippy::too_many_arguments)]
-pub fn create_spend_to_address<E, N, P, D, R>(
+#[allow(clippy::type_complexity)]
-    wallet_db: &mut D,
+#[deprecated(note = "Use `spend` instead.")]
-    params: &P,
+pub fn create_spend_to_address<DbT, ParamsT>(
-    prover: impl TxProver,
+    wallet_db: &mut DbT,
+    params: &ParamsT,
+    prover: impl SaplingProver,
     usk: &UnifiedSpendingKey,
     to: &RecipientAddress,
     amount: Amount,
     memo: Option<MemoBytes>,
     ovk_policy: OvkPolicy,
     min_confirmations: u32,
-) -> Result<R, E>
+) -> Result<
+    DbT::TxRef,
+    Error<
+        DbT::Error,
+        GreedyInputSelectorError<BalanceError>,
+        core::convert::Infallible,
+        DbT::NoteRef,
+    >,
+>
 where
-    E: From<Error<N>>,
+    ParamsT: consensus::Parameters + Clone,
-    P: consensus::Parameters + Clone,
+    DbT: WalletWrite,
-    R: Copy + Debug,
+    DbT::NoteRef: Copy + Eq + Ord,
-    D: WalletWrite<Error = E, TxRef = R>,
 {
-    let req = TransactionRequest::new(vec![Payment {
+    let req = zip321::TransactionRequest::new(vec![Payment {
         recipient_address: to.clone(),
         amount,
         memo,
@@ -191,12 +213,14 @@ where
         "It should not be possible for this to violate ZIP 321 request construction invariants.",
     );
 
+    let change_strategy = SingleOutputFixedFeeChangeStrategy::new(FixedFeeRule::new(DEFAULT_FEE));
     spend(
         wallet_db,
         params,
         prover,
+        &GreedyInputSelector::<DbT, _>::new(change_strategy),
         usk,
-        &req,
+        req,
         ovk_policy,
         min_confirmations,
     )
@@ -235,7 +259,10 @@ where
 /// Parameters:
 /// * `wallet_db`: A read/write reference to the wallet database
 /// * `params`: Consensus parameters
-/// * `prover`: The TxProver to use in constructing the shielded transaction.
+/// * `prover`: The [`sapling::TxProver`] to use in constructing the shielded transaction.
+/// * `input_selector`: The [`InputSelector`] that will be used to select available
+///   inputs from the wallet database, choose change amounts and compute required
+///   transaction fees.
 /// * `usk`: The unified spending key that controls the funds that will be spent
 ///   in the resulting transaction. This procedure will return an error if the
 ///   USK does not correspond to an account known to the wallet.
@@ -246,24 +273,33 @@ where
 /// * `min_confirmations`: The minimum number of confirmations that a previously
 ///   received note must have in the blockchain in order to be considered for being
 ///   spent. A value of 10 confirmations is recommended.
+///
+/// [`sapling::TxProver`]: zcash_primitives::sapling::prover::TxProver
 #[allow(clippy::too_many_arguments)]
-pub fn spend<E, N, P, D, R>(
+#[allow(clippy::type_complexity)]
-    wallet_db: &mut D,
+pub fn spend<DbT, ParamsT, InputsT>(
-    params: &P,
+    wallet_db: &mut DbT,
-    prover: impl TxProver,
+    params: &ParamsT,
+    prover: impl SaplingProver,
+    input_selector: &InputsT,
     usk: &UnifiedSpendingKey,
-    request: &TransactionRequest,
+    request: zip321::TransactionRequest,
     ovk_policy: OvkPolicy,
     min_confirmations: u32,
-) -> Result<R, E>
+) -> Result<
+    DbT::TxRef,
+    Error<DbT::Error, InputsT::Error, <InputsT::FeeRule as FeeRule>::Error, DbT::NoteRef>,
+>
 where
-    E: From<Error<N>>,
+    DbT: WalletWrite,
-    P: consensus::Parameters + Clone,
+    DbT::TxRef: Copy + Debug,
-    R: Copy + Debug,
+    DbT::NoteRef: Copy + Eq + Ord,
-    D: WalletWrite<Error = E, TxRef = R>,
+    ParamsT: consensus::Parameters + Clone,
+    InputsT: InputSelector<DataSource = DbT>,
 {
     let account = wallet_db
-        .get_account_for_ufvk(&usk.to_unified_full_viewing_key())?
+        .get_account_for_ufvk(&usk.to_unified_full_viewing_key())
+        .map_err(Error::DataSource)?
         .ok_or(Error::KeyNotRecognized)?;
 
     let dfvk = usk.sapling().to_diversifiable_full_viewing_key();
@@ -278,135 +314,74 @@ where
     // Target the next block, assuming we are up-to-date.
     let (target_height, anchor_height) = wallet_db
         .get_target_and_anchor_heights(min_confirmations)
-        .and_then(|x| x.ok_or_else(|| Error::ScanRequired.into()))?;
+        .map_err(Error::DataSource)
+        .and_then(|x| x.ok_or(Error::ScanRequired))?;
 
-    let value = request
+    let proposal = input_selector.propose_transaction(
-        .payments()
+        params,
-        .iter()
+        wallet_db,
-        .map(|p| p.amount)
+        account,
-        .sum::<Option<Amount>>()
+        anchor_height,
-        .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?;
+        target_height,
-    let target_value = (value + DEFAULT_FEE)
+        request,
-        .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?;
+    )?;
-    let spendable_notes =
-        wallet_db.select_spendable_sapling_notes(account, target_value, anchor_height)?;
 
-    // Confirm we were able to select sufficient value
+    // Create the transaction. The type of the proposal ensures that there
-    let selected_value = spendable_notes
+    // are no possible transparent inputs, so we ignore those
-        .iter()
-        .map(|n| n.note_value)
-        .sum::<Option<_>>()
-        .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?;
-    if selected_value < target_value {
-        return Err(E::from(Error::InsufficientBalance(
-            selected_value,
-            target_value,
-        )));
-    }
-
-    // Create the transaction
     let mut builder = Builder::new(params.clone(), target_height);
-    for selected in spendable_notes {
+    for selected in proposal.sapling_inputs() {
-        let merkle_path = selected.witness.path().expect("the tree is not empty");
+        let (note, key, merkle_path) = select_key_for_note(selected, usk.sapling(), &dfvk)
+            .ok_or(Error::NoteMismatch(selected.note_id))?;
 
-        // Attempt to reconstruct the note being spent using both the internal and external dfvks
+        builder.add_sapling_spend(key, selected.diversifier, note, merkle_path)?;
-        // corresponding to the unified spending key, checking against the witness we are using
-        // to spend the note that we've used the correct key.
-        let (note, key) = {
-            let external_note = dfvk
-                .diversified_address(selected.diversifier)
-                .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
-            let internal_note = dfvk
-                .diversified_change_address(selected.diversifier)
-                .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
-
-            let expected_root = selected.witness.root();
-            external_note
-                .filter(|n| expected_root == merkle_path.root(n.commitment()))
-                .map(|n| (n, usk.sapling().clone()))
-                .or_else(|| {
-                    internal_note
-                        .filter(|n| expected_root == merkle_path.root(n.commitment()))
-                        .map(|n| (n, usk.sapling().derive_internal()))
-                })
-                .ok_or_else(|| E::from(Error::NoteMismatch))
-        }?;
-
-        builder
-            .add_sapling_spend(key, selected.diversifier, note, merkle_path)
-            .map_err(Error::Builder)?;
     }
 
-    for payment in request.payments() {
+    for payment in proposal.transaction_request().payments() {
         match &payment.recipient_address {
-            RecipientAddress::Unified(ua) => builder
+            RecipientAddress::Unified(ua) => {
-                .add_sapling_output(
+                builder.add_sapling_output(
                     ovk,
                     ua.sapling()
                         .expect("TODO: Add Orchard support to builder")
                         .clone(),
                     payment.amount,
                     payment.memo.clone().unwrap_or_else(MemoBytes::empty),
-                )
+                )?;
-                .map_err(Error::Builder),
+            }
-            RecipientAddress::Shielded(to) => builder
+            RecipientAddress::Shielded(to) => {
-                .add_sapling_output(
+                builder.add_sapling_output(
                     ovk,
                     to.clone(),
                     payment.amount,
                     payment.memo.clone().unwrap_or_else(MemoBytes::empty),
-                )
+                )?;
-                .map_err(Error::Builder),
+            }
             RecipientAddress::Transparent(to) => {
                 if payment.memo.is_some() {
-                    Err(Error::MemoForbidden)
+                    return Err(Error::MemoForbidden);
                 } else {
-                    builder
+                    builder.add_transparent_output(to, payment.amount)?;
-                        .add_transparent_output(to, payment.amount)
-                        .map_err(Error::Builder)
                 }
             }
-        }?
-    }
-
-    let fee_strategy = BasicFixedFeeChangeStrategy::new(DEFAULT_FEE);
-    let balance = fee_strategy
-        .compute_balance(
-            params,
-            target_height,
-            builder.transparent_inputs(),
-            builder.transparent_outputs(),
-            builder.sapling_inputs(),
-            builder.sapling_outputs(),
-        )
-        .map_err(|e| match e {
-            ChangeError::InsufficientFunds {
-                available,
-                required,
-            } => Error::InsufficientBalance(available, required),
-            ChangeError::StrategyError(e) => Error::BalanceError(e),
-        })?;
-
-    for change_value in balance.proposed_change() {
-        match change_value {
-            ChangeValue::Sapling(amount) => {
-                builder
-                    .add_sapling_output(
-                        Some(dfvk.to_ovk(Scope::Internal)),
-                        dfvk.change_address().1,
-                        *amount,
-                        MemoBytes::empty(),
-                    )
-                    .map_err(Error::Builder)?;
-            }
         }
     }
 
-    let (tx, tx_metadata) = builder
+    for change_value in proposal.balance().proposed_change() {
-        .build(&prover, &fee_strategy.fee_rule())
+        match change_value {
-        .map_err(Error::Builder)?;
+            ChangeValue::Sapling(amount) => {
+                builder.add_sapling_output(
+                    Some(dfvk.to_ovk(Scope::Internal)),
+                    dfvk.change_address().1,
+                    *amount,
+                    MemoBytes::empty(),
+                )?;
+            }
+        }
+    }
 
-    let sent_outputs = request.payments().iter().enumerate().map(|(i, payment)| {
+    let (tx, tx_metadata) = builder.build(&prover, proposal.fee_rule())?;
+
+    let sent_outputs = proposal.transaction_request().payments().iter().enumerate().map(|(i, payment)| {
         let (output_index, recipient) = match &payment.recipient_address {
             // Sapling outputs are shuffled, so we need to look up where the output ended up.
             RecipientAddress::Shielded(addr) => {
@@ -443,15 +418,17 @@ where
         }
     }).collect();
 
-    wallet_db.store_sent_tx(&SentTransaction {
+    wallet_db
-        tx: &tx,
+        .store_sent_tx(&SentTransaction {
-        created: time::OffsetDateTime::now_utc(),
+            tx: &tx,
-        account,
+            created: time::OffsetDateTime::now_utc(),
-        outputs: sent_outputs,
+            account,
-        fee_amount: balance.fee_required(),
+            outputs: sent_outputs,
-        #[cfg(feature = "transparent-inputs")]
+            fee_amount: proposal.balance().fee_required(),
-        utxos_spent: vec![],
+            #[cfg(feature = "transparent-inputs")]
-    })
+            utxos_spent: vec![],
+        })
+        .map_err(Error::DataSource)
 }
 
 /// Constructs a transaction that consumes available transparent UTXOs belonging to
@@ -465,13 +442,18 @@ where
 /// Parameters:
 /// * `wallet_db`: A read/write reference to the wallet database
 /// * `params`: Consensus parameters
-/// * `prover`: The TxProver to use in constructing the shielded transaction.
+/// * `prover`: The [`sapling::TxProver`] to use in constructing the shielded transaction.
+/// * `input_selector`: The [`InputSelector`] to for note selection and change and fee
+///   determination
 /// * `usk`: The unified spending key that will be used to detect and spend transparent UTXOs,
 ///   and that will provide the shielded address to which funds will be sent. Funds will be
 ///   shielded to the internal (change) address associated with the most preferred shielded
 ///   receiver corresponding to this account, or if no shielded receiver can be used for this
 ///   account, this function will return an error. This procedure will return an error if the
 ///   USK does not correspond to an account known to the wallet.
+/// * `from_addrs`: The list of transparent addresses that will be used to filter transaparent
+///   UTXOs received by the wallet. Only UTXOs received at one of the provided addresses will
+///   be selected to be shielded.
 /// * `memo`: A memo to be included in the output to the (internal) recipient.
 ///   This can be used to take notes about auto-shielding operations internal
 ///   to the wallet that the wallet can use to improve how it represents those
@@ -479,25 +461,33 @@ where
 /// * `min_confirmations`: The minimum number of confirmations that a previously
 ///   received UTXO must have in the blockchain in order to be considered for being
 ///   spent.
+///
+/// [`sapling::TxProver`]: zcash_primitives::sapling::prover::TxProver
 #[cfg(feature = "transparent-inputs")]
 #[allow(clippy::too_many_arguments)]
-pub fn shield_transparent_funds<E, N, P, D, R, U>(
+#[allow(clippy::type_complexity)]
-    wallet_db: &mut D,
+pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
-    params: &P,
+    wallet_db: &mut DbT,
-    prover: impl TxProver,
+    params: &ParamsT,
+    prover: impl SaplingProver,
+    input_selector: &InputsT,
     usk: &UnifiedSpendingKey,
     from_addrs: &[TransparentAddress],
     memo: &MemoBytes,
     min_confirmations: u32,
-) -> Result<D::TxRef, E>
+) -> Result<
+    DbT::TxRef,
+    Error<DbT::Error, InputsT::Error, <InputsT::FeeRule as FeeRule>::Error, DbT::NoteRef>,
+>
 where
-    E: From<Error<N>>,
+    ParamsT: consensus::Parameters,
-    P: consensus::Parameters,
+    DbT: WalletWrite,
-    R: Copy + Debug,
+    DbT::NoteRef: Copy + Eq + Ord,
-    D: WalletWrite<Error = E, TxRef = R, UtxoRef = U>,
+    InputsT: InputSelector<DataSource = DbT>,
 {
     let account = wallet_db
-        .get_account_for_ufvk(&usk.to_unified_full_viewing_key())?
+        .get_account_for_ufvk(&usk.to_unified_full_viewing_key())
+        .map_err(Error::DataSource)?
         .ok_or(Error::KeyNotRecognized)?;
 
     let shielding_address = usk
@@ -507,29 +497,39 @@ where
         .1;
     let (target_height, latest_anchor) = wallet_db
         .get_target_and_anchor_heights(min_confirmations)
-        .and_then(|x| x.ok_or_else(|| Error::ScanRequired.into()))?;
+        .map_err(Error::DataSource)
+        .and_then(|x| x.ok_or(Error::ScanRequired))?;
 
+    let dfvk = usk.sapling().to_diversifiable_full_viewing_key();
     let account_pubkey = usk.transparent().to_account_pubkey();
     let ovk = OutgoingViewingKey(account_pubkey.internal_ovk().as_bytes());
 
     // get UTXOs from DB for each address
-    let mut utxos: Vec<WalletTransparentOutput> = vec![];
+    let proposal = input_selector.propose_shielding(
-    for from_addr in from_addrs {
+        params,
-        let mut outputs = wallet_db.get_unspent_transparent_outputs(from_addr, latest_anchor)?;
+        wallet_db,
-        utxos.append(&mut outputs);
+        NonNegativeAmount::from_u64(100000).unwrap(),
-    }
+        from_addrs,
+        latest_anchor,
+        target_height,
+    )?;
 
-    let _total_amount = utxos
+    let known_addrs = wallet_db
-        .iter()
+        .get_transparent_receivers(account)
-        .map(|utxo| utxo.txout().value)
+        .map_err(Error::DataSource)?;
-        .sum::<Option<Amount>>()
-        .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?;
-
-    let addr_metadata = wallet_db.get_transparent_receivers(account)?;
     let mut builder = Builder::new(params.clone(), target_height);
 
-    for utxo in &utxos {
+    let mut utxos = vec![];
-        let diversifier_index = addr_metadata
+    for selected in proposal.sapling_inputs() {
+        let (note, key, merkle_path) = select_key_for_note(selected, usk.sapling(), &dfvk)
+            .ok_or(Error::NoteMismatch(selected.note_id))?;
+
+        builder.add_sapling_spend(key, selected.diversifier, note, merkle_path)?;
+    }
+
+    for utxo in proposal.transparent_inputs() {
+        utxos.push(utxo);
+        let diversifier_index = known_addrs
             .get(utxo.recipient_address())
             .ok_or_else(|| Error::AddressNotRecognized(*utxo.recipient_address()))?
             .diversifier_index();
@@ -542,64 +542,85 @@ where
             .derive_external_secret_key(child_index)
             .unwrap();
 
-        builder
+        builder.add_transparent_input(secret_key, utxo.outpoint().clone(), utxo.txout().clone())?;
-            .add_transparent_input(secret_key, utxo.outpoint().clone(), utxo.txout().clone())
-            .map_err(Error::Builder)?;
     }
 
-    // Compute the balance of the transaction. We have only added inputs, so the total change
+    for change_value in proposal.balance().proposed_change() {
-    // amount required will be the total of the UTXOs minus fees.
-    let fee_strategy = BasicFixedFeeChangeStrategy::new(DEFAULT_FEE);
-    let balance = fee_strategy
-        .compute_balance(
-            params,
-            target_height,
-            builder.transparent_inputs(),
-            builder.transparent_outputs(),
-            builder.sapling_inputs(),
-            builder.sapling_outputs(),
-        )
-        .map_err(|e| match e {
-            ChangeError::InsufficientFunds {
-                available,
-                required,
-            } => Error::InsufficientBalance(available, required),
-            ChangeError::StrategyError(e) => Error::BalanceError(e),
-        })?;
-
-    let fee = balance.fee_required();
-    let mut total_out = Amount::zero();
-    for change_value in balance.proposed_change() {
-        total_out = (total_out + change_value.value())
-            .ok_or(Error::BalanceError(BalanceError::Overflow))?;
         match change_value {
             ChangeValue::Sapling(amount) => {
-                builder
+                builder.add_sapling_output(
-                    .add_sapling_output(Some(ovk), shielding_address.clone(), *amount, memo.clone())
+                    Some(ovk),
-                    .map_err(Error::Builder)?;
+                    shielding_address.clone(),
+                    *amount,
+                    memo.clone(),
+                )?;
             }
         }
     }
 
     // The transaction build process will check that the inputs and outputs balance
-    let (tx, tx_metadata) = builder
+    let (tx, tx_metadata) = builder.build(&prover, proposal.fee_rule())?;
-        .build(&prover, &fee_strategy.fee_rule())
-        .map_err(Error::Builder)?;
-    let output_index = tx_metadata.output_index(0).expect(
-        "No sapling note was created in autoshielding transaction. This is a programming error.",
-    );
 
-    wallet_db.store_sent_tx(&SentTransaction {
+    wallet_db
-        tx: &tx,
+        .store_sent_tx(&SentTransaction {
-        created: time::OffsetDateTime::now_utc(),
+            tx: &tx,
-        account,
+            created: time::OffsetDateTime::now_utc(),
-        outputs: vec![SentTransactionOutput {
+            account,
-            output_index,
+            // TODO: After Orchard is implemented, this will need to change to correctly
-            recipient: Recipient::InternalAccount(account, PoolType::Sapling),
+            // determine the Sapling output indices; `enumerate` will no longer suffice
-            value: total_out,
+            outputs: proposal
-            memo: Some(memo.clone()),
+                .balance()
-        }],
+                .proposed_change()
-        fee_amount: fee,
+                .iter()
-        utxos_spent: utxos.iter().map(|utxo| utxo.outpoint().clone()).collect(),
+                .enumerate()
-    })
+                .map(|(idx, change_value)| match change_value {
+                    ChangeValue::Sapling(value) => {
+                        let output_index = tx_metadata.output_index(idx).expect(
+                            "Missing Sapling output of autoshielding transaction. This is a programming error.",
+                        );
+                        SentTransactionOutput {
+                            output_index,
+                            recipient: Recipient::InternalAccount(account, PoolType::Sapling),
+                            value: *value,
+                            memo: Some(memo.clone()),
+                        }
+                    }
+                })
+                .collect(),
+            fee_amount: proposal.balance().fee_required(),
+            utxos_spent: utxos.iter().map(|utxo| utxo.outpoint().clone()).collect(),
+        })
+        .map_err(Error::DataSource)
+}
+
+fn select_key_for_note<N>(
+    selected: &SpendableNote<N>,
+    extsk: &ExtendedSpendingKey,
+    dfvk: &DiversifiableFullViewingKey,
+) -> Option<(
+    sapling::Note,
+    ExtendedSpendingKey,
+    MerklePath<sapling::Node>,
+)> {
+    let merkle_path = selected.witness.path().expect("the tree is not empty");
+
+    // Attempt to reconstruct the note being spent using both the internal and external dfvks
+    // corresponding to the unified spending key, checking against the witness we are using
+    // to spend the note that we've used the correct key.
+    let external_note = dfvk
+        .diversified_address(selected.diversifier)
+        .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
+    let internal_note = dfvk
+        .diversified_change_address(selected.diversifier)
+        .and_then(|addr| addr.create_note(selected.note_value.into(), selected.rseed));
+
+    let expected_root = selected.witness.root();
+    external_note
+        .filter(|n| expected_root == merkle_path.root(n.commitment()))
+        .map(|n| (n, extsk.clone(), merkle_path.clone()))
+        .or_else(|| {
+            internal_note
+                .filter(|n| expected_root == merkle_path.root(n.commitment()))
+                .map(|n| (n, extsk.derive_internal(), merkle_path))
+        })
 }

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -0,0 +1,407 @@
+//! Types related to the process of selecting inputs to be spent given a transaction request.
+
+use core::marker::PhantomData;
+use std::fmt;
+
+use zcash_primitives::{
+    consensus::{self, BlockHeight},
+    legacy::TransparentAddress,
+    transaction::{
+        components::{
+            amount::{Amount, BalanceError, NonNegativeAmount},
+            sapling::fees as sapling,
+            TxOut,
+        },
+        fees::FeeRule,
+    },
+    zip32::AccountId,
+};
+
+use crate::{
+    address::{RecipientAddress, UnifiedAddress},
+    data_api::WalletRead,
+    fees::{ChangeError, ChangeStrategy, TransactionBalance},
+    wallet::{SpendableNote, WalletTransparentOutput},
+    zip321::TransactionRequest,
+};
+
+/// The type of errors that may be produced in input selection.
+pub enum InputSelectorError<DbErrT, SelectorErrT> {
+    /// An error occurred accessing the underlying data store.
+    DataSource(DbErrT),
+    /// An error occurred specific to the provided input selector's selection rules.
+    Selection(SelectorErrT),
+    /// Insufficient funds were available to satisfy the payment request that inputs were being
+    /// selected to attempt to satisfy.
+    InsufficientFunds { available: Amount, required: Amount },
+}
+
+impl<DE: fmt::Display, SE: fmt::Display> fmt::Display for InputSelectorError<DE, SE> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match &self {
+            InputSelectorError::DataSource(e) => {
+                write!(
+                    f,
+                    "The underlying datasource produced the following error: {}",
+                    e
+                )
+            }
+            InputSelectorError::Selection(e) => {
+                write!(f, "Note selection encountered the following error: {}", e)
+            }
+            InputSelectorError::InsufficientFunds {
+                available,
+                required,
+            } => write!(
+                f,
+                "Insufficient balance (have {}, need {} including fee)",
+                i64::from(*available),
+                i64::from(*required)
+            ),
+        }
+    }
+}
+
+/// A data structure that describes the inputs to be consumed and outputs to
+/// be produced in a proposed transaction.
+pub struct Proposal<FeeRuleT, TransparentInput, NoteRef> {
+    transaction_request: TransactionRequest,
+    transparent_inputs: Vec<TransparentInput>,
+    sapling_inputs: Vec<SpendableNote<NoteRef>>,
+    balance: TransactionBalance,
+    fee_rule: FeeRuleT,
+}
+
+impl<FeeRuleT, TransparentInput, NoteRef> Proposal<FeeRuleT, TransparentInput, NoteRef> {
+    /// Returns the transaction request that describes the payments to be made.
+    pub fn transaction_request(&self) -> &TransactionRequest {
+        &self.transaction_request
+    }
+    /// Returns the transparent inputs that have been selected to fund the transaction.
+    pub fn transparent_inputs(&self) -> &[TransparentInput] {
+        &self.transparent_inputs
+    }
+    /// Returns the Sapling inputs that have been selected to fund the transaction.
+    pub fn sapling_inputs(&self) -> &[SpendableNote<NoteRef>] {
+        &self.sapling_inputs
+    }
+    /// Returns the change outputs to be added to the transaction and the fee to be paid.
+    pub fn balance(&self) -> &TransactionBalance {
+        &self.balance
+    }
+    /// Returns the fee rule to be used by the transaction builder.
+    pub fn fee_rule(&self) -> &FeeRuleT {
+        &self.fee_rule
+    }
+}
+
+/// A strategy for selecting transaction inputs and proposing transaction outputs.
+///
+/// Proposals should include only economically useful inputs, as determined by `Self::FeeRule`;
+/// that is, do not return inputs that cause fees to increase by an amount greater than the value
+/// of the input.
+pub trait InputSelector {
+    /// The type of errors that may be generated in input selection
+    type Error;
+    /// The type of data source that the input selector expects to access to obtain input notes and
+    /// UTXOs. This associated type permits input selectors that may use specialized knowledge of
+    /// the internals of a particular backing data store, if the generic API of `WalletRead` does
+    /// not provide sufficiently fine-grained operations for a particular backing store to
+    /// optimally perform input selection.
+    type DataSource: WalletRead;
+    /// The type of the fee rule that this input selector uses when computing fees.
+    type FeeRule: FeeRule;
+
+    /// Performs input selection and returns a proposal for transaction construction including
+    /// change and fee outputs.
+    ///
+    /// Implementations of this method should return inputs sufficient to satisfy the given
+    /// transaction request using a best-effort strategy to preserve user privacy, as follows:
+    /// * If it is possible to satisfy the specified transaction request by creating
+    ///   a fully-shielded transaction without requiring value to cross pool boundaries,
+    ///   return the inputs necessary to construct such a transaction; otherwise
+    /// * If it is possible to satisfy the transaction request by creating a fully-shielded
+    ///   transaction with some amounts crossing between shielded pools, return the inputs
+    ///   necessary.
+    ///
+    /// If insufficient funds are available to satisfy the required outputs for the shielding
+    /// request, this operation must fail and return [`InputSelectorError::InsufficientFunds`].
+    #[allow(clippy::too_many_arguments)]
+    #[allow(clippy::type_complexity)]
+    fn propose_transaction<ParamsT>(
+        &self,
+        params: &ParamsT,
+        wallet_db: &Self::DataSource,
+        account: AccountId,
+        anchor_height: BlockHeight,
+        target_height: BlockHeight,
+        transaction_request: TransactionRequest,
+    ) -> Result<
+        Proposal<
+            Self::FeeRule,
+            std::convert::Infallible,
+            <<Self as InputSelector>::DataSource as WalletRead>::NoteRef,
+        >,
+        InputSelectorError<<<Self as InputSelector>::DataSource as WalletRead>::Error, Self::Error>,
+    >
+    where
+        ParamsT: consensus::Parameters;
+
+    /// Performs input selection and returns a proposal for the construction of a shielding
+    /// transaction.
+    ///
+    /// Implementations should return the maximum possible number of economically useful inputs
+    /// required to supply at least the requested value, choosing only inputs received at the
+    /// specified source addresses. If insufficient funds are available to satisfy the required
+    /// outputs for the shielding request, this operation must fail and return
+    /// [`InputSelectorError::InsufficientFunds`].
+    #[allow(clippy::too_many_arguments)]
+    #[allow(clippy::type_complexity)]
+    fn propose_shielding<ParamsT>(
+        &self,
+        params: &ParamsT,
+        wallet_db: &Self::DataSource,
+        shielding_threshold: NonNegativeAmount,
+        source_addrs: &[TransparentAddress],
+        confirmed_height: BlockHeight,
+        target_height: BlockHeight,
+    ) -> Result<
+        Proposal<
+            Self::FeeRule,
+            WalletTransparentOutput,
+            <<Self as InputSelector>::DataSource as WalletRead>::NoteRef,
+        >,
+        InputSelectorError<<<Self as InputSelector>::DataSource as WalletRead>::Error, Self::Error>,
+    >
+    where
+        ParamsT: consensus::Parameters;
+}
+
+/// Errors that can occur as a consequence of greedy input selection.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum GreedyInputSelectorError<ChangeErrT> {
+    /// An intermediate value overflowed or underflowed the valid monetary range.
+    Balance(BalanceError),
+    /// A unified address did not contain a supported receiver.
+    UnsupportedAddress(Box<UnifiedAddress>),
+    /// An error was encountered in change selection.
+    Change(ChangeError<ChangeErrT>),
+}
+
+impl<DbErrT, ChangeErrT> From<GreedyInputSelectorError<ChangeErrT>>
+    for InputSelectorError<DbErrT, GreedyInputSelectorError<ChangeErrT>>
+{
+    fn from(err: GreedyInputSelectorError<ChangeErrT>) -> Self {
+        InputSelectorError::Selection(err)
+    }
+}
+
+impl<DbErrT, ChangeErrT> From<ChangeError<ChangeErrT>>
+    for InputSelectorError<DbErrT, GreedyInputSelectorError<ChangeErrT>>
+{
+    fn from(err: ChangeError<ChangeErrT>) -> Self {
+        InputSelectorError::Selection(GreedyInputSelectorError::Change(err))
+    }
+}
+
+impl<DbErrT, ChangeErrT> From<BalanceError>
+    for InputSelectorError<DbErrT, GreedyInputSelectorError<ChangeErrT>>
+{
+    fn from(err: BalanceError) -> Self {
+        InputSelectorError::Selection(GreedyInputSelectorError::Balance(err))
+    }
+}
+
+struct SaplingPayment(Amount);
+impl sapling::OutputView for SaplingPayment {
+    fn value(&self) -> Amount {
+        self.0
+    }
+}
+
+/// An [`InputSelector`] implementation that uses a greedy strategy to select between available
+/// notes.
+///
+/// This implementation performs input selection using methods available via the [`WalletRead`]
+/// interface.
+pub struct GreedyInputSelector<DbT, ChangeT> {
+    change_strategy: ChangeT,
+    _ds_type: PhantomData<DbT>,
+}
+
+impl<DbT, ChangeT: ChangeStrategy> GreedyInputSelector<DbT, ChangeT> {
+    /// Constructs a new greedy input selector that uses the provided change strategy to determine
+    /// change values and fee amounts.
+    pub fn new(change_strategy: ChangeT) -> Self {
+        GreedyInputSelector {
+            change_strategy,
+            _ds_type: PhantomData,
+        }
+    }
+}
+
+impl<DbT, ChangeT> InputSelector for GreedyInputSelector<DbT, ChangeT>
+where
+    DbT: WalletRead,
+    ChangeT: ChangeStrategy,
+    ChangeT::FeeRule: Clone,
+{
+    type Error = GreedyInputSelectorError<<ChangeT as ChangeStrategy>::Error>;
+    type DataSource = DbT;
+    type FeeRule = ChangeT::FeeRule;
+
+    #[allow(clippy::type_complexity)]
+    fn propose_transaction<ParamsT>(
+        &self,
+        params: &ParamsT,
+        wallet_db: &Self::DataSource,
+        account: AccountId,
+        anchor_height: BlockHeight,
+        target_height: BlockHeight,
+        transaction_request: TransactionRequest,
+    ) -> Result<
+        Proposal<Self::FeeRule, std::convert::Infallible, DbT::NoteRef>,
+        InputSelectorError<DbT::Error, Self::Error>,
+    >
+    where
+        ParamsT: consensus::Parameters,
+    {
+        let mut transparent_outputs = vec![];
+        let mut sapling_outputs = vec![];
+        let mut output_total = Amount::zero();
+        for payment in transaction_request.payments() {
+            output_total = (output_total + payment.amount).ok_or(BalanceError::Overflow)?;
+
+            let mut push_transparent = |taddr: TransparentAddress| {
+                transparent_outputs.push(TxOut {
+                    value: payment.amount,
+                    script_pubkey: taddr.script(),
+                });
+            };
+            let mut push_sapling = || {
+                sapling_outputs.push(SaplingPayment(payment.amount));
+            };
+
+            match &payment.recipient_address {
+                RecipientAddress::Transparent(addr) => {
+                    push_transparent(*addr);
+                }
+                RecipientAddress::Shielded(_) => {
+                    push_sapling();
+                }
+                RecipientAddress::Unified(addr) => {
+                    if addr.sapling().is_some() {
+                        push_sapling();
+                    } else if let Some(addr) = addr.transparent() {
+                        push_transparent(*addr);
+                    } else {
+                        return Err(InputSelectorError::Selection(
+                            GreedyInputSelectorError::UnsupportedAddress(Box::new(addr.clone())),
+                        ));
+                    }
+                }
+            }
+        }
+
+        let mut sapling_inputs: Vec<SpendableNote<DbT::NoteRef>> = vec![];
+        let mut prior_amount = Amount::zero();
+        // This loop is guaranteed to terminate because on each iteration we check that the amount
+        // of funds selected is strictly increasing. The loop will either return a successful
+        // result or the wallet will eventually run out of funds to select.
+        loop {
+            let balance = self.change_strategy.compute_balance(
+                params,
+                target_height,
+                &Vec::<WalletTransparentOutput>::new(),
+                &transparent_outputs,
+                &sapling_inputs,
+                &sapling_outputs,
+            );
+
+            match balance {
+                Ok(balance) => {
+                    return Ok(Proposal {
+                        transaction_request,
+                        transparent_inputs: vec![],
+                        sapling_inputs,
+                        balance,
+                        fee_rule: (*self.change_strategy.fee_rule()).clone(),
+                    });
+                }
+                Err(ChangeError::InsufficientFunds { required, .. }) => {
+                    sapling_inputs = wallet_db
+                        .select_spendable_sapling_notes(account, required, anchor_height)
+                        .map_err(InputSelectorError::DataSource)?;
+
+                    let new_amount = sapling_inputs
+                        .iter()
+                        .map(|n| n.note_value)
+                        .sum::<Option<Amount>>()
+                        .ok_or(BalanceError::Overflow)?;
+
+                    if new_amount <= prior_amount {
+                        return Err(InputSelectorError::InsufficientFunds {
+                            required,
+                            available: new_amount,
+                        });
+                    } else {
+                        // If the set of selected inputs has changed after selection, we will loop again
+                        // and see whether we now have enough funds.
+                        prior_amount = new_amount;
+                    }
+                }
+                Err(other) => return Err(other.into()),
+            }
+        }
+    }
+
+    #[allow(clippy::type_complexity)]
+    fn propose_shielding<ParamsT>(
+        &self,
+        params: &ParamsT,
+        wallet_db: &Self::DataSource,
+        shielding_threshold: NonNegativeAmount,
+        source_addrs: &[TransparentAddress],
+        confirmed_height: BlockHeight,
+        target_height: BlockHeight,
+    ) -> Result<
+        Proposal<Self::FeeRule, WalletTransparentOutput, DbT::NoteRef>,
+        InputSelectorError<DbT::Error, Self::Error>,
+    >
+    where
+        ParamsT: consensus::Parameters,
+    {
+        let transparent_inputs: Vec<WalletTransparentOutput> = source_addrs
+            .iter()
+            .map(|taddr| wallet_db.get_unspent_transparent_outputs(taddr, confirmed_height))
+            .collect::<Result<Vec<Vec<_>>, _>>()
+            .map_err(InputSelectorError::DataSource)?
+            .into_iter()
+            .flat_map(|v| v.into_iter())
+            .collect();
+
+        let balance = self.change_strategy.compute_balance(
+            params,
+            target_height,
+            &transparent_inputs,
+            &Vec::<TxOut>::new(),
+            &Vec::<SpendableNote<DbT::NoteRef>>::new(),
+            &Vec::<SaplingPayment>::new(),
+        )?;
+
+        if balance.total() >= shielding_threshold.into() {
+            Ok(Proposal {
+                transaction_request: TransactionRequest::empty(),
+                transparent_inputs,
+                sapling_inputs: vec![],
+                balance,
+                fee_rule: (*self.change_strategy.fee_rule()).clone(),
+            })
+        } else {
+            Err(InputSelectorError::InsufficientFunds {
+                available: balance.total(),
+                required: shielding_threshold.into(),
+            })
+        }
+    }
+}

zcash_client_backend/src/fees.rs

@@ -29,15 +29,22 @@ impl ChangeValue {
 pub struct TransactionBalance {
     proposed_change: Vec<ChangeValue>,
     fee_required: Amount,
+    total: Amount,
 }
 
 impl TransactionBalance {
     /// Constructs a new balance from its constituent parts.
-    pub fn new(proposed_change: Vec<ChangeValue>, fee_required: Amount) -> Self {
+    pub fn new(proposed_change: Vec<ChangeValue>, fee_required: Amount) -> Option<Self> {
-        TransactionBalance {
+        proposed_change
-            proposed_change,
+            .iter()
-            fee_required,
+            .map(|v| v.value())
-        }
+            .chain(Some(fee_required))
+            .sum::<Option<Amount>>()
+            .map(|total| TransactionBalance {
+                proposed_change,
+                fee_required,
+                total,
+            })
     }
 
     /// The change values proposed by the [`ChangeStrategy`] that computed this balance.  
@@ -50,11 +57,26 @@ impl TransactionBalance {
     pub fn fee_required(&self) -> Amount {
         self.fee_required
     }
+
+    /// Returns the sum of the proposed change outputs and the required fee.
+    pub fn total(&self) -> Amount {
+        self.total
+    }
 }
 
 /// Errors that can occur in computing suggested change and/or fees.
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ChangeError<E> {
-    InsufficientFunds { available: Amount, required: Amount },
+    /// Insufficient inputs were provided to change selection to fund the
+    /// required outputs and fees.
+    InsufficientFunds {
+        /// The total of the inputs provided to change selection
+        available: Amount,
+        /// The total amount of input value required to fund the requested outputs,
+        /// including the required fees.
+        required: Amount,
+    },
+    /// An error occurred that was specific to the change selection strategy in use.
     StrategyError(E),
 }
 
@@ -66,7 +88,7 @@ pub trait ChangeStrategy {
 
     /// Returns the fee rule that this change strategy will respect when performing
     /// balance computations.
-    fn fee_rule(&self) -> Self::FeeRule;
+    fn fee_rule(&self) -> &Self::FeeRule;
 
     /// Computes the totals of inputs, suggested change amounts, and fees given the
     /// provided inputs and outputs being used to construct a transaction.
@@ -86,78 +108,90 @@ pub trait ChangeStrategy {
     ) -> Result<TransactionBalance, ChangeError<Self::Error>>;
 }
 
-/// A change strategy that uses a fixed fee amount and proposes change as a single output
+/// A change strategy that and proposes change as a single output to the most current supported
-/// to the most current supported pool.
+/// shielded pool and delegates fee calculation to the provided fee rule.
-pub struct BasicFixedFeeChangeStrategy {
+pub struct SingleOutputFixedFeeChangeStrategy {
-    fixed_fee: Amount,
+    fee_rule: FixedFeeRule,
 }
 
-impl BasicFixedFeeChangeStrategy {
+impl SingleOutputFixedFeeChangeStrategy {
-    // Constructs a new [`BasicFixedFeeChangeStrategy`] with the specified fixed fee
+    /// Constructs a new [`SingleOutputFixedFeeChangeStrategy`] with the specified fee rule.
-    // amount.
+    pub fn new(fee_rule: FixedFeeRule) -> Self {
-    pub fn new(fixed_fee: Amount) -> Self {
+        Self { fee_rule }
-        Self { fixed_fee }
     }
 }
 
-impl ChangeStrategy for BasicFixedFeeChangeStrategy {
+impl From<BalanceError> for ChangeError<BalanceError> {
+    fn from(err: BalanceError) -> ChangeError<BalanceError> {
+        ChangeError::StrategyError(err)
+    }
+}
+
+impl ChangeStrategy for SingleOutputFixedFeeChangeStrategy {
     type FeeRule = FixedFeeRule;
     type Error = BalanceError;
 
-    fn fee_rule(&self) -> Self::FeeRule {
+    fn fee_rule(&self) -> &Self::FeeRule {
-        FixedFeeRule::new(self.fixed_fee)
+        &self.fee_rule
     }
 
     fn compute_balance<P: consensus::Parameters>(
         &self,
-        _params: &P,
+        params: &P,
-        _target_height: BlockHeight,
+        target_height: BlockHeight,
         transparent_inputs: &[impl transparent::InputView],
         transparent_outputs: &[impl transparent::OutputView],
         sapling_inputs: &[impl sapling::InputView],
         sapling_outputs: &[impl sapling::OutputView],
     ) -> Result<TransactionBalance, ChangeError<Self::Error>> {
-        let overflow = || ChangeError::StrategyError(BalanceError::Overflow);
-        let underflow = || ChangeError::StrategyError(BalanceError::Underflow);
-
         let t_in = transparent_inputs
             .iter()
             .map(|t_in| t_in.coin().value)
             .sum::<Option<_>>()
-            .ok_or_else(overflow)?;
+            .ok_or(BalanceError::Overflow)?;
         let t_out = transparent_outputs
             .iter()
             .map(|t_out| t_out.value())
             .sum::<Option<_>>()
-            .ok_or_else(overflow)?;
+            .ok_or(BalanceError::Overflow)?;
         let sapling_in = sapling_inputs
             .iter()
             .map(|s_in| s_in.value())
             .sum::<Option<_>>()
-            .ok_or_else(overflow)?;
+            .ok_or(BalanceError::Overflow)?;
         let sapling_out = sapling_outputs
             .iter()
             .map(|s_out| s_out.value())
             .sum::<Option<_>>()
-            .ok_or_else(overflow)?;
+            .ok_or(BalanceError::Overflow)?;
 
-        let total_in = (t_in + sapling_in).ok_or_else(overflow)?;
+        let fee_amount = self
-        let total_out = [t_out, sapling_out, self.fixed_fee]
+            .fee_rule
+            .fee_required(
+                params,
+                target_height,
+                transparent_inputs,
+                transparent_outputs,
+                sapling_inputs.len(),
+                sapling_outputs.len() + 1,
+            )
+            .unwrap(); // FixedFeeRule::fee_required is infallible.
+
+        let total_in = (t_in + sapling_in).ok_or(BalanceError::Overflow)?;
+        let total_out = [t_out, sapling_out, fee_amount]
             .iter()
             .sum::<Option<Amount>>()
-            .ok_or_else(overflow)?;
+            .ok_or(BalanceError::Overflow)?;
 
-        let proposed_change = (total_in - total_out).ok_or_else(underflow)?;
+        let proposed_change = (total_in - total_out).ok_or(BalanceError::Underflow)?;
         if proposed_change < Amount::zero() {
             Err(ChangeError::InsufficientFunds {
                 available: total_in,
                 required: total_out,
             })
         } else {
-            Ok(TransactionBalance::new(
+            TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount)
-                vec![ChangeValue::Sapling(proposed_change)],
+                .ok_or_else(|| BalanceError::Overflow.into())
-                self.fixed_fee,
-            ))
         }
     }
 }

zcash_client_backend/src/wallet.rs

@@ -10,7 +10,8 @@ use zcash_primitives::{
     sapling::{Diversifier, Node, Note, Nullifier, PaymentAddress, Rseed},
     transaction::{
         components::{
-            transparent::{OutPoint, TxOut},
+            sapling,
+            transparent::{self, OutPoint, TxOut},
             Amount,
         },
         TxId,
@@ -69,6 +70,19 @@ impl WalletTransparentOutput {
     pub fn recipient_address(&self) -> &TransparentAddress {
         &self.recipient_address
     }
+
+    pub fn value(&self) -> Amount {
+        self.txout.value
+    }
+}
+
+impl transparent::fees::InputView for WalletTransparentOutput {
+    fn outpoint(&self) -> &OutPoint {
+        &self.outpoint
+    }
+    fn coin(&self) -> &TxOut {
+        &self.txout
+    }
 }
 
 /// A subset of a [`SpendDescription`] relevant to wallets and light clients.
@@ -97,13 +111,20 @@ pub struct WalletShieldedOutput<N> {
 
 /// Information about a note that is tracked by the wallet that is available for spending,
 /// with sufficient information for use in note selection.
-pub struct SpendableNote {
+pub struct SpendableNote<NoteRef> {
+    pub note_id: NoteRef,
     pub diversifier: Diversifier,
     pub note_value: Amount,
     pub rseed: Rseed,
     pub witness: IncrementalWitness<Node>,
 }
 
+impl<NoteRef> sapling::fees::InputView for SpendableNote<NoteRef> {
+    fn value(&self) -> Amount {
+        self.note_value
+    }
+}
+
 /// Describes a policy for which outgoing viewing key should be able to decrypt
 /// transaction outputs.
 ///

zcash_client_backend/src/zip321.rs

@@ -115,6 +115,11 @@ pub struct TransactionRequest {
 }
 
 impl TransactionRequest {
+    /// Constructs a new empty transaction request.
+    pub fn empty() -> Self {
+        Self { payments: vec![] }
+    }
+
     /// Constructs a new transaction request that obeys the ZIP-321 invariants
     pub fn new(payments: Vec<Payment>) -> Result<TransactionRequest, Zip321Error> {
         let request = TransactionRequest { payments };

zcash_client_sqlite/CHANGELOG.md

@@ -22,6 +22,8 @@ and this library adheres to Rust's notion of
     to initialize the accounts table with a noncontiguous set of account identifiers.
   - `SqliteClientError::AccountIdOutOfRange`, to report when the maximum account
     identifier has been reached.
+  - `SqliteClientError::Protobuf`, to support handling of errors in serialized
+    protobuf data decoding.
 - An `unstable` feature flag; this is added to parts of the API that may change
   in any release. It enables `zcash_client_backend`'s `unstable` feature flag.
 - New summary views that may be directly accessed in the sqlite database.
@@ -40,6 +42,7 @@ and this library adheres to Rust's notion of
   this block source.
   - `zcash_client_sqlite::chain::init::init_blockmeta_db` creates the required
     metadata cache database.
+- Implementations of `PartialEq`, `Eq`, `PartialOrd`, and `Ord` for `NoteId`
 
 ### Changed
 - Various **BREAKING CHANGES** have been made to the database tables. These will
@@ -95,6 +98,10 @@ and this library adheres to Rust's notion of
   - `delete_utxos_above` (use `WalletWrite::rewind_to_height` instead)
 - `zcash_client_sqlite::with_blocks` (use
   `zcash_client_backend::data_api::BlockSource::with_blocks` instead)
+- `zcash_client_sqlite::error::SqliteClientError` variants:
+  - `SqliteClientError::IncorrectHrpExtFvk`
+  - `SqliteClientError::Base58`
+  - `SqliteClientError::BackendError`
 
 ### Fixed
 - The `zcash_client_backend::data_api::WalletRead::get_address` implementation

zcash_client_sqlite/Cargo.toml

@@ -42,6 +42,7 @@ uuid = "1.1"
 # (Breaking upgrades to these are usually backwards-compatible, but check MSRVs.)
 
 [dev-dependencies]
+assert_matches = "1.5"
 proptest = "1.0.0"
 rand_core = "0.6"
 regex = "1.4"

zcash_client_sqlite/src/chain.rs

@@ -5,13 +5,13 @@ use rusqlite::params;
 
 use zcash_primitives::consensus::BlockHeight;
 
-use zcash_client_backend::{data_api::error::Error, proto::compact_formats::CompactBlock};
+use zcash_client_backend::{data_api::chain::error::Error, proto::compact_formats::CompactBlock};
 
 use crate::{error::SqliteClientError, BlockDb};
 
 #[cfg(feature = "unstable")]
 use {
-    crate::{BlockHash, FsBlockDb},
+    crate::{BlockHash, FsBlockDb, FsBlockDbError},
     rusqlite::Connection,
     std::fs::File,
     std::io::Read,
@@ -21,53 +21,51 @@ use {
 pub mod init;
 pub mod migrations;
 
-struct CompactBlockRow {
-    height: BlockHeight,
-    data: Vec<u8>,
-}
-
 /// Implements a traversal of `limit` blocks of the block cache database.
 ///
 /// Starting at the next block above `last_scanned_height`, the `with_row` callback is invoked with
 /// each block retrieved from the backing store. If the `limit` value provided is `None`, all
 /// blocks are traversed up to the maximum height.
-pub(crate) fn blockdb_with_blocks<F>(
+pub(crate) fn blockdb_with_blocks<F, DbErrT, NoteRef>(
-    cache: &BlockDb,
+    block_source: &BlockDb,
     last_scanned_height: BlockHeight,
     limit: Option<u32>,
     mut with_row: F,
-) -> Result<(), SqliteClientError>
+) -> Result<(), Error<DbErrT, SqliteClientError, NoteRef>>
 where
-    F: FnMut(CompactBlock) -> Result<(), SqliteClientError>,
+    F: FnMut(CompactBlock) -> Result<(), Error<DbErrT, SqliteClientError, NoteRef>>,
 {
-    // Fetch the CompactBlocks we need to scan
+    fn to_chain_error<D, E: Into<SqliteClientError>, N>(err: E) -> Error<D, SqliteClientError, N> {
-    let mut stmt_blocks = cache.0.prepare(
+        Error::BlockSource(err.into())
-        "SELECT height, data FROM compactblocks WHERE height > ? ORDER BY height ASC LIMIT ?",
+    }
-    )?;
 
-    let rows = stmt_blocks.query_map(
+    // Fetch the CompactBlocks we need to scan
-        params![
+    let mut stmt_blocks = block_source
+        .0
+        .prepare(
+            "SELECT height, data FROM compactblocks 
+            WHERE height > ? 
+            ORDER BY height ASC LIMIT ?",
+        )
+        .map_err(to_chain_error)?;
+
+    let mut rows = stmt_blocks
+        .query(params![
             u32::from(last_scanned_height),
             limit.unwrap_or(u32::max_value()),
-        ],
+        ])
-        |row| {
+        .map_err(to_chain_error)?;
-            Ok(CompactBlockRow {
-                height: BlockHeight::from_u32(row.get(0)?),
-                data: row.get(1)?,
-            })
-        },
-    )?;
 
-    for row_result in rows {
+    while let Some(row) = rows.next().map_err(to_chain_error)? {
-        let cbr = row_result?;
+        let height = BlockHeight::from_u32(row.get(0).map_err(to_chain_error)?);
-        let block = CompactBlock::decode(&cbr.data[..]).map_err(Error::from)?;
+        let data: Vec<u8> = row.get(1).map_err(to_chain_error)?;
-
+        let block = CompactBlock::decode(&data[..]).map_err(to_chain_error)?;
-        if block.height() != cbr.height {
+        if block.height() != height {
-            return Err(SqliteClientError::CorruptedData(format!(
+            return Err(to_chain_error(SqliteClientError::CorruptedData(format!(
                 "Block height {} did not match row's height field value {}",
                 block.height(),
-                cbr.height
+                height
-            )));
+            ))));
         }
 
         with_row(block)?;
@@ -160,53 +158,65 @@ pub(crate) fn blockmetadb_get_max_cached_height(
 /// invoked with each block retrieved from the backing store. If the `limit` value provided is
 /// `None`, all blocks are traversed up to the maximum height for which metadata is available.
 #[cfg(feature = "unstable")]
-pub(crate) fn fsblockdb_with_blocks<F>(
+pub(crate) fn fsblockdb_with_blocks<F, DbErrT, NoteRef>(
     cache: &FsBlockDb,
     last_scanned_height: BlockHeight,
     limit: Option<u32>,
     mut with_block: F,
-) -> Result<(), SqliteClientError>
+) -> Result<(), Error<DbErrT, FsBlockDbError, NoteRef>>
 where
-    F: FnMut(CompactBlock) -> Result<(), SqliteClientError>,
+    F: FnMut(CompactBlock) -> Result<(), Error<DbErrT, FsBlockDbError, NoteRef>>,
 {
+    fn to_chain_error<D, E: Into<FsBlockDbError>, N>(err: E) -> Error<D, FsBlockDbError, N> {
+        Error::BlockSource(err.into())
+    }
+
     // Fetch the CompactBlocks we need to scan
-    let mut stmt_blocks = cache.conn.prepare(
+    let mut stmt_blocks = cache
-        "SELECT height, blockhash, time, sapling_outputs_count, orchard_actions_count
+        .conn
+        .prepare(
+            "SELECT height, blockhash, time, sapling_outputs_count, orchard_actions_count
          FROM compactblocks_meta
          WHERE height > ?
          ORDER BY height ASC LIMIT ?",
-    )?;
+        )
+        .map_err(to_chain_error)?;
 
-    let rows = stmt_blocks.query_map(
+    let rows = stmt_blocks
-        params![
+        .query_map(
-            u32::from(last_scanned_height),
+            params![
-            limit.unwrap_or(u32::max_value()),
+                u32::from(last_scanned_height),
-        ],
+                limit.unwrap_or(u32::max_value()),
-        |row| {
+            ],
-            Ok(BlockMeta {
+            |row| {
-                height: BlockHeight::from_u32(row.get(0)?),
+                Ok(BlockMeta {
-                block_hash: BlockHash::from_slice(&row.get::<_, Vec<_>>(1)?),
+                    height: BlockHeight::from_u32(row.get(0)?),
-                block_time: row.get(2)?,
+                    block_hash: BlockHash::from_slice(&row.get::<_, Vec<_>>(1)?),
-                sapling_outputs_count: row.get(3)?,
+                    block_time: row.get(2)?,
-                orchard_actions_count: row.get(4)?,
+                    sapling_outputs_count: row.get(3)?,
-            })
+                    orchard_actions_count: row.get(4)?,
-        },
+                })
-    )?;
+            },
+        )
+        .map_err(to_chain_error)?;
 
     for row_result in rows {
-        let cbr = row_result?;
+        let cbr = row_result.map_err(to_chain_error)?;
-        let mut block_file = File::open(cbr.block_file_path(&cache.blocks_dir))?;
+        let mut block_file =
+            File::open(cbr.block_file_path(&cache.blocks_dir)).map_err(to_chain_error)?;
         let mut block_data = vec![];
-        block_file.read_to_end(&mut block_data)?;
+        block_file
+            .read_to_end(&mut block_data)
+            .map_err(to_chain_error)?;
 
-        let block = CompactBlock::decode(&block_data[..]).map_err(Error::from)?;
+        let block = CompactBlock::decode(&block_data[..]).map_err(to_chain_error)?;
 
         if block.height() != cbr.height {
-            return Err(SqliteClientError::CorruptedData(format!(
+            return Err(to_chain_error(FsBlockDbError::CorruptedData(format!(
                 "Block height {} did not match row's height field value {}",
                 block.height(),
                 cbr.height
-            )));
+            ))));
         }
 
         with_block(block)?;
@@ -225,21 +235,20 @@ mod tests {
         block::BlockHash, transaction::components::Amount, zip32::ExtendedSpendingKey,
     };
 
-    use zcash_client_backend::data_api::WalletRead;
+    use zcash_client_backend::data_api::chain::{
-    use zcash_client_backend::data_api::{
+        error::{Cause, Error},
-        chain::{scan_cached_blocks, validate_chain},
+        scan_cached_blocks, validate_chain,
-        error::{ChainInvalid, Error},
     };
+    use zcash_client_backend::data_api::WalletRead;
 
     use crate::{
         chain::init::init_cache_database,
-        error::SqliteClientError,
         tests::{
             self, fake_compact_block, fake_compact_block_spending, init_test_accounts_table,
             insert_into_cache, sapling_activation_height, AddressType,
         },
         wallet::{get_balance, init::init_wallet_db, rewind_to_height},
-        AccountId, BlockDb, NoteId, WalletDb,
+        AccountId, BlockDb, WalletDb,
     };
 
     #[test]
@@ -385,16 +394,13 @@ mod tests {
         insert_into_cache(&db_cache, &cb4);
 
         // Data+cache chain should be invalid at the data/cache boundary
-        match validate_chain(
+        let val_result = validate_chain(
             &tests::network(),
             &db_cache,
             db_data.get_max_height_hash().unwrap(),
-        ) {
+        );
-            Err(SqliteClientError::BackendError(Error::InvalidChain(lower_bound, _))) => {
+
-                assert_eq!(lower_bound, sapling_activation_height() + 2)
+        assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 2);
-            }
-            _ => panic!(),
-        }
     }
 
     #[test]
@@ -459,16 +465,13 @@ mod tests {
         insert_into_cache(&db_cache, &cb4);
 
         // Data+cache chain should be invalid inside the cache
-        match validate_chain(
+        let val_result = validate_chain(
             &tests::network(),
             &db_cache,
             db_data.get_max_height_hash().unwrap(),
-        ) {
+        );
-            Err(SqliteClientError::BackendError(Error::InvalidChain(lower_bound, _))) => {
+
-                assert_eq!(lower_bound, sapling_activation_height() + 3)
+        assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 3);
-            }
-            _ => panic!(),
-        }
     }
 
     #[test]
@@ -590,14 +593,11 @@ mod tests {
         );
         insert_into_cache(&db_cache, &cb3);
         match scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None) {
-            Err(SqliteClientError::BackendError(e)) => {
+            Err(Error::Chain(e)) => {
-                assert_eq!(
+                assert_matches!(
-                    e.to_string(),
+                    e.cause(),
-                    ChainInvalid::block_height_discontinuity::<NoteId>(
+                    Cause::BlockHeightDiscontinuity(h) if *h
-                        sapling_activation_height() + 1,
+                        == sapling_activation_height() + 2
-                        sapling_activation_height() + 2
-                    )
-                    .to_string()
                 );
             }
             Ok(_) | Err(_) => panic!("Should have failed"),

zcash_client_sqlite/src/error.rs

@@ -3,13 +3,10 @@
 use std::error;
 use std::fmt;
 
-use zcash_client_backend::{
+use zcash_client_backend::encoding::{Bech32DecodeError, TransparentCodecError};
-    data_api,
-    encoding::{Bech32DecodeError, TransparentCodecError},
-};
 use zcash_primitives::{consensus::BlockHeight, zip32::AccountId};
 
-use crate::{NoteId, PRUNING_HEIGHT};
+use crate::PRUNING_HEIGHT;
 
 #[cfg(feature = "transparent-inputs")]
 use zcash_primitives::legacy::TransparentAddress;
@@ -23,8 +20,8 @@ pub enum SqliteClientError {
     /// Decoding of a stored value from its serialized form has failed.
     CorruptedData(String),
 
-    /// Decoding of the extended full viewing key has failed (for the specified network)
+    /// An error occurred decoding a protobuf message.
-    IncorrectHrpExtFvk,
+    Protobuf(prost::DecodeError),
 
     /// The rcm value for a note cannot be decoded to a valid JubJub point.
     InvalidNote,
@@ -43,14 +40,12 @@ pub enum SqliteClientError {
     /// A Bech32-encoded key or address decoding error
     Bech32DecodeError(Bech32DecodeError),
 
-    /// Base58 decoding error
+    /// An error produced in legacy transparent address derivation
-    Base58(bs58::decode::Error),
-
-    ///
     #[cfg(feature = "transparent-inputs")]
     HdwalletError(hdwallet::error::Error),
 
-    /// Base58 decoding error
+    /// An error encountered in decoding a transparent address from its
+    /// serialized form.
     TransparentAddress(TransparentCodecError),
 
     /// Wrapper for rusqlite errors.
@@ -67,9 +62,6 @@ pub enum SqliteClientError {
     /// (safe rewind height, requested height).
     RequestedRewindInvalid(BlockHeight, BlockHeight),
 
-    /// Wrapper for errors from zcash_client_backend
-    BackendError(data_api::error::Error<NoteId>),
-
     /// The space of allocatable diversifier indices has been exhausted for
     /// the given account.
     DiversifierIndexOutOfRange,
@@ -109,14 +101,13 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::CorruptedData(reason) => {
                 write!(f, "Data DB is corrupted: {}", reason)
             }
-            SqliteClientError::IncorrectHrpExtFvk => write!(f, "Incorrect HRP for extfvk"),
+            SqliteClientError::Protobuf(e) => write!(f, "Failed to parse protobuf-encoded record: {}", e),
             SqliteClientError::InvalidNote => write!(f, "Invalid note"),
             SqliteClientError::InvalidNoteId =>
                 write!(f, "The note ID associated with an inserted witness must correspond to a received note."),
             SqliteClientError::RequestedRewindInvalid(h, r) =>
                 write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_HEIGHT, h, r),
             SqliteClientError::Bech32DecodeError(e) => write!(f, "{}", e),
-            SqliteClientError::Base58(e) => write!(f, "{}", e),
             #[cfg(feature = "transparent-inputs")]
             SqliteClientError::HdwalletError(e) => write!(f, "{:?}", e),
             SqliteClientError::TransparentAddress(e) => write!(f, "{}", e),
@@ -126,7 +117,6 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::DbError(e) => write!(f, "{}", e),
             SqliteClientError::Io(e) => write!(f, "{}", e),
             SqliteClientError::InvalidMemo(e) => write!(f, "{}", e),
-            SqliteClientError::BackendError(e) => write!(f, "{}", e),
             SqliteClientError::DiversifierIndexOutOfRange => write!(f, "The space of available diversifier indices is exhausted"),
             SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id),
             SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."),
@@ -155,9 +145,9 @@ impl From<Bech32DecodeError> for SqliteClientError {
     }
 }
 
-impl From<bs58::decode::Error> for SqliteClientError {
+impl From<prost::DecodeError> for SqliteClientError {
-    fn from(e: bs58::decode::Error) -> Self {
+    fn from(e: prost::DecodeError) -> Self {
-        SqliteClientError::Base58(e)
+        SqliteClientError::Protobuf(e)
     }
 }
 
@@ -179,9 +169,3 @@ impl From<zcash_primitives::memo::Error> for SqliteClientError {
         SqliteClientError::InvalidMemo(e)
     }
 }
-
-impl From<data_api::error::Error<NoteId>> for SqliteClientError {
-    fn from(e: data_api::error::Error<NoteId>) -> Self {
-        SqliteClientError::BackendError(e)
-    }
-}

zcash_client_sqlite/src/lib.rs

@@ -25,7 +25,7 @@
 //!
 //! [`WalletRead`]: zcash_client_backend::data_api::WalletRead
 //! [`WalletWrite`]: zcash_client_backend::data_api::WalletWrite
-//! [`BlockSource`]: zcash_client_backend::data_api::BlockSource
+//! [`BlockSource`]: zcash_client_backend::data_api::chain::BlockSource
 //! [`CompactBlock`]: zcash_client_backend::proto::compact_formats::CompactBlock
 //! [`init_cache_database`]: crate::chain::init::init_cache_database
 
@@ -52,8 +52,8 @@ use zcash_primitives::{
 use zcash_client_backend::{
     address::{AddressMetadata, UnifiedAddress},
     data_api::{
-        BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, SentTransaction,
+        self, chain::BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient,
-        WalletRead, WalletWrite,
+        SentTransaction, WalletRead, WalletWrite,
     },
     keys::{UnifiedFullViewingKey, UnifiedSpendingKey},
     proto::compact_formats::CompactBlock,
@@ -63,9 +63,6 @@ use zcash_client_backend::{
 
 use crate::error::SqliteClientError;
 
-#[cfg(not(feature = "transparent-inputs"))]
-use zcash_client_backend::data_api::error::Error;
-
 #[cfg(feature = "unstable")]
 use {
     crate::chain::{fsblockdb_with_blocks, BlockMeta},
@@ -87,7 +84,7 @@ pub(crate) const PRUNING_HEIGHT: u32 = 100;
 
 /// A newtype wrapper for sqlite primary key values for the notes
 /// table.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub enum NoteId {
     SentNoteId(i64),
     ReceivedNoteId(i64),
@@ -230,7 +227,7 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         &self,
         account: AccountId,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error> {
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
         #[allow(deprecated)]
         wallet::transact::get_spendable_sapling_notes(self, account, anchor_height)
     }
@@ -240,7 +237,7 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         account: AccountId,
         target_value: Amount,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error> {
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
         #[allow(deprecated)]
         wallet::transact::select_spendable_sapling_notes(self, account, target_value, anchor_height)
     }
@@ -253,9 +250,9 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         return wallet::get_transparent_receivers(&self.params, &self.conn, _account);
 
         #[cfg(not(feature = "transparent-inputs"))]
-        return Err(SqliteClientError::BackendError(
+        panic!(
-            Error::TransparentInputsNotSupported,
+            "The wallet must be compiled with the transparent-inputs feature to use this method."
-        ));
+        );
     }
 
     fn get_unspent_transparent_outputs(
@@ -267,9 +264,9 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         return wallet::get_unspent_transparent_outputs(self, _address, _max_height);
 
         #[cfg(not(feature = "transparent-inputs"))]
-        return Err(SqliteClientError::BackendError(
+        panic!(
-            Error::TransparentInputsNotSupported,
+            "The wallet must be compiled with the transparent-inputs feature to use this method."
-        ));
+        );
     }
 
     fn get_transparent_balances(
@@ -281,9 +278,9 @@ impl<P: consensus::Parameters> WalletRead for WalletDb<P> {
         return wallet::get_transparent_balances(self, _account, _max_height);
 
         #[cfg(not(feature = "transparent-inputs"))]
-        return Err(SqliteClientError::BackendError(
+        panic!(
-            Error::TransparentInputsNotSupported,
+            "The wallet must be compiled with the transparent-inputs feature to use this method."
-        ));
+        );
     }
 }
 
@@ -375,7 +372,7 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> {
         &self,
         account: AccountId,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error> {
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
         self.wallet_db
             .get_spendable_sapling_notes(account, anchor_height)
     }
@@ -385,7 +382,7 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> {
         account: AccountId,
         target_value: Amount,
         anchor_height: BlockHeight,
-    ) -> Result<Vec<SpendableNote>, Self::Error> {
+    ) -> Result<Vec<SpendableNote<Self::NoteRef>>, Self::Error> {
         self.wallet_db
             .select_spendable_sapling_notes(account, target_value, anchor_height)
     }
@@ -745,9 +742,9 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> {
         return wallet::put_received_transparent_utxo(self, _output);
 
         #[cfg(not(feature = "transparent-inputs"))]
-        return Err(SqliteClientError::BackendError(
+        panic!(
-            Error::TransparentInputsNotSupported,
+            "The wallet must be compiled with the transparent-inputs feature to use this method."
-        ));
+        );
     }
 }
 
@@ -764,14 +761,17 @@ impl BlockDb {
 impl BlockSource for BlockDb {
     type Error = SqliteClientError;
 
-    fn with_blocks<F>(
+    fn with_blocks<F, DbErrT, NoteRef>(
         &self,
         from_height: BlockHeight,
         limit: Option<u32>,
         with_row: F,
-    ) -> Result<(), Self::Error>
+    ) -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>
     where
-        F: FnMut(CompactBlock) -> Result<(), Self::Error>,
+        F: FnMut(
+            CompactBlock,
+        )
+            -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>,
     {
         chain::blockdb_with_blocks(self, from_height, limit, with_row)
     }
@@ -788,7 +788,7 @@ impl BlockSource for BlockDb {
 ///
 /// where `<block_height>` is the decimal value of the height at which the block was mined, and
 /// `<block_hash>` is the hexadecimal representation of the block hash, as produced by the
-/// [`Display`] implementation for [`zcash_primitives::block::BlockHash`].
+/// [`fmt::Display`] implementation for [`zcash_primitives::block::BlockHash`].
 ///
 /// This block source is intended to be used with the following data flow:
 /// * When the cache is being filled:
@@ -828,6 +828,7 @@ pub struct FsBlockDb {
 pub enum FsBlockDbError {
     FsError(io::Error),
     DbError(rusqlite::Error),
+    Protobuf(prost::DecodeError),
     InvalidBlockstoreRoot(PathBuf),
     InvalidBlockPath(PathBuf),
     CorruptedData(String),
@@ -847,6 +848,13 @@ impl From<rusqlite::Error> for FsBlockDbError {
     }
 }
 
+#[cfg(feature = "unstable")]
+impl From<prost::DecodeError> for FsBlockDbError {
+    fn from(e: prost::DecodeError) -> Self {
+        FsBlockDbError::Protobuf(e)
+    }
+}
+
 #[cfg(feature = "unstable")]
 impl FsBlockDb {
     /// Creates a filesystem-backed block store at the given path.
@@ -896,21 +904,28 @@ impl FsBlockDb {
 
 #[cfg(feature = "unstable")]
 impl BlockSource for FsBlockDb {
-    type Error = SqliteClientError;
+    type Error = FsBlockDbError;
 
-    fn with_blocks<F>(
+    fn with_blocks<F, DbErrT, NoteRef>(
         &self,
         from_height: BlockHeight,
         limit: Option<u32>,
         with_row: F,
-    ) -> Result<(), Self::Error>
+    ) -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>
     where
-        F: FnMut(CompactBlock) -> Result<(), Self::Error>,
+        F: FnMut(
+            CompactBlock,
+        )
+            -> Result<(), data_api::chain::error::Error<DbErrT, Self::Error, NoteRef>>,
     {
         fsblockdb_with_blocks(self, from_height, limit, with_row)
     }
 }
 
+#[cfg(test)]
+#[macro_use]
+extern crate assert_matches;
+
 #[cfg(test)]
 #[allow(deprecated)]
 mod tests {

zcash_client_sqlite/src/wallet.rs

@@ -27,7 +27,7 @@ use zcash_primitives::{
 
 use zcash_client_backend::{
     address::{RecipientAddress, UnifiedAddress},
-    data_api::{error::Error, PoolType, Recipient, SentTransactionOutput},
+    data_api::{PoolType, Recipient, SentTransactionOutput},
     keys::UnifiedFullViewingKey,
     wallet::{WalletShieldedOutput, WalletTx},
     DecryptedOutput,
@@ -745,7 +745,7 @@ pub(crate) fn rewind_to_height<P: consensus::Parameters>(
     let sapling_activation_height = wdb
         .params
         .activation_height(NetworkUpgrade::Sapling)
-        .ok_or(SqliteClientError::BackendError(Error::SaplingNotActive))?;
+        .expect("Sapling activation height mutst be available.");
 
     // Recall where we synced up to previously.
     let last_scanned_height = wdb

zcash_client_sqlite/src/wallet/transact.rs

@@ -14,11 +14,12 @@ use zcash_primitives::{
 
 use zcash_client_backend::wallet::SpendableNote;
 
-use crate::{error::SqliteClientError, WalletDb};
+use crate::{error::SqliteClientError, NoteId, WalletDb};
 
-fn to_spendable_note(row: &Row) -> Result<SpendableNote, SqliteClientError> {
+fn to_spendable_note(row: &Row) -> Result<SpendableNote<NoteId>, SqliteClientError> {
+    let note_id = NoteId::ReceivedNoteId(row.get(0)?);
     let diversifier = {
-        let d: Vec<_> = row.get(0)?;
+        let d: Vec<_> = row.get(1)?;
         if d.len() != 11 {
             return Err(SqliteClientError::CorruptedData(
                 "Invalid diversifier length".to_string(),
@@ -29,10 +30,10 @@ fn to_spendable_note(row: &Row) -> Result<SpendableNote, SqliteClientError> {
         Diversifier(tmp)
     };
 
-    let note_value = Amount::from_i64(row.get(1)?).unwrap();
+    let note_value = Amount::from_i64(row.get(2)?).unwrap();
 
     let rseed = {
-        let rcm_bytes: Vec<_> = row.get(2)?;
+        let rcm_bytes: Vec<_> = row.get(3)?;
 
         // We store rcm directly in the data DB, regardless of whether the note
         // used a v1 or v2 note plaintext, so for the purposes of spending let's
@@ -47,11 +48,12 @@ fn to_spendable_note(row: &Row) -> Result<SpendableNote, SqliteClientError> {
     };
 
     let witness = {
-        let d: Vec<_> = row.get(3)?;
+        let d: Vec<_> = row.get(4)?;
         IncrementalWitness::read(&d[..])?
     };
 
     Ok(SpendableNote {
+        note_id,
         diversifier,
         note_value,
         rseed,
@@ -66,9 +68,9 @@ pub fn get_spendable_sapling_notes<P>(
     wdb: &WalletDb<P>,
     account: AccountId,
     anchor_height: BlockHeight,
-) -> Result<Vec<SpendableNote>, SqliteClientError> {
+) -> Result<Vec<SpendableNote<NoteId>>, SqliteClientError> {
     let mut stmt_select_notes = wdb.conn.prepare(
-        "SELECT diversifier, value, rcm, witness
+        "SELECT id_note, diversifier, value, rcm, witness
             FROM received_notes
             INNER JOIN transactions ON transactions.id_tx = received_notes.tx
             INNER JOIN sapling_witnesses ON sapling_witnesses.note = received_notes.id_note
@@ -98,7 +100,7 @@ pub fn select_spendable_sapling_notes<P>(
     account: AccountId,
     target_value: Amount,
     anchor_height: BlockHeight,
-) -> Result<Vec<SpendableNote>, SqliteClientError> {
+) -> Result<Vec<SpendableNote<NoteId>>, SqliteClientError> {
     // The goal of this SQL statement is to select the oldest notes until the required
     // value has been reached, and then fetch the witnesses at the desired height for the
     // selected notes. This is achieved in several steps:
@@ -134,7 +136,7 @@ pub fn select_spendable_sapling_notes<P>(
             SELECT note, witness FROM sapling_witnesses
             WHERE block = :anchor_height
         )
-        SELECT selected.diversifier, selected.value, selected.rcm, witnesses.witness
+        SELECT selected.id_note, selected.diversifier, selected.value, selected.rcm, witnesses.witness
         FROM selected
         INNER JOIN witnesses ON selected.id_note = witnesses.note",
     )?;
@@ -220,7 +222,7 @@ mod tests {
 
         // Attempting to spend with a USK that is not in the wallet results in an error
         let mut db_write = db_data.get_update_ops().unwrap();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -232,10 +234,8 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::KeyNotRecognized)
-                data_api::error::Error::KeyNotRecognized
+        );
-            ))
-        ));
     }
 
     #[test]
@@ -253,7 +253,7 @@ mod tests {
 
         // We cannot do anything if we aren't synchronised
         let mut db_write = db_data.get_update_ops().unwrap();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -265,10 +265,8 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::ScanRequired)
-                data_api::error::Error::ScanRequired
+        );
-            ))
-        ));
     }
 
     #[test]
@@ -300,7 +298,7 @@ mod tests {
 
         // We cannot spend anything
         let mut db_write = db_data.get_update_ops().unwrap();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -312,14 +310,12 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::InsufficientFunds {
-                data_api::error::Error::InsufficientBalance(
+                available,
-                    available,
+                required
-                    required
+            })
-                )
-            ))
             if available == Amount::zero() && required == Amount::from_u64(1001).unwrap()
-        ));
+        );
     }
 
     #[test]
@@ -384,7 +380,7 @@ mod tests {
         // Spend fails because there are insufficient verified notes
         let extsk2 = ExtendedSpendingKey::master(&[]);
         let to = extsk2.default_address().1.into();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -396,15 +392,13 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::InsufficientFunds {
-                data_api::error::Error::InsufficientBalance(
+                available,
-                    available,
+                required
-                    required
+            })
-                )
-            ))
             if available == Amount::from_u64(50000).unwrap()
                 && required == Amount::from_u64(71000).unwrap()
-        ));
+        );
 
         // Mine blocks SAPLING_ACTIVATION_HEIGHT + 2 to 9 until just before the second
         // note is verified
@@ -421,7 +415,7 @@ mod tests {
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Second spend still fails
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -433,15 +427,13 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::InsufficientFunds {
-                data_api::error::Error::InsufficientBalance(
+                available,
-                    available,
+                required
-                    required
+            })
-                )
-            ))
             if available == Amount::from_u64(50000).unwrap()
                 && required == Amount::from_u64(71000).unwrap()
-        ));
+        );
 
         // Mine block 11 so that the second note becomes verified
         let (cb, _) = fake_compact_block(
@@ -455,7 +447,7 @@ mod tests {
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Second spend should now succeed
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -468,7 +460,7 @@ mod tests {
                 10,
             ),
             Ok(_)
-        ));
+        );
     }
 
     #[test]
@@ -504,7 +496,7 @@ mod tests {
         // Send some of the funds to another address
         let extsk2 = ExtendedSpendingKey::master(&[]);
         let to = extsk2.default_address().1.into();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -517,10 +509,10 @@ mod tests {
                 10,
             ),
             Ok(_)
-        ));
+        );
 
         // A second spend fails because there are no usable notes
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -532,14 +524,12 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::InsufficientFunds {
-                data_api::error::Error::InsufficientBalance(
+                available,
-                    available,
+                required
-                    required
+            })
-                )
-            ))
             if available == Amount::zero() && required == Amount::from_u64(3000).unwrap()
-        ));
+        );
 
         // Mine blocks SAPLING_ACTIVATION_HEIGHT + 1 to 21 (that don't send us funds)
         // until just before the first transaction expires
@@ -556,7 +546,7 @@ mod tests {
         scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap();
 
         // Second spend still fails
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -568,14 +558,12 @@ mod tests {
                 OvkPolicy::Sender,
                 10,
             ),
-            Err(crate::SqliteClientError::BackendError(
+            Err(data_api::error::Error::InsufficientFunds {
-                data_api::error::Error::InsufficientBalance(
+                available,
-                    available,
+                required
-                    required
+            })
-                )
-            ))
             if available == Amount::zero() && required == Amount::from_u64(3000).unwrap()
-        ));
+        );
 
         // Mine block SAPLING_ACTIVATION_HEIGHT + 22 so that the first transaction expires
         let (cb, _) = fake_compact_block(
@@ -804,7 +792,7 @@ mod tests {
         );
 
         let to = TransparentAddress::PublicKey([7; 20]).into();
-        assert!(matches!(
+        assert_matches!(
             create_spend_to_address(
                 &mut db_write,
                 &tests::network(),
@@ -817,6 +805,6 @@ mod tests {
                 10,
             ),
             Ok(_)
-        ));
+        );
     }
 }

zcash_primitives/CHANGELOG.md

@@ -10,9 +10,12 @@ and this library adheres to Rust's notion of
 ### Added
 - Added in `zcash_primitives::zip32`
   - An implementation of `TryFrom<DiversifierIndex>` for `u32`
+- `zcash_primitives::transaction::components::amount::NonNegativeAmount`
 - Added to `zcash_primitives::transaction::builder`
   - `Error::InsufficientFunds`
   - `Error::ChangeRequired`
+  - `Error::Balance`
+  - `Error::Fee`
   - `Builder` state accessor methods:
     - `Builder::params()`
     - `Builder::target_height()`
@@ -33,14 +36,21 @@ and this library adheres to Rust's notion of
 - `zcash_primitives::sapling::Note::commitment`
 - Added to `zcash_primitives::zip32::sapling::DiversifiableFullViewingKey`
   - `DiversifiableFullViewingKey::{diversified_address, diversified_change_address}`
+- `impl Eq for zcash_primitves::sapling::PaymentAddress`
 
 ### Changed
 - `zcash_primitives::transaction::builder::Builder::build` now takes a `FeeRule`
   argument which is used to compute the fee for the transaction as part of the
   build process.
+- `zcash_primitives::transaction::builder::Builder::value_balance` now
+  returns `Result<Amount, BalanceError>` instead of `Option<Amount>`.
 - `zcash_primitives::transaction::builder::Builder::{new, new_with_rng}` no
   longer fixes the fee for transactions to 0.00001 ZEC; the builder instead
   computes the fee using a `FeeRule` implementation at build time.
+- `zcash_primitives::transaction::builder::Error` now is parameterized by the
+  types that can now be produced by fee calculation.
+- `zcash_primitives::transaction::components::tze::builder::Builder::value_balance` now
+  returns `Result<Amount, BalanceError>` instead of `Option<Amount>`.
 
 ### Deprecated
 - `zcash_primitives::zip32::sapling::to_extended_full_viewing_key` Use
@@ -48,13 +58,14 @@ and this library adheres to Rust's notion of
 
 ### Removed
 - Removed from `zcash_primitives::transaction::builder::Builder`
-  - `Builder::{new_with_fee, new_with_rng_and_fee`} (use `Builder::{new, new_with_rng}` 
+  - `Builder::{new_with_fee, new_with_rng_and_fee`} (use `Builder::{new, new_with_rng}`
     instead along with a `FeeRule` implementation passed to `Builder::build`.)
   - `Builder::send_change_to` has been removed. Change outputs must be added to the
     builder by the caller, just like any other output.
 - Removed from `zcash_primitives::transaction::builder::Error`
   - `Error::ChangeIsNegative`
   - `Error::NoChangeAddress`
+  - `Error::InvalidAmount` (replaced by `Error::BalanceError`)
 - `zcash_primitives::transaction::components::sapling::builder::SaplingBuilder::get_candidate_change_address`
    has been removed; change outputs must now be added by the caller.
 - The `From<&ExtendedSpendingKey>` instance for `ExtendedFullViewingKey` has been

zcash_primitives/src/sapling.rs

@@ -280,6 +280,8 @@ impl PartialEq for PaymentAddress {
     }
 }
 
+impl Eq for PaymentAddress {}
+
 impl PaymentAddress {
     /// Constructs a PaymentAddress from a diversifier and a Jubjub point.
     ///

zcash_primitives/src/transaction/builder.rs

@@ -1,7 +1,6 @@
 //! Structs for building transactions.
 
 use std::cmp::Ordering;
-use std::convert::Infallible;
 use std::error;
 use std::fmt;
 use std::sync::mpsc::Sender;
@@ -20,7 +19,7 @@ use crate::{
     sapling::{prover::TxProver, Diversifier, Node, Note, PaymentAddress},
     transaction::{
         components::{
-            amount::Amount,
+            amount::{Amount, BalanceError},
             sapling::{
                 self,
                 builder::{SaplingBuilder, SaplingMetadata},
@@ -54,15 +53,17 @@ const DEFAULT_TX_EXPIRY_DELTA: u32 = 20;
 
 /// Errors that can occur during transaction construction.
 #[derive(Debug, PartialEq, Eq)]
-pub enum Error {
+pub enum Error<FeeError> {
     /// Insufficient funds were provided to the transaction builder; the given
     /// additional amount is required in order to construct the transaction.
     InsufficientFunds(Amount),
     /// The transaction has inputs in excess of outputs and fees; the user must
     /// add a change output.
     ChangeRequired(Amount),
+    /// An error occurred in computing the fees for a transaction.
+    Fee(FeeError),
     /// An overflow or underflow occurred when computing value balances
-    InvalidAmount,
+    Balance(BalanceError),
     /// An error occurred in constructing the transparent parts of a transaction.
     TransparentBuild(transparent::builder::Error),
     /// An error occurred in constructing the Sapling parts of a transaction.
@@ -72,7 +73,7 @@ pub enum Error {
     TzeBuild(tze::builder::Error),
 }
 
-impl fmt::Display for Error {
+impl<FE: fmt::Display> fmt::Display for Error<FE> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             Error::InsufficientFunds(amount) => write!(
@@ -85,7 +86,8 @@ impl fmt::Display for Error {
                 "The transaction requires an additional change output of {:?} zatoshis",
                 amount
             ),
-            Error::InvalidAmount => write!(f, "Invalid amount (overflow or underflow)"),
+            Error::Balance(e) => write!(f, "Invalid amount {:?}", e),
+            Error::Fee(e) => write!(f, "An error occurred in fee calculation: {}", e),
             Error::TransparentBuild(err) => err.fmt(f),
             Error::SaplingBuild(err) => err.fmt(f),
             #[cfg(feature = "zfuture")]
@@ -94,11 +96,11 @@ impl fmt::Display for Error {
     }
 }
 
-impl error::Error for Error {}
+impl<FE: fmt::Debug + fmt::Display> error::Error for Error<FE> {}
 
-impl From<Infallible> for Error {
+impl<FE> From<BalanceError> for Error<FE> {
-    fn from(_: Infallible) -> Error {
+    fn from(e: BalanceError) -> Self {
-        unreachable!()
+        Error::Balance(e)
     }
 }
 
@@ -239,10 +241,9 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         diversifier: Diversifier,
         note: Note,
         merkle_path: MerklePath<Node>,
-    ) -> Result<(), Error> {
+    ) -> Result<(), sapling::builder::Error> {
         self.sapling_builder
             .add_spend(&mut self.rng, extsk, diversifier, note, merkle_path)
-            .map_err(Error::SaplingBuild)
     }
 
     /// Adds a Sapling address to send funds to.
@@ -252,10 +253,9 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         to: PaymentAddress,
         value: Amount,
         memo: MemoBytes,
-    ) -> Result<(), Error> {
+    ) -> Result<(), sapling::builder::Error> {
         self.sapling_builder
             .add_output(&mut self.rng, ovk, to, value, memo)
-            .map_err(Error::SaplingBuild)
     }
 
     /// Adds a transparent coin to be spent in this transaction.
@@ -266,10 +266,8 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         sk: secp256k1::SecretKey,
         utxo: transparent::OutPoint,
         coin: TxOut,
-    ) -> Result<(), Error> {
+    ) -> Result<(), transparent::builder::Error> {
-        self.transparent_builder
+        self.transparent_builder.add_input(sk, utxo, coin)
-            .add_input(sk, utxo, coin)
-            .map_err(Error::TransparentBuild)
     }
 
     /// Adds a transparent address to send funds to.
@@ -277,10 +275,8 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         &mut self,
         to: &TransparentAddress,
         value: Amount,
-    ) -> Result<(), Error> {
+    ) -> Result<(), transparent::builder::Error> {
-        self.transparent_builder
+        self.transparent_builder.add_output(to, value)
-            .add_output(to, value)
-            .map_err(Error::TransparentBuild)
     }
 
     /// Sets the notifier channel, where progress of building the transaction is sent.
@@ -294,22 +290,18 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
     }
 
     /// Returns the sum of the transparent, Sapling, and TZE value balances.
-    fn value_balance(&self) -> Result<Amount, Error> {
+    fn value_balance(&self) -> Result<Amount, BalanceError> {
         let value_balances = [
-            self.transparent_builder
+            self.transparent_builder.value_balance()?,
-                .value_balance()
-                .ok_or(Error::InvalidAmount)?,
             self.sapling_builder.value_balance(),
             #[cfg(feature = "zfuture")]
-            self.tze_builder
+            self.tze_builder.value_balance()?,
-                .value_balance()
-                .ok_or(Error::InvalidAmount)?,
         ];
 
         value_balances
             .into_iter()
             .sum::<Option<_>>()
-            .ok_or(Error::InvalidAmount)
+            .ok_or(BalanceError::Overflow)
     }
 
     /// Builds a transaction from the configured spends and outputs.
@@ -320,18 +312,17 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         self,
         prover: &impl TxProver,
         fee_rule: &FR,
-    ) -> Result<(Transaction, SaplingMetadata), Error>
+    ) -> Result<(Transaction, SaplingMetadata), Error<FR::Error>> {
-    where
+        let fee = fee_rule
-        Error: From<FR::Error>,
+            .fee_required(
-    {
+                &self.params,
-        let fee = fee_rule.fee_required(
+                self.target_height,
-            &self.params,
+                self.transparent_builder.inputs(),
-            self.target_height,
+                self.transparent_builder.outputs(),
-            self.transparent_builder.inputs(),
+                self.sapling_builder.inputs().len(),
-            self.transparent_builder.outputs(),
+                self.sapling_builder.outputs().len(),
-            self.sapling_builder.inputs(),
+            )
-            self.sapling_builder.outputs(),
+            .map_err(Error::Fee)?;
-        )?;
         self.build_internal(prover, fee)
     }
 
@@ -344,28 +335,28 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         self,
         prover: &impl TxProver,
         fee_rule: &FR,
-    ) -> Result<(Transaction, SaplingMetadata), Error>
+    ) -> Result<(Transaction, SaplingMetadata), Error<FR::Error>> {
-    where
+        let fee = fee_rule
-        Error: From<FR::Error>,
+            .fee_required_zfuture(
-    {
+                &self.params,
-        let fee = fee_rule.fee_required_zfuture(
+                self.target_height,
-            &self.params,
+                self.transparent_builder.inputs(),
-            self.target_height,
+                self.transparent_builder.outputs(),
-            self.transparent_builder.inputs(),
+                self.sapling_builder.inputs().len(),
-            self.transparent_builder.outputs(),
+                self.sapling_builder.outputs().len(),
-            self.sapling_builder.inputs(),
+                self.tze_builder.inputs(),
-            self.sapling_builder.outputs(),
+                self.tze_builder.outputs(),
-            self.tze_builder.inputs(),
+            )
-            self.tze_builder.outputs(),
+            .map_err(Error::Fee)?;
-        )?;
+
         self.build_internal(prover, fee)
     }
 
-    fn build_internal(
+    fn build_internal<FE>(
         self,
         prover: &impl TxProver,
         fee: Amount,
-    ) -> Result<(Transaction, SaplingMetadata), Error> {
+    ) -> Result<(Transaction, SaplingMetadata), Error<FE>> {
         let consensus_branch_id = BranchId::for_height(&self.params, self.target_height);
 
         // determine transaction version
@@ -376,7 +367,7 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> {
         //
 
         // After fees are accounted for, the value balance of the transaction must be zero.
-        let balance_after_fees = (self.value_balance()? - fee).ok_or(Error::InvalidAmount)?;
+        let balance_after_fees = (self.value_balance()? - fee).ok_or(BalanceError::Underflow)?;
 
         match balance_after_fees.cmp(&Amount::zero()) {
             Ordering::Less => {
@@ -514,6 +505,7 @@ impl<'a, P: consensus::Parameters, R: RngCore + CryptoRng> ExtensionTxBuilder<'a
 #[cfg(any(test, feature = "test-dependencies"))]
 mod testing {
     use rand::RngCore;
+    use std::convert::Infallible;
 
     use super::{Builder, Error, SaplingMetadata};
     use crate::{
@@ -536,7 +528,7 @@ mod testing {
             Self::new_internal(params, rng, height)
         }
 
-        pub fn mock_build(self) -> Result<(Transaction, SaplingMetadata), Error> {
+        pub fn mock_build(self) -> Result<(Transaction, SaplingMetadata), Error<Infallible>> {
             self.build(&MockTxProver, &FixedFeeRule::new(DEFAULT_FEE))
         }
     }
@@ -599,7 +591,7 @@ mod tests {
                 Amount::from_i64(-1).unwrap(),
                 MemoBytes::empty()
             ),
-            Err(Error::SaplingBuild(build_s::Error::InvalidAmount))
+            Err(build_s::Error::InvalidAmount)
         );
     }
 
@@ -714,7 +706,7 @@ mod tests {
                 &TransparentAddress::PublicKey([0; 20]),
                 Amount::from_i64(-1).unwrap(),
             ),
-            Err(Error::TransparentBuild(build_t::Error::InvalidAmount))
+            Err(build_t::Error::InvalidAmount)
         );
     }
 

zcash_primitives/src/transaction/components/amount.rs

@@ -213,6 +213,35 @@ impl TryFrom<orchard::ValueSum> for Amount {
     }
 }
 
+/// A type-safe representation of some nonnegative amount of Zcash.
+///
+/// A NonNegativeAmount can only be constructed from an integer that is within the valid monetary
+/// range of `{0..MAX_MONEY}` (where `MAX_MONEY` = 21,000,000 × 10⁸ zatoshis).
+#[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Eq, Ord)]
+pub struct NonNegativeAmount(Amount);
+
+impl NonNegativeAmount {
+    /// Creates a NonNegativeAmount from a u64.
+    ///
+    /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`.
+    pub fn from_u64(amount: u64) -> Result<Self, ()> {
+        Amount::from_u64(amount).map(NonNegativeAmount)
+    }
+
+    /// Creates a NonNegativeAmount from an i64.
+    ///
+    /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`.
+    pub fn from_nonnegative_i64(amount: i64) -> Result<Self, ()> {
+        Amount::from_nonnegative_i64(amount).map(NonNegativeAmount)
+    }
+}
+
+impl From<NonNegativeAmount> for Amount {
+    fn from(n: NonNegativeAmount) -> Self {
+        n.0
+    }
+}
+
 /// A type for balance violations in amount addition and subtraction
 /// (overflow and underflow of allowed ranges)
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]

zcash_primitives/src/transaction/components/transparent/builder.rs

@@ -6,7 +6,7 @@ use crate::{
     legacy::{Script, TransparentAddress},
     transaction::{
         components::{
-            amount::Amount,
+            amount::{Amount, BalanceError},
             transparent::{self, fees, Authorization, Authorized, Bundle, TxIn, TxOut},
         },
         sighash::TransparentAuthorizingContext,
@@ -176,23 +176,26 @@ impl TransparentBuilder {
         Ok(())
     }
 
-    pub fn value_balance(&self) -> Option<Amount> {
+    pub fn value_balance(&self) -> Result<Amount, BalanceError> {
         #[cfg(feature = "transparent-inputs")]
         let input_sum = self
             .inputs
             .iter()
             .map(|input| input.coin.value)
-            .sum::<Option<Amount>>()?;
+            .sum::<Option<Amount>>()
+            .ok_or(BalanceError::Overflow)?;
 
         #[cfg(not(feature = "transparent-inputs"))]
         let input_sum = Amount::zero();
 
-        input_sum
+        let output_sum = self
-            - self
+            .vout
-                .vout
+            .iter()
-                .iter()
+            .map(|vo| vo.value)
-                .map(|vo| vo.value)
+            .sum::<Option<Amount>>()
-                .sum::<Option<Amount>>()?
+            .ok_or(BalanceError::Overflow)?;
+
+        (input_sum - output_sum).ok_or(BalanceError::Underflow)
     }
 
     pub fn build(self) -> Option<transparent::Bundle<Unauthorized>> {

zcash_primitives/src/transaction/components/tze/builder.rs

@@ -8,7 +8,7 @@ use crate::{
     transaction::{
         self as tx,
         components::{
-            amount::Amount,
+            amount::{Amount, BalanceError},
             tze::{fees, Authorization, Authorized, Bundle, OutPoint, TzeIn, TzeOut},
         },
     },
@@ -121,16 +121,22 @@ impl<'a, BuildCtx> TzeBuilder<'a, BuildCtx> {
         Ok(())
     }
 
-    pub fn value_balance(&self) -> Option<Amount> {
+    pub fn value_balance(&self) -> Result<Amount, BalanceError> {
-        self.vin
+        let total_in = self
+            .vin
             .iter()
             .map(|tzi| tzi.coin.value)
-            .sum::<Option<Amount>>()?
+            .sum::<Option<Amount>>()
-            - self
+            .ok_or(BalanceError::Overflow)?;
-                .vout
+
-                .iter()
+        let total_out = self
-                .map(|tzo| tzo.value)
+            .vout
-                .sum::<Option<Amount>>()?
+            .iter()
+            .map(|tzo| tzo.value)
+            .sum::<Option<Amount>>()
+            .ok_or(BalanceError::Overflow)?;
+
+        (total_in - total_out).ok_or(BalanceError::Underflow)
     }
 
     pub fn build(self) -> (Option<Bundle<Unauthorized>>, Vec<TzeSigner<'a, BuildCtx>>) {

zcash_primitives/src/transaction/fees.rs

@@ -2,9 +2,7 @@
 
 use crate::{
     consensus::{self, BlockHeight},
-    transaction::components::{
+    transaction::components::{amount::Amount, transparent::fees as transparent},
-        amount::Amount, sapling::fees as sapling, transparent::fees as transparent,
-    },
 };
 
 #[cfg(feature = "zfuture")]
@@ -26,8 +24,8 @@ pub trait FeeRule {
         target_height: BlockHeight,
         transparent_inputs: &[impl transparent::InputView],
         transparent_outputs: &[impl transparent::OutputView],
-        sapling_inputs: &[impl sapling::InputView],
+        sapling_input_count: usize,
-        sapling_outputs: &[impl sapling::OutputView],
+        sapling_output_count: usize,
     ) -> Result<Amount, Self::Error>;
 }
 
@@ -47,8 +45,8 @@ pub trait FutureFeeRule: FeeRule {
         target_height: BlockHeight,
         transparent_inputs: &[impl transparent::InputView],
         transparent_outputs: &[impl transparent::OutputView],
-        sapling_inputs: &[impl sapling::InputView],
+        sapling_input_count: usize,
-        sapling_outputs: &[impl sapling::OutputView],
+        sapling_output_count: usize,
         tze_inputs: &[impl tze::InputView],
         tze_outputs: &[impl tze::OutputView],
     ) -> Result<Amount, Self::Error>;
@@ -56,6 +54,7 @@ pub trait FutureFeeRule: FeeRule {
 
 /// A fee rule that always returns a fixed fee, irrespective of the structure of
 /// the transaction being constructed.
+#[derive(Clone, Copy, Debug)]
 pub struct FixedFeeRule {
     fixed_fee: Amount,
 }
@@ -76,8 +75,8 @@ impl FeeRule for FixedFeeRule {
         _target_height: BlockHeight,
         _transparent_inputs: &[impl transparent::InputView],
         _transparent_outputs: &[impl transparent::OutputView],
-        _sapling_inputs: &[impl sapling::InputView],
+        _sapling_input_count: usize,
-        _sapling_outputs: &[impl sapling::OutputView],
+        _sapling_output_count: usize,
     ) -> Result<Amount, Self::Error> {
         Ok(self.fixed_fee)
     }
@@ -91,8 +90,8 @@ impl FutureFeeRule for FixedFeeRule {
         _target_height: BlockHeight,
         _transparent_inputs: &[impl transparent::InputView],
         _transparent_outputs: &[impl transparent::OutputView],
-        _sapling_inputs: &[impl sapling::InputView],
+        _sapling_input_count: usize,
-        _sapling_outputs: &[impl sapling::OutputView],
+        _sapling_output_count: usize,
         _tze_inputs: &[impl tze::InputView],
         _tze_outputs: &[impl tze::OutputView],
     ) -> Result<Amount, Self::Error> {