From 9a7dc0db8453da801519edb95ad30dc86163e231 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 17 Oct 2022 11:35:14 -0600 Subject: [PATCH 1/4] 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 | 89 +++- zcash_client_backend/src/address.rs | 2 +- zcash_client_backend/src/data_api.rs | 65 +-- zcash_client_backend/src/data_api/chain.rs | 365 +++++++------ .../src/data_api/chain/error.rs | 190 +++++++ zcash_client_backend/src/data_api/error.rs | 201 ++++---- zcash_client_backend/src/data_api/wallet.rs | 479 +++++++++--------- .../src/data_api/wallet/input_selection.rs | 407 +++++++++++++++ zcash_client_backend/src/fees.rs | 106 ++-- zcash_client_backend/src/wallet.rs | 25 +- zcash_client_backend/src/zip321.rs | 5 + zcash_client_sqlite/CHANGELOG.md | 7 + zcash_client_sqlite/Cargo.toml | 1 + zcash_client_sqlite/src/chain.rs | 176 +++---- zcash_client_sqlite/src/error.rs | 38 +- zcash_client_sqlite/src/lib.rs | 77 +-- zcash_client_sqlite/src/wallet.rs | 4 +- zcash_client_sqlite/src/wallet/transact.rs | 120 ++--- zcash_primitives/CHANGELOG.md | 13 +- zcash_primitives/src/sapling.rs | 2 + zcash_primitives/src/transaction/builder.rs | 116 ++--- .../src/transaction/components/amount.rs | 29 ++ .../components/transparent/builder.rs | 21 +- .../src/transaction/components/tze/builder.rs | 24 +- zcash_primitives/src/transaction/fees.rs | 21 +- 25 files changed, 1674 insertions(+), 909 deletions(-) create mode 100644 zcash_client_backend/src/data_api/chain/error.rs create mode 100644 zcash_client_backend/src/data_api/wallet/input_selection.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index f68c6a16b..9ff9677a8 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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 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`. - `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 - `protobuf::ProtobufError`. -- `data_api::error::Error` has the following additional cases: - - `Error::BalanceError` in the case of amount addition overflow - or subtraction underflow. - - `Error::MemoForbidden` to report the condition where a memo was - specified to be sent to a transparent recipient. - - `Error::TransparentInputsNotSupported` to represent the condition - where a transparent spend has been requested of a wallet compiled without - the `transparent-inputs` feature. - - `Error::AddressNotRecognized` to indicate that a transparent address from - which funds are being requested to be spent does not appear to be associated - with this wallet. - - `Error::ChildIndexOutOfRange` to indicate that a diversifier index for an - address is out of range for valid transparent child indices. - - `Error::NoteMismatch` to indicate that a note being spent is not associated - with either the internal or external full viewing keys corresponding to the - provided spending key. +- `data_api::error::Error` has been substantially modified. It now wraps database, + note selection, builder, and other errors + - `Error::DataSource` has been added. + - `Error::NoteSelection` has been added. + - `Error::BalanceError` has been added. + - `Error::MemoForbidden` has been added. + - `Error::AddressNotRecognized` has been added. + - `Error::ChildIndexOutOfRange` has been added. + - `Error::NoteMismatch` has been added. + - `Error::InsufficientBalance` has been renamed `InsufficientFunds` and + restructured to have named fields. + - `Error::Protobuf` has been removed; these decoding errors are now + produced as data source and/or block-source implementation-specific + errors. + - `Error::InvalidChain` has been removed; its former purpose is now served + by `zcash_client_backend::data_api::chain::ChainError`. + - `Error::InvalidNewWitnessAnchor` and `Error::InvalidWitnessAnchor` have + 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 - spending key rather than a Sapling extended spending key. +- `zcash_client_backend::wallet::SpendableNote` is now parameterized by a note + 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`: diff --git a/zcash_client_backend/src/address.rs b/zcash_client_backend/src/address.rs index b83cea085..a5f4a36d6 100644 --- a/zcash_client_backend/src/address.rs +++ b/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, sapling: Option, diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 28a4032ee..a06847471 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/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, Self::Error>; + ) -> Result>, 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, Self::Error>; + ) -> Result>, 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; } -/// 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( - &self, - from_height: BlockHeight, - limit: Option, - 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::{ - error::Error, BlockSource, DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead, - WalletWrite, - }; - - pub struct MockBlockSource {} - - impl BlockSource for MockBlockSource { - type Error = Error; - - fn with_blocks( - &self, - _from_height: BlockHeight, - _limit: Option, - _with_row: F, - ) -> Result<(), Self::Error> - where - F: FnMut(CompactBlock) -> Result<(), Self::Error>, - { - Ok(()) - } - } + use super::{DecryptedTransaction, PrunedBlock, SentTransaction, WalletRead, WalletWrite}; pub struct MockWalletDb { pub network: Network, } impl WalletRead for MockWalletDb { - type Error = Error; + type Error = (); type NoteRef = u32; type TxRef = TxId; @@ -514,7 +477,7 @@ pub mod testing { } fn get_transaction(&self, _id_tx: Self::TxRef) -> Result { - 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, Self::Error> { + ) -> Result>, Self::Error> { Ok(Vec::new()) } @@ -553,7 +516,7 @@ pub mod testing { _account: AccountId, _target_value: Amount, _anchor_height: BlockHeight, - ) -> Result, Self::Error> { + ) -> Result>, 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( diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index c70dba4f9..401b2e9f9 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/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> { +//! # 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::{ - error::{ChainInvalid, Error}, - BlockSource, PrunedBlock, WalletWrite, - }, + data_api::{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( + &self, + from_height: BlockHeight, + limit: Option, + with_row: F, + ) -> Result<(), error::Error> + where + F: FnMut(CompactBlock) -> Result<(), error::Error>; +} + /// 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. -/// `upper_bound` is the height of the highest invalid block (on the assumption that the -/// highest block in the cache database is correct). +/// - `Err(Error::Chain(cause))` if the combined chain is invalid. /// - `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( - parameters: &P, - cache: &C, +pub fn validate_chain( + parameters: &ParamsT, + block_source: &BlockSourceT, validate_from: Option<(BlockHeight, BlockHash)>, -) -> Result<(), E> +) -> Result<(), Error> where - E: From>, - P: consensus::Parameters, - C: BlockSource, + ParamsT: consensus::Parameters, + BlockSourceT: BlockSource, { 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 - // height up to the chain tip, returning the hash of the block found in the cache at the - // `validate_from` height, which can then be used to verify chain integrity by comparing - // against the `validate_from` hash. + // The block source will contain blocks above the `validate_from` height. Validate from that + // maximum height up to the chain tip, returning the hash of the block found in the block + // source at the `validate_from` height, which can then be used to verify chain integrity by + // 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 = 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( - prev_height + 1, - current_height, - )) + Err(ChainError::block_height_discontinuity(prev_height + 1, current_height).into()) } 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 cache for any transactions received by -/// the tracked accounts. +/// Scans at most `limit` new blocks added to the block source for any transactions received by the +/// tracked accounts. /// -/// This function will return without error after scanning at most `limit` new blocks, to -/// enable the caller to update their UI with scanning progress. Repeatedly calling this -/// function will process sequential ranges of blocks, and is equivalent to calling -/// `scan_cached_blocks` and passing `None` for the optional `limit` value. +/// This function will return without error after scanning at most `limit` new blocks, to enable +/// the caller to update their UI with scanning progress. Repeatedly calling this function will +/// process sequential ranges of blocks, and is equivalent to calling `scan_cached_blocks` and +/// passing `None` for the optional `limit` value. /// -/// This function pays attention only to cached blocks with heights greater than the -/// highest scanned block in `data`. Cached blocks with lower heights are not verified -/// against previously-scanned blocks. In particular, this function **assumes** that the -/// caller is handling rollbacks. +/// This function pays attention only to cached blocks with heights greater than the highest +/// scanned block in `data`. Cached blocks with lower heights are not verified against +/// previously-scanned blocks. In particular, this function **assumes** that the caller is handling +/// rollbacks. /// -/// For brand-new light client databases, this function starts scanning from the Sapling -/// activation height. This height can be fast-forwarded to a more recent block by -/// initializing the client database with a starting block (for example, calling -/// `init_blocks_table` before this function if using `zcash_client_sqlite`). +/// For brand-new light client databases, this function starts scanning from the Sapling activation +/// height. This height can be fast-forwarded to a more recent block by initializing the client +/// database with a starting block (for example, calling `init_blocks_table` before this function +/// if using `zcash_client_sqlite`). /// -/// Scanned blocks are required to be height-sequential. If a block is missing from the -/// cache, an error will be returned with kind [`ChainInvalid::BlockHeightDiscontinuity`]. -pub fn scan_cached_blocks( - params: &P, - cache: &C, - data: &mut D, +/// Scanned blocks are required to be height-sequential. If a block is missing from the block +/// source, an error will be returned with cause [`error::Cause::BlockHeightDiscontinuity`]. +#[allow(clippy::type_complexity)] +pub fn scan_cached_blocks( + params: &ParamsT, + block_source: &BlockSourceT, + data_db: &mut DbT, limit: Option, -) -> Result<(), E> +) -> Result<(), Error> where - P: consensus::Parameters + Send + 'static, - C: BlockSource, - D: WalletWrite, - N: Copy + Debug, - E: From>, + ParamsT: consensus::Parameters + Send + 'static, + BlockSourceT: BlockSource, + DbT: WalletWrite, { 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| { - opt.map(|(_, max)| max) - .unwrap_or(sapling_activation_height - 1) - })?; + let mut last_height = data_db + .block_height_extrema() + .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| { - add_block_to_runner(params, block, &mut batch_runner); - Ok(()) - })?; + block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>( + last_height, + limit, + |block: CompactBlock| { + add_block_to_runner(params, block, &mut batch_runner); + Ok(()) + }, + )?; batch_runner.flush(); - cache.with_blocks(last_height, limit, |block: CompactBlock| { - let current_height = block.height(); + block_source.with_blocks::<_, DbT::Error, DbT::NoteRef>( + last_height, + limit, + |block: CompactBlock| { + let current_height = block.height(); - // Scanned blocks MUST be height-sequential. - if current_height != (last_height + 1) { - return Err( - ChainInvalid::block_height_discontinuity(last_height + 1, current_height).into(), - ); - } - - let block_hash = BlockHash::from_slice(&block.hash); - let block_time = block.time; - - let txs: Vec> = { - 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()); - } + // Scanned blocks MUST be height-sequential. + if current_height != (last_height + 1) { + return Err(ChainError::block_height_discontinuity( + last_height + 1, + current_height, + ) + .into()); } - for tx in &txs { - for output in tx.shielded_outputs.iter() { - if output.witness.root() != cur_root { - return Err(Error::InvalidNewWitnessAnchor( - output.index, - tx.txid, - current_height, - output.witness.root(), - ) - .into()); + + let block_hash = BlockHash::from_slice(&block.hash); + let block_time = block.time; + + let txs: Vec> = { + 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( + 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( - &(PrunedBlock { - block_height: current_height, - block_hash, - block_time, - commitment_tree: &tree, - transactions: &txs, - }), - &witnesses, - )?; + let new_witnesses = data_db + .advance_by_block( + &(PrunedBlock { + block_height: current_height, + block_hash, + block_time, + commitment_tree: &tree, + transactions: &txs, + }), + &witnesses, + ) + .map_err(Error::Wallet)?; - let spent_nf: Vec = txs - .iter() - .flat_map(|tx| tx.shielded_spends.iter().map(|spend| spend.nf)) - .collect(); - nullifiers.retain(|(_, nf)| !spent_nf.contains(nf)); - nullifiers.extend( - txs.iter() - .flat_map(|tx| tx.shielded_outputs.iter().map(|out| (out.account, out.nf))), - ); + let spent_nf: Vec = txs + .iter() + .flat_map(|tx| tx.shielded_spends.iter().map(|spend| spend.nf)) + .collect(); + nullifiers.retain(|(_, nf)| !spent_nf.contains(nf)); + nullifiers.extend( + txs.iter() + .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( + &self, + _from_height: BlockHeight, + _limit: Option, + _with_row: F, + ) -> Result<(), Error> + where + F: FnMut(CompactBlock) -> Result<(), Error>, + { + Ok(()) + } + } +} diff --git a/zcash_client_backend/src/data_api/chain/error.rs b/zcash_client_backend/src/data_api/chain/error.rs new file mode 100644 index 000000000..b35334c6a --- /dev/null +++ b/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 { + /// 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 { + at_height: BlockHeight, + cause: Cause, +} + +impl fmt::Display for ChainError { + 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 ChainError { + /// 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 { + &self.cause + } +} + +/// Errors related to chain validation and scanning. +#[derive(Debug)] +pub enum Error { + /// 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), +} + +impl fmt::Display for Error { + 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 error::Error for Error +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 From> for Error { + fn from(e: ChainError) -> Self { + Error::Chain(e) + } +} diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index efb5da5c3..0614a612d 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -1,36 +1,32 @@ //! Types for wallet error handling. use std::error; -use std::fmt; -use zcash_address::unified::Typecode; +use std::fmt::{self, Debug, Display}; use zcash_primitives::{ - consensus::BlockHeight, - sapling::Node, transaction::{ builder, - components::amount::{Amount, BalanceError}, - TxId, + components::{ + 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 { - /// The hash of the parent block given by a proposed new chain tip does - /// not match the hash of the current chain tip. - PrevHashMismatch, +pub enum Error { + /// An error occurred retrieving data from the underlying data source + DataSource(DataSourceError), - /// 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), -} + /// An error in note selection + NoteSelection(SelectionError), -#[derive(Debug)] -pub enum Error { /// No account could be found corresponding to a provided spending key. KeyNotRecognized, @@ -41,60 +37,21 @@ pub enum Error { 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 - /// 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), + InsufficientFunds { available: Amount, required: Amount }, /// 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), - - /// 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, + Builder(builder::Error), /// 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, - - /// An error indicating that a call was attempted to a method providing - /// support - #[cfg(not(feature = "transparent-inputs"))] - TransparentInputsNotSupported, + NoteMismatch(NoteRef), #[cfg(feature = "transparent-inputs")] AddressNotRecognized(TransparentAddress), @@ -103,95 +60,119 @@ pub enum Error { ChildIndexOutOfRange(DiversifierIndex), } -impl ChainInvalid { - pub fn prev_hash_mismatch(at_height: BlockHeight) -> Error { - Error::InvalidChain(at_height, ChainInvalid::PrevHashMismatch) - } - - pub fn block_height_discontinuity(at_height: BlockHeight, found: BlockHeight) -> Error { - Error::InvalidChain(at_height, ChainInvalid::BlockHeightDiscontinuity(found)) - } -} - -impl fmt::Display for Error { +impl fmt::Display for Error +where + DE: fmt::Display, + SE: fmt::Display, + FE: fmt::Display, + N: fmt::Display, +{ 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::Protobuf(e) => write!(f, "{}", e), - Error::SaplingNotActive => write!(f, "Could not determine Sapling upgrade activation height."), + Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e), 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 => write!(f, "A note being spent does not correspond to either the internal or external full viewing key for the provided spending key."), + 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), - #[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 error::Error for Error { +impl error::Error for Error +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 From for Error { - fn from(e: builder::Error) -> Self { +impl From> for Error { + fn from(e: builder::Error) -> Self { Error::Builder(e) } } -impl From for Error { - fn from(e: prost::DecodeError) -> Self { - Error::Protobuf(e) +impl From for Error { + fn from(e: BalanceError) -> Self { + Error::BalanceError(e) + } +} + +impl From> for Error { + fn from(e: InputSelectorError) -> Self { + match e { + InputSelectorError::DataSource(e) => Error::DataSource(e), + InputSelectorError::Selection(e) => Error::NoteSelection(e), + InputSelectorError::InsufficientFunds { + available, + required, + } => Error::InsufficientFunds { + available, + required, + }, + } + } +} + +impl From for Error { + fn from(e: sapling::builder::Error) -> Self { + Error::Builder(builder::Error::SaplingBuild(e)) + } +} + +impl From for Error { + fn from(e: transparent::builder::Error) -> Self { + Error::Builder(builder::Error::TransparentBuild(e)) } } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index bff79260e..ae247e6dc 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/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, - zip321::{Payment, TransactionRequest}, + wallet::{OvkPolicy, SpendableNote}, + zip321::{self, Payment}, }; +pub mod input_selection; +use input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector}; + #[cfg(feature = "transparent-inputs")] -use { - crate::wallet::WalletTransparentOutput, - zcash_primitives::{legacy::TransparentAddress, sapling::keys::OutgoingViewingKey}, +use 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( - params: &P, - data: &mut D, +pub fn decrypt_and_store_transaction( + params: &ParamsT, + data: &mut DbT, tx: &Transaction, -) -> Result<(), E> +) -> Result<(), DbT::Error> where - E: From>, - P: consensus::Parameters, - D: WalletWrite, + ParamsT: consensus::Parameters, + DbT: WalletWrite, { // 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> { +/// # #[allow(deprecated)] +/// # fn test() -> Result, 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( - wallet_db: &mut D, - params: &P, - prover: impl TxProver, +#[allow(clippy::type_complexity)] +#[deprecated(note = "Use `spend` instead.")] +pub fn create_spend_to_address( + wallet_db: &mut DbT, + params: &ParamsT, + prover: impl SaplingProver, usk: &UnifiedSpendingKey, to: &RecipientAddress, amount: Amount, memo: Option, ovk_policy: OvkPolicy, min_confirmations: u32, -) -> Result +) -> Result< + DbT::TxRef, + Error< + DbT::Error, + GreedyInputSelectorError, + core::convert::Infallible, + DbT::NoteRef, + >, +> where - E: From>, - P: consensus::Parameters + Clone, - R: Copy + Debug, - D: WalletWrite, + ParamsT: consensus::Parameters + Clone, + DbT: WalletWrite, + DbT::NoteRef: Copy + Eq + Ord, { - 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::::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( - wallet_db: &mut D, - params: &P, - prover: impl TxProver, +#[allow(clippy::type_complexity)] +pub fn spend( + wallet_db: &mut DbT, + params: &ParamsT, + prover: impl SaplingProver, + input_selector: &InputsT, usk: &UnifiedSpendingKey, - request: &TransactionRequest, + request: zip321::TransactionRequest, ovk_policy: OvkPolicy, min_confirmations: u32, -) -> Result +) -> Result< + DbT::TxRef, + Error::Error, DbT::NoteRef>, +> where - E: From>, - P: consensus::Parameters + Clone, - R: Copy + Debug, - D: WalletWrite, + DbT: WalletWrite, + DbT::TxRef: Copy + Debug, + DbT::NoteRef: Copy + Eq + Ord, + ParamsT: consensus::Parameters + Clone, + InputsT: InputSelector, { 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 - .payments() - .iter() - .map(|p| p.amount) - .sum::>() - .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?; - let target_value = (value + DEFAULT_FEE) - .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?; - let spendable_notes = - wallet_db.select_spendable_sapling_notes(account, target_value, anchor_height)?; + let proposal = input_selector.propose_transaction( + params, + wallet_db, + account, + anchor_height, + target_height, + request, + )?; - // Confirm we were able to select sufficient value - let selected_value = spendable_notes - .iter() - .map(|n| n.note_value) - .sum::>() - .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 + // Create the transaction. The type of the proposal ensures that there + // are no possible transparent inputs, so we ignore those let mut builder = Builder::new(params.clone(), target_height); - for selected in spendable_notes { - let merkle_path = selected.witness.path().expect("the tree is not empty"); + 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))?; - // 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 (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)?; + builder.add_sapling_spend(key, selected.diversifier, note, merkle_path)?; } - for payment in request.payments() { + for payment in proposal.transaction_request().payments() { match &payment.recipient_address { - RecipientAddress::Unified(ua) => builder - .add_sapling_output( + RecipientAddress::Unified(ua) => { + 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 - .add_sapling_output( + )?; + } + RecipientAddress::Shielded(to) => { + 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 - .add_transparent_output(to, payment.amount) - .map_err(Error::Builder) + builder.add_transparent_output(to, payment.amount)?; } } - }? - } - - 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 - .build(&prover, &fee_strategy.fee_rule()) - .map_err(Error::Builder)?; + for change_value in proposal.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(), + )?; + } + } + } - 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 { - tx: &tx, - created: time::OffsetDateTime::now_utc(), - account, - outputs: sent_outputs, - fee_amount: balance.fee_required(), - #[cfg(feature = "transparent-inputs")] - utxos_spent: vec![], - }) + wallet_db + .store_sent_tx(&SentTransaction { + tx: &tx, + created: time::OffsetDateTime::now_utc(), + account, + outputs: sent_outputs, + fee_amount: proposal.balance().fee_required(), + #[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( - wallet_db: &mut D, - params: &P, - prover: impl TxProver, +#[allow(clippy::type_complexity)] +pub fn shield_transparent_funds( + wallet_db: &mut DbT, + params: &ParamsT, + prover: impl SaplingProver, + input_selector: &InputsT, usk: &UnifiedSpendingKey, from_addrs: &[TransparentAddress], memo: &MemoBytes, min_confirmations: u32, -) -> Result +) -> Result< + DbT::TxRef, + Error::Error, DbT::NoteRef>, +> where - E: From>, - P: consensus::Parameters, - R: Copy + Debug, - D: WalletWrite, + ParamsT: consensus::Parameters, + DbT: WalletWrite, + DbT::NoteRef: Copy + Eq + Ord, + InputsT: InputSelector, { 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 = vec![]; - for from_addr in from_addrs { - let mut outputs = wallet_db.get_unspent_transparent_outputs(from_addr, latest_anchor)?; - utxos.append(&mut outputs); - } + let proposal = input_selector.propose_shielding( + params, + wallet_db, + NonNegativeAmount::from_u64(100000).unwrap(), + from_addrs, + latest_anchor, + target_height, + )?; - let _total_amount = utxos - .iter() - .map(|utxo| utxo.txout().value) - .sum::>() - .ok_or_else(|| E::from(Error::BalanceError(BalanceError::Overflow)))?; - - let addr_metadata = wallet_db.get_transparent_receivers(account)?; + let known_addrs = wallet_db + .get_transparent_receivers(account) + .map_err(Error::DataSource)?; let mut builder = Builder::new(params.clone(), target_height); - for utxo in &utxos { - let diversifier_index = addr_metadata + let mut utxos = vec![]; + 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 - .add_transparent_input(secret_key, utxo.outpoint().clone(), utxo.txout().clone()) - .map_err(Error::Builder)?; + builder.add_transparent_input(secret_key, utxo.outpoint().clone(), utxo.txout().clone())?; } - // Compute the balance of the transaction. We have only added inputs, so the total 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))?; + for change_value in proposal.balance().proposed_change() { match change_value { ChangeValue::Sapling(amount) => { - builder - .add_sapling_output(Some(ovk), shielding_address.clone(), *amount, memo.clone()) - .map_err(Error::Builder)?; + builder.add_sapling_output( + Some(ovk), + shielding_address.clone(), + *amount, + memo.clone(), + )?; } } } // The transaction build process will check that the inputs and outputs balance - let (tx, tx_metadata) = builder - .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.", - ); + let (tx, tx_metadata) = builder.build(&prover, proposal.fee_rule())?; - wallet_db.store_sent_tx(&SentTransaction { - tx: &tx, - created: time::OffsetDateTime::now_utc(), - account, - outputs: vec![SentTransactionOutput { - output_index, - recipient: Recipient::InternalAccount(account, PoolType::Sapling), - value: total_out, - memo: Some(memo.clone()), - }], - fee_amount: fee, - utxos_spent: utxos.iter().map(|utxo| utxo.outpoint().clone()).collect(), - }) + wallet_db + .store_sent_tx(&SentTransaction { + tx: &tx, + created: time::OffsetDateTime::now_utc(), + account, + // TODO: After Orchard is implemented, this will need to change to correctly + // determine the Sapling output indices; `enumerate` will no longer suffice + outputs: proposal + .balance() + .proposed_change() + .iter() + .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( + selected: &SpendableNote, + extsk: &ExtendedSpendingKey, + dfvk: &DiversifiableFullViewingKey, +) -> Option<( + sapling::Note, + ExtendedSpendingKey, + MerklePath, +)> { + 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)) + }) } diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs new file mode 100644 index 000000000..fe5a0f732 --- /dev/null +++ b/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 { + /// 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 fmt::Display for InputSelectorError { + 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 { + transaction_request: TransactionRequest, + transparent_inputs: Vec, + sapling_inputs: Vec>, + balance: TransactionBalance, + fee_rule: FeeRuleT, +} + +impl Proposal { + /// 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] { + &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( + &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, + <::DataSource as WalletRead>::NoteRef, + >, + InputSelectorError<<::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( + &self, + params: &ParamsT, + wallet_db: &Self::DataSource, + shielding_threshold: NonNegativeAmount, + source_addrs: &[TransparentAddress], + confirmed_height: BlockHeight, + target_height: BlockHeight, + ) -> Result< + Proposal< + Self::FeeRule, + WalletTransparentOutput, + <::DataSource as WalletRead>::NoteRef, + >, + InputSelectorError<<::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 { + /// An intermediate value overflowed or underflowed the valid monetary range. + Balance(BalanceError), + /// A unified address did not contain a supported receiver. + UnsupportedAddress(Box), + /// An error was encountered in change selection. + Change(ChangeError), +} + +impl From> + for InputSelectorError> +{ + fn from(err: GreedyInputSelectorError) -> Self { + InputSelectorError::Selection(err) + } +} + +impl From> + for InputSelectorError> +{ + fn from(err: ChangeError) -> Self { + InputSelectorError::Selection(GreedyInputSelectorError::Change(err)) + } +} + +impl From + for InputSelectorError> +{ + 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 { + change_strategy: ChangeT, + _ds_type: PhantomData, +} + +impl GreedyInputSelector { + /// 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 InputSelector for GreedyInputSelector +where + DbT: WalletRead, + ChangeT: ChangeStrategy, + ChangeT::FeeRule: Clone, +{ + type Error = GreedyInputSelectorError<::Error>; + type DataSource = DbT; + type FeeRule = ChangeT::FeeRule; + + #[allow(clippy::type_complexity)] + fn propose_transaction( + &self, + params: &ParamsT, + wallet_db: &Self::DataSource, + account: AccountId, + anchor_height: BlockHeight, + target_height: BlockHeight, + transaction_request: TransactionRequest, + ) -> Result< + Proposal, + InputSelectorError, + > + 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> = 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::::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::>() + .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( + &self, + params: &ParamsT, + wallet_db: &Self::DataSource, + shielding_threshold: NonNegativeAmount, + source_addrs: &[TransparentAddress], + confirmed_height: BlockHeight, + target_height: BlockHeight, + ) -> Result< + Proposal, + InputSelectorError, + > + where + ParamsT: consensus::Parameters, + { + let transparent_inputs: Vec = source_addrs + .iter() + .map(|taddr| wallet_db.get_unspent_transparent_outputs(taddr, confirmed_height)) + .collect::>, _>>() + .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::::new(), + &Vec::>::new(), + &Vec::::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(), + }) + } + } +} diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 9b13d2e24..35fd1e255 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -29,15 +29,22 @@ impl ChangeValue { pub struct TransactionBalance { proposed_change: Vec, fee_required: Amount, + total: Amount, } impl TransactionBalance { /// Constructs a new balance from its constituent parts. - pub fn new(proposed_change: Vec, fee_required: Amount) -> Self { - TransactionBalance { - proposed_change, - fee_required, - } + pub fn new(proposed_change: Vec, fee_required: Amount) -> Option { + proposed_change + .iter() + .map(|v| v.value()) + .chain(Some(fee_required)) + .sum::>() + .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 { - 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>; } -/// A change strategy that uses a fixed fee amount and proposes change as a single output -/// to the most current supported pool. -pub struct BasicFixedFeeChangeStrategy { - fixed_fee: Amount, +/// A change strategy that and proposes change as a single output to the most current supported +/// shielded pool and delegates fee calculation to the provided fee rule. +pub struct SingleOutputFixedFeeChangeStrategy { + fee_rule: FixedFeeRule, } -impl BasicFixedFeeChangeStrategy { - // Constructs a new [`BasicFixedFeeChangeStrategy`] with the specified fixed fee - // amount. - pub fn new(fixed_fee: Amount) -> Self { - Self { fixed_fee } +impl SingleOutputFixedFeeChangeStrategy { + /// Constructs a new [`SingleOutputFixedFeeChangeStrategy`] with the specified fee rule. + pub fn new(fee_rule: FixedFeeRule) -> Self { + Self { fee_rule } } } -impl ChangeStrategy for BasicFixedFeeChangeStrategy { +impl From for ChangeError { + fn from(err: BalanceError) -> ChangeError { + ChangeError::StrategyError(err) + } +} + +impl ChangeStrategy for SingleOutputFixedFeeChangeStrategy { type FeeRule = FixedFeeRule; type Error = BalanceError; - fn fee_rule(&self) -> Self::FeeRule { - FixedFeeRule::new(self.fixed_fee) + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule } fn compute_balance( &self, - _params: &P, - _target_height: BlockHeight, + params: &P, + target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], sapling_inputs: &[impl sapling::InputView], sapling_outputs: &[impl sapling::OutputView], ) -> Result> { - 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::>() - .ok_or_else(overflow)?; + .ok_or(BalanceError::Overflow)?; let t_out = transparent_outputs .iter() .map(|t_out| t_out.value()) .sum::>() - .ok_or_else(overflow)?; + .ok_or(BalanceError::Overflow)?; let sapling_in = sapling_inputs .iter() .map(|s_in| s_in.value()) .sum::>() - .ok_or_else(overflow)?; + .ok_or(BalanceError::Overflow)?; let sapling_out = sapling_outputs .iter() .map(|s_out| s_out.value()) .sum::>() - .ok_or_else(overflow)?; + .ok_or(BalanceError::Overflow)?; - let total_in = (t_in + sapling_in).ok_or_else(overflow)?; - let total_out = [t_out, sapling_out, self.fixed_fee] + let fee_amount = self + .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::>() - .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( - vec![ChangeValue::Sapling(proposed_change)], - self.fixed_fee, - )) + TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount) + .ok_or_else(|| BalanceError::Overflow.into()) } } } diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 619e73ef6..66b4d0fce 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/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 { /// 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 { + pub note_id: NoteRef, pub diversifier: Diversifier, pub note_value: Amount, pub rseed: Rseed, pub witness: IncrementalWitness, } +impl sapling::fees::InputView for SpendableNote { + fn value(&self) -> Amount { + self.note_value + } +} + /// Describes a policy for which outgoing viewing key should be able to decrypt /// transaction outputs. /// diff --git a/zcash_client_backend/src/zip321.rs b/zcash_client_backend/src/zip321.rs index cf433ecd0..391c96e46 100644 --- a/zcash_client_backend/src/zip321.rs +++ b/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) -> Result { let request = TransactionRequest { payments }; diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index f8f71bbc1..821cb749e 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/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 diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index e17e1c250..3334aac98 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/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" diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 6a2ae177e..4c3f422c2 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/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, -} - /// 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( - cache: &BlockDb, +pub(crate) fn blockdb_with_blocks( + block_source: &BlockDb, last_scanned_height: BlockHeight, limit: Option, mut with_row: F, -) -> Result<(), SqliteClientError> +) -> Result<(), Error> where - F: FnMut(CompactBlock) -> Result<(), SqliteClientError>, + F: FnMut(CompactBlock) -> Result<(), Error>, { - // Fetch the CompactBlocks we need to scan - let mut stmt_blocks = cache.0.prepare( - "SELECT height, data FROM compactblocks WHERE height > ? ORDER BY height ASC LIMIT ?", - )?; + fn to_chain_error, N>(err: E) -> Error { + Error::BlockSource(err.into()) + } - let rows = stmt_blocks.query_map( - params![ + // Fetch the CompactBlocks we need to scan + 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| { - Ok(CompactBlockRow { - height: BlockHeight::from_u32(row.get(0)?), - data: row.get(1)?, - }) - }, - )?; + ]) + .map_err(to_chain_error)?; - for row_result in rows { - let cbr = row_result?; - let block = CompactBlock::decode(&cbr.data[..]).map_err(Error::from)?; - - if block.height() != cbr.height { - return Err(SqliteClientError::CorruptedData(format!( + while let Some(row) = rows.next().map_err(to_chain_error)? { + let height = BlockHeight::from_u32(row.get(0).map_err(to_chain_error)?); + let data: Vec = row.get(1).map_err(to_chain_error)?; + let block = CompactBlock::decode(&data[..]).map_err(to_chain_error)?; + if block.height() != height { + 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( +pub(crate) fn fsblockdb_with_blocks( cache: &FsBlockDb, last_scanned_height: BlockHeight, limit: Option, mut with_block: F, -) -> Result<(), SqliteClientError> +) -> Result<(), Error> where - F: FnMut(CompactBlock) -> Result<(), SqliteClientError>, + F: FnMut(CompactBlock) -> Result<(), Error>, { + fn to_chain_error, N>(err: E) -> Error { + Error::BlockSource(err.into()) + } + // Fetch the CompactBlocks we need to scan - let mut stmt_blocks = cache.conn.prepare( - "SELECT height, blockhash, time, sapling_outputs_count, orchard_actions_count + let mut stmt_blocks = cache + .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( - params![ - u32::from(last_scanned_height), - limit.unwrap_or(u32::max_value()), - ], - |row| { - Ok(BlockMeta { - height: BlockHeight::from_u32(row.get(0)?), - block_hash: BlockHash::from_slice(&row.get::<_, Vec<_>>(1)?), - block_time: row.get(2)?, - sapling_outputs_count: row.get(3)?, - orchard_actions_count: row.get(4)?, - }) - }, - )?; + let rows = stmt_blocks + .query_map( + params![ + u32::from(last_scanned_height), + limit.unwrap_or(u32::max_value()), + ], + |row| { + Ok(BlockMeta { + height: BlockHeight::from_u32(row.get(0)?), + block_hash: BlockHash::from_slice(&row.get::<_, Vec<_>>(1)?), + block_time: row.get(2)?, + 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 mut block_file = File::open(cbr.block_file_path(&cache.blocks_dir))?; + let cbr = row_result.map_err(to_chain_error)?; + 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::{scan_cached_blocks, validate_chain}, - error::{ChainInvalid, Error}, + use zcash_client_backend::data_api::chain::{ + error::{Cause, Error}, + scan_cached_blocks, validate_chain, }; + 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) - } - _ => panic!(), - } + ); + + assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 2); } #[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) - } - _ => panic!(), - } + ); + + assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 3); } #[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)) => { - assert_eq!( - e.to_string(), - ChainInvalid::block_height_discontinuity::( - sapling_activation_height() + 1, - sapling_activation_height() + 2 - ) - .to_string() + Err(Error::Chain(e)) => { + assert_matches!( + e.cause(), + Cause::BlockHeightDiscontinuity(h) if *h + == sapling_activation_height() + 2 ); } Ok(_) | Err(_) => panic!("Should have failed"), diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index 1c7fb6873..29c09ec75 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -3,13 +3,10 @@ use std::error; use std::fmt; -use zcash_client_backend::{ - data_api, - encoding::{Bech32DecodeError, TransparentCodecError}, -}; +use zcash_client_backend::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) - IncorrectHrpExtFvk, + /// An error occurred decoding a protobuf message. + 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 - Base58(bs58::decode::Error), - - /// + /// An error produced in legacy transparent address derivation #[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), - /// 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 for SqliteClientError { } } -impl From for SqliteClientError { - fn from(e: bs58::decode::Error) -> Self { - SqliteClientError::Base58(e) +impl From for SqliteClientError { + fn from(e: prost::DecodeError) -> Self { + SqliteClientError::Protobuf(e) } } @@ -179,9 +169,3 @@ impl From for SqliteClientError { SqliteClientError::InvalidMemo(e) } } - -impl From> for SqliteClientError { - fn from(e: data_api::error::Error) -> Self { - SqliteClientError::BackendError(e) - } -} diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 1c2fa0e52..7430ad75a 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/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, - WalletRead, WalletWrite, + self, chain::BlockSource, DecryptedTransaction, PoolType, PrunedBlock, Recipient, + 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 WalletRead for WalletDb

{ &self, account: AccountId, anchor_height: BlockHeight, - ) -> Result, Self::Error> { + ) -> Result>, Self::Error> { #[allow(deprecated)] wallet::transact::get_spendable_sapling_notes(self, account, anchor_height) } @@ -240,7 +237,7 @@ impl WalletRead for WalletDb

{ account: AccountId, target_value: Amount, anchor_height: BlockHeight, - ) -> Result, Self::Error> { + ) -> Result>, Self::Error> { #[allow(deprecated)] wallet::transact::select_spendable_sapling_notes(self, account, target_value, anchor_height) } @@ -253,9 +250,9 @@ impl WalletRead for WalletDb

{ return wallet::get_transparent_receivers(&self.params, &self.conn, _account); #[cfg(not(feature = "transparent-inputs"))] - return Err(SqliteClientError::BackendError( - Error::TransparentInputsNotSupported, - )); + panic!( + "The wallet must be compiled with the transparent-inputs feature to use this method." + ); } fn get_unspent_transparent_outputs( @@ -267,9 +264,9 @@ impl WalletRead for WalletDb

{ return wallet::get_unspent_transparent_outputs(self, _address, _max_height); #[cfg(not(feature = "transparent-inputs"))] - return Err(SqliteClientError::BackendError( - Error::TransparentInputsNotSupported, - )); + panic!( + "The wallet must be compiled with the transparent-inputs feature to use this method." + ); } fn get_transparent_balances( @@ -281,9 +278,9 @@ impl WalletRead for WalletDb

{ return wallet::get_transparent_balances(self, _account, _max_height); #[cfg(not(feature = "transparent-inputs"))] - return Err(SqliteClientError::BackendError( - Error::TransparentInputsNotSupported, - )); + panic!( + "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, Self::Error> { + ) -> Result>, 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, Self::Error> { + ) -> Result>, 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( - Error::TransparentInputsNotSupported, - )); + panic!( + "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( + fn with_blocks( &self, from_height: BlockHeight, limit: Option, with_row: F, - ) -> Result<(), Self::Error> + ) -> Result<(), data_api::chain::error::Error> where - F: FnMut(CompactBlock) -> Result<(), Self::Error>, + F: FnMut( + CompactBlock, + ) + -> Result<(), data_api::chain::error::Error>, { chain::blockdb_with_blocks(self, from_height, limit, with_row) } @@ -788,7 +788,7 @@ impl BlockSource for BlockDb { /// /// where `` is the decimal value of the height at which the block was mined, and /// `` 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 for FsBlockDbError { } } +#[cfg(feature = "unstable")] +impl From 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( + fn with_blocks( &self, from_height: BlockHeight, limit: Option, with_row: F, - ) -> Result<(), Self::Error> + ) -> Result<(), data_api::chain::error::Error> where - F: FnMut(CompactBlock) -> Result<(), Self::Error>, + F: FnMut( + CompactBlock, + ) + -> Result<(), data_api::chain::error::Error>, { fsblockdb_with_blocks(self, from_height, limit, with_row) } } +#[cfg(test)] +#[macro_use] +extern crate assert_matches; + #[cfg(test)] #[allow(deprecated)] mod tests { diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 138a4154c..57f1ac1f6 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/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( 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 diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index ba386bcd0..ba006c076 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/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 { +fn to_spendable_note(row: &Row) -> Result, 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 { 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 { }; 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

( wdb: &WalletDb

, account: AccountId, anchor_height: BlockHeight, -) -> Result, SqliteClientError> { +) -> Result>, 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

( account: AccountId, target_value: Amount, anchor_height: BlockHeight, -) -> Result, SqliteClientError> { +) -> Result>, 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

( 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( - data_api::error::Error::KeyNotRecognized - )) - )); + Err(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( - data_api::error::Error::ScanRequired - )) - )); + Err(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( - data_api::error::Error::InsufficientBalance( - available, - required - ) - )) + Err(data_api::error::Error::InsufficientFunds { + available, + 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( - data_api::error::Error::InsufficientBalance( - available, - required - ) - )) + Err(data_api::error::Error::InsufficientFunds { + available, + 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( - data_api::error::Error::InsufficientBalance( - available, - required - ) - )) + Err(data_api::error::Error::InsufficientFunds { + available, + 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( - data_api::error::Error::InsufficientBalance( - available, - required - ) - )) + Err(data_api::error::Error::InsufficientFunds { + available, + 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( - data_api::error::Error::InsufficientBalance( - available, - required - ) - )) + Err(data_api::error::Error::InsufficientFunds { + available, + 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(_) - )); + ); } } diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 903bee6e1..347b64989 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/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` 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` instead of `Option`. - `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` instead of `Option`. ### 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 diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index 30a1afc4f..f4a47f008 100644 --- a/zcash_primitives/src/sapling.rs +++ b/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. /// diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 91dce2122..99e747323 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/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 { /// 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 fmt::Display for Error { 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 error::Error for Error {} -impl From for Error { - fn from(_: Infallible) -> Error { - unreachable!() +impl From for Error { + fn from(e: BalanceError) -> Self { + 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, - ) -> 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> { - self.transparent_builder - .add_input(sk, utxo, coin) - .map_err(Error::TransparentBuild) + ) -> Result<(), transparent::builder::Error> { + self.transparent_builder.add_input(sk, utxo, coin) } /// 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> { - self.transparent_builder - .add_output(to, value) - .map_err(Error::TransparentBuild) + ) -> Result<(), transparent::builder::Error> { + self.transparent_builder.add_output(to, value) } /// 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 { + fn value_balance(&self) -> Result { let value_balances = [ - self.transparent_builder - .value_balance() - .ok_or(Error::InvalidAmount)?, + self.transparent_builder.value_balance()?, self.sapling_builder.value_balance(), #[cfg(feature = "zfuture")] - self.tze_builder - .value_balance() - .ok_or(Error::InvalidAmount)?, + self.tze_builder.value_balance()?, ]; value_balances .into_iter() .sum::>() - .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> - where - Error: From, - { - let fee = fee_rule.fee_required( - &self.params, - self.target_height, - self.transparent_builder.inputs(), - self.transparent_builder.outputs(), - self.sapling_builder.inputs(), - self.sapling_builder.outputs(), - )?; + ) -> Result<(Transaction, SaplingMetadata), Error> { + let fee = fee_rule + .fee_required( + &self.params, + self.target_height, + self.transparent_builder.inputs(), + self.transparent_builder.outputs(), + self.sapling_builder.inputs().len(), + self.sapling_builder.outputs().len(), + ) + .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> - where - Error: From, - { - let fee = fee_rule.fee_required_zfuture( - &self.params, - self.target_height, - self.transparent_builder.inputs(), - self.transparent_builder.outputs(), - self.sapling_builder.inputs(), - self.sapling_builder.outputs(), - self.tze_builder.inputs(), - self.tze_builder.outputs(), - )?; + ) -> Result<(Transaction, SaplingMetadata), Error> { + let fee = fee_rule + .fee_required_zfuture( + &self.params, + self.target_height, + self.transparent_builder.inputs(), + self.transparent_builder.outputs(), + self.sapling_builder.inputs().len(), + self.sapling_builder.outputs().len(), + self.tze_builder.inputs(), + self.tze_builder.outputs(), + ) + .map_err(Error::Fee)?; + self.build_internal(prover, fee) } - fn build_internal( + fn build_internal( self, prover: &impl TxProver, fee: Amount, - ) -> Result<(Transaction, SaplingMetadata), Error> { + ) -> Result<(Transaction, SaplingMetadata), Error> { 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> { 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) ); } diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 89f73dd5a..11881f0e6 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -213,6 +213,35 @@ impl TryFrom 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 { + 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 { + Amount::from_nonnegative_i64(amount).map(NonNegativeAmount) + } +} + +impl From 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)] diff --git a/zcash_primitives/src/transaction/components/transparent/builder.rs b/zcash_primitives/src/transaction/components/transparent/builder.rs index 5d2780ad6..e32398072 100644 --- a/zcash_primitives/src/transaction/components/transparent/builder.rs +++ b/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 { + pub fn value_balance(&self) -> Result { #[cfg(feature = "transparent-inputs")] let input_sum = self .inputs .iter() .map(|input| input.coin.value) - .sum::>()?; + .sum::>() + .ok_or(BalanceError::Overflow)?; #[cfg(not(feature = "transparent-inputs"))] let input_sum = Amount::zero(); - input_sum - - self - .vout - .iter() - .map(|vo| vo.value) - .sum::>()? + let output_sum = self + .vout + .iter() + .map(|vo| vo.value) + .sum::>() + .ok_or(BalanceError::Overflow)?; + + (input_sum - output_sum).ok_or(BalanceError::Underflow) } pub fn build(self) -> Option> { diff --git a/zcash_primitives/src/transaction/components/tze/builder.rs b/zcash_primitives/src/transaction/components/tze/builder.rs index 8555a4870..82bd1f2e0 100644 --- a/zcash_primitives/src/transaction/components/tze/builder.rs +++ b/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 { - self.vin + pub fn value_balance(&self) -> Result { + let total_in = self + .vin .iter() .map(|tzi| tzi.coin.value) - .sum::>()? - - self - .vout - .iter() - .map(|tzo| tzo.value) - .sum::>()? + .sum::>() + .ok_or(BalanceError::Overflow)?; + + let total_out = self + .vout + .iter() + .map(|tzo| tzo.value) + .sum::>() + .ok_or(BalanceError::Overflow)?; + + (total_in - total_out).ok_or(BalanceError::Underflow) } pub fn build(self) -> (Option>, Vec>) { diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index 706ea9984..2c9e844de 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -2,9 +2,7 @@ use crate::{ consensus::{self, BlockHeight}, - transaction::components::{ - amount::Amount, sapling::fees as sapling, transparent::fees as transparent, - }, + transaction::components::{amount::Amount, 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_outputs: &[impl sapling::OutputView], + sapling_input_count: usize, + sapling_output_count: usize, ) -> Result; } @@ -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_outputs: &[impl sapling::OutputView], + sapling_input_count: usize, + sapling_output_count: usize, tze_inputs: &[impl tze::InputView], tze_outputs: &[impl tze::OutputView], ) -> Result; @@ -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_outputs: &[impl sapling::OutputView], + _sapling_input_count: usize, + _sapling_output_count: usize, ) -> Result { 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_outputs: &[impl sapling::OutputView], + _sapling_input_count: usize, + _sapling_output_count: usize, _tze_inputs: &[impl tze::InputView], _tze_outputs: &[impl tze::OutputView], ) -> Result { From 847ba49761fdfec1213a6d6417db6165c549241e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 9 Nov 2022 08:13:34 -0700 Subject: [PATCH 2/4] Add dust note detection to change selection. The change selection algorithm has the most useful information for determining whether or not a note is dust, so this adds a new error case to `ChangeError` that allows the change selection to report the presence of input notes without economic value back to its caller. --- zcash_client_backend/CHANGELOG.md | 8 +- zcash_client_backend/Cargo.toml | 1 + zcash_client_backend/src/data_api.rs | 19 +- zcash_client_backend/src/data_api/wallet.rs | 12 +- .../src/data_api/wallet/input_selection.rs | 125 +++++++--- zcash_client_backend/src/fees.rs | 190 +++++++------- zcash_client_backend/src/fees/fixed.rs | 234 ++++++++++++++++++ zcash_client_backend/src/lib.rs | 4 + zcash_client_backend/src/wallet.rs | 6 +- zcash_client_sqlite/CHANGELOG.md | 3 + zcash_client_sqlite/Cargo.toml | 2 +- zcash_client_sqlite/src/lib.rs | 34 ++- zcash_client_sqlite/src/wallet.rs | 16 +- zcash_client_sqlite/src/wallet/transact.rs | 35 ++- zcash_extensions/src/transparent/demo.rs | 10 +- zcash_primitives/CHANGELOG.md | 3 + zcash_primitives/src/transaction/builder.rs | 6 +- .../transaction/components/sapling/builder.rs | 12 +- .../transaction/components/sapling/fees.rs | 4 +- .../src/transaction/components/transparent.rs | 2 +- zcash_primitives/src/transaction/fees.rs | 49 +--- .../src/transaction/fees/fixed.rs | 69 ++++++ 22 files changed, 628 insertions(+), 216 deletions(-) create mode 100644 zcash_client_backend/src/fees/fixed.rs create mode 100644 zcash_primitives/src/transaction/fees/fixed.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 9ff9677a8..d631e8b12 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -71,6 +71,8 @@ and this library adheres to Rust's notion of - `TransactionBalance` - `BasicFixedFeeChangeStrategy` - a `ChangeStrategy` implementation that reproduces current wallet change behavior + - `fixed` a new module containing of change selection strategies for the + existing fixed fee rule. - New experimental APIs that should be considered unstable, and are likely to be modified and/or moved to a different module in a future release: @@ -107,9 +109,11 @@ and this library adheres to Rust's notion of the anchor height being returned; this had previously been hardcoded to `data_api::wallet::ANCHOR_OFFSET`. - `WalletRead::get_spendable_notes` has been renamed to - `get_spendable_sapling_notes` + `get_spendable_sapling_notes` and now takes as an argument a vector of + note IDs to be excluded from consideration. - `WalletRead::select_spendable_notes` has been renamed to - `select_spendable_sapling_notes` + `select_spendable_sapling_notes` and now takes as an argument a vector of + note IDs to be excluded from consideration. - 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 diff --git a/zcash_client_backend/Cargo.toml b/zcash_client_backend/Cargo.toml index 3314c9498..5b0310e51 100644 --- a/zcash_client_backend/Cargo.toml +++ b/zcash_client_backend/Cargo.toml @@ -76,6 +76,7 @@ tonic-build = "0.8" which = "4" [dev-dependencies] +assert_matches = "1.5" gumdrop = "0.8" hex = "0.4" jubjub = "0.9" diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index a06847471..6379b186b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -12,7 +12,10 @@ use zcash_primitives::{ memo::{Memo, MemoBytes}, merkle_tree::{CommitmentTree, IncrementalWitness}, sapling::{Node, Nullifier, PaymentAddress}, - transaction::{components::Amount, Transaction, TxId}, + transaction::{ + components::{amount::Amount, OutPoint}, + Transaction, TxId, + }, zip32::{AccountId, ExtendedFullViewingKey}, }; @@ -23,9 +26,6 @@ use crate::{ wallet::{SpendableNote, WalletTransparentOutput, WalletTx}, }; -#[cfg(feature = "transparent-inputs")] -use zcash_primitives::transaction::components::transparent::OutPoint; - pub mod chain; pub mod error; pub mod wallet; @@ -187,6 +187,7 @@ pub trait WalletRead { &self, account: AccountId, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error>; /// Returns a list of spendable Sapling notes sufficient to cover the specified @@ -196,6 +197,7 @@ pub trait WalletRead { account: AccountId, target_value: Amount, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error>; /// Returns the set of all transparent receivers associated with the given account. @@ -214,6 +216,7 @@ pub trait WalletRead { &self, address: &TransparentAddress, max_height: BlockHeight, + exclude: &[OutPoint], ) -> Result, Self::Error>; /// Returns a mapping from transparent receiver to not-yet-shielded UTXO balance, @@ -400,7 +403,10 @@ pub mod testing { memo::Memo, merkle_tree::{CommitmentTree, IncrementalWitness}, sapling::{Node, Nullifier}, - transaction::{components::Amount, Transaction, TxId}, + transaction::{ + components::{Amount, OutPoint}, + Transaction, TxId, + }, zip32::{AccountId, ExtendedFullViewingKey}, }; @@ -507,6 +513,7 @@ pub mod testing { &self, _account: AccountId, _anchor_height: BlockHeight, + _exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { Ok(Vec::new()) } @@ -516,6 +523,7 @@ pub mod testing { _account: AccountId, _target_value: Amount, _anchor_height: BlockHeight, + _exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { Ok(Vec::new()) } @@ -531,6 +539,7 @@ pub mod testing { &self, _address: &TransparentAddress, _anchor_height: BlockHeight, + _exclude: &[OutPoint], ) -> Result, Self::Error> { Ok(Vec::new()) } diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index ae247e6dc..a280bbc7f 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -7,8 +7,8 @@ use zcash_primitives::{ sapling::{self, prover::TxProver as SaplingProver}, transaction::{ builder::Builder, - components::amount::{Amount, BalanceError, DEFAULT_FEE}, - fees::{FeeRule, FixedFeeRule}, + components::amount::{Amount, BalanceError}, + fees::{fixed, FeeRule}, Transaction, }, zip32::{sapling::DiversifiableFullViewingKey, sapling::ExtendedSpendingKey, Scope}, @@ -21,7 +21,7 @@ use crate::{ SentTransactionOutput, WalletWrite, }, decrypt_transaction, - fees::{ChangeValue, SingleOutputFixedFeeChangeStrategy}, + fees::{self, ChangeValue, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::{OvkPolicy, SpendableNote}, zip321::{self, Payment}, @@ -191,7 +191,7 @@ pub fn create_spend_to_address( DbT::TxRef, Error< DbT::Error, - GreedyInputSelectorError, + GreedyInputSelectorError, core::convert::Infallible, DbT::NoteRef, >, @@ -213,12 +213,12 @@ where "It should not be possible for this to violate ZIP 321 request construction invariants.", ); - let change_strategy = SingleOutputFixedFeeChangeStrategy::new(FixedFeeRule::new(DEFAULT_FEE)); + let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(fixed::FeeRule::standard()); spend( wallet_db, params, prover, - &GreedyInputSelector::::new(change_strategy), + &GreedyInputSelector::::new(change_strategy, DustOutputPolicy::default()), usk, req, ovk_policy, diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index fe5a0f732..d95fed8c9 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -1,6 +1,7 @@ //! Types related to the process of selecting inputs to be spent given a transaction request. use core::marker::PhantomData; +use std::collections::BTreeSet; use std::fmt; use zcash_primitives::{ @@ -10,7 +11,7 @@ use zcash_primitives::{ components::{ amount::{Amount, BalanceError, NonNegativeAmount}, sapling::fees as sapling, - TxOut, + OutPoint, TxOut, }, fees::FeeRule, }, @@ -20,7 +21,7 @@ use zcash_primitives::{ use crate::{ address::{RecipientAddress, UnifiedAddress}, data_api::WalletRead, - fees::{ChangeError, ChangeStrategy, TransactionBalance}, + fees::{ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance}, wallet::{SpendableNote, WalletTransparentOutput}, zip321::TransactionRequest, }; @@ -179,40 +180,49 @@ pub trait InputSelector { /// Errors that can occur as a consequence of greedy input selection. #[derive(Debug, Clone, PartialEq, Eq)] -pub enum GreedyInputSelectorError { +pub enum GreedyInputSelectorError { /// An intermediate value overflowed or underflowed the valid monetary range. Balance(BalanceError), /// A unified address did not contain a supported receiver. UnsupportedAddress(Box), /// An error was encountered in change selection. - Change(ChangeError), + Change(ChangeError), } -impl From> - for InputSelectorError> +impl + From> + for InputSelectorError> { - fn from(err: GreedyInputSelectorError) -> Self { + fn from(err: GreedyInputSelectorError) -> Self { InputSelectorError::Selection(err) } } -impl From> - for InputSelectorError> +impl From> + for InputSelectorError> { - fn from(err: ChangeError) -> Self { + fn from(err: ChangeError) -> Self { InputSelectorError::Selection(GreedyInputSelectorError::Change(err)) } } -impl From - for InputSelectorError> +impl From + for InputSelectorError> { fn from(err: BalanceError) -> Self { InputSelectorError::Selection(GreedyInputSelectorError::Balance(err)) } } -struct SaplingPayment(Amount); +pub(crate) struct SaplingPayment(Amount); + +#[cfg(test)] +impl SaplingPayment { + pub(crate) fn new(amount: Amount) -> Self { + SaplingPayment(amount) + } +} + impl sapling::OutputView for SaplingPayment { fn value(&self) -> Amount { self.0 @@ -226,15 +236,17 @@ impl sapling::OutputView for SaplingPayment { /// interface. pub struct GreedyInputSelector { change_strategy: ChangeT, + dust_output_policy: DustOutputPolicy, _ds_type: PhantomData, } impl GreedyInputSelector { /// 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 { + pub fn new(change_strategy: ChangeT, dust_output_policy: DustOutputPolicy) -> Self { GreedyInputSelector { change_strategy, + dust_output_policy, _ds_type: PhantomData, } } @@ -246,7 +258,7 @@ where ChangeT: ChangeStrategy, ChangeT::FeeRule: Clone, { - type Error = GreedyInputSelectorError<::Error>; + type Error = GreedyInputSelectorError; type DataSource = DbT; type FeeRule = ChangeT::FeeRule; @@ -304,7 +316,9 @@ where } let mut sapling_inputs: Vec> = vec![]; - let mut prior_amount = Amount::zero(); + let mut prior_available = Amount::zero(); + let mut amount_required = Amount::zero(); + let mut exclude: Vec = vec![]; // 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. @@ -316,6 +330,7 @@ where &transparent_outputs, &sapling_inputs, &sapling_outputs, + &self.dust_output_policy, ); match balance { @@ -328,30 +343,35 @@ where fee_rule: (*self.change_strategy.fee_rule()).clone(), }); } + Err(ChangeError::DustInputs { mut sapling, .. }) => { + exclude.append(&mut sapling); + } 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::>() - .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; - } + amount_required = required; } Err(other) => return Err(other.into()), } + + sapling_inputs = wallet_db + .select_spendable_sapling_notes(account, amount_required, anchor_height, &exclude) + .map_err(InputSelectorError::DataSource)?; + + let new_available = sapling_inputs + .iter() + .map(|n| n.note_value) + .sum::>() + .ok_or(BalanceError::Overflow)?; + + if new_available <= prior_available { + return Err(InputSelectorError::InsufficientFunds { + required: amount_required, + available: new_available, + }); + } else { + // If the set of selected inputs has changed after selection, we will loop again + // and see whether we now have enough funds. + prior_available = new_available; + } } } @@ -371,23 +391,48 @@ where where ParamsT: consensus::Parameters, { - let transparent_inputs: Vec = source_addrs + let mut transparent_inputs: Vec = source_addrs .iter() - .map(|taddr| wallet_db.get_unspent_transparent_outputs(taddr, confirmed_height)) + .map(|taddr| wallet_db.get_unspent_transparent_outputs(taddr, confirmed_height, &[])) .collect::>, _>>() .map_err(InputSelectorError::DataSource)? .into_iter() .flat_map(|v| v.into_iter()) .collect(); - let balance = self.change_strategy.compute_balance( + let trial_balance = self.change_strategy.compute_balance( params, target_height, &transparent_inputs, &Vec::::new(), &Vec::>::new(), &Vec::::new(), - )?; + &self.dust_output_policy, + ); + + let balance = match trial_balance { + Ok(balance) => balance, + Err(ChangeError::DustInputs { transparent, .. }) => { + let exclusions: BTreeSet = transparent.into_iter().collect(); + transparent_inputs = transparent_inputs + .into_iter() + .filter(|i| !exclusions.contains(i.outpoint())) + .collect(); + + self.change_strategy.compute_balance( + params, + target_height, + &transparent_inputs, + &Vec::::new(), + &Vec::>::new(), + &Vec::::new(), + &self.dust_output_policy, + )? + } + Err(other) => { + return Err(other.into()); + } + }; if balance.total() >= shielding_threshold.into() { Ok(Proposal { diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 35fd1e255..55b9a7a9e 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -5,12 +5,16 @@ use zcash_primitives::{ amount::{Amount, BalanceError}, sapling::fees as sapling, transparent::fees as transparent, + OutPoint, }, - fees::{FeeRule, FixedFeeRule}, + fees::FeeRule, }, }; +pub mod fixed; + /// A proposed change amount and output pool. +#[derive(Clone, Debug, PartialEq, Eq)] pub enum ChangeValue { Sapling(Amount), } @@ -26,6 +30,7 @@ impl ChangeValue { /// The amount of change and fees required to make a transaction's inputs and /// outputs balance under a specific fee rule, as computed by a particular /// [`ChangeStrategy`] that is aware of that rule. +#[derive(Clone, Debug, PartialEq, Eq)] pub struct TransactionBalance { proposed_change: Vec, fee_required: Amount, @@ -66,7 +71,7 @@ impl TransactionBalance { /// Errors that can occur in computing suggested change and/or fees. #[derive(Clone, Debug, PartialEq, Eq)] -pub enum ChangeError { +pub enum ChangeError { /// Insufficient inputs were provided to change selection to fund the /// required outputs and fees. InsufficientFunds { @@ -76,10 +81,76 @@ pub enum ChangeError { /// including the required fees. required: Amount, }, + /// Some of the inputs provided to the transaction were determined to currently have no + /// economic value (i.e. their inclusion in a transaction causes fees to rise in an amount + /// greater than their value.) + DustInputs { + /// The outpoints corresponding to transparent inputs having no current economic value. + transparent: Vec, + /// The identifiers for Sapling inputs having not current economic value + sapling: Vec, + }, /// An error occurred that was specific to the change selection strategy in use. StrategyError(E), } +impl From for ChangeError { + fn from(err: BalanceError) -> ChangeError { + ChangeError::StrategyError(err) + } +} + +/// An enumeration of actions to tak when a transaction would potentially create dust +/// outputs (outputs that are likely to be without economic value due to fee rules.) +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum DustAction { + /// Do not allow creation of dust outputs; instead, require that additional inputs be provided. + Reject, + /// Explicitly allow the creation of dust change amounts greater than the specified value. + AllowDustChange, + /// Allow dust amounts to be added to the transaction fee + AddDustToFee, +} + +/// A policy describing how a [`ChangeStrategy`] should treat potentially dust-valued change +/// outputs (outputs that are likely to be without economic value due to fee rules.) +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct DustOutputPolicy { + action: DustAction, + dust_threshold: Option, +} + +impl DustOutputPolicy { + /// Constructs a new dust output policy. + /// + /// A dust policy created with `None` as the dust threshold will delegate determination + /// of the dust threshold to the change strategy that is evaluating the strategy; this + /// recommended, but an explicit value (including zero) may be provided to explicitly + /// override the determination of the change strategy. + pub fn new(action: DustAction, dust_threshold: Option) -> Self { + Self { + action, + dust_threshold, + } + } + + /// Returns the action to take in the event that a dust change amount would be produced + pub fn action(&self) -> DustAction { + self.action + } + /// Returns a value that will be used to override the dust determination logic of the + /// change policy, if any. + pub fn dust_threshold(&self) -> Option { + self.dust_threshold + } +} + +impl Default for DustOutputPolicy { + fn default() -> Self { + DustOutputPolicy::new(DustAction::Reject, None) + } +} + /// A trait that represents the ability to compute the suggested change and fees that must be paid /// by a transaction having a specified set of inputs and outputs. pub trait ChangeStrategy { @@ -97,101 +168,52 @@ pub trait ChangeStrategy { /// change outputs recommended by this operation. If insufficient funds are available to /// supply the requested outputs and required fees, implementations should return /// [`ChangeError::InsufficientFunds`]. - fn compute_balance( + #[allow(clippy::too_many_arguments)] + fn compute_balance( &self, params: &P, target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], + sapling_inputs: &[impl sapling::InputView], sapling_outputs: &[impl sapling::OutputView], - ) -> Result>; + dust_output_policy: &DustOutputPolicy, + ) -> Result>; } -/// A change strategy that and proposes change as a single output to the most current supported -/// shielded pool and delegates fee calculation to the provided fee rule. -pub struct SingleOutputFixedFeeChangeStrategy { - fee_rule: FixedFeeRule, -} +#[cfg(test)] +pub(crate) mod tests { + use zcash_primitives::transaction::components::{ + amount::Amount, + sapling::fees as sapling, + transparent::{fees as transparent, OutPoint, TxOut}, + }; -impl SingleOutputFixedFeeChangeStrategy { - /// Constructs a new [`SingleOutputFixedFeeChangeStrategy`] with the specified fee rule. - pub fn new(fee_rule: FixedFeeRule) -> Self { - Self { fee_rule } - } -} - -impl From for ChangeError { - fn from(err: BalanceError) -> ChangeError { - ChangeError::StrategyError(err) - } -} - -impl ChangeStrategy for SingleOutputFixedFeeChangeStrategy { - type FeeRule = FixedFeeRule; - type Error = BalanceError; - - fn fee_rule(&self) -> &Self::FeeRule { - &self.fee_rule + pub(crate) struct TestTransparentInput { + pub outpoint: OutPoint, + pub coin: TxOut, } - fn compute_balance( - &self, - params: &P, - target_height: BlockHeight, - transparent_inputs: &[impl transparent::InputView], - transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], - ) -> Result> { - let t_in = transparent_inputs - .iter() - .map(|t_in| t_in.coin().value) - .sum::>() - .ok_or(BalanceError::Overflow)?; - let t_out = transparent_outputs - .iter() - .map(|t_out| t_out.value()) - .sum::>() - .ok_or(BalanceError::Overflow)?; - let sapling_in = sapling_inputs - .iter() - .map(|s_in| s_in.value()) - .sum::>() - .ok_or(BalanceError::Overflow)?; - let sapling_out = sapling_outputs - .iter() - .map(|s_out| s_out.value()) - .sum::>() - .ok_or(BalanceError::Overflow)?; + impl transparent::InputView for TestTransparentInput { + fn outpoint(&self) -> &OutPoint { + &self.outpoint + } + fn coin(&self) -> &TxOut { + &self.coin + } + } - let fee_amount = self - .fee_rule - .fee_required( - params, - target_height, - transparent_inputs, - transparent_outputs, - sapling_inputs.len(), - sapling_outputs.len() + 1, - ) - .unwrap(); // FixedFeeRule::fee_required is infallible. + pub(crate) struct TestSaplingInput { + pub note_id: u32, + pub value: Amount, + } - let total_in = (t_in + sapling_in).ok_or(BalanceError::Overflow)?; - let total_out = [t_out, sapling_out, fee_amount] - .iter() - .sum::>() - .ok_or(BalanceError::Overflow)?; - - 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 { - TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount) - .ok_or_else(|| BalanceError::Overflow.into()) + impl sapling::InputView for TestSaplingInput { + fn note_id(&self) -> &u32 { + &self.note_id + } + fn value(&self) -> Amount { + self.value } } } diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs new file mode 100644 index 000000000..d57953399 --- /dev/null +++ b/zcash_client_backend/src/fees/fixed.rs @@ -0,0 +1,234 @@ +//! Change strategies designed for use with a fixed fee. +use std::cmp::Ordering; + +use zcash_primitives::{ + consensus::{self, BlockHeight}, + transaction::{ + components::{ + amount::{Amount, BalanceError}, + sapling::fees as sapling, + transparent::fees as transparent, + }, + fees::{fixed::FeeRule as FixedFeeRule, FeeRule}, + }, +}; + +use super::{ + ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, TransactionBalance, +}; + +/// A change strategy that and proposes change as a single output to the most current supported +/// shielded pool and delegates fee calculation to the provided fee rule. +pub struct SingleOutputChangeStrategy { + fee_rule: FixedFeeRule, +} + +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule. + pub fn new(fee_rule: FixedFeeRule) -> Self { + Self { fee_rule } + } +} + +impl ChangeStrategy for SingleOutputChangeStrategy { + type FeeRule = FixedFeeRule; + type Error = BalanceError; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn compute_balance( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_inputs: &[impl sapling::InputView], + sapling_outputs: &[impl sapling::OutputView], + dust_output_policy: &DustOutputPolicy, + ) -> Result> { + let t_in = transparent_inputs + .iter() + .map(|t_in| t_in.coin().value) + .sum::>() + .ok_or(BalanceError::Overflow)?; + let t_out = transparent_outputs + .iter() + .map(|t_out| t_out.value()) + .sum::>() + .ok_or(BalanceError::Overflow)?; + let sapling_in = sapling_inputs + .iter() + .map(|s_in| s_in.value()) + .sum::>() + .ok_or(BalanceError::Overflow)?; + let sapling_out = sapling_outputs + .iter() + .map(|s_out| s_out.value()) + .sum::>() + .ok_or(BalanceError::Overflow)?; + + let fee_amount = self + .fee_rule + .fee_required( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs.len(), + sapling_outputs.len() + 1, + ) + .unwrap(); // fixed::FeeRule::fee_required is infallible. + + let total_in = (t_in + sapling_in).ok_or(BalanceError::Overflow)?; + + if (!transparent_inputs.is_empty() || !sapling_inputs.is_empty()) && fee_amount > total_in { + // For the fixed-fee selection rule, the only time we consider inputs dust is when the fee + // exceeds the value of all input values. + Err(ChangeError::DustInputs { + transparent: transparent_inputs + .iter() + .map(|i| i.outpoint()) + .cloned() + .collect(), + sapling: sapling_inputs + .iter() + .map(|i| i.note_id()) + .cloned() + .collect(), + }) + } else { + let total_out = [t_out, sapling_out, fee_amount] + .iter() + .sum::>() + .ok_or(BalanceError::Overflow)?; + + let proposed_change = (total_in - total_out).ok_or(BalanceError::Underflow)?; + match proposed_change.cmp(&Amount::zero()) { + Ordering::Less => Err(ChangeError::InsufficientFunds { + available: total_in, + required: total_out, + }), + Ordering::Equal => TransactionBalance::new(vec![], fee_amount) + .ok_or_else(|| BalanceError::Overflow.into()), + Ordering::Greater => { + let dust_threshold = dust_output_policy + .dust_threshold() + .unwrap_or_else(|| self.fee_rule.fixed_fee()); + + if dust_threshold > proposed_change { + match dust_output_policy.action() { + DustAction::Reject => { + let shortfall = (dust_threshold - proposed_change) + .ok_or(BalanceError::Underflow)?; + Err(ChangeError::InsufficientFunds { + available: total_in, + required: (total_in + shortfall) + .ok_or(BalanceError::Overflow)?, + }) + } + DustAction::AllowDustChange => TransactionBalance::new( + vec![ChangeValue::Sapling(proposed_change)], + fee_amount, + ) + .ok_or_else(|| BalanceError::Overflow.into()), + DustAction::AddDustToFee => TransactionBalance::new( + vec![], + (fee_amount + proposed_change).unwrap(), + ) + .ok_or_else(|| BalanceError::Overflow.into()), + } + } else { + TransactionBalance::new( + vec![ChangeValue::Sapling(proposed_change)], + fee_amount, + ) + .ok_or_else(|| BalanceError::Overflow.into()) + } + } + } + } + } +} + +#[cfg(test)] +mod tests { + use zcash_primitives::{ + consensus::{Network, NetworkUpgrade, Parameters}, + transaction::{ + components::{amount::Amount, transparent::TxOut}, + fees::fixed::FeeRule as FixedFeeRule, + }, + }; + + use super::SingleOutputChangeStrategy; + use crate::{ + data_api::wallet::input_selection::SaplingPayment, + fees::{ + tests::{TestSaplingInput, TestTransparentInput}, + ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, + }, + }; + + #[test] + fn change_without_dust() { + let change_strategy = SingleOutputChangeStrategy::new(FixedFeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[TestSaplingInput { + note_id: 0, + value: Amount::from_u64(45000).unwrap(), + }], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(Amount::from_u64(4000).unwrap())] + && balance.fee_required() == Amount::from_u64(1000).unwrap() + ); + } + + #[test] + fn dust_change() { + let change_strategy = SingleOutputChangeStrategy::new(FixedFeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[ + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(40000).unwrap(), + }, + // enough to pay a fee, plus dust + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(1100).unwrap(), + }, + ], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Err(ChangeError::InsufficientFunds { available, required }) + if available == Amount::from_u64(41100).unwrap() && required == Amount::from_u64(42000).unwrap() + ); + } +} diff --git a/zcash_client_backend/src/lib.rs b/zcash_client_backend/src/lib.rs index 931cf99a8..f73711662 100644 --- a/zcash_client_backend/src/lib.rs +++ b/zcash_client_backend/src/lib.rs @@ -21,3 +21,7 @@ pub mod welding_rig; pub mod zip321; pub use decrypt::{decrypt_transaction, DecryptedOutput, TransferType}; + +#[cfg(test)] +#[macro_use] +extern crate assert_matches; diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 66b4d0fce..f9f5d4fe8 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -119,7 +119,11 @@ pub struct SpendableNote { pub witness: IncrementalWitness, } -impl sapling::fees::InputView for SpendableNote { +impl sapling::fees::InputView for SpendableNote { + fn note_id(&self) -> &NoteRef { + &self.note_id + } + fn value(&self) -> Amount { self.note_value } diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 821cb749e..b34436ad6 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -79,6 +79,9 @@ and this library adheres to Rust's notion of - `zcash_client_sqlite::wallet`: - `get_spendable_notes` to `get_spendable_sapling_notes`. - `select_spendable_notes` to `select_spendable_sapling_notes`. +- `zcash_client_sqlite::wallet::{get_spendable_sapling_notes, select_spendable_sapling_notes}` + have also been changed to take a parameter that permits the caller to + specify a set of notes to exclude from consideration. - `zcash_client_sqlite::wallet::init_wallet_db` has been modified to take the wallet seed as an argument so that it can correctly perform migrations that require re-deriving key material. In particular for diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 3334aac98..76d2110b4 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -32,7 +32,7 @@ secrecy = "0.8" # - SQLite databases group = "0.12" jubjub = "0.9" -rusqlite = { version = "0.25", features = ["bundled", "time"] } +rusqlite = { version = "0.25", features = ["bundled", "time", "array"] } schemer = "0.2" schemer-rusqlite = "0.2" time = "0.2" diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 7430ad75a..e7a0ebebd 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -45,7 +45,10 @@ use zcash_primitives::{ memo::Memo, merkle_tree::{CommitmentTree, IncrementalWitness}, sapling::{Node, Nullifier}, - transaction::{components::Amount, Transaction, TxId}, + transaction::{ + components::{amount::Amount, OutPoint}, + Transaction, TxId, + }, zip32::{AccountId, DiversifierIndex, ExtendedFullViewingKey}, }; @@ -113,7 +116,10 @@ pub struct WalletDb

{ impl WalletDb

{ /// Construct a connection to the wallet database stored at the specified path. pub fn for_path>(path: F, params: P) -> Result { - Connection::open(path).map(move |conn| WalletDb { conn, params }) + Connection::open(path).and_then(move |conn| { + rusqlite::vtab::array::load_module(&conn)?; + Ok(WalletDb { conn, params }) + }) } /// Given a wallet database connection, obtain a handle for the write operations @@ -227,9 +233,10 @@ impl WalletRead for WalletDb

{ &self, account: AccountId, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { #[allow(deprecated)] - wallet::transact::get_spendable_sapling_notes(self, account, anchor_height) + wallet::transact::get_spendable_sapling_notes(self, account, anchor_height, exclude) } fn select_spendable_sapling_notes( @@ -237,9 +244,16 @@ impl WalletRead for WalletDb

{ account: AccountId, target_value: Amount, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { #[allow(deprecated)] - wallet::transact::select_spendable_sapling_notes(self, account, target_value, anchor_height) + wallet::transact::select_spendable_sapling_notes( + self, + account, + target_value, + anchor_height, + exclude, + ) } fn get_transparent_receivers( @@ -259,9 +273,10 @@ impl WalletRead for WalletDb

{ &self, _address: &TransparentAddress, _max_height: BlockHeight, + _exclude: &[OutPoint], ) -> Result, Self::Error> { #[cfg(feature = "transparent-inputs")] - return wallet::get_unspent_transparent_outputs(self, _address, _max_height); + return wallet::get_unspent_transparent_outputs(self, _address, _max_height, _exclude); #[cfg(not(feature = "transparent-inputs"))] panic!( @@ -372,9 +387,10 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> { &self, account: AccountId, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { self.wallet_db - .get_spendable_sapling_notes(account, anchor_height) + .get_spendable_sapling_notes(account, anchor_height, exclude) } fn select_spendable_sapling_notes( @@ -382,9 +398,10 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> { account: AccountId, target_value: Amount, anchor_height: BlockHeight, + exclude: &[Self::NoteRef], ) -> Result>, Self::Error> { self.wallet_db - .select_spendable_sapling_notes(account, target_value, anchor_height) + .select_spendable_sapling_notes(account, target_value, anchor_height, exclude) } fn get_transparent_receivers( @@ -398,9 +415,10 @@ impl<'a, P: consensus::Parameters> WalletRead for DataConnStmtCache<'a, P> { &self, address: &TransparentAddress, max_height: BlockHeight, + exclude: &[OutPoint], ) -> Result, Self::Error> { self.wallet_db - .get_unspent_transparent_outputs(address, max_height) + .get_unspent_transparent_outputs(address, max_height, exclude) } fn get_transparent_balances( diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 57f1ac1f6..fa6044f89 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -42,6 +42,7 @@ use crate::{ use { crate::UtxoId, rusqlite::{params, Connection}, + std::collections::BTreeSet, zcash_client_backend::{ address::AddressMetadata, encoding::AddressCodec, wallet::WalletTransparentOutput, }, @@ -955,6 +956,7 @@ pub(crate) fn get_unspent_transparent_outputs( wdb: &WalletDb

, address: &TransparentAddress, max_height: BlockHeight, + exclude: &[OutPoint], ) -> Result, SqliteClientError> { let mut stmt_blocks = wdb.conn.prepare( "SELECT u.prevout_txid, u.prevout_idx, u.script, @@ -971,6 +973,7 @@ pub(crate) fn get_unspent_transparent_outputs( let mut utxos = Vec::::new(); let mut rows = stmt_blocks.query(params![addr_str, u32::from(max_height)])?; + let excluded: BTreeSet = exclude.iter().cloned().collect(); while let Some(row) = rows.next()? { let txid: Vec = row.get(0)?; let mut txid_bytes = [0u8; 32]; @@ -981,8 +984,13 @@ pub(crate) fn get_unspent_transparent_outputs( let value = Amount::from_i64(row.get(3)?).unwrap(); let height: u32 = row.get(4)?; + let outpoint = OutPoint::new(txid_bytes, index); + if excluded.contains(&outpoint) { + continue; + } + let output = WalletTransparentOutput::from_parts( - OutPoint::new(txid_bytes, index), + outpoint, TxOut { value, script_pubkey, @@ -1394,7 +1402,8 @@ mod tests { super::get_unspent_transparent_outputs( &db_data, taddr, - BlockHeight::from_u32(12345) + BlockHeight::from_u32(12345), + &[] ), Ok(utxos) if utxos.is_empty() )); @@ -1403,7 +1412,8 @@ mod tests { super::get_unspent_transparent_outputs( &db_data, taddr, - BlockHeight::from_u32(34567) + BlockHeight::from_u32(34567), + &[] ), Ok(utxos) if { utxos.len() == 1 && diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index ba006c076..c099798f7 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/zcash_client_sqlite/src/wallet/transact.rs @@ -1,6 +1,7 @@ //! Functions for creating transactions. //! -use rusqlite::{named_params, Row}; +use rusqlite::{named_params, types::Value, Row}; +use std::rc::Rc; use group::ff::PrimeField; @@ -68,6 +69,7 @@ pub fn get_spendable_sapling_notes

( wdb: &WalletDb

, account: AccountId, anchor_height: BlockHeight, + exclude: &[NoteId], ) -> Result>, SqliteClientError> { let mut stmt_select_notes = wdb.conn.prepare( "SELECT id_note, diversifier, value, rcm, witness @@ -77,14 +79,24 @@ pub fn get_spendable_sapling_notes

( WHERE account = :account AND spent IS NULL AND transactions.block <= :anchor_height - AND sapling_witnesses.block = :anchor_height", + AND sapling_witnesses.block = :anchor_height + AND id_note NOT IN rarray(:exclude)", )?; - // Select notes + let excluded: Vec = exclude + .iter() + .filter_map(|n| match n { + NoteId::ReceivedNoteId(i) => Some(Value::from(*i)), + NoteId::SentNoteId(_) => None, + }) + .collect(); + let excluded_ptr = Rc::new(excluded); + let notes = stmt_select_notes.query_and_then( named_params![ ":account": &u32::from(account), ":anchor_height": &u32::from(anchor_height), + ":exclude": &excluded_ptr, ], to_spendable_note, )?; @@ -100,6 +112,7 @@ pub fn select_spendable_sapling_notes

( account: AccountId, target_value: Amount, anchor_height: BlockHeight, + exclude: &[NoteId], ) -> Result>, 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 @@ -127,7 +140,10 @@ pub fn select_spendable_sapling_notes

( (PARTITION BY account, spent ORDER BY id_note) AS so_far FROM received_notes INNER JOIN transactions ON transactions.id_tx = received_notes.tx - WHERE account = :account AND spent IS NULL AND transactions.block <= :anchor_height + WHERE account = :account + AND spent IS NULL + AND transactions.block <= :anchor_height + AND id_note NOT IN rarray(:exclude) ) SELECT * FROM eligible WHERE so_far < :target_value UNION @@ -141,12 +157,21 @@ pub fn select_spendable_sapling_notes

( INNER JOIN witnesses ON selected.id_note = witnesses.note", )?; - // Select notes + let excluded: Vec = exclude + .iter() + .filter_map(|n| match n { + NoteId::ReceivedNoteId(i) => Some(Value::from(*i)), + NoteId::SentNoteId(_) => None, + }) + .collect(); + let excluded_ptr = Rc::new(excluded); + let notes = stmt_select_notes.query_and_then( named_params![ ":account": &u32::from(account), ":anchor_height": &u32::from(anchor_height), ":target_value": &i64::from(target_value), + ":exclude": &excluded_ptr ], to_spendable_note, )?; diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 764f4d408..bce8d0a21 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -492,10 +492,10 @@ mod tests { transaction::{ builder::Builder, components::{ - amount::{Amount, DEFAULT_FEE}, + amount::Amount, tze::{Authorized, Bundle, OutPoint, TzeIn, TzeOut}, }, - fees::FixedFeeRule, + fees::fixed, Transaction, TransactionData, TxVersion, }, zip32::ExtendedSpendingKey, @@ -809,7 +809,7 @@ mod tests { // let mut rng = OsRng; - let fee_rule = FixedFeeRule::new(DEFAULT_FEE); + let fee_rule = fixed::FeeRule::standard(); // create some inputs to spend let extsk = ExtendedSpendingKey::master(&[]); @@ -848,7 +848,7 @@ mod tests { let mut builder_b = demo_builder(tx_height + 1); let prevout_a = (OutPoint::new(tx_a.txid(), 0), tze_a.vout[0].clone()); - let value_xfr = (value - DEFAULT_FEE).unwrap(); + let value_xfr = (value - fee_rule.fixed_fee()).unwrap(); builder_b .demo_transfer_to_close(prevout_a, value_xfr, preimage_1, h2) .map_err(|e| format!("transfer failure: {:?}", e)) @@ -874,7 +874,7 @@ mod tests { builder_c .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - (value_xfr - DEFAULT_FEE).unwrap(), + (value_xfr - fee_rule.fixed_fee()).unwrap(), ) .unwrap(); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 347b64989..10b701eeb 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -27,6 +27,8 @@ and this library adheres to Rust's notion of and types related to fee calculations. - `FeeRule` a trait that describes how to compute the fee required for a transaction given inputs and outputs to the transaction. + - `zcash_primitives::transaction::fees::fixed` a new module containing an implementation + of the existing fixed fee rule. - Added to `zcash_primitives::transaction::components::sapling::builder` - `SaplingBuilder::{inputs, outputs}`: accessors for Sapling builder state. - `zcash_primitives::transaction::components::sapling::fees` @@ -37,6 +39,7 @@ and this library adheres to Rust's notion of - Added to `zcash_primitives::zip32::sapling::DiversifiableFullViewingKey` - `DiversifiableFullViewingKey::{diversified_address, diversified_change_address}` - `impl Eq for zcash_primitves::sapling::PaymentAddress` +- `impl {PartialOrd, Ord} for zcash_primitves::transaction::components::transparent::OutPoint` ### Changed - `zcash_primitives::transaction::builder::Builder::build` now takes a `FeeRule` diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 99e747323..cf53cdbb7 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -173,7 +173,7 @@ impl<'a, P, R> Builder<'a, P, R> { /// Returns the set of Sapling inputs currently committed to be consumed /// by the transaction. - pub fn sapling_inputs(&self) -> &[impl sapling::fees::InputView] { + pub fn sapling_inputs(&self) -> &[impl sapling::fees::InputView<()>] { self.sapling_builder.inputs() } @@ -511,7 +511,7 @@ mod testing { use crate::{ consensus::{self, BlockHeight}, sapling::prover::mock::MockTxProver, - transaction::{components::amount::DEFAULT_FEE, fees::FixedFeeRule, Transaction}, + transaction::{fees::fixed, Transaction}, }; impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> { @@ -529,7 +529,7 @@ mod testing { } pub fn mock_build(self) -> Result<(Transaction, SaplingMetadata), Error> { - self.build(&MockTxProver, &FixedFeeRule::new(DEFAULT_FEE)) + self.build(&MockTxProver, &fixed::FeeRule::standard()) } } } diff --git a/zcash_primitives/src/transaction/components/sapling/builder.rs b/zcash_primitives/src/transaction/components/sapling/builder.rs index 9351af856..60bf7a835 100644 --- a/zcash_primitives/src/transaction/components/sapling/builder.rs +++ b/zcash_primitives/src/transaction/components/sapling/builder.rs @@ -69,10 +69,14 @@ pub struct SpendDescriptionInfo { merkle_path: MerklePath, } -impl fees::InputView for SpendDescriptionInfo { +impl fees::InputView<()> for SpendDescriptionInfo { + fn note_id(&self) -> &() { + // The builder does not make use of note identifiers, so we can just return the unit value. + &() + } + fn value(&self) -> Amount { - // An existing note to be spent must have a valid - // amount value. + // An existing note to be spent must have a valid amount value. Amount::from_u64(self.note.value).unwrap() } } @@ -243,7 +247,7 @@ impl

SaplingBuilder

{ /// Returns the list of Sapling inputs that will be consumed by the transaction being /// constructed. - pub fn inputs(&self) -> &[impl fees::InputView] { + pub fn inputs(&self) -> &[impl fees::InputView<()>] { &self.spends } diff --git a/zcash_primitives/src/transaction/components/sapling/fees.rs b/zcash_primitives/src/transaction/components/sapling/fees.rs index a1ad1659d..10d72adb6 100644 --- a/zcash_primitives/src/transaction/components/sapling/fees.rs +++ b/zcash_primitives/src/transaction/components/sapling/fees.rs @@ -5,7 +5,9 @@ use crate::transaction::components::amount::Amount; /// A trait that provides a minimized view of a Sapling input suitable for use in /// fee and change calculation. -pub trait InputView { +pub trait InputView { + /// An identifier for the input being spent. + fn note_id(&self) -> &NoteRef; /// The value of the input being spent. fn value(&self) -> Amount; } diff --git a/zcash_primitives/src/transaction/components/transparent.rs b/zcash_primitives/src/transaction/components/transparent.rs index ed1108259..988174e2f 100644 --- a/zcash_primitives/src/transaction/components/transparent.rs +++ b/zcash_primitives/src/transaction/components/transparent.rs @@ -90,7 +90,7 @@ impl Bundle { } } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct OutPoint { hash: [u8; 32], n: u32, diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index 2c9e844de..2a9a9021a 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -8,6 +8,8 @@ use crate::{ #[cfg(feature = "zfuture")] use crate::transaction::components::tze::fees as tze; +pub mod fixed; + /// A trait that represents the ability to compute the fees that must be paid /// by a transaction having a specified set of inputs and outputs. pub trait FeeRule { @@ -51,50 +53,3 @@ pub trait FutureFeeRule: FeeRule { tze_outputs: &[impl tze::OutputView], ) -> Result; } - -/// 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, -} - -impl FixedFeeRule { - /// Creates a new fixed fee rule with the specified fixed fee. - pub fn new(fixed_fee: Amount) -> Self { - Self { fixed_fee } - } -} - -impl FeeRule for FixedFeeRule { - type Error = std::convert::Infallible; - - fn fee_required( - &self, - _params: &P, - _target_height: BlockHeight, - _transparent_inputs: &[impl transparent::InputView], - _transparent_outputs: &[impl transparent::OutputView], - _sapling_input_count: usize, - _sapling_output_count: usize, - ) -> Result { - Ok(self.fixed_fee) - } -} - -#[cfg(feature = "zfuture")] -impl FutureFeeRule for FixedFeeRule { - fn fee_required_zfuture( - &self, - _params: &P, - _target_height: BlockHeight, - _transparent_inputs: &[impl transparent::InputView], - _transparent_outputs: &[impl transparent::OutputView], - _sapling_input_count: usize, - _sapling_output_count: usize, - _tze_inputs: &[impl tze::InputView], - _tze_outputs: &[impl tze::OutputView], - ) -> Result { - Ok(self.fixed_fee) - } -} diff --git a/zcash_primitives/src/transaction/fees/fixed.rs b/zcash_primitives/src/transaction/fees/fixed.rs new file mode 100644 index 000000000..e9dfd7613 --- /dev/null +++ b/zcash_primitives/src/transaction/fees/fixed.rs @@ -0,0 +1,69 @@ +use crate::{ + consensus::{self, BlockHeight}, + transaction::components::{ + amount::{Amount, DEFAULT_FEE}, + transparent::fees as transparent, + }, +}; + +#[cfg(feature = "zfuture")] +use crate::transaction::components::tze::fees as tze; + +/// A fee rule that always returns a fixed fee, irrespective of the structure of +/// the transaction being constructed. +#[derive(Clone, Copy, Debug)] +pub struct FeeRule { + fixed_fee: Amount, +} + +impl FeeRule { + /// Creates a new nonstandard fixed fee rule with the specified fixed fee. + pub fn non_standard(fixed_fee: Amount) -> Self { + Self { fixed_fee } + } + + /// Creates a new fixed fee rule with the standard default fee. + pub fn standard() -> Self { + Self { + fixed_fee: DEFAULT_FEE, + } + } + + /// Returns the fixed fee amount which which this rule was configured. + pub fn fixed_fee(&self) -> Amount { + self.fixed_fee + } +} + +impl super::FeeRule for FeeRule { + type Error = std::convert::Infallible; + + fn fee_required( + &self, + _params: &P, + _target_height: BlockHeight, + _transparent_inputs: &[impl transparent::InputView], + _transparent_outputs: &[impl transparent::OutputView], + _sapling_input_count: usize, + _sapling_output_count: usize, + ) -> Result { + Ok(self.fixed_fee) + } +} + +#[cfg(feature = "zfuture")] +impl super::FutureFeeRule for FeeRule { + fn fee_required_zfuture( + &self, + _params: &P, + _target_height: BlockHeight, + _transparent_inputs: &[impl transparent::InputView], + _transparent_outputs: &[impl transparent::OutputView], + _sapling_input_count: usize, + _sapling_output_count: usize, + _tze_inputs: &[impl tze::InputView], + _tze_outputs: &[impl tze::OutputView], + ) -> Result { + Ok(self.fixed_fee) + } +} From 73ab884073183cf7a87e05ddf5e303b88c761cf0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Nov 2022 17:44:49 -0600 Subject: [PATCH 3/4] Implement ZIP 317 fee estimation, calculation, & change selection --- zcash_client_backend/CHANGELOG.md | 4 +- zcash_client_backend/src/fees.rs | 1 + zcash_client_backend/src/fees/zip317.rs | 336 ++++++++++++++++++ zcash_client_sqlite/src/wallet/transact.rs | 121 ++++++- zcash_primitives/CHANGELOG.md | 6 +- .../src/transaction/components/amount.rs | 13 +- zcash_primitives/src/transaction/fees.rs | 1 + .../src/transaction/fees/zip317.rs | 139 ++++++++ 8 files changed, 614 insertions(+), 7 deletions(-) create mode 100644 zcash_client_backend/src/fees/zip317.rs create mode 100644 zcash_primitives/src/transaction/fees/zip317.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d631e8b12..720c7b35c 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -71,8 +71,10 @@ and this library adheres to Rust's notion of - `TransactionBalance` - `BasicFixedFeeChangeStrategy` - a `ChangeStrategy` implementation that reproduces current wallet change behavior - - `fixed` a new module containing of change selection strategies for the + - `fixed`, a new module containing of change selection strategies for the existing fixed fee rule. + - `zip317`, a new module containing change selection strategies for the ZIP + 317 fee rule. - New experimental APIs that should be considered unstable, and are likely to be modified and/or moved to a different module in a future release: diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 55b9a7a9e..aaaca4ed7 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -12,6 +12,7 @@ use zcash_primitives::{ }; pub mod fixed; +pub mod zip317; /// A proposed change amount and output pool. #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs new file mode 100644 index 000000000..fb03298bf --- /dev/null +++ b/zcash_client_backend/src/fees/zip317.rs @@ -0,0 +1,336 @@ +//! Change strategies designed to implement the ZIP 317 fee rules. +//! +//! Change selection in ZIP 317 requires careful handling of low-valued inputs +//! to ensure that inputs added to a transaction do not cause fees to rise by +//! an amount greater than their value. +use core::cmp::Ordering; + +use zcash_primitives::{ + consensus::{self, BlockHeight}, + transaction::{ + components::{ + amount::{Amount, BalanceError}, + sapling::fees as sapling, + transparent::fees as transparent, + }, + fees::{ + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + FeeRule, + }, + }, +}; + +use super::{ + ChangeError, ChangeStrategy, ChangeValue, DustAction, DustOutputPolicy, TransactionBalance, +}; + +/// A change strategy that and proposes change as a single output to the most current supported +/// shielded pool and delegates fee calculation to the provided fee rule. +pub struct SingleOutputChangeStrategy { + fee_rule: Zip317FeeRule, +} + +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 + /// fee parameters. + pub fn new(fee_rule: Zip317FeeRule) -> Self { + Self { fee_rule } + } +} + +impl ChangeStrategy for SingleOutputChangeStrategy { + type FeeRule = Zip317FeeRule; + type Error = Zip317FeeError; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn compute_balance( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_inputs: &[impl sapling::InputView], + sapling_outputs: &[impl sapling::OutputView], + dust_output_policy: &DustOutputPolicy, + ) -> Result> { + let mut transparent_dust: Vec<_> = transparent_inputs + .iter() + .filter_map(|i| { + // for now, we're just assuming p2pkh inputs, so we don't check the size of the input + // script + if i.coin().value < self.fee_rule.marginal_fee() { + Some(i.outpoint().clone()) + } else { + None + } + }) + .collect(); + + let mut sapling_dust: Vec<_> = sapling_inputs + .iter() + .filter_map(|i| { + if i.value() < self.fee_rule.marginal_fee() { + Some(i.note_id().clone()) + } else { + None + } + }) + .collect(); + + // Depending on the shape of the transaction, we may be able to spend up to + // `grace_actions - 1` dust inputs. If we don't have any dust inputs though, + // we don't need to worry about any of that. + if !(transparent_dust.is_empty() && sapling_dust.is_empty()) { + let t_non_dust = transparent_inputs.len() - transparent_dust.len(); + let t_allowed_dust = transparent_outputs.len().saturating_sub(t_non_dust); + + // We add one to the sapling outputs for the (single) change output Note that this + // means that wallet-internal shielding transactions are an opportunity to spend a dust + // note. + let s_non_dust = sapling_inputs.len() - sapling_dust.len(); + let s_allowed_dust = (sapling_outputs.len() + 1).saturating_sub(s_non_dust); + + let available_grace_inputs = self + .fee_rule + .grace_actions() + .saturating_sub(t_non_dust) + .saturating_sub(s_non_dust); + + let mut t_disallowed_dust = transparent_dust.len().saturating_sub(t_allowed_dust); + let mut s_disallowed_dust = sapling_dust.len().saturating_sub(s_allowed_dust); + + if available_grace_inputs > 0 { + // if we have available grace inputs, allocate them first to transparent dust + // and then to sapling dust + let t_grace_dust = available_grace_inputs.saturating_sub(t_disallowed_dust); + t_disallowed_dust = t_disallowed_dust.saturating_sub(t_grace_dust); + + let s_grace_dust = available_grace_inputs + .saturating_sub(t_grace_dust) + .saturating_sub(s_disallowed_dust); + s_disallowed_dust = s_disallowed_dust.saturating_sub(s_grace_dust); + } + + // truncate the lists of inputs to be disregarded in input selection to just the + // disallowed lengths this has the effect of prioritizing inputs for inclusion by the + // order of th original input slices, with the most preferred inputs first + transparent_dust.reverse(); + transparent_dust.truncate(t_disallowed_dust); + sapling_dust.reverse(); + sapling_dust.truncate(s_disallowed_dust); + + if !(transparent_dust.is_empty() && sapling_dust.is_empty()) { + return Err(ChangeError::DustInputs { + transparent: transparent_dust, + sapling: sapling_dust, + }); + } + } + + let overflow = || ChangeError::StrategyError(Zip317FeeError::from(BalanceError::Overflow)); + let underflow = + || ChangeError::StrategyError(Zip317FeeError::from(BalanceError::Underflow)); + + let t_in = transparent_inputs + .iter() + .map(|t_in| t_in.coin().value) + .sum::>() + .ok_or_else(overflow)?; + let t_out = transparent_outputs + .iter() + .map(|t_out| t_out.value()) + .sum::>() + .ok_or_else(overflow)?; + let sapling_in = sapling_inputs + .iter() + .map(|s_in| s_in.value()) + .sum::>() + .ok_or_else(overflow)?; + let sapling_out = sapling_outputs + .iter() + .map(|s_out| s_out.value()) + .sum::>() + .ok_or_else(overflow)?; + + let fee_amount = self + .fee_rule + .fee_required( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs.len(), + sapling_outputs.len() + 1, + ) + .map_err(ChangeError::StrategyError)?; + + let total_in = (t_in + sapling_in).ok_or_else(overflow)?; + + let total_out = [t_out, sapling_out, fee_amount] + .iter() + .sum::>() + .ok_or_else(overflow)?; + + let proposed_change = (total_in - total_out).ok_or_else(underflow)?; + match proposed_change.cmp(&Amount::zero()) { + Ordering::Less => Err(ChangeError::InsufficientFunds { + available: total_in, + required: total_out, + }), + Ordering::Equal => TransactionBalance::new(vec![], fee_amount).ok_or_else(overflow), + Ordering::Greater => { + let dust_threshold = dust_output_policy + .dust_threshold() + .unwrap_or_else(|| self.fee_rule.marginal_fee()); + + if dust_threshold > proposed_change { + match dust_output_policy.action() { + DustAction::Reject => { + let shortfall = + (dust_threshold - proposed_change).ok_or_else(underflow)?; + + Err(ChangeError::InsufficientFunds { + available: total_in, + required: (total_in + shortfall).ok_or_else(overflow)?, + }) + } + DustAction::AllowDustChange => TransactionBalance::new( + vec![ChangeValue::Sapling(proposed_change)], + fee_amount, + ) + .ok_or_else(overflow), + DustAction::AddDustToFee => { + TransactionBalance::new(vec![], (fee_amount + proposed_change).unwrap()) + .ok_or_else(overflow) + } + } + } else { + TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount) + .ok_or_else(overflow) + } + } + } + } +} + +#[cfg(test)] +mod tests { + + use zcash_primitives::{ + consensus::{Network, NetworkUpgrade, Parameters}, + transaction::{ + components::{amount::Amount, transparent::TxOut}, + fees::zip317::FeeRule as Zip317FeeRule, + }, + }; + + use super::SingleOutputChangeStrategy; + use crate::{ + data_api::wallet::input_selection::SaplingPayment, + fees::{ + tests::{TestSaplingInput, TestTransparentInput}, + ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, + }, + }; + + #[test] + fn change_without_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[TestSaplingInput { + note_id: 0, + value: Amount::from_u64(55000).unwrap(), + }], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(Amount::from_u64(5000).unwrap())] + && balance.fee_required() == Amount::from_u64(10000).unwrap() + ); + } + + #[test] + fn change_with_allowable_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[ + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(49000).unwrap(), + }, + TestSaplingInput { + note_id: 1, + value: Amount::from_u64(1000).unwrap(), + }, + ], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + assert_matches!( + result, + Ok(balance) if balance.proposed_change().is_empty() + && balance.fee_required() == Amount::from_u64(10000).unwrap() + ); + } + + #[test] + fn change_with_disallowed_dust() { + let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard()); + + // spend a single Sapling note that is sufficient to pay the fee + let result = change_strategy.compute_balance( + &Network::TestNetwork, + Network::TestNetwork + .activation_height(NetworkUpgrade::Nu5) + .unwrap(), + &Vec::::new(), + &Vec::::new(), + &[ + TestSaplingInput { + note_id: 0, + value: Amount::from_u64(29000).unwrap(), + }, + TestSaplingInput { + note_id: 1, + value: Amount::from_u64(20000).unwrap(), + }, + TestSaplingInput { + note_id: 2, + value: Amount::from_u64(1000).unwrap(), + }, + ], + &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &DustOutputPolicy::default(), + ); + + // We will get an error here, because the dust input now isn't free to add + // to the transaction. + assert_matches!( + result, + Err(ChangeError::DustInputs { sapling, .. }) if sapling == vec![2] + ); + } +} diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index c099798f7..23e87c5de 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/zcash_client_sqlite/src/wallet/transact.rs @@ -193,17 +193,23 @@ mod tests { consensus::{BlockHeight, BranchId}, legacy::TransparentAddress, sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver}, - transaction::{components::Amount, Transaction}, + transaction::{components::Amount, fees::zip317::FeeRule as Zip317FeeRule, Transaction}, zip32::sapling::ExtendedSpendingKey, }; use zcash_client_backend::{ + address::RecipientAddress, data_api::{ - self, chain::scan_cached_blocks, wallet::create_spend_to_address, WalletRead, - WalletWrite, + self, + chain::scan_cached_blocks, + error::Error, + wallet::{create_spend_to_address, input_selection::GreedyInputSelector, spend}, + WalletRead, WalletWrite, }, + fees::{zip317, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::OvkPolicy, + zip321::{Payment, TransactionRequest}, }; use crate::{ @@ -832,4 +838,113 @@ mod tests { Ok(_) ); } + + #[test] + fn zip317_spend() { + let cache_file = NamedTempFile::new().unwrap(); + let db_cache = BlockDb(Connection::open(cache_file.path()).unwrap()); + init_cache_database(&db_cache).unwrap(); + + let data_file = NamedTempFile::new().unwrap(); + let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap(); + init_wallet_db(&mut db_data, None).unwrap(); + + // Add an account to the wallet + let mut ops = db_data.get_update_ops().unwrap(); + let seed = Secret::new([0u8; 32].to_vec()); + let (_, usk) = ops.create_account(&seed).unwrap(); + let dfvk = usk.sapling().to_diversifiable_full_viewing_key(); + + // Add funds to the wallet + let (cb, _) = fake_compact_block( + sapling_activation_height(), + BlockHash([0; 32]), + &dfvk, + AddressType::Internal, + Amount::from_u64(50000).unwrap(), + ); + insert_into_cache(&db_cache, &cb); + + // Add 10 dust notes to the wallet + for i in 1..=10 { + let (cb, _) = fake_compact_block( + sapling_activation_height() + i, + cb.hash(), + &dfvk, + AddressType::DefaultExternal, + Amount::from_u64(1000).unwrap(), + ); + insert_into_cache(&db_cache, &cb); + } + + let mut db_write = db_data.get_update_ops().unwrap(); + scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); + + // Verified balance matches total balance + let total = Amount::from_u64(60000).unwrap(); + let (_, anchor_height) = db_data.get_target_and_anchor_heights(1).unwrap().unwrap(); + assert_eq!(get_balance(&db_data, AccountId::from(0)).unwrap(), total); + assert_eq!( + get_balance_at(&db_data, AccountId::from(0), anchor_height).unwrap(), + total + ); + + let input_selector = GreedyInputSelector::new( + zip317::SingleOutputChangeStrategy::new(Zip317FeeRule::standard()), + DustOutputPolicy::default(), + ); + + // This first request will fail due to insufficient non-dust funds + let req = TransactionRequest::new(vec![Payment { + recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), + amount: Amount::from_u64(50000).unwrap(), + memo: None, + label: None, + message: None, + other_params: vec![], + }]) + .unwrap(); + + assert_matches!( + spend( + &mut db_write, + &tests::network(), + test_prover(), + &input_selector, + &usk, + req, + OvkPolicy::Sender, + 1, + ), + Err(Error::InsufficientFunds { available, required }) + if available == Amount::from_u64(51000).unwrap() + && required == Amount::from_u64(60000).unwrap() + ); + + // This request will succeed, spending a single dust input to pay the 10000 + // ZAT fee in addition to the 41000 ZAT output to the recipient + let req = TransactionRequest::new(vec![Payment { + recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), + amount: Amount::from_u64(41000).unwrap(), + memo: None, + label: None, + message: None, + other_params: vec![], + }]) + .unwrap(); + + assert_matches!( + spend( + &mut db_write, + &tests::network(), + test_prover(), + &input_selector, + &usk, + req, + OvkPolicy::Sender, + 1, + ), + Ok(_) + ); + } } diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 10b701eeb..5a9bae7ca 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -27,8 +27,10 @@ and this library adheres to Rust's notion of and types related to fee calculations. - `FeeRule` a trait that describes how to compute the fee required for a transaction given inputs and outputs to the transaction. - - `zcash_primitives::transaction::fees::fixed` a new module containing an implementation - of the existing fixed fee rule. + - `fixed`, a new module containing an implementation of the existing fixed + fee rule. + - `zip317`, a new module containing an implementation of the ZIP 317 fee + rules. - Added to `zcash_primitives::transaction::components::sapling::builder` - `SaplingBuilder::{inputs, outputs}`: accessors for Sapling builder state. - `zcash_primitives::transaction::components::sapling::fees` diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 11881f0e6..532e225f8 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -1,6 +1,6 @@ use std::convert::TryFrom; use std::iter::Sum; -use std::ops::{Add, AddAssign, Neg, Sub, SubAssign}; +use std::ops::{Add, AddAssign, Mul, Neg, Sub, SubAssign}; use memuse::DynamicUsage; use orchard::value as orchard; @@ -205,6 +205,17 @@ impl Neg for Amount { } } +impl Mul for Amount { + type Output = Option; + + fn mul(self, rhs: usize) -> Option { + let rhs: i64 = rhs.try_into().ok()?; + self.0 + .checked_mul(rhs) + .and_then(|i| Amount::try_from(i).ok()) + } +} + impl TryFrom for Amount { type Error = (); diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index 2a9a9021a..e29913acc 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -9,6 +9,7 @@ use crate::{ use crate::transaction::components::tze::fees as tze; pub mod fixed; +pub mod zip317; /// A trait that represents the ability to compute the fees that must be paid /// by a transaction having a specified set of inputs and outputs. diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs new file mode 100644 index 000000000..6f5218129 --- /dev/null +++ b/zcash_primitives/src/transaction/fees/zip317.rs @@ -0,0 +1,139 @@ +//! Types related to implementing a [`FeeRule`] provides [ZIP 317] fee calculation. +//! +//! [`FeeRule`]: crate::transaction::fees::FeeRule +//! [ZIP 317]: https//zips.z.cash/zip-0317 +use core::cmp::max; + +use crate::{ + consensus::{self, BlockHeight}, + legacy::TransparentAddress, + transaction::components::{ + amount::{Amount, BalanceError}, + transparent::{fees as transparent, OutPoint}, + }, +}; + +/// A [`FeeRule`] implementation that implements the [ZIP 317] fee rule. +/// +/// This fee rule supports only P2pkh transparent inputs; an error will be returned if a coin +/// containing a non-p2pkh script is provided as an input. This fee rule may slightly overestimate +/// fees in case where the user is attempting to spend more than ~150 transparent inputs. +/// +/// [`FeeRule`]: crate::transaction::fees::FeeRule +/// [ZIP 317]: https//zips.z.cash/zip-0317 +#[derive(Clone, Debug)] +pub struct FeeRule { + marginal_fee: Amount, + grace_actions: usize, + p2pkh_standard_input_size: usize, + p2pkh_standard_output_size: usize, +} + +impl FeeRule { + /// Construct a new FeeRule using the standard [ZIP 317] constants. + /// + /// [ZIP 317]: https//zips.z.cash/zip-0317 + pub fn standard() -> Self { + Self { + marginal_fee: Amount::from_u64(5000).unwrap(), + grace_actions: 2, + p2pkh_standard_input_size: 150, + p2pkh_standard_output_size: 34, + } + } + + /// Construct a new FeeRule instance with the specified parameter values. + /// + /// Returns `None` if either `p2pkh_standard_input_size` or `p2pkh_standard_output_size` are + /// zero. + pub fn non_standard( + marginal_fee: Amount, + grace_actions: usize, + p2pkh_standard_input_size: usize, + p2pkh_standard_output_size: usize, + ) -> Option { + if p2pkh_standard_input_size == 0 || p2pkh_standard_output_size == 0 { + None + } else { + Some(Self { + marginal_fee, + grace_actions, + p2pkh_standard_input_size, + p2pkh_standard_output_size, + }) + } + } + + /// Returns the ZIP 317 marginal fee. + pub fn marginal_fee(&self) -> Amount { + self.marginal_fee + } + /// Returns the ZIP 317 number of grace actions + pub fn grace_actions(&self) -> usize { + self.grace_actions + } + /// Returns the ZIP 317 standard P2PKH input size + pub fn p2pkh_standard_input_size(&self) -> usize { + self.p2pkh_standard_input_size + } + /// Returns the ZIP 317 standard P2PKH output size + pub fn p2pkh_standard_output_size(&self) -> usize { + self.p2pkh_standard_output_size + } +} + +/// Errors that can occur in ZIP 317 fee computation +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum FeeError { + /// An overflow or underflow of amount computation occurred. + Balance(BalanceError), + /// Transparent inputs provided to the fee calculation included coins that do not pay to + /// standard P2pkh scripts. + NonP2pkhInputs(Vec), +} + +impl From for FeeError { + fn from(err: BalanceError) -> Self { + FeeError::Balance(err) + } +} + +impl super::FeeRule for FeeRule { + type Error = FeeError; + + fn fee_required( + &self, + _params: &P, + _target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_input_count: usize, + sapling_output_count: usize, + ) -> Result { + let non_p2pkh_inputs: Vec<_> = transparent_inputs + .iter() + .filter_map(|t_in| match t_in.coin().script_pubkey.address() { + Some(TransparentAddress::PublicKey(_)) => None, + _ => Some(t_in.outpoint()), + }) + .cloned() + .collect(); + + if !non_p2pkh_inputs.is_empty() { + return Err(FeeError::NonP2pkhInputs(non_p2pkh_inputs)); + } + + let t_in_total_size = transparent_inputs.len() * 150; + let t_out_total_size = transparent_outputs.len() * 34; + + let ceildiv = |num: usize, den: usize| (num + den - 1) / den; + + let logical_actions = max( + ceildiv(t_in_total_size, self.p2pkh_standard_input_size), + ceildiv(t_out_total_size, self.p2pkh_standard_output_size), + ) + max(sapling_input_count, sapling_output_count); + + (self.marginal_fee * max(self.grace_actions, logical_actions)) + .ok_or_else(|| BalanceError::Overflow.into()) + } +} From 796b5a4fed8a0afc542ccaf4185416bc4e056d7e Mon Sep 17 00:00:00 2001 From: str4d Date: Fri, 11 Nov 2022 00:58:31 +0000 Subject: [PATCH 4/4] ZIP 317: Improve code comments --- zcash_client_backend/src/fees/zip317.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index fb03298bf..174671eb1 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -103,8 +103,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { let mut s_disallowed_dust = sapling_dust.len().saturating_sub(s_allowed_dust); if available_grace_inputs > 0 { - // if we have available grace inputs, allocate them first to transparent dust - // and then to sapling dust + // If we have available grace inputs, allocate them first to transparent dust + // and then to Sapling dust. The caller has provided inputs that it is willing + // to spend, so we don't need to consider privacy effects at this layer. let t_grace_dust = available_grace_inputs.saturating_sub(t_disallowed_dust); t_disallowed_dust = t_disallowed_dust.saturating_sub(t_grace_dust); @@ -114,9 +115,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { s_disallowed_dust = s_disallowed_dust.saturating_sub(s_grace_dust); } - // truncate the lists of inputs to be disregarded in input selection to just the - // disallowed lengths this has the effect of prioritizing inputs for inclusion by the - // order of th original input slices, with the most preferred inputs first + // Truncate the lists of inputs to be disregarded in input selection to just the + // disallowed lengths. This has the effect of prioritizing inputs for inclusion by the + // order of the original input slices, with the most preferred inputs first. transparent_dust.reverse(); transparent_dust.truncate(t_disallowed_dust); sapling_dust.reverse();