From 46f1e7b4e894a7417bb1eb6dae9afa73546d324a Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sun, 28 Jul 2024 00:20:46 +0100 Subject: [PATCH] Report proposal errors earlier where possible, and split some error cases. Signed-off-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 28 ++++- zcash_client_backend/src/data_api/error.rs | 47 ++++----- zcash_client_backend/src/data_api/wallet.rs | 58 +++++------ zcash_client_backend/src/fees.rs | 14 +++ zcash_client_backend/src/fees/common.rs | 5 + zcash_client_backend/src/proposal.rs | 109 +++++++++++++++++--- 6 files changed, 181 insertions(+), 80 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 8d8b49e082..a7ccacb604 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -67,8 +67,25 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method when the "transparent-inputs" feature is enabled. - `WalletWrite` has new methods `import_account_hd` and `import_account_ufvk`. - - `error::Error` has new `Address` and (when the "transparent-inputs" feature - is enabled) `PaysEphemeralTransparentAddress` variants. + - `error::Error` and `proposal::ProposalError`: + - `ProposalError` has new variants `SpendsChange`, `EphemeralOutputLeftUnspent`, + `PaysTexFromShielded`, `SpendsPaymentFromUnsupportedPool`, and + `PaysUnsupportedPoolRecipient` (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 new `Address` and (when the "transparent-inputs" feature + is enabled) `PaysEphemeralTransparentAddress` variants. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. - `DecryptedTransaction::new` takes an additional `mined_height` argument. - `zcash_client_backend::data_api::fees` @@ -83,11 +100,12 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail outputs. Passing `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::proposal::ProposalError` has new variants - `SpendsChange`, `EphemeralOutputLeftUnspent`, and `PaysTexFromShielded`. - (the last two are conditional on the "transparent-inputs" feature). - `zcash_client_backend::proto`: - `ProposalDecodingError` has a new variant `InvalidEphemeralRecipient`. - `proposal::Proposal::{from_standard_proposal, try_into_standard_proposal}` diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index fd1d35aaae..2abf5a6171 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,14 +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. - 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, @@ -64,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), @@ -123,12 +118,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).", - ), + Error::ProposalNotSupported(e) => { + // The `ProposalError` message is complete in this context too. + write!(f, "{}", e) + } Error::KeyNotRecognized => { write!( f, @@ -149,7 +142,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 {}", @@ -186,6 +178,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, } @@ -200,7 +193,14 @@ 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(_) => Error::ProposalNotSupported(e), + + // These errors concern balance or structural validity. + _ => Error::Proposal(e), + } } } @@ -221,7 +221,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 ef4f70e7be..1096367faa 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -679,9 +679,15 @@ where Ok(NonEmpty::from_vec(txids).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( @@ -706,41 +712,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. - #[allow(clippy::never_loop)] - 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 (sapling_anchor, sapling_inputs) = if proposal_step.involves(PoolType::Shielded(ShieldedProtocol::Sapling)) { @@ -1078,7 +1059,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) => { @@ -1102,11 +1085,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); @@ -1139,8 +1124,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")] { @@ -1162,8 +1148,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!() } } } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index fea9424243..629edf3d7b 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 f1b0b7319e..d64e0032bf 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -187,6 +187,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(); @@ -432,6 +435,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 e88dbcf8d5..5a68209f03 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,18 +48,34 @@ 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, /// The change strategy provided to input selection failed to correctly generate an ephemeral /// change output when needed for sending to a TEX address. #[cfg(feature = "transparent-inputs")] EphemeralOutputsInvalid, + + // 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), } impl Display for ProposalError { @@ -104,7 +121,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")] @@ -123,6 +140,33 @@ impl Display for ProposalError { f, "The change strategy provided to input selection failed to correctly generate an ephemeral change output when needed for sending to a TEX address." ), + + #[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."#, + ), + + // 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), } } } @@ -415,6 +459,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() @@ -430,6 +477,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| { @@ -437,18 +485,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::, _>>()? @@ -459,6 +531,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)?; @@ -550,7 +625,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()