From 56f2ac573c48a5551f68c39461513894d9920019 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 Dec 2023 14:45:22 -0700 Subject: [PATCH 1/3] zcash_client_backend: Add Orchard support to change strategies. This modifies the `compute_balance` method to operate in a bundle-oriented fashion, which simplifies the API and makes it easier to elide Orchard functionality in the case that the `orchard` feature is not enabled. --- zcash_client_backend/CHANGELOG.md | 11 +- zcash_client_backend/src/data_api/wallet.rs | 23 ++- .../src/data_api/wallet/input_selection.rs | 77 +++++++-- zcash_client_backend/src/fees.rs | 69 ++++++-- zcash_client_backend/src/fees/common.rs | 102 +++++++++--- zcash_client_backend/src/fees/fixed.rs | 79 ++++++---- zcash_client_backend/src/fees/orchard.rs | 76 +++++++++ zcash_client_backend/src/fees/sapling.rs | 37 ++++- zcash_client_backend/src/fees/standard.rs | 25 +-- zcash_client_backend/src/fees/zip317.rs | 149 +++++++++++------- zcash_client_backend/src/proto.rs | 11 +- zcash_primitives/CHANGELOG.md | 2 + .../src/transaction/components/amount.rs | 14 ++ 13 files changed, 522 insertions(+), 153 deletions(-) create mode 100644 zcash_client_backend/src/fees/orchard.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 2780788b2..c2fa863e2 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -27,7 +27,8 @@ and this library adheres to Rust's notion of - `wallet::input_selection::ShieldingSelector` has been factored out from the `InputSelector` trait to separate out transparent functionality and move it behind the `transparent-inputs` feature flag. -- `zcash_client_backend::fees::{standard, sapling}` +- `zcash_client_backend::fees::{standard, orchard, sapling}` +- `zcash_client_backend::fees::ChangeValue::{new, orchard}` - `zcash_client_backend::wallet`: - `Note` - `ReceivedNote` @@ -141,9 +142,10 @@ and this library adheres to Rust's notion of for its `InputSource` associated type. - `zcash_client_backend::fees`: - - `ChangeValue::Sapling` is now a structured variant. In addition to the - existing change value, it now also carries an optional memo to be associated - with the change output. + - `ChangeStrategy::compute_balance` arguments have changed. + - `ChangeValue` is now a struct. In addition to the existing change value, it + now also provides the output pool to which change should be sent and an + optional memo to be associated with the change output. - `fixed::SingleOutputChangeStrategy::new` and `zip317::SingleOutputChangeStrategy::new` each now accept an additional `change_memo` argument. @@ -157,7 +159,6 @@ and this library adheres to Rust's notion of - `error::Error::InsufficientFunds.{available, required}` - `wallet::input_selection::InputSelectorError::InsufficientFunds.{available, required}` - `zcash_client_backend::fees`: - - `ChangeValue::Sapling.value` - `ChangeError::InsufficientFunds.{available, required}` - `zcash_client_backend::zip321::Payment.amount` - The following methods now take `NonNegativeAmount` instead of `Amount`: diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 24e10367c..20c46ec62 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -24,7 +24,7 @@ use crate::{ SentTransactionOutput, WalletCommitmentTrees, WalletRead, WalletWrite, }, decrypt_transaction, - fees::{self, ChangeValue, DustOutputPolicy}, + fees::{self, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::{Note, OvkPolicy, Recipient}, zip321::{self, Payment}, @@ -718,13 +718,15 @@ where } for change_value in proposal.balance().proposed_change() { - match change_value { - ChangeValue::Sapling { value, memo } => { - let memo = memo.as_ref().map_or_else(MemoBytes::empty, |m| m.clone()); + let memo = change_value + .memo() + .map_or_else(MemoBytes::empty, |m| m.clone()); + match change_value.output_pool() { + ShieldedProtocol::Sapling => { builder.add_sapling_output( internal_ovk(), dfvk.change_address().1, - *value, + change_value.value(), memo.clone(), )?; sapling_output_meta.push(( @@ -732,10 +734,19 @@ where account, PoolType::Shielded(ShieldedProtocol::Sapling), ), - *value, + change_value.value(), Some(memo), )) } + ShieldedProtocol::Orchard => { + #[cfg(not(feature = "orchard"))] + return Err(Error::UnsupportedPoolType(PoolType::Shielded( + ShieldedProtocol::Orchard, + ))); + + #[cfg(feature = "orchard")] + unimplemented!("FIXME: implement Orchard change output creation.") + } } } 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 c2973341f..550b96f3b 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -27,10 +27,10 @@ use crate::{ }; #[cfg(feature = "transparent-inputs")] -use { - std::collections::BTreeSet, std::convert::Infallible, - zcash_primitives::transaction::components::OutPoint, -}; +use {std::collections::BTreeSet, zcash_primitives::transaction::components::OutPoint}; + +#[cfg(feature = "orchard")] +use {crate::fees::orchard as orchard_fees, std::convert::Infallible}; /// The type of errors that may be produced in input selection. pub enum InputSelectorError { @@ -447,6 +447,23 @@ impl sapling::OutputView for SaplingPayment { } } +pub(crate) struct OrchardPayment(NonNegativeAmount); + +// TODO: introduce this method when it is needed for testing. +// #[cfg(test)] +// impl OrchardPayment { +// pub(crate) fn new(amount: NonNegativeAmount) -> Self { +// OrchardPayment(amount) +// } +// } + +#[cfg(feature = "orchard")] +impl orchard_fees::OutputView for OrchardPayment { + fn value(&self) -> NonNegativeAmount { + self.0 + } +} + /// An [`InputSelector`] implementation that uses a greedy strategy to select between available /// notes. /// @@ -499,6 +516,7 @@ where { let mut transparent_outputs = vec![]; let mut sapling_outputs = vec![]; + let mut orchard_outputs = vec![]; for payment in transaction_request.payments() { let mut push_transparent = |taddr: TransparentAddress| { transparent_outputs.push(TxOut { @@ -509,6 +527,9 @@ where let mut push_sapling = || { sapling_outputs.push(SaplingPayment(payment.amount)); }; + let mut push_orchard = || { + orchard_outputs.push(OrchardPayment(payment.amount)); + }; match &payment.recipient_address { Address::Transparent(addr) => { @@ -518,7 +539,14 @@ where push_sapling(); } Address::Unified(addr) => { - if addr.sapling().is_some() { + #[cfg(feature = "orchard")] + let has_orchard = addr.orchard().is_some(); + #[cfg(not(feature = "orchard"))] + let has_orchard = false; + + if has_orchard { + push_orchard(); + } else if addr.sapling().is_some() { push_sapling(); } else if let Some(addr) = addr.transparent() { push_transparent(*addr); @@ -544,8 +572,17 @@ where target_height, &Vec::::new(), &transparent_outputs, - &sapling_inputs, - &sapling_outputs, + &( + ::sapling::builder::BundleType::DEFAULT, + &sapling_inputs[..], + &sapling_outputs[..], + ), + #[cfg(feature = "orchard")] + &( + ::orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &orchard_outputs[..], + ), &self.dust_output_policy, ); @@ -652,8 +689,17 @@ where target_height, &transparent_inputs, &Vec::::new(), - &Vec::>::new(), - &Vec::::new(), + &( + ::sapling::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &self.dust_output_policy, ); @@ -668,8 +714,17 @@ where target_height, &transparent_inputs, &Vec::::new(), - &Vec::>::new(), - &Vec::::new(), + &( + ::sapling::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &self.dust_output_policy, )? } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 1aca511d2..25f2f9402 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -12,38 +12,69 @@ use zcash_primitives::{ }, }; +use crate::ShieldedProtocol; + pub(crate) mod common; pub mod fixed; +pub mod orchard; pub mod sapling; pub mod standard; pub mod zip317; /// A proposed change amount and output pool. #[derive(Clone, Debug, PartialEq, Eq)] -pub enum ChangeValue { - Sapling { - value: NonNegativeAmount, - memo: Option, - }, +pub struct ChangeValue { + output_pool: ShieldedProtocol, + value: NonNegativeAmount, + memo: Option, } impl ChangeValue { + /// Constructs a new change value from its constituent parts. + pub fn new( + output_pool: ShieldedProtocol, + value: NonNegativeAmount, + memo: Option, + ) -> Self { + Self { + output_pool, + value, + memo, + } + } + + /// Constructs a new change value that will be created as a Sapling output. pub fn sapling(value: NonNegativeAmount, memo: Option) -> Self { - Self::Sapling { value, memo } + Self { + output_pool: ShieldedProtocol::Sapling, + value, + memo, + } + } + + /// Constructs a new change value that will be created as an Orchard output. + #[cfg(feature = "orchard")] + pub fn orchard(value: NonNegativeAmount, memo: Option) -> Self { + Self { + output_pool: ShieldedProtocol::Orchard, + value, + memo, + } + } + + /// Returns the pool to which the change output should be sent. + pub fn output_pool(&self) -> ShieldedProtocol { + self.output_pool } /// Returns the value of the change output to be created, in zatoshis. pub fn value(&self) -> NonNegativeAmount { - match self { - ChangeValue::Sapling { value, .. } => *value, - } + self.value } /// Returns the memo to be associated with the change output. pub fn memo(&self) -> Option<&MemoBytes> { - match self { - ChangeValue::Sapling { memo, .. } => memo.as_ref(), - } + self.memo.as_ref() } } @@ -120,6 +151,8 @@ pub enum ChangeError { }, /// An error occurred that was specific to the change selection strategy in use. StrategyError(E), + /// The proposed bundle structure would violate bundle type construction rules. + BundleError(&'static str), } impl ChangeError { @@ -140,6 +173,7 @@ impl ChangeError { sapling, }, ChangeError::StrategyError(e) => ChangeError::StrategyError(f(e)), + ChangeError::BundleError(e) => ChangeError::BundleError(e), } } } @@ -167,6 +201,13 @@ impl fmt::Display for ChangeError { ChangeError::StrategyError(err) => { write!(f, "{}", err) } + ChangeError::BundleError(err) => { + write!( + f, + "The proposed transaction structure violates bundle type constrints: {}", + err + ) + } } } } @@ -252,8 +293,8 @@ pub trait ChangeStrategy { target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], + sapling: &impl sapling::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView, dust_output_policy: &DustOutputPolicy, ) -> Result>; } diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 4368bda4d..63a5a1280 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -7,7 +7,15 @@ use zcash_primitives::{ }, }; -use super::{sapling, ChangeError, ChangeValue, DustAction, DustOutputPolicy, TransactionBalance}; +use crate::ShieldedProtocol; + +use super::{ + sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy, + TransactionBalance, +}; + +#[cfg(feature = "orchard")] +use super::orchard as orchard_fees; #[allow(clippy::too_many_arguments)] pub(crate) fn single_change_output_balance< @@ -21,8 +29,8 @@ pub(crate) fn single_change_output_balance< target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], + sapling: &impl sapling_fees::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, default_dust_threshold: NonNegativeAmount, change_memo: Option, @@ -43,40 +51,88 @@ where .map(|t_out| t_out.value()) .sum::>() .ok_or_else(overflow)?; - let sapling_in = sapling_inputs + let sapling_in = sapling + .inputs() .iter() - .map(|s_in| s_in.value()) + .map(sapling_fees::InputView::::value) .sum::>() .ok_or_else(overflow)?; - let sapling_out = sapling_outputs + let sapling_out = sapling + .outputs() .iter() - .map(|s_out| s_out.value()) + .map(sapling_fees::OutputView::value) .sum::>() .ok_or_else(overflow)?; + #[cfg(feature = "orchard")] + let orchard_in = orchard + .inputs() + .iter() + .map(orchard_fees::InputView::::value) + .sum::>() + .ok_or_else(overflow)?; + #[cfg(not(feature = "orchard"))] + let orchard_in = NonNegativeAmount::ZERO; + + #[cfg(feature = "orchard")] + let orchard_out = orchard + .outputs() + .iter() + .map(orchard_fees::OutputView::value) + .sum::>() + .ok_or_else(overflow)?; + #[cfg(not(feature = "orchard"))] + let orchard_out = NonNegativeAmount::ZERO; + + // TODO: implement a less naive strategy for selecting the pool to which change will be sent. + #[cfg(feature = "orchard")] + let (change_pool, sapling_change, orchard_change) = + if orchard_in > NonNegativeAmount::ZERO || orchard_out > NonNegativeAmount::ZERO { + // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs + (ShieldedProtocol::Orchard, 0, 1) + } else if sapling_in > NonNegativeAmount::ZERO || sapling_out > NonNegativeAmount::ZERO { + // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any + // Sapling outputs, so that we avoid pool-crossing. + (ShieldedProtocol::Sapling, 1, 0) + } else { + // For all other transactions, send change to Sapling. + // FIXME: Change this to Orchard once Orchard outputs are enabled. + (ShieldedProtocol::Sapling, 1, 0) + }; + #[cfg(not(feature = "orchard"))] + let (change_pool, sapling_change) = (ShieldedProtocol::Sapling, 1); + + #[cfg(feature = "orchard")] + let orchard_num_actions = orchard + .bundle_type() + .num_actions( + orchard.inputs().len(), + orchard.outputs().len() + orchard_change, + ) + .map_err(ChangeError::BundleError)?; + #[cfg(not(feature = "orchard"))] + let orchard_num_actions = 0; + let fee_amount = fee_rule .fee_required( params, target_height, transparent_inputs, transparent_outputs, - sapling_inputs.len(), - if sapling_inputs.is_empty() { - sapling_outputs.len() + 1 - } else { - std::cmp::max(sapling_outputs.len() + 1, 2) - }, - //Orchard is not yet supported in zcash_client_backend - 0, + sapling.inputs().len(), + sapling + .bundle_type() + .num_outputs( + sapling.inputs().len(), + sapling.outputs().len() + sapling_change, + ) + .map_err(ChangeError::BundleError)?, + orchard_num_actions, ) .map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?; - 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 total_in = (t_in + sapling_in + orchard_in).ok_or_else(overflow)?; + let total_out = (t_out + sapling_out + orchard_out + fee_amount).ok_or_else(overflow)?; let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds { available: total_in, @@ -101,7 +157,7 @@ where }) } DustAction::AllowDustChange => TransactionBalance::new( - vec![ChangeValue::sapling(proposed_change, change_memo)], + vec![ChangeValue::new(change_pool, proposed_change, change_memo)], fee_amount, ) .map_err(|_| overflow()), @@ -113,7 +169,7 @@ where } } else { TransactionBalance::new( - vec![ChangeValue::sapling(proposed_change, change_memo)], + vec![ChangeValue::new(change_pool, proposed_change, change_memo)], fee_amount, ) .map_err(|_| overflow()) diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index b17863ec2..ec4fef86d 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -10,10 +10,13 @@ use zcash_primitives::{ }; use super::{ - common::single_change_output_balance, sapling, ChangeError, ChangeStrategy, DustOutputPolicy, - TransactionBalance, + common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, + DustOutputPolicy, TransactionBalance, }; +#[cfg(feature = "orchard")] +use super::orchard as orchard_fees; + /// 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 { @@ -46,8 +49,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], + sapling: &impl sapling_fees::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, ) -> Result> { single_change_output_balance( @@ -56,8 +59,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height, transparent_inputs, transparent_outputs, - sapling_inputs, - sapling_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, dust_output_policy, self.fee_rule().fixed_fee(), self.change_memo.clone(), @@ -67,6 +71,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(test)] mod tests { + #[cfg(feature = "orchard")] + use std::convert::Infallible; + use zcash_primitives::{ consensus::{Network, NetworkUpgrade, Parameters}, transaction::{ @@ -98,13 +105,22 @@ mod tests { .unwrap(), &Vec::::new(), &Vec::::new(), - &[TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(60000), - }], - &[SaplingPayment::new(NonNegativeAmount::const_from_u64( - 40000, - ))], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(60000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))][..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &[] as &[Infallible], + &[] as &[Infallible], + ), &DustOutputPolicy::default(), ); @@ -130,20 +146,29 @@ mod tests { .unwrap(), &Vec::::new(), &Vec::::new(), - &[ - TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(40000), - }, - // enough to pay a fee, plus dust - TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(10100), - }, - ], - &[SaplingPayment::new(NonNegativeAmount::const_from_u64( - 40000, - ))], + &( + sapling::builder::BundleType::DEFAULT, + &[ + TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(40000), + }, + // enough to pay a fee, plus dust + TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(10100), + }, + ][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))][..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &[] as &[Infallible], + &[] as &[Infallible], + ), &DustOutputPolicy::default(), ); diff --git a/zcash_client_backend/src/fees/orchard.rs b/zcash_client_backend/src/fees/orchard.rs new file mode 100644 index 000000000..790bdff78 --- /dev/null +++ b/zcash_client_backend/src/fees/orchard.rs @@ -0,0 +1,76 @@ +//! Types related to computation of fees and change related to the Orchard components +//! of a transaction. + +use std::convert::Infallible; +use zcash_primitives::transaction::components::amount::NonNegativeAmount; + +#[cfg(feature = "orchard")] +use orchard::builder::BundleType; + +/// A trait that provides a minimized view of Orchard bundle configuration +/// suitable for use in fee and change calculation. +#[cfg(feature = "orchard")] +pub trait BundleView { + /// The type of inputs to the bundle. + type In: InputView; + /// The type of inputs of the bundle. + type Out: OutputView; + + /// Returns the type of the bundle + fn bundle_type(&self) -> BundleType; + /// Returns the inputs to the bundle. + fn inputs(&self) -> &[Self::In]; + /// Returns the outputs of the bundle. + fn outputs(&self) -> &[Self::Out]; +} + +#[cfg(feature = "orchard")] +impl<'a, NoteRef, In: InputView, Out: OutputView> BundleView + for (BundleType, &'a [In], &'a [Out]) +{ + type In = In; + type Out = Out; + + fn bundle_type(&self) -> BundleType { + self.0 + } + + fn inputs(&self) -> &[In] { + self.1 + } + + fn outputs(&self) -> &[Out] { + self.2 + } +} + +/// A trait that provides a minimized view of an Orchard input suitable for use in fee and change +/// calculation. +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) -> NonNegativeAmount; +} + +impl InputView for Infallible { + fn note_id(&self) -> &N { + unreachable!() + } + fn value(&self) -> NonNegativeAmount { + unreachable!() + } +} + +/// A trait that provides a minimized view of a Orchard output suitable for use in fee and change +/// calculation. +pub trait OutputView { + /// The value of the output being produced. + fn value(&self) -> NonNegativeAmount; +} + +impl OutputView for Infallible { + fn value(&self) -> NonNegativeAmount { + unreachable!() + } +} diff --git a/zcash_client_backend/src/fees/sapling.rs b/zcash_client_backend/src/fees/sapling.rs index df2702b19..2653310a9 100644 --- a/zcash_client_backend/src/fees/sapling.rs +++ b/zcash_client_backend/src/fees/sapling.rs @@ -3,9 +3,44 @@ use std::convert::Infallible; -use sapling::builder::{OutputInfo, SpendInfo}; +use sapling::builder::{BundleType, OutputInfo, SpendInfo}; use zcash_primitives::transaction::components::amount::NonNegativeAmount; +/// A trait that provides a minimized view of Sapling bundle configuration +/// suitable for use in fee and change calculation. +pub trait BundleView { + /// The type of inputs to the bundle. + type In: InputView; + /// The type of inputs of the bundle. + type Out: OutputView; + + /// Returns the type of the bundle + fn bundle_type(&self) -> BundleType; + /// Returns the inputs to the bundle. + fn inputs(&self) -> &[Self::In]; + /// Returns the outputs of the bundle. + fn outputs(&self) -> &[Self::Out]; +} + +impl<'a, NoteRef, In: InputView, Out: OutputView> BundleView + for (BundleType, &'a [In], &'a [Out]) +{ + type In = In; + type Out = Out; + + fn bundle_type(&self) -> BundleType { + self.0 + } + + fn inputs(&self) -> &[In] { + self.1 + } + + fn outputs(&self) -> &[Out] { + self.2 + } +} + /// A trait that provides a minimized view of a Sapling input suitable for use in /// fee and change calculation. pub trait InputView { diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index 7dbd740ef..d9d40b1c3 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -15,9 +15,13 @@ use zcash_primitives::{ }; use super::{ - fixed, sapling, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance, + fixed, sapling as sapling_fees, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, + TransactionBalance, }; +#[cfg(feature = "orchard")] +use super::orchard as orchard_fees; + /// A change strategy that 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 { @@ -50,8 +54,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], + sapling: &impl sapling_fees::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, ) -> Result> { #[allow(deprecated)] @@ -65,8 +69,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height, transparent_inputs, transparent_outputs, - sapling_inputs, - sapling_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, dust_output_policy, ) .map_err(|e| e.map(Zip317FeeError::Balance)), @@ -79,8 +84,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height, transparent_inputs, transparent_outputs, - sapling_inputs, - sapling_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, dust_output_policy, ) .map_err(|e| e.map(Zip317FeeError::Balance)), @@ -93,8 +99,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height, transparent_inputs, transparent_outputs, - sapling_inputs, - sapling_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, dust_output_policy, ), } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index ab88bcd26..fe08501e6 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -14,10 +14,13 @@ use zcash_primitives::{ }; use super::{ - common::single_change_output_balance, sapling, ChangeError, ChangeStrategy, DustOutputPolicy, - TransactionBalance, + common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy, + DustOutputPolicy, TransactionBalance, }; +#[cfg(feature = "orchard")] +use super::orchard as orchard_fees; + /// 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 { @@ -50,8 +53,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height: BlockHeight, transparent_inputs: &[impl transparent::InputView], transparent_outputs: &[impl transparent::OutputView], - sapling_inputs: &[impl sapling::InputView], - sapling_outputs: &[impl sapling::OutputView], + sapling: &impl sapling_fees::BundleView, + #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView, dust_output_policy: &DustOutputPolicy, ) -> Result> { let mut transparent_dust: Vec<_> = transparent_inputs @@ -67,11 +70,12 @@ impl ChangeStrategy for SingleOutputChangeStrategy { }) .collect(); - let mut sapling_dust: Vec<_> = sapling_inputs + let mut sapling_dust: Vec<_> = sapling + .inputs() .iter() .filter_map(|i| { - if i.value() < self.fee_rule.marginal_fee() { - Some(i.note_id().clone()) + if sapling_fees::InputView::::value(i) < self.fee_rule.marginal_fee() { + Some(sapling_fees::InputView::::note_id(i).clone()) } else { None } @@ -88,8 +92,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { // 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 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 @@ -135,8 +139,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy { target_height, transparent_inputs, transparent_outputs, - sapling_inputs, - sapling_outputs, + sapling, + #[cfg(feature = "orchard")] + orchard, dust_output_policy, self.fee_rule.marginal_fee(), self.change_memo.clone(), @@ -147,6 +152,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy { #[cfg(test)] mod tests { + use std::convert::Infallible; + use zcash_primitives::{ consensus::{Network, NetworkUpgrade, Parameters}, legacy::Script, @@ -177,13 +184,22 @@ mod tests { .unwrap(), &Vec::::new(), &Vec::::new(), - &[TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(55000), - }], - &[SaplingPayment::new(NonNegativeAmount::const_from_u64( - 40000, - ))], + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(55000), + }][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))][..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &DustOutputPolicy::default(), ); @@ -210,11 +226,20 @@ mod tests { value: NonNegativeAmount::const_from_u64(40000), script_pubkey: Script(vec![]), }], - &[TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(55000), - }], - &Vec::::new(), + &( + sapling::builder::BundleType::DEFAULT, + &[TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(55000), + }][..], + &Vec::::new()[..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &DustOutputPolicy::default(), ); @@ -237,19 +262,28 @@ mod tests { .unwrap(), &Vec::::new(), &Vec::::new(), - &[ - TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(49000), - }, - TestSaplingInput { - note_id: 1, - value: NonNegativeAmount::const_from_u64(1000), - }, - ], - &[SaplingPayment::new(NonNegativeAmount::const_from_u64( - 40000, - ))], + &( + sapling::builder::BundleType::DEFAULT, + &[ + TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(49000), + }, + TestSaplingInput { + note_id: 1, + value: NonNegativeAmount::const_from_u64(1000), + }, + ][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))][..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &DustOutputPolicy::default(), ); @@ -272,23 +306,32 @@ mod tests { .unwrap(), &Vec::::new(), &Vec::::new(), - &[ - TestSaplingInput { - note_id: 0, - value: NonNegativeAmount::const_from_u64(29000), - }, - TestSaplingInput { - note_id: 1, - value: NonNegativeAmount::const_from_u64(20000), - }, - TestSaplingInput { - note_id: 2, - value: NonNegativeAmount::const_from_u64(1000), - }, - ], - &[SaplingPayment::new(NonNegativeAmount::const_from_u64( - 40000, - ))], + &( + sapling::builder::BundleType::DEFAULT, + &[ + TestSaplingInput { + note_id: 0, + value: NonNegativeAmount::const_from_u64(29000), + }, + TestSaplingInput { + note_id: 1, + value: NonNegativeAmount::const_from_u64(20000), + }, + TestSaplingInput { + note_id: 2, + value: NonNegativeAmount::const_from_u64(1000), + }, + ][..], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))][..], + ), + #[cfg(feature = "orchard")] + &( + orchard::builder::BundleType::DEFAULT, + &Vec::::new()[..], + &Vec::::new()[..], + ), &DustOutputPolicy::default(), ); diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 13af86322..1c9f33a16 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -341,17 +341,20 @@ impl proposal::Proposal { .balance() .proposed_change() .iter() - .map(|change| match change { - ChangeValue::Sapling { value, memo } => proposal::ChangeValue { + .map(|change| match change.output_pool() { + ShieldedProtocol::Sapling => proposal::ChangeValue { value: Some(proposal::change_value::Value::SaplingValue( proposal::SaplingChange { - amount: (*value).into(), - memo: memo.as_ref().map(|memo_bytes| proposal::MemoBytes { + amount: change.value().into(), + memo: change.memo().map(|memo_bytes| proposal::MemoBytes { value: memo_bytes.as_slice().to_vec(), }), }, )), }, + ShieldedProtocol::Orchard => { + unimplemented!("FIXME: implement Orchard change outputs!") + } }) .collect(), fee_required: value.balance().fee_required().into(), diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 6cc4d3090..84cf87f89 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -44,9 +44,11 @@ and this library adheres to Rust's notion of - `impl From<&NonNegativeAmount> for Amount` - `impl From for u64` - `impl From for zcash_primitives::sapling::value::NoteValue` + - `impl From for orchard::::NoteValue` - `impl Sum for Option` - `impl<'a> Sum<&'a NonNegativeAmount> for Option` - `impl TryFrom for NonNegativeAmount` + - `impl TryFrom for NonNegativeAmount` - `impl {Clone, PartialEq, Eq} for zcash_primitives::memo::Error` - `impl {PartialEq, Eq} for zcash_primitives::sapling::note::Rseed` - `impl From for [u8; 32]` diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index b005bd43d..3474e74ea 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -331,6 +331,20 @@ impl TryFrom for NonNegativeAmount { } } +impl From for orchard::NoteValue { + fn from(n: NonNegativeAmount) -> Self { + orchard::NoteValue::from_raw(n.into()) + } +} + +impl TryFrom for NonNegativeAmount { + type Error = (); + + fn try_from(value: orchard::NoteValue) -> Result { + Self::from_u64(value.inner()) + } +} + impl TryFrom for NonNegativeAmount { type Error = (); From adc75566a0455ef65e135515302ed8ac7838aedb Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 Dec 2023 14:45:22 -0700 Subject: [PATCH 2/3] zcash_client_backend: Add Orchard support to transaction proposals. --- Cargo.lock | 1 + zcash_client_backend/CHANGELOG.md | 21 +- zcash_client_backend/README.md | 6 + zcash_client_backend/proto/proposal.proto | 54 ++-- zcash_client_backend/src/data_api.rs | 35 ++- zcash_client_backend/src/data_api/wallet.rs | 2 +- .../src/data_api/wallet/input_selection.rs | 85 ++++--- zcash_client_backend/src/proto.rs | 237 +++++++++--------- zcash_client_backend/src/proto/proposal.rs | 102 +++++--- zcash_client_backend/src/wallet.rs | 76 +++++- zcash_client_sqlite/Cargo.toml | 1 + zcash_client_sqlite/src/lib.rs | 14 +- zcash_client_sqlite/src/wallet/sapling.rs | 6 +- 13 files changed, 393 insertions(+), 247 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2815edac3..7622809e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3056,6 +3056,7 @@ dependencies = [ "incrementalmerkletree", "jubjub", "maybe-rayon", + "orchard", "proptest", "prost", "rand_core", diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c2fa863e2..c1dcddfbd 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -21,9 +21,10 @@ and this library adheres to Rust's notion of with_orchard_balance_mut, add_unshielded_value }` + - `WalletRead::get_orchard_nullifiers` - `wallet::propose_standard_transfer_to_address` - - `wallet::input_selection::Proposal::from_parts` - - `wallet::input_selection::SaplingInputs` + - `wallet::input_selection::Proposal::{from_parts, shielded_inputs}` + - `wallet::input_selection::ShieldedInputs` - `wallet::input_selection::ShieldingSelector` has been factored out from the `InputSelector` trait to separate out transparent functionality and move it behind the `transparent-inputs` feature flag. @@ -134,8 +135,7 @@ and this library adheres to Rust's notion of - `wallet::create_proposed_transaction` now forces implementations to ignore the database identifiers for its contained notes by universally quantifying the `NoteRef` type parameter. - - `wallet::input_selection::Proposal::sapling_inputs` now returns type - `Option<&SaplingInputs>`. + - Arguments to `wallet::input_selection::Proposal::from_parts` have changed. - `wallet::input_selection::Proposal::min_anchor_height` has been removed in favor of storing this value in `SaplingInputs`. - `wallet::input_selection::GreedyInputSelector` now has relaxed requirements @@ -146,9 +146,8 @@ and this library adheres to Rust's notion of - `ChangeValue` is now a struct. In addition to the existing change value, it now also provides the output pool to which change should be sent and an optional memo to be associated with the change output. - - `fixed::SingleOutputChangeStrategy::new` and - `zip317::SingleOutputChangeStrategy::new` each now accept an additional - `change_memo` argument. + - `fixed::SingleOutputChangeStrategy::new` and `zip317::SingleOutputChangeStrategy::new` + each now accept an additional `change_memo` argument - `zcash_client_backend::wallet`: - The fields of `ReceivedSaplingNote` are now private. Use `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods @@ -200,9 +199,11 @@ and this library adheres to Rust's notion of ### Removed - `zcash_client_backend::wallet::ReceivedSaplingNote` has been replaced by `zcash_client_backend::ReceivedNote`. -- `zcash_client_backend::data_api::WalletRead::is_valid_account_extfvk` has been - removed; it was unused in the ECC mobile wallet SDKs and has been superseded by - `get_account_for_ufvk`. +- `zcash_client_backend::data_api` + - `WalletRead::is_valid_account_extfvk` has been removed; it was unused in + the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`. + - `wallet::input_selection::Proposal::sapling_inputs` has been replaced + by `Proposal::shielded_inputs` - `zcash_client_backend::data_api::WalletRead::{ get_spendable_sapling_notes select_spendable_sapling_notes, diff --git a/zcash_client_backend/README.md b/zcash_client_backend/README.md index eb8a6cd61..63eeadcd4 100644 --- a/zcash_client_backend/README.md +++ b/zcash_client_backend/README.md @@ -3,6 +3,12 @@ This library contains Rust structs and traits for creating shielded Zcash light clients. +## Building + +Note that in order to (re)build the GRPC interface, you will need `protoc` on +your `$PATH`. This is not required unless you make changes to any of the files +in `./proto/`. + ## License Licensed under either of diff --git a/zcash_client_backend/proto/proposal.proto b/zcash_client_backend/proto/proposal.proto index c1d03f60c..e56353d84 100644 --- a/zcash_client_backend/proto/proposal.proto +++ b/zcash_client_backend/proto/proposal.proto @@ -11,11 +11,13 @@ message Proposal { uint32 protoVersion = 1; // ZIP 321 serialized transaction request string transactionRequest = 2; - // The transparent UTXOs to use as inputs to the transaction. - repeated ProposedInput transparentInputs = 3; - // The Sapling input notes and anchor height to be used in creating the transaction. - SaplingInputs saplingInputs = 4; - // The total value, fee amount, and change outputs of the proposed + // The anchor height to be used in creating the transaction, if any. + // Setting the anchor height to zero will disallow the use of any shielded + // inputs. + uint32 anchorHeight = 3; + // The inputs to be used in creating the transaction. + repeated ProposedInput inputs = 4; + // The total value, fee value, and change outputs of the proposed // transaction TransactionBalance balance = 5; // The fee rule used in constructing this proposal @@ -30,18 +32,26 @@ message Proposal { bool isShielding = 8; } -message SaplingInputs { - // The Sapling anchor height to be used in creating the transaction - uint32 anchorHeight = 1; - // The unique identifier and amount for each proposed Sapling input - repeated ProposedInput inputs = 2; +enum ValuePool { + // Protobuf requires that enums have a zero discriminant as the default + // value. However, we need to require that a known value pool is selected, + // and we do not want to fall back to any default, so sending the + // PoolNotSpecified value will be treated as an error. + PoolNotSpecified = 0; + // The transparent value pool (P2SH is not distinguished from P2PKH) + Transparent = 1; + // The Sapling value pool + Sapling = 2; + // The Orchard value pool + Orchard = 3; } -// The unique identifier and amount for each proposed input. +// The unique identifier and value for each proposed input. message ProposedInput { bytes txid = 1; - uint32 index = 2; - uint64 value = 3; + ValuePool valuePool = 2; + uint32 index = 3; + uint64 value = 4; } // The fee rule used in constructing a Proposal @@ -59,17 +69,18 @@ enum FeeRule { Zip317 = 3; } -// The proposed change outputs and fee amount. +// The proposed change outputs and fee value. message TransactionBalance { repeated ChangeValue proposedChange = 1; uint64 feeRequired = 2; } -// An enumeration of change value types. +// A proposed change output. If the transparent value pool is selected, +// the `memo` field must be null. message ChangeValue { - oneof value { - SaplingChange saplingValue = 1; - } + uint64 value = 1; + ValuePool valuePool = 2; + MemoBytes memo = 3; } // An object wrapper for memo bytes, to facilitate representing the @@ -77,10 +88,3 @@ message ChangeValue { message MemoBytes { bytes value = 1; } - -// The amount and memo for a proposed Sapling change output. -message SaplingChange { - uint64 amount = 1; - MemoBytes memo = 2; -} - diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index a1015e574..b00433632 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -28,7 +28,7 @@ use crate::{ decrypt::DecryptedOutput, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::service::TreeState, - wallet::{NoteId, ReceivedNote, Recipient, WalletTransparentOutput, WalletTx}, + wallet::{Note, NoteId, ReceivedNote, Recipient, WalletTransparentOutput, WalletTx}, ShieldedProtocol, }; @@ -360,7 +360,7 @@ pub trait InputSource { txid: &TxId, protocol: ShieldedProtocol, index: u32, - ) -> Result>, Self::Error>; + ) -> Result>, Self::Error>; /// Returns a list of spendable notes sufficient to cover the specified target value, if /// possible. Only spendable notes corresponding to the specified shielded protocol will @@ -372,7 +372,7 @@ pub trait InputSource { sources: &[ShieldedProtocol], anchor_height: BlockHeight, exclude: &[Self::NoteRef], - ) -> Result>, Self::Error>; + ) -> Result>, Self::Error>; /// Fetches a spendable transparent output. /// @@ -526,14 +526,23 @@ pub trait WalletRead { /// Returns a transaction. fn get_transaction(&self, txid: TxId) -> Result; - /// Returns the nullifiers for notes that the wallet is tracking, along with their associated - /// account IDs, that are either unspent or have not yet been confirmed as spent (in that a - /// spending transaction known to the wallet has not yet been included in a block). + /// Returns the nullifiers for Sapling notes that the wallet is tracking, along with their + /// associated account IDs, that are either unspent or have not yet been confirmed as spent (in + /// that a spending transaction known to the wallet has not yet been included in a block). fn get_sapling_nullifiers( &self, query: NullifierQuery, ) -> Result, Self::Error>; + /// Returns the nullifiers for Orchard notes that the wallet is tracking, along with their + /// associated account IDs, that are either unspent or have not yet been confirmed as spent (in + /// that a spending transaction known to the wallet has not yet been included in a block). + #[cfg(feature = "orchard")] + fn get_orchard_nullifiers( + &self, + query: NullifierQuery, + ) -> Result, Self::Error>; + /// Returns the set of all transparent receivers associated with the given account. /// /// The set contains all transparent receivers that are known to have been derived @@ -1096,7 +1105,7 @@ pub mod testing { use crate::{ address::{AddressMetadata, UnifiedAddress}, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, - wallet::{NoteId, ReceivedNote, WalletTransparentOutput}, + wallet::{Note, NoteId, ReceivedNote, WalletTransparentOutput}, ShieldedProtocol, }; @@ -1133,7 +1142,7 @@ pub mod testing { _txid: &TxId, _protocol: ShieldedProtocol, _index: u32, - ) -> Result>, Self::Error> { + ) -> Result>, Self::Error> { Ok(None) } @@ -1144,7 +1153,7 @@ pub mod testing { _sources: &[ShieldedProtocol], _anchor_height: BlockHeight, _exclude: &[Self::NoteRef], - ) -> Result>, Self::Error> { + ) -> Result>, Self::Error> { Ok(Vec::new()) } } @@ -1251,6 +1260,14 @@ pub mod testing { Ok(Vec::new()) } + #[cfg(feature = "orchard")] + fn get_orchard_nullifiers( + &self, + _query: NullifierQuery, + ) -> Result, Self::Error> { + Ok(Vec::new()) + } + fn get_transparent_receivers( &self, _account: AccountId, diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 20c46ec62..18dd96eda 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -573,7 +573,7 @@ where Some(dfvk.to_ovk(Scope::Internal)) }; - let (sapling_anchor, sapling_inputs) = proposal.sapling_inputs().map_or_else( + let (sapling_anchor, sapling_inputs) = proposal.shielded_inputs().map_or_else( || Ok((sapling::Anchor::empty_tree(), vec![])), |inputs| { wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| { 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 550b96f3b..10142c9e5 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -21,16 +21,19 @@ use crate::{ address::{Address, UnifiedAddress}, data_api::InputSource, fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance}, - wallet::{ReceivedNote, WalletTransparentOutput}, + wallet::{Note, ReceivedNote, WalletTransparentOutput}, zip321::TransactionRequest, ShieldedProtocol, }; +#[cfg(any(feature = "transparent-inputs", feature = "orchard"))] +use std::convert::Infallible; + #[cfg(feature = "transparent-inputs")] use {std::collections::BTreeSet, zcash_primitives::transaction::components::OutPoint}; #[cfg(feature = "orchard")] -use {crate::fees::orchard as orchard_fees, std::convert::Infallible}; +use crate::fees::orchard as orchard_fees; /// The type of errors that may be produced in input selection. pub enum InputSelectorError { @@ -83,7 +86,7 @@ impl fmt::Display for InputSelectorError { transaction_request: TransactionRequest, transparent_inputs: Vec, - sapling_inputs: Option>, + shielded_inputs: Option>, balance: TransactionBalance, fee_rule: FeeRuleT, min_target_height: BlockHeight, @@ -151,7 +154,7 @@ impl Proposal { pub fn from_parts( transaction_request: TransactionRequest, transparent_inputs: Vec, - sapling_inputs: Option>, + shielded_inputs: Option>, balance: TransactionBalance, fee_rule: FeeRuleT, min_target_height: BlockHeight, @@ -163,14 +166,14 @@ impl Proposal { .fold(Ok(NonNegativeAmount::ZERO), |acc, a| { (acc? + a).ok_or(ProposalError::Overflow) })?; - let sapling_input_total = sapling_inputs + let shielded_input_total = shielded_inputs .iter() .flat_map(|s_in| s_in.notes().iter()) - .map(|out| out.value()) + .map(|out| out.note().value()) .fold(Some(NonNegativeAmount::ZERO), |acc, a| (acc? + a)) .ok_or(ProposalError::Overflow)?; let input_total = - (transparent_input_total + sapling_input_total).ok_or(ProposalError::Overflow)?; + (transparent_input_total + shielded_input_total).ok_or(ProposalError::Overflow)?; let request_total = transaction_request .total() @@ -179,7 +182,7 @@ impl Proposal { if is_shielding && (transparent_input_total == NonNegativeAmount::ZERO - || sapling_input_total > NonNegativeAmount::ZERO + || shielded_input_total > NonNegativeAmount::ZERO || request_total > NonNegativeAmount::ZERO) { return Err(ProposalError::ShieldingInvalid); @@ -189,7 +192,7 @@ impl Proposal { Ok(Self { transaction_request, transparent_inputs, - sapling_inputs, + shielded_inputs, balance, fee_rule, min_target_height, @@ -212,8 +215,8 @@ impl Proposal { &self.transparent_inputs } /// Returns the Sapling inputs that have been selected to fund the transaction. - pub fn sapling_inputs(&self) -> Option<&SaplingInputs> { - self.sapling_inputs.as_ref() + pub fn shielded_inputs(&self) -> Option<&ShieldedInputs> { + self.shielded_inputs.as_ref() } /// Returns the change outputs to be added to the transaction and the fee to be paid. pub fn balance(&self) -> &TransactionBalance { @@ -244,12 +247,12 @@ impl Debug for Proposal { .field("transaction_request", &self.transaction_request) .field("transparent_inputs", &self.transparent_inputs) .field( - "sapling_inputs", - &self.sapling_inputs().map(|i| i.notes.len()), + "shielded_inputs", + &self.shielded_inputs().map(|i| i.notes.len()), ) .field( - "sapling_anchor_height", - &self.sapling_inputs().map(|i| i.anchor_height), + "anchor_height", + &self.shielded_inputs().map(|i| i.anchor_height), ) .field("balance", &self.balance) //.field("fee_rule", &self.fee_rule) @@ -261,14 +264,17 @@ impl Debug for Proposal { /// The Sapling inputs to a proposed transaction. #[derive(Clone, PartialEq, Eq)] -pub struct SaplingInputs { +pub struct ShieldedInputs { anchor_height: BlockHeight, - notes: NonEmpty>, + notes: NonEmpty>, } -impl SaplingInputs { - /// Constructs a [`SaplingInputs`] from its constituent parts. - pub fn from_parts(anchor_height: BlockHeight, notes: NonEmpty>) -> Self { +impl ShieldedInputs { + /// Constructs a [`ShieldedInputs`] from its constituent parts. + pub fn from_parts( + anchor_height: BlockHeight, + notes: NonEmpty>, + ) -> Self { Self { anchor_height, notes, @@ -282,7 +288,7 @@ impl SaplingInputs { } /// Returns the list of Sapling notes to be used as inputs to the proposed transaction. - pub fn notes(&self) -> &NonEmpty> { + pub fn notes(&self) -> &NonEmpty> { &self.notes } } @@ -559,7 +565,7 @@ where } } - let mut sapling_inputs: Vec> = vec![]; + let mut shielded_inputs: Vec> = vec![]; let mut prior_available = NonNegativeAmount::ZERO; let mut amount_required = NonNegativeAmount::ZERO; let mut exclude: Vec = vec![]; @@ -574,13 +580,30 @@ where &transparent_outputs, &( ::sapling::builder::BundleType::DEFAULT, - &sapling_inputs[..], + &shielded_inputs + .iter() + .filter_map(|i| { + i.clone().traverse_opt(|wn| match wn { + Note::Sapling(n) => Some(n), + #[cfg(feature = "orchard")] + _ => None, + }) + }) + .collect::>()[..], &sapling_outputs[..], ), #[cfg(feature = "orchard")] &( ::orchard::builder::BundleType::DEFAULT, - &Vec::::new()[..], + &shielded_inputs + .iter() + .filter_map(|i| { + i.clone().traverse_opt(|wn| match wn { + Note::Orchard(n) => Some(n), + _ => None, + }) + }) + .collect::>()[..], &orchard_outputs[..], ), &self.dust_output_policy, @@ -591,8 +614,8 @@ where return Ok(Proposal { transaction_request, transparent_inputs: vec![], - sapling_inputs: NonEmpty::from_vec(sapling_inputs).map(|notes| { - SaplingInputs { + shielded_inputs: NonEmpty::from_vec(shielded_inputs).map(|notes| { + ShieldedInputs { anchor_height, notes, } @@ -612,19 +635,19 @@ where Err(other) => return Err(other.into()), } - sapling_inputs = wallet_db + shielded_inputs = wallet_db .select_spendable_notes( account, amount_required.into(), - &[ShieldedProtocol::Sapling], + &[ShieldedProtocol::Sapling, ShieldedProtocol::Orchard], anchor_height, &exclude, ) .map_err(InputSelectorError::DataSource)?; - let new_available = sapling_inputs + let new_available = shielded_inputs .iter() - .map(|n| n.value()) + .map(|n| n.note().value()) .sum::>() .ok_or(BalanceError::Overflow)?; @@ -737,7 +760,7 @@ where Ok(Proposal { transaction_request: TransactionRequest::empty(), transparent_inputs, - sapling_inputs: None, + shielded_inputs: None, balance, fee_rule: (*self.change_strategy.fee_rule()).clone(), min_target_height: target_height, diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 1c9f33a16..6747d9be2 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -22,7 +22,7 @@ use zcash_note_encryption::{EphemeralKeyBytes, COMPACT_NOTE_SIZE}; use crate::{ data_api::{ - wallet::input_selection::{Proposal, ProposalError, SaplingInputs}, + wallet::input_selection::{Proposal, ProposalError, ShieldedInputs}, InputSource, }, fees::{ChangeValue, TransactionBalance}, @@ -221,6 +221,8 @@ pub enum ProposalDecodingError { Zip321(Zip321Error), /// A transaction identifier string did not decode to a valid transaction ID. TxIdInvalid(TryFromSliceError), + /// An invalid value pool identifier was encountered. + ValuePoolInvalid(i32), /// A failure occurred trying to retrieve an unspent note or UTXO from the wallet database. InputRetrieval(DbError), /// The unspent note or UTXO corresponding to a proposal input was not found in the wallet @@ -238,6 +240,8 @@ pub enum ProposalDecodingError { ProposalInvalid(ProposalError), /// An inputs field for the given protocol was present, but contained no input note references. EmptyShieldedInputs(ShieldedProtocol), + /// Change outputs to the specified pool are not supported. + InvalidChangeRecipient(PoolType), } impl From for ProposalDecodingError { @@ -253,6 +257,9 @@ impl Display for ProposalDecodingError { ProposalDecodingError::TxIdInvalid(err) => { write!(f, "Invalid transaction id: {:?}", err) } + ProposalDecodingError::ValuePoolInvalid(id) => { + write!(f, "Invalid value pool identifier: {:?}", id) + } ProposalDecodingError::InputRetrieval(err) => write!( f, "An error occurred retrieving a transaction input: {}", @@ -281,6 +288,11 @@ impl Display for ProposalDecodingError { "An inputs field was present for {:?}, but contained no note references.", protocol ), + ProposalDecodingError::InvalidChangeRecipient(pool_type) => write!( + f, + "Change outputs to the {} pool are not supported.", + pool_type + ), } } } @@ -296,10 +308,38 @@ impl std::error::Error for ProposalDecodingError } } +fn pool_type(pool_id: i32) -> Result> { + match proposal::ValuePool::try_from(pool_id) { + Ok(proposal::ValuePool::Transparent) => Ok(PoolType::Transparent), + Ok(proposal::ValuePool::Sapling) => Ok(PoolType::Shielded(ShieldedProtocol::Sapling)), + Ok(proposal::ValuePool::Orchard) => Ok(PoolType::Shielded(ShieldedProtocol::Orchard)), + _ => Err(ProposalDecodingError::ValuePoolInvalid(pool_id)), + } +} + impl proposal::ProposedInput { pub fn parse_txid(&self) -> Result { Ok(TxId::from_bytes(self.txid[..].try_into()?)) } + + pub fn pool_type(&self) -> Result> { + pool_type(self.value_pool) + } +} + +impl proposal::ChangeValue { + pub fn pool_type(&self) -> Result> { + pool_type(self.value_pool) + } +} + +impl From for proposal::ValuePool { + fn from(value: ShieldedProtocol) -> Self { + match value { + ShieldedProtocol::Sapling => proposal::ValuePool::Sapling, + ShieldedProtocol::Orchard => proposal::ValuePool::Orchard, + } + } } impl proposal::Proposal { @@ -311,50 +351,40 @@ impl proposal::Proposal { ) -> Option { let transaction_request = value.transaction_request().to_uri(params)?; - let transparent_inputs = value + let anchor_height = value + .shielded_inputs() + .map_or_else(|| 0, |i| u32::from(i.anchor_height())); + + let inputs = value .transparent_inputs() .iter() .map(|utxo| proposal::ProposedInput { txid: utxo.outpoint().hash().to_vec(), + value_pool: proposal::ValuePool::Transparent.into(), index: utxo.outpoint().n(), value: utxo.txout().value.into(), }) + .chain(value.shielded_inputs().iter().flat_map(|s_in| { + s_in.notes().iter().map(|rec_note| proposal::ProposedInput { + txid: rec_note.txid().as_ref().to_vec(), + value_pool: proposal::ValuePool::from(rec_note.note().protocol()).into(), + index: rec_note.output_index().into(), + value: rec_note.note().value().into(), + }) + })) .collect(); - let sapling_inputs = value - .sapling_inputs() - .map(|sapling_inputs| proposal::SaplingInputs { - anchor_height: sapling_inputs.anchor_height().into(), - inputs: sapling_inputs - .notes() - .iter() - .map(|rec_note| proposal::ProposedInput { - txid: rec_note.txid().as_ref().to_vec(), - index: rec_note.output_index().into(), - value: rec_note.value().into(), - }) - .collect(), - }); - let balance = Some(proposal::TransactionBalance { proposed_change: value .balance() .proposed_change() .iter() - .map(|change| match change.output_pool() { - ShieldedProtocol::Sapling => proposal::ChangeValue { - value: Some(proposal::change_value::Value::SaplingValue( - proposal::SaplingChange { - amount: change.value().into(), - memo: change.memo().map(|memo_bytes| proposal::MemoBytes { - value: memo_bytes.as_slice().to_vec(), - }), - }, - )), - }, - ShieldedProtocol::Orchard => { - unimplemented!("FIXME: implement Orchard change outputs!") - } + .map(|change| proposal::ChangeValue { + value: change.value().into(), + value_pool: proposal::ValuePool::from(change.output_pool()).into(), + memo: change.memo().map(|memo_bytes| proposal::MemoBytes { + value: memo_bytes.as_slice().to_vec(), + }), }) .collect(), fee_required: value.balance().fee_required().into(), @@ -364,8 +394,8 @@ impl proposal::Proposal { Some(proposal::Proposal { proto_version: PROPOSAL_SER_V1, transaction_request, - transparent_inputs, - sapling_inputs, + anchor_height, + inputs, balance, fee_rule: match value.fee_rule() { StandardFeeRule::PreZip313 => proposal::FeeRule::PreZip313, @@ -402,72 +432,59 @@ impl proposal::Proposal { let transaction_request = TransactionRequest::from_uri(params, &self.transaction_request)?; - let transparent_inputs = self - .transparent_inputs - .iter() - .map(|t_in| { - let txid = t_in - .parse_txid() - .map_err(ProposalDecodingError::TxIdInvalid)?; - #[cfg(not(feature = "transparent-inputs"))] - return Err(ProposalDecodingError::InputNotFound( - txid, - PoolType::Transparent, - t_in.index, - )); + #[cfg(not(feature = "transparent-inputs"))] + let transparent_inputs = vec![]; + #[cfg(feature = "transparent-inputs")] + let mut transparent_inputs = vec![]; - #[cfg(feature = "transparent-inputs")] - { - let outpoint = OutPoint::new(txid.into(), t_in.index); - wallet_db - .get_unspent_transparent_output(&outpoint) - .map_err(ProposalDecodingError::InputRetrieval)? - .ok_or({ - ProposalDecodingError::InputNotFound( - txid, - PoolType::Transparent, - t_in.index, - ) - }) + let mut received_notes = vec![]; + for input in self.inputs.iter() { + let txid = input + .parse_txid() + .map_err(ProposalDecodingError::TxIdInvalid)?; + + match input.pool_type()? { + PoolType::Transparent => { + #[cfg(not(feature = "transparent-inputs"))] + return Err(ProposalDecodingError::ValuePoolInvalid(1)); + + #[cfg(feature = "transparent-inputs")] + { + let outpoint = OutPoint::new(txid.into(), input.index); + transparent_inputs.push( + wallet_db + .get_unspent_transparent_output(&outpoint) + .map_err(ProposalDecodingError::InputRetrieval)? + .ok_or({ + ProposalDecodingError::InputNotFound( + txid, + PoolType::Transparent, + input.index, + ) + })?, + ); + } } - }) - .collect::, _>>()?; - - let sapling_inputs = self.sapling_inputs.as_ref().map(|s_in| { - s_in.inputs - .iter() - .map(|s_in| { - let txid = s_in - .parse_txid() - .map_err(ProposalDecodingError::TxIdInvalid)?; - + PoolType::Shielded(protocol) => received_notes.push( wallet_db - .get_spendable_note(&txid, ShieldedProtocol::Sapling, s_in.index) + .get_spendable_note(&txid, protocol, input.index) .map_err(ProposalDecodingError::InputRetrieval) .and_then(|opt| { opt.ok_or({ ProposalDecodingError::InputNotFound( txid, - PoolType::Shielded(ShieldedProtocol::Sapling), - s_in.index, + PoolType::Shielded(protocol), + input.index, ) }) - }) - }) - .collect::, _>>() - .and_then(|notes| { - NonEmpty::from_vec(notes) - .map(|notes| { - SaplingInputs::from_parts(s_in.anchor_height.into(), notes) - }) - .ok_or({ - ProposalDecodingError::EmptyShieldedInputs( - ShieldedProtocol::Sapling, - ) - }) - }) - }); + })?, + ), + } + } + + let shielded_inputs = NonEmpty::from_vec(received_notes) + .map(|notes| ShieldedInputs::from_parts(self.anchor_height.into(), notes)); let proto_balance = self .balance @@ -477,31 +494,23 @@ impl proposal::Proposal { proto_balance .proposed_change .iter() - .filter_map(|change| { - // An empty `value` field can be treated as though the whole - // `ChangeValue` was absent; this optionality is an artifact of - // protobuf encoding. - change.value.as_ref().map( - |cv| -> Result> { - match cv { - proposal::change_value::Value::SaplingValue(sc) => { - Ok(ChangeValue::sapling( - NonNegativeAmount::from_u64(sc.amount).map_err( - |_| ProposalDecodingError::BalanceInvalid, - )?, - sc.memo - .as_ref() - .map(|bytes| { - MemoBytes::from_bytes(&bytes.value).map_err( - ProposalDecodingError::MemoInvalid, - ) - }) - .transpose()?, - )) - } - } - }, - ) + .map(|cv| -> Result> { + match cv.pool_type()? { + PoolType::Shielded(ShieldedProtocol::Sapling) => { + Ok(ChangeValue::sapling( + NonNegativeAmount::from_u64(cv.value) + .map_err(|_| ProposalDecodingError::BalanceInvalid)?, + cv.memo + .as_ref() + .map(|bytes| { + MemoBytes::from_bytes(&bytes.value) + .map_err(ProposalDecodingError::MemoInvalid) + }) + .transpose()?, + )) + } + t => Err(ProposalDecodingError::InvalidChangeRecipient(t)), + } }) .collect::, _>>()?, NonNegativeAmount::from_u64(proto_balance.fee_required) @@ -512,7 +521,7 @@ impl proposal::Proposal { Proposal::from_parts( transaction_request, transparent_inputs, - sapling_inputs.transpose()?, + shielded_inputs, balance, fee_rule, self.min_target_height.into(), diff --git a/zcash_client_backend/src/proto/proposal.rs b/zcash_client_backend/src/proto/proposal.rs index c4bed23b7..986b46277 100644 --- a/zcash_client_backend/src/proto/proposal.rs +++ b/zcash_client_backend/src/proto/proposal.rs @@ -8,13 +8,15 @@ pub struct Proposal { /// ZIP 321 serialized transaction request #[prost(string, tag = "2")] pub transaction_request: ::prost::alloc::string::String, - /// The transparent UTXOs to use as inputs to the transaction. - #[prost(message, repeated, tag = "3")] - pub transparent_inputs: ::prost::alloc::vec::Vec, - /// The Sapling input notes and anchor height to be used in creating the transaction. - #[prost(message, optional, tag = "4")] - pub sapling_inputs: ::core::option::Option, - /// The total value, fee amount, and change outputs of the proposed + /// The anchor height to be used in creating the transaction, if any. + /// Setting the anchor height to zero will disallow the use of any shielded + /// inputs. + #[prost(uint32, tag = "3")] + pub anchor_height: u32, + /// The inputs to be used in creating the transaction. + #[prost(message, repeated, tag = "4")] + pub inputs: ::prost::alloc::vec::Vec, + /// The total value, fee value, and change outputs of the proposed /// transaction #[prost(message, optional, tag = "5")] pub balance: ::core::option::Option, @@ -32,28 +34,20 @@ pub struct Proposal { #[prost(bool, tag = "8")] pub is_shielding: bool, } -#[allow(clippy::derive_partial_eq_without_eq)] -#[derive(Clone, PartialEq, ::prost::Message)] -pub struct SaplingInputs { - /// The Sapling anchor height to be used in creating the transaction - #[prost(uint32, tag = "1")] - pub anchor_height: u32, - /// The unique identifier and amount for each proposed Sapling input - #[prost(message, repeated, tag = "2")] - pub inputs: ::prost::alloc::vec::Vec, -} -/// The unique identifier and amount for each proposed input. +/// The unique identifier and value for each proposed input. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ProposedInput { #[prost(bytes = "vec", tag = "1")] pub txid: ::prost::alloc::vec::Vec, - #[prost(uint32, tag = "2")] + #[prost(enumeration = "ValuePool", tag = "2")] + pub value_pool: i32, + #[prost(uint32, tag = "3")] pub index: u32, - #[prost(uint64, tag = "3")] + #[prost(uint64, tag = "4")] pub value: u64, } -/// The proposed change outputs and fee amount. +/// The proposed change outputs and fee value. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct TransactionBalance { @@ -62,21 +56,17 @@ pub struct TransactionBalance { #[prost(uint64, tag = "2")] pub fee_required: u64, } -/// An enumeration of change value types. +/// A proposed change output. If the transparent value pool is selected, +/// the `memo` field must be null. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ChangeValue { - #[prost(oneof = "change_value::Value", tags = "1")] - pub value: ::core::option::Option, -} -/// Nested message and enum types in `ChangeValue`. -pub mod change_value { - #[allow(clippy::derive_partial_eq_without_eq)] - #[derive(Clone, PartialEq, ::prost::Oneof)] - pub enum Value { - #[prost(message, tag = "1")] - SaplingValue(super::SaplingChange), - } + #[prost(uint64, tag = "1")] + pub value: u64, + #[prost(enumeration = "ValuePool", tag = "2")] + pub value_pool: i32, + #[prost(message, optional, tag = "3")] + pub memo: ::core::option::Option, } /// An object wrapper for memo bytes, to facilitate representing the /// `change_memo == None` case. @@ -86,14 +76,44 @@ pub struct MemoBytes { #[prost(bytes = "vec", tag = "1")] pub value: ::prost::alloc::vec::Vec, } -/// The amount and memo for a proposed Sapling change output. -#[allow(clippy::derive_partial_eq_without_eq)] -#[derive(Clone, PartialEq, ::prost::Message)] -pub struct SaplingChange { - #[prost(uint64, tag = "1")] - pub amount: u64, - #[prost(message, optional, tag = "2")] - pub memo: ::core::option::Option, +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] +#[repr(i32)] +pub enum ValuePool { + /// Protobuf requires that enums have a zero discriminant as the default + /// value. However, we need to require that a known value pool is selected, + /// and we do not want to fall back to any default, so sending the + /// PoolNotSpecified value will be treated as an error. + PoolNotSpecified = 0, + /// The transparent value pool (P2SH is not distinguished from P2PKH) + Transparent = 1, + /// The Sapling value pool + Sapling = 2, + /// The Orchard value pool + Orchard = 3, +} +impl ValuePool { + /// String value of the enum field names used in the ProtoBuf definition. + /// + /// The values are not transformed in any way and thus are considered stable + /// (if the ProtoBuf definition does not change) and safe for programmatic use. + pub fn as_str_name(&self) -> &'static str { + match self { + ValuePool::PoolNotSpecified => "PoolNotSpecified", + ValuePool::Transparent => "Transparent", + ValuePool::Sapling => "Sapling", + ValuePool::Orchard => "Orchard", + } + } + /// Creates an enum from field names used in the ProtoBuf definition. + pub fn from_str_name(value: &str) -> ::core::option::Option { + match value { + "PoolNotSpecified" => Some(Self::PoolNotSpecified), + "Transparent" => Some(Self::Transparent), + "Sapling" => Some(Self::Sapling), + "Orchard" => Some(Self::Orchard), + _ => None, + } + } } /// The fee rule used in constructing a Proposal #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index dd5f71513..9e4ca4f0c 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -19,6 +19,9 @@ use zcash_primitives::{ use crate::{address::UnifiedAddress, fees::sapling as sapling_fees, PoolType, ShieldedProtocol}; +#[cfg(feature = "orchard")] +use crate::fees::orchard as orchard_fees; + /// A unique identifier for a shielded transaction output #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct NoteId { @@ -255,26 +258,34 @@ impl Note { ), } } + + pub fn protocol(&self) -> ShieldedProtocol { + match self { + Note::Sapling(_) => ShieldedProtocol::Sapling, + #[cfg(feature = "orchard")] + Note::Orchard(_) => ShieldedProtocol::Orchard, + } + } } /// Information about a note that is tracked by the wallet that is available for spending, /// with sufficient information for use in note selection. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct ReceivedNote { +pub struct ReceivedNote { note_id: NoteRef, txid: TxId, output_index: u16, - note: Note, + note: NoteT, spending_key_scope: Scope, note_commitment_tree_position: Position, } -impl ReceivedNote { +impl ReceivedNote { pub fn from_parts( note_id: NoteRef, txid: TxId, output_index: u16, - note: Note, + note: NoteT, spending_key_scope: Scope, note_commitment_tree_position: Position, ) -> Self { @@ -297,27 +308,72 @@ impl ReceivedNote { pub fn output_index(&self) -> u16 { self.output_index } - pub fn note(&self) -> &Note { + pub fn note(&self) -> &NoteT { &self.note } - pub fn value(&self) -> NonNegativeAmount { - self.note.value() - } pub fn spending_key_scope(&self) -> Scope { self.spending_key_scope } pub fn note_commitment_tree_position(&self) -> Position { self.note_commitment_tree_position } + + /// Applies the given function to the `note` field of this ReceivedNote and returns + /// `None` if that function returns `None`, or otherwise a `Some` containing + /// a `ReceivedNote` with its `note` field swapped out for the result of the function. + /// + /// The name `traverse` refers to the general operation that has the Haskell type + /// `Applicative f => (a -> f b) -> t a -> f (t b)`, that this method specializes + /// with `ReceivedNote` for `t` and `Option<_>` for `f`. + pub fn traverse_opt( + self, + f: impl FnOnce(NoteT) -> Option, + ) -> Option> { + f(self.note).map(|n0| ReceivedNote { + note_id: self.note_id, + txid: self.txid, + output_index: self.output_index, + note: n0, + spending_key_scope: self.spending_key_scope, + note_commitment_tree_position: self.note_commitment_tree_position, + }) + } } -impl sapling_fees::InputView for ReceivedNote { +impl ReceivedNote { + pub fn protocol(&self) -> ShieldedProtocol { + match self.note() { + Note::Sapling(_) => ShieldedProtocol::Sapling, + #[cfg(feature = "orchard")] + Note::Orchard(_) => ShieldedProtocol::Orchard, + } + } +} + +impl sapling_fees::InputView for ReceivedNote { fn note_id(&self) -> &NoteRef { &self.note_id } fn value(&self) -> NonNegativeAmount { - self.note.value() + self.note + .value() + .try_into() + .expect("Sapling note values are indirectly checked by consensus.") + } +} + +#[cfg(feature = "orchard")] +impl orchard_fees::InputView for ReceivedNote { + fn note_id(&self) -> &NoteRef { + &self.note_id + } + + fn value(&self) -> NonNegativeAmount { + self.note + .value() + .try_into() + .expect("Orchard note values are indirectly checked by consensus.") } } diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 17ee481e0..35d906b83 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -22,6 +22,7 @@ rustdoc-args = ["--cfg", "docsrs"] zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] } zcash_encoding.workspace = true zcash_primitives.workspace = true +orchard.workspace = true # Dependencies exposed in a public API: # (Breaking upgrades to these require a breaking upgrade to this crate.) diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 403368429..40164475c 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -69,7 +69,7 @@ use zcash_client_backend::{ }, keys::{UnifiedFullViewingKey, UnifiedSpendingKey}, proto::compact_formats::CompactBlock, - wallet::{NoteId, ReceivedNote, Recipient, WalletTransparentOutput}, + wallet::{Note, NoteId, ReceivedNote, Recipient, WalletTransparentOutput}, DecryptedOutput, PoolType, ShieldedProtocol, TransferType, }; @@ -172,7 +172,7 @@ impl, P: consensus::Parameters> InputSource for txid: &TxId, _protocol: ShieldedProtocol, index: u32, - ) -> Result>, Self::Error> { + ) -> Result>, Self::Error> { wallet::sapling::get_spendable_sapling_note(self.conn.borrow(), &self.params, txid, index) } @@ -183,7 +183,7 @@ impl, P: consensus::Parameters> InputSource for _sources: &[ShieldedProtocol], anchor_height: BlockHeight, exclude: &[Self::NoteRef], - ) -> Result>, Self::Error> { + ) -> Result>, Self::Error> { wallet::sapling::select_spendable_sapling_notes( self.conn.borrow(), &self.params, @@ -364,6 +364,14 @@ impl, P: consensus::Parameters> WalletRead for W "The wallet must be compiled with the transparent-inputs feature to use this method." ); } + + #[cfg(feature = "orchard")] + fn get_orchard_nullifiers( + &self, + _query: NullifierQuery, + ) -> Result, Self::Error> { + todo!() + } } impl WalletWrite for WalletDb { diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 825c4983a..4e4c96f71 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -100,7 +100,7 @@ impl ReceivedSaplingOutput for DecryptedOutput { fn to_spendable_note( params: &P, row: &Row, -) -> Result, SqliteClientError> { +) -> Result, SqliteClientError> { let note_id = ReceivedNoteId(row.get(0)?); let txid = row.get::<_, [u8; 32]>(1).map(TxId::from_bytes)?; let output_index = row.get(2)?; @@ -182,7 +182,7 @@ pub(crate) fn get_spendable_sapling_note( params: &P, txid: &TxId, index: u32, -) -> Result>, SqliteClientError> { +) -> Result>, SqliteClientError> { let mut stmt_select_note = conn.prepare_cached( "SELECT id_note, txid, output_index, diversifier, value, rcm, commitment_tree_position, accounts.ufvk, recipient_key_scope @@ -239,7 +239,7 @@ pub(crate) fn select_spendable_sapling_notes( target_value: Amount, anchor_height: BlockHeight, exclude: &[ReceivedNoteId], -) -> Result>, SqliteClientError> { +) -> Result>, SqliteClientError> { let birthday_height = match wallet_birthday(conn)? { Some(birthday) => birthday, None => { From 24ebe4c6430e41f27ea5648fa24058de6389edb7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 5 Jan 2024 13:05:39 -0700 Subject: [PATCH 3/3] Address comments from code review. Co-authored-by: Jack Grigg --- zcash_client_backend/CHANGELOG.md | 43 ++++++++++--------- zcash_client_backend/src/fees.rs | 1 + zcash_client_backend/src/fees/common.rs | 11 +++-- zcash_client_backend/src/fees/orchard.rs | 3 -- zcash_client_backend/src/wallet.rs | 11 +---- zcash_client_sqlite/Cargo.toml | 2 +- zcash_primitives/CHANGELOG.md | 2 + .../src/transaction/components/amount.rs | 10 +++++ 8 files changed, 44 insertions(+), 39 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index c1dcddfbd..0a15aca9c 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -17,11 +17,10 @@ and this library adheres to Rust's notion of - `ScannedBlockCommitments` - `Balance::{add_spendable_value, add_pending_change_value, add_pending_spendable_value}` - `AccountBalance::{ - with_sapling_balance_mut, - with_orchard_balance_mut, + with_sapling_balance_mut, + with_orchard_balance_mut, add_unshielded_value }` - - `WalletRead::get_orchard_nullifiers` - `wallet::propose_standard_transfer_to_address` - `wallet::input_selection::Proposal::{from_parts, shielded_inputs}` - `wallet::input_selection::ShieldedInputs` @@ -59,7 +58,7 @@ and this library adheres to Rust's notion of - `zcash_client_backend::data_api::{NoteId, Recipient}` have been moved into the `zcash_client_backend::wallet` module. - `ScannedBlock::{sapling_tree_size, sapling_nullifier_map, sapling_commitments}` - have been moved to `ScannedBlockSapling` and in that context are now + have been moved to `ScannedBlockSapling` and in that context are now named `{tree_size, nullifier_map, commitments}` respectively. ### Changed @@ -70,9 +69,9 @@ and this library adheres to Rust's notion of - `WalletShieldedOutput` has an additional type parameter which is used for key scope. `WalletShieldedOutput::from_parts` now takes an additional argument of this type. - - `WalletTx` has an additional type parameter as a consequence of the + - `WalletTx` has an additional type parameter as a consequence of the `WalletShieldedOutput` change. - - `ScannedBlock` has an additional type parameter as a consequence of the + - `ScannedBlock` has an additional type parameter as a consequence of the `WalletTx` change. - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now returns an owned value rather than a reference. @@ -126,9 +125,16 @@ and this library adheres to Rust's notion of argument, instead taking explicit `target_height` and `anchor_height` arguments. This helps to minimize the set of capabilities that the `data_api::InputSource` must expose. - - `WalletRead::get_checkpoint_depth` has been removed without replacement. This - is no longer needed given the change to use the stored anchor height for transaction - proposal execution. + - Changes to the `WalletRead` trait: + - Added `get_orchard_nullifiers` (under the `orchard` feature flag.) + - `get_checkpoint_depth` has been removed without replacement. This + is no longer needed given the change to use the stored anchor height for transaction + proposal execution. + - `is_valid_account_extfvk` has been removed; it was unused in + the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`. + - `get_spendable_sapling_notes`, `select_spendable_sapling_notes`, and + `get_unspent_transparent_outputs` have been removed; use + `data_api::InputSource` instead. - `wallet::{propose_shielding, shield_transparent_funds}` now takes their `min_confirmations` arguments as `u32` rather than a `NonZeroU32` to permit implmentations to enable zero-conf shielding. @@ -146,8 +152,10 @@ and this library adheres to Rust's notion of - `ChangeValue` is now a struct. In addition to the existing change value, it now also provides the output pool to which change should be sent and an optional memo to be associated with the change output. - - `fixed::SingleOutputChangeStrategy::new` and `zip317::SingleOutputChangeStrategy::new` - each now accept an additional `change_memo` argument + - `ChangeError` has a new `BundleError` variant. + - `fixed::SingleOutputChangeStrategy::new` and + `zip317::SingleOutputChangeStrategy::new` each now accept an additional + `change_memo` argument. - `zcash_client_backend::wallet`: - The fields of `ReceivedSaplingNote` are now private. Use `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods @@ -183,7 +191,7 @@ and this library adheres to Rust's notion of - `zcash_client_backend::address`: - `RecipientAddress` has been renamed to `Address` - `Address::Shielded` has been renamed to `Address::Sapling` - - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver + - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver argument unless the `orchard` feature is enabled. - `UnifiedAddress::orchard` is now only available when the `orchard` feature is enabled. @@ -199,16 +207,9 @@ and this library adheres to Rust's notion of ### Removed - `zcash_client_backend::wallet::ReceivedSaplingNote` has been replaced by `zcash_client_backend::ReceivedNote`. +- `zcash_client_backend::wallet::input_selection::Proposal::sapling_inputs` has + been replaced by `Proposal::shielded_inputs` - `zcash_client_backend::data_api` - - `WalletRead::is_valid_account_extfvk` has been removed; it was unused in - the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`. - - `wallet::input_selection::Proposal::sapling_inputs` has been replaced - by `Proposal::shielded_inputs` -- `zcash_client_backend::data_api::WalletRead::{ - get_spendable_sapling_notes - select_spendable_sapling_notes, - get_unspent_transparent_outputs, - }` - use `data_api::InputSource` instead. - `zcash_client_backend::data_api::ScannedBlock::from_parts` has been made crate-private. - `zcash_client_backend::data_api::ScannedBlock::into_sapling_commitments` has been replaced by `into_commitments` which returns both Sapling and Orchard note commitments diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 25f2f9402..a79c216e6 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -16,6 +16,7 @@ use crate::ShieldedProtocol; pub(crate) mod common; pub mod fixed; +#[cfg(feature = "orchard")] pub mod orchard; pub mod sapling; pub mod standard; diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 63a5a1280..2bef424cb 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -87,10 +87,10 @@ where // TODO: implement a less naive strategy for selecting the pool to which change will be sent. #[cfg(feature = "orchard")] let (change_pool, sapling_change, orchard_change) = - if orchard_in > NonNegativeAmount::ZERO || orchard_out > NonNegativeAmount::ZERO { + if orchard_in.is_positive() || orchard_out.is_positive() { // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs (ShieldedProtocol::Orchard, 0, 1) - } else if sapling_in > NonNegativeAmount::ZERO || sapling_out > NonNegativeAmount::ZERO { + } else if sapling_in.is_positive() || sapling_out.is_positive() { // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any // Sapling outputs, so that we avoid pool-crossing. (ShieldedProtocol::Sapling, 1, 0) @@ -119,7 +119,10 @@ where target_height, transparent_inputs, transparent_outputs, - sapling.inputs().len(), + sapling + .bundle_type() + .num_spends(sapling.inputs().len()) + .map_err(ChangeError::BundleError)?, sapling .bundle_type() .num_outputs( @@ -139,7 +142,7 @@ where required: total_out, })?; - if proposed_change == NonNegativeAmount::ZERO { + if proposed_change.is_zero() { TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow()) } else { let dust_threshold = dust_output_policy diff --git a/zcash_client_backend/src/fees/orchard.rs b/zcash_client_backend/src/fees/orchard.rs index 790bdff78..ac90a0113 100644 --- a/zcash_client_backend/src/fees/orchard.rs +++ b/zcash_client_backend/src/fees/orchard.rs @@ -4,12 +4,10 @@ use std::convert::Infallible; use zcash_primitives::transaction::components::amount::NonNegativeAmount; -#[cfg(feature = "orchard")] use orchard::builder::BundleType; /// A trait that provides a minimized view of Orchard bundle configuration /// suitable for use in fee and change calculation. -#[cfg(feature = "orchard")] pub trait BundleView { /// The type of inputs to the bundle. type In: InputView; @@ -24,7 +22,6 @@ pub trait BundleView { fn outputs(&self) -> &[Self::Out]; } -#[cfg(feature = "orchard")] impl<'a, NoteRef, In: InputView, Out: OutputView> BundleView for (BundleType, &'a [In], &'a [Out]) { diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 9e4ca4f0c..851bc31ad 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -259,6 +259,7 @@ impl Note { } } + /// Returns the shielded protocol used by this note. pub fn protocol(&self) -> ShieldedProtocol { match self { Note::Sapling(_) => ShieldedProtocol::Sapling, @@ -340,16 +341,6 @@ impl ReceivedNote { } } -impl ReceivedNote { - pub fn protocol(&self) -> ShieldedProtocol { - match self.note() { - Note::Sapling(_) => ShieldedProtocol::Sapling, - #[cfg(feature = "orchard")] - Note::Orchard(_) => ShieldedProtocol::Orchard, - } - } -} - impl sapling_fees::InputView for ReceivedNote { fn note_id(&self) -> &NoteRef { &self.note_id diff --git a/zcash_client_sqlite/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 35d906b83..680eec5fc 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -22,7 +22,6 @@ rustdoc-args = ["--cfg", "docsrs"] zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] } zcash_encoding.workspace = true zcash_primitives.workspace = true -orchard.workspace = true # Dependencies exposed in a public API: # (Breaking upgrades to these require a breaking upgrade to this crate.) @@ -43,6 +42,7 @@ jubjub.workspace = true secrecy.workspace = true # - Shielded protocols +orchard.workspace = true sapling.workspace = true # - Note commitment trees diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 84cf87f89..ddb6765ef 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -41,6 +41,8 @@ and this library adheres to Rust's notion of - `NonNegativeAmount::const_from_u64` - `NonNegativeAmount::from_nonnegative_i64_le_bytes` - `NonNegativeAmount::to_i64_le_bytes` + - `NonNegativeAmount::is_zero` + - `NonNegativeAmount::is_positive` - `impl From<&NonNegativeAmount> for Amount` - `impl From for u64` - `impl From for zcash_primitives::sapling::value::NoteValue` diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 3474e74ea..766368202 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -297,6 +297,16 @@ impl NonNegativeAmount { pub fn to_i64_le_bytes(self) -> [u8; 8] { self.0.to_i64_le_bytes() } + + /// Returns whether or not this `NonNegativeAmount` is the zero value. + pub fn is_zero(&self) -> bool { + self == &NonNegativeAmount::ZERO + } + + /// Returns whether or not this `NonNegativeAmount` is positive. + pub fn is_positive(&self) -> bool { + self > &NonNegativeAmount::ZERO + } } impl From for Amount {