diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 7c5b19843..2eadf3244 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -56,11 +56,25 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `WalletRead` has new `get_reserved_ephemeral_addresses` and `get_transparent_address_metadata` methods. - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method. - - `error::Error` has a new `Address` variant. + - `error::Error` and `proposal::ProposalError`: + - `ProposalError` has new variants `SpendsChange`, `EphemeralOutputLeftUnspent`, + `PaysTexFromShielded`, `TooLarge`, `SpendsPaymentFromUnsupportedPool`, + `PaysUnsupportedPoolRecipient`, and `PaysUnsupportedTexRecipient` + (some of which are conditional on the "transparent-inputs" feature). + Of these, `SpendsChange` and `SpendsPaymentFromUnsupportedPool` are + currently reported when a `Proposal` or one of its `Step`s is constructed, + and the others are reported from `create_proposed_transactions`. + This may be changed in future to report these errors earlier; callers + should not rely on being able to construct `Proposal`s or `Step`s that + would result in these errors if a transaction were created from them. + - The `Error::ProposalNotSupported` variant now has an argument of type + `ProposalError` giving the more specific error. This will be used to + report the errors mentioned above that have "Unsupported" in their + names. + - The `Error::UnsupportedChangeType` variant has been removed since it + cannot occur (see `data_api::fees::TransactionBalance` below). + - `Error` has a new variant `Address`. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. -- `zcash_client_backend::proto::proposal::Proposal::{from_standard_proposal, - try_into_standard_proposal}` each no longer require a `consensus::Parameters` - argument. - `zcash_client_backend::data_api::fees` - When the "transparent-inputs" feature is enabled, `ChangeValue` can also represent an ephemeral transparent output in a proposal. Accordingly, the @@ -73,10 +87,16 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail outputs. Passing `&EphemeralParameters::NONE` will retain the previous behaviour (and is necessary when the "transparent-inputs" feature is not enabled). + - `TransactionBalance::new` now enforces that the change and ephemeral + outputs are for supported pools, and will return an error if they are + not. This makes `data_api::error::Error::UnsupportedChangeType` impossible, + so it has been removed as mentioned above. - `zcash_client_backend::input_selection::GreedyInputSelectorError` has a new variant `UnsupportedTexAddress`. -- `zcash_client_backend::proto::ProposalDecodingError` has a new variant - `InvalidEphemeralRecipient`. +- `zcash_client_backend::proto`: + - `ProposalDecodingError` has a new variant `InvalidEphemeralRecipient`. + - `proposal::Proposal::{from_standard_proposal, try_into_standard_proposal}` + each no longer require a `consensus::Parameters` argument. - `zcash_client_backend::wallet::Recipient` variants have changed. Instead of wrapping protocol-address types, the `External` and `InternalAccount` variants now wrap a `zcash_address::ZcashAddress`. This simplifies the process of diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 371d57706..1157071f3 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -14,7 +14,6 @@ use zcash_primitives::transaction::{ use crate::address::UnifiedAddress; use crate::data_api::wallet::input_selection::InputSelectorError; use crate::proposal::ProposalError; -use crate::PoolType; #[cfg(feature = "transparent-inputs")] use zcash_primitives::legacy::TransparentAddress; @@ -33,15 +32,17 @@ pub enum Error { /// An error in note selection NoteSelection(SelectionError), - /// An error in transaction proposal construction + /// An error in transaction proposal construction. This indicates that the proposal + /// violated balance or structural constraints. Proposal(ProposalError), - /// The proposal was structurally valid, but tried to do one of these unsupported things: - /// * spend a prior shielded output; - /// * pay to an output pool for which the corresponding feature is not enabled; - /// * pay to a TEX address if the "transparent-inputs" feature is not enabled; - /// or exceeded an implementation limit. - ProposalNotSupported, + /// A proposal was structurally valid, but not supported by the current feature or + /// chain configuration. + /// + /// This is indicative of a programming error; a transaction proposal that presumes + /// support for the specified pool was decoded or executed using an application that + /// does not provide such support. + ProposalNotSupported(ProposalError), /// No account could be found corresponding to a provided spending key. KeyNotRecognized, @@ -65,13 +66,6 @@ pub enum Error { /// It is forbidden to provide a memo when constructing a transparent output. MemoForbidden, - /// Attempted to send change to an unsupported pool. - /// - /// This is indicative of a programming error; execution of a transaction proposal that - /// presumes support for the specified pool was performed using an application that does not - /// provide such support. - UnsupportedChangeType(PoolType), - /// Attempted to create a spend to an unsupported Unified Address receiver NoSupportedReceivers(Box), @@ -119,12 +113,10 @@ where Error::Proposal(e) => { write!(f, "Input selection attempted to construct an invalid proposal: {}", e) } - Error::ProposalNotSupported => write!( - f, - "The proposal was valid but tried to do something that is not supported \ - (spend shielded outputs of prior transaction steps or use a feature that \ - is not enabled), or exceeded an implementation limit.", - ), + Error::ProposalNotSupported(e) => { + // The `ProposalError` message is complete in this context too. + write!(f, "{}", e) + } Error::KeyNotRecognized => { write!( f, @@ -145,7 +137,6 @@ where Error::ScanRequired => write!(f, "Must scan blocks first"), 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::UnsupportedChangeType(t) => write!(f, "Attempted to send change to an unsupported pool type: {}", t), Error::NoSupportedReceivers(ua) => write!( f, "A recipient's unified address does not contain any receivers to which the wallet can send funds; required one of {}", @@ -178,6 +169,7 @@ where Error::CommitmentTree(e) => Some(e), Error::NoteSelection(e) => Some(e), Error::Proposal(e) => Some(e), + Error::ProposalNotSupported(e) => Some(e), Error::Builder(e) => Some(e), _ => None, } @@ -192,7 +184,15 @@ impl From> for Error { impl From for Error { fn from(e: ProposalError) -> Self { - Error::Proposal(e) + match e { + // These errors concern feature support or unimplemented functionality. + ProposalError::SpendsPaymentFromUnsupportedPool(_) + | ProposalError::PaysUnsupportedPoolRecipient(_) + | ProposalError::PaysUnsupportedTexRecipient => Error::ProposalNotSupported(e), + + // These errors concern balance or structural validity. + _ => Error::Proposal(e), + } } } @@ -213,7 +213,8 @@ impl From> for Error match e { InputSelectorError::DataSource(e) => Error::DataSource(e), InputSelectorError::Selection(e) => Error::NoteSelection(e), - InputSelectorError::Proposal(e) => Error::Proposal(e), + // Choose `Error::Proposal` or `Error::ProposalNotSupported` as applicable. + InputSelectorError::Proposal(e) => e.into(), InputSelectorError::InsufficientFunds { available, required, diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 439e8a2b1..268d75630 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -645,9 +645,15 @@ where .expect("proposal.steps is NonEmpty")) } -// `unused_transparent_outputs` maps `StepOutput`s for transparent outputs -// that have not been consumed so far, to the corresponding pair of -// `TransparentAddress` and `Outpoint`. +/// Creates a transaction corresponding to a proposal step. +/// +/// Since this is only called by `create_proposed_transactions` which takes +/// a fully validated `Proposal` as input, we may assume that the proposal, +/// including references to prior steps, is structurally valid. +/// +/// `unused_transparent_outputs` maps `StepOutput`s for transparent outputs +/// that have not been consumed so far, to the corresponding pair of +/// `TransparentAddress` and `Outpoint`. #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] fn create_proposed_transaction( @@ -671,40 +677,16 @@ where ParamsT: consensus::Parameters + Clone, FeeRuleT: FeeRule, { - #[cfg(feature = "transparent-inputs")] + #[allow(unused_variables)] let step_index = prior_step_results.len(); // We only support spending transparent payments or transparent ephemeral outputs from a - // prior step (when "transparent-inputs" is enabled). + // prior step (when "transparent-inputs" is enabled). We don't need to check that here + // because it is checked by construction in `proposal::Step`. // // TODO: Maybe support spending prior shielded outputs at some point? Doing so would require // a higher-level approach in the wallet that waits for transactions with shielded outputs to // be mined and only then attempts to perform the next step. - for input_ref in proposal_step.prior_step_inputs() { - let (prior_step, _) = prior_step_results - .get(input_ref.step_index()) - .ok_or(ProposalError::ReferenceError(*input_ref))?; - - #[allow(unused_variables)] - let output_pool = match input_ref.output_index() { - StepOutputIndex::Payment(i) => prior_step.payment_pools().get(&i).cloned(), - StepOutputIndex::Change(i) => match prior_step.balance().proposed_change().get(i) { - Some(change) if !change.is_ephemeral() => { - return Err(ProposalError::SpendsChange(*input_ref).into()); - } - other => other.map(|change| change.output_pool()), - }, - } - .ok_or(ProposalError::ReferenceError(*input_ref))?; - - // Return an error on trying to spend a prior output that is not supported. - #[cfg(feature = "transparent-inputs")] - if output_pool != PoolType::TRANSPARENT { - return Err(Error::ProposalNotSupported); - } - #[cfg(not(feature = "transparent-inputs"))] - return Err(Error::ProposalNotSupported); - } let account_id = wallet_db .get_account_for_ufvk(&usk.to_unified_full_viewing_key()) @@ -1037,7 +1019,9 @@ where Address::Unified(ua) => match output_pool { #[cfg(not(feature = "orchard"))] PoolType::Shielded(ShieldedProtocol::Orchard) => { - return Err(Error::ProposalNotSupported); + // TODO: check this in `Step::from_parts`. We cannot do so currently + // because we don't know the `network_type` there. + return Err(ProposalError::PaysUnsupportedPoolRecipient(*output_pool).into()); } #[cfg(feature = "orchard")] PoolType::Shielded(ShieldedProtocol::Orchard) => { @@ -1061,11 +1045,13 @@ where } #[cfg(not(feature = "transparent-inputs"))] Address::Tex(_) => { - return Err(Error::ProposalNotSupported); + // TODO: check this in `Step::from_parts`. + return Err(ProposalError::PaysUnsupportedPoolRecipient(*output_pool).into()); } #[cfg(feature = "transparent-inputs")] Address::Tex(data) => { if has_shielded_inputs { + // TODO: check this in `Step::from_parts`. return Err(ProposalError::PaysTexFromShielded.into()); } let to = TransparentAddress::PublicKeyHash(data); @@ -1098,8 +1084,9 @@ where )) } PoolType::Shielded(ShieldedProtocol::Orchard) => { + // `TransactionBalance` enforces that change is for a supported pool. #[cfg(not(feature = "orchard"))] - return Err(Error::UnsupportedChangeType(output_pool)); + unreachable!(); #[cfg(feature = "orchard")] { @@ -1121,8 +1108,10 @@ where } } PoolType::Transparent => { + // `ChangeValue` cannot be constructed with a transparent output pool + // if "transparent-inputs" is not enabled. #[cfg(not(feature = "transparent-inputs"))] - return Err(Error::UnsupportedChangeType(output_pool)); + unreachable!() } } } @@ -1142,7 +1131,7 @@ where }) .collect(); if ephemeral_outputs.len() > i32::MAX as usize { - return Err(Error::ProposalNotSupported); + return Err(ProposalError::TooLarge("too many ephemeral outputs".to_owned()).into()); } let addresses_and_metadata = wallet_db .reserve_next_n_ephemeral_addresses( diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 51867a9f1..43e386d77 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -126,6 +126,9 @@ pub struct TransactionBalance { impl TransactionBalance { /// Constructs a new balance from its constituent parts. + /// + /// Returns `Err(())` if the proposed change is for an unsupported pool or + /// the total value overflows. pub fn new( proposed_change: Vec, fee_required: NonNegativeAmount, @@ -137,6 +140,17 @@ impl TransactionBalance { .sum::>() .ok_or(())?; + #[cfg(not(feature = "orchard"))] + if proposed_change + .iter() + .any(|cv| cv.output_pool() == PoolType::ORCHARD) + { + return Err(()); + } + // A `ChangeValue` in the transparent pool can only be ephemeral by + // construction, and only when "transparent-inputs" is enabled, so we + // do not need to check that. + Ok(Self { proposed_change, fee_required, diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index d0b3eebca..82235b626 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -186,6 +186,9 @@ where let (change_pool, sapling_change, orchard_change) = single_change_output_policy(&net_flows, fallback_change_pool); + #[cfg(not(feature = "orchard"))] + assert!(change_pool != ShieldedProtocol::Orchard); + // We don't create a fully-transparent transaction if a change memo is used. let transparent = net_flows.is_transparent() && change_memo.is_none(); @@ -431,6 +434,8 @@ where .map(ChangeValue::ephemeral_transparent), ); + // Any error here can only be overflow, because we checked that `change_pool` + // is supported and only constructed `change` to that pool. TransactionBalance::new(change, fee).map_err(|_| overflow()) } diff --git a/zcash_client_backend/src/proposal.rs b/zcash_client_backend/src/proposal.rs index e67e3e676..e7b2c548b 100644 --- a/zcash_client_backend/src/proposal.rs +++ b/zcash_client_backend/src/proposal.rs @@ -18,7 +18,8 @@ use crate::{ PoolType, ShieldedProtocol, }; -/// Errors that can occur in construction of a [`Step`]. +/// A [`Proposal`] or a proposal [`Step`] violated balance or structural constraints, +/// or is unsupported. #[derive(Debug, Clone)] pub enum ProposalError { /// The total output value of the transaction request is not a valid Zcash amount. @@ -47,14 +48,36 @@ pub enum ProposalError { /// There was a mismatch between the payments in the proposal's transaction request /// and the payment pool selection values. PaymentPoolsMismatch, - /// The proposal tried to spend a change output. Mark the `ChangeValue` as ephemeral if this is intended. + /// The proposal tried to spend a change output. Mark the `ChangeValue` as ephemeral + /// if this is intended. SpendsChange(StepOutput), /// A proposal step created an ephemeral output that was not spent in any later step. #[cfg(feature = "transparent-inputs")] EphemeralOutputLeftUnspent(StepOutput), - /// The proposal included a payment to a TEX address and a spend from a shielded input in the same step. + /// The proposal included a payment to a TEX address and a spend from a shielded input + /// in the same step. #[cfg(feature = "transparent-inputs")] PaysTexFromShielded, + /// A proposal step would would result in an invalid transaction due to limits on size, + /// number of inputs, or number of outputs. + TooLarge(String), + + // The errors below are concerned with support rather than structural validity. + // However, some unsupported cases are already checked at construction of `Proposal` + // or `Step`, and we want to allow for all of them to be. So, in practice these + // errors either can now, or might in future be reported in the same places as the + // ones above. When a `ProposalError` is converted to a `data_api::Error`, they will + // be represented via the `ProposalNotSupported` variant rather than the `Proposal` + // variant. + // ---- + /// The proposal tried to spend a prior shielded payment output; or tried to spend a + /// prior transparent payment output when the "transparent-inputs" feature is disabled. + SpendsPaymentFromUnsupportedPool(PoolType), + /// The proposal tried to pay to a recipient in a feature-disabled pool. + PaysUnsupportedPoolRecipient(PoolType), + /// The proposal tried to pay to a TEX address when the "transparent-inputs" feature + /// is disabled. + PaysUnsupportedTexRecipient, } impl Display for ProposalError { @@ -100,7 +123,7 @@ impl Display for ProposalError { ), ProposalError::SpendsChange(r) => write!( f, - "The proposal attempts to spends the change output created at step {:?}.", + "The proposal attempts to spend the change output created at step {:?}.", r, ), #[cfg(feature = "transparent-inputs")] @@ -114,6 +137,48 @@ impl Display for ProposalError { f, "The proposal included a payment to a TEX address and a spend from a shielded input in the same step.", ), + ProposalError::TooLarge(s) => write!( + f, + r#"A proposal step would result in an invalid transaction due to limits on size, \ + number of inputs, or number of outputs: {}"#, + s, + ), + + #[cfg(not(feature = "transparent-inputs"))] + ProposalError::SpendsPaymentFromUnsupportedPool(PoolType::Transparent) => write!( + f, + r#"A proposal tried to spend a prior transparent output, which is not supported \ + because the "transparent-inputs" feature is not enabled."#, + ), + ProposalError::SpendsPaymentFromUnsupportedPool(pool @ PoolType::Shielded(_)) => write!( + f, + "A proposal tried to spend a prior shielded ({}) output, which is not supported.", + pool, + ), + #[cfg(not(feature = "orchard"))] + ProposalError::PaysUnsupportedPoolRecipient(PoolType::Shielded( + ShieldedProtocol::Orchard, + )) => write!( + f, + r#"A proposal tried to pay to a recipient in the Orchard pool, which is not supported \ + because the "orchard" feature is not enabled."#, + ), + #[cfg(not(feature = "transparent-inputs"))] + ProposalError::PaysUnsupportedTexRecipient => write!( + f, + r#"A proposal tried to pay to a TEX address recipient, which is not supported \ + because the "transparent-inputs" feature is not enabled."#, + ), + + // List cases that the compiler needs to check exhaustivity so that we don't need a catch-all. + #[cfg(feature = "transparent-inputs")] + ProposalError::SpendsPaymentFromUnsupportedPool(PoolType::Transparent) => + unreachable!("SpendsPaymentFromUnsupportedPool(Transparent) but it should be supported"), + ProposalError::PaysUnsupportedPoolRecipient(pool) => + unreachable!("PaysUnsupportedPoolRecipient({}) but it should be supported", pool), + #[cfg(feature = "transparent-inputs")] + ProposalError::PaysUnsupportedTexRecipient => + unreachable!("PaysUnsupportedTexRecipient but it should be supported"), } } } @@ -406,6 +471,9 @@ impl Step { return Err(ProposalError::PaymentPoolsMismatch); } } + // Since `payments` and `payment_pools` are the same size, every element + // of `payments` must also have a corresponding entry with the same index + // in `payment_pools`. let transparent_input_total = transparent_inputs .iter() @@ -421,6 +489,7 @@ impl Step { .fold(Some(NonNegativeAmount::ZERO), |acc, a| (acc? + a)) .ok_or(ProposalError::Overflow)?; + // Also check that the spends of prior outputs are supported. let prior_step_input_total = prior_step_inputs .iter() .map(|s_ref| { @@ -428,18 +497,42 @@ impl Step { .get(s_ref.step_index) .ok_or(ProposalError::ReferenceError(*s_ref))?; Ok(match s_ref.output_index { - StepOutputIndex::Payment(i) => step - .transaction_request - .payments() - .get(&i) - .ok_or(ProposalError::ReferenceError(*s_ref))? - .amount(), - StepOutputIndex::Change(i) => step - .balance - .proposed_change() - .get(i) - .ok_or(ProposalError::ReferenceError(*s_ref))? - .value(), + StepOutputIndex::Payment(i) => { + let payment = step + .transaction_request + .payments() + .get(&i) + .ok_or(ProposalError::ReferenceError(*s_ref))?; + + match step + .payment_pools() + .get(&i) + .expect("payments and payment_pools are 1:1, checked when the prior step was constructed") + { + PoolType::Transparent => payment.amount(), + &other => Err(ProposalError::SpendsPaymentFromUnsupportedPool(other))?, + } + } + StepOutputIndex::Change(i) => { + let cv = step + .balance + .proposed_change() + .get(i) + .ok_or(ProposalError::ReferenceError(*s_ref))?; + + if !cv.is_ephemeral() { + Err(ProposalError::SpendsChange(*s_ref))?; + } + assert_eq!(cv.output_pool(), PoolType::TRANSPARENT); + + // An ephemeral transparent `ChangeValue` could not have + // been constructed unless "transparent-inputs" is enabled. + #[cfg(not(feature = "transparent-inputs"))] + unreachable!(); + + #[cfg(feature = "transparent-inputs")] + cv.value() + } }) }) .collect::, _>>()? @@ -450,6 +543,9 @@ impl Step { let input_total = (transparent_input_total + shielded_input_total + prior_step_input_total) .ok_or(ProposalError::Overflow)?; + // TODO: check for unsupported and invalid payments here. We cannot do so + // currently because we don't have access to the `network_type`. + let request_total = transaction_request .total() .map_err(|_| ProposalError::RequestTotalInvalid)?; @@ -541,7 +637,7 @@ impl Step { self.balance .proposed_change() .iter() - .any(|c| c.output_pool() == pool_type) + .any(|cv| cv.output_pool() == pool_type) }; input_in_this_pool() || output_in_this_pool() || change_in_this_pool()