Report proposal errors earlier and more precisely where possible

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}`

zcash_client_backend/src/data_api/error.rs

@@ -14,7 +14,6 @@
 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 @@
     /// 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 @@
     /// 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<UnifiedAddress>),
 
@@ -123,12 +118,10 @@
             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 @@
             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 @@
             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<DE, CE, SE, FE> From<ProposalError> for Error<DE, CE, SE, FE> {
     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 @@
         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,

zcash_client_backend/src/data_api/wallet.rs

@@ -679,9 +679,15 @@
     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<DbT, ParamsT, InputsErrT, FeeRuleT, N>(
@@ -706,41 +712,16 @@
     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 @@
             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 @@
             }
             #[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 @@
                 ))
             }
             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 @@
                 }
             }
             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!()
             }
         }
     }

zcash_client_backend/src/fees.rs

@@ -126,6 +126,9 @@
 
 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<ChangeValue>,
         fee_required: NonNegativeAmount,
@@ -137,6 +140,17 @@
             .sum::<Option<NonNegativeAmount>>()
             .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,

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())
 }