From c6520cf6a680fa21fdcf57af5ff543f7006471d0 Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Sat, 22 Jun 2024 19:20:36 +0100 Subject: [PATCH] Change the protobuf schema to explicitly specify whether a `ChangeValue` is ephemeral. This also fixes `try_into_standard_proposal` to allow decoding from the protobuf representation into a proposal that uses references to prior ephemeral transparent outputs, provided that the "transparent-inputs" feature is enabled. Signed-off-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 7 ++- zcash_client_backend/proto/proposal.proto | 29 +++++----- zcash_client_backend/src/data_api/wallet.rs | 51 ++++++++++------- .../src/data_api/wallet/input_selection.rs | 2 +- zcash_client_backend/src/fees.rs | 36 +++++++++--- zcash_client_backend/src/fees/common.rs | 2 +- zcash_client_backend/src/proto.rs | 31 ++++++++--- zcash_client_backend/src/proto/proposal.rs | 30 +++++----- zcash_client_sqlite/src/testing/pool.rs | 55 ++++++++++--------- 9 files changed, 154 insertions(+), 89 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 7c7eaeb84..7de96c9a9 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -29,7 +29,7 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `chain::BlockCache` trait, behind the `sync` feature flag. - `WalletRead::get_spendable_transparent_outputs`. - `zcash_client_backend::fees`: - - `ChangeValue::{transparent, shielded}` + - `ChangeValue::{ephemeral_transparent, shielded}` - `sapling::EmptyBundleView` - `orchard::EmptyBundleView` - `zcash_client_backend::proposal`: @@ -64,6 +64,9 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - The return type of `ChangeValue::output_pool`, and the type of the `output_pool` argument to `ChangeValue::new`, have changed from `ShieldedProtocol` to `zcash_protocol::PoolType`. + - `ChangeValue::new` takes an additional `is_ephemeral` parameter indicating + whether the value is ephemeral (i.e. for use in a subsequent proposal step) + or change. - The return type of `ChangeValue::new` is now optional; it returns `None` if a memo is given for the transparent pool. Use `ChangeValue::shielded` to avoid this error case when creating a `ChangeValue` known to be for a @@ -75,6 +78,8 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail Empty slices can be passed to obtain the previous behaviour. - `zcash_client_backend::input_selection::GreedyInputSelectorError` has a new variant `UnsupportedTexAddress`. +- `zcash_client_backend::proto::ProposalDecodingError` has a new variant + `InvalidEphemeralRecipient`. - `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/proto/proposal.proto b/zcash_client_backend/proto/proposal.proto index 0935d66a1..db548c6e2 100644 --- a/zcash_client_backend/proto/proposal.proto +++ b/zcash_client_backend/proto/proposal.proto @@ -73,14 +73,14 @@ message ReceivedOutput { uint64 value = 4; } -// A reference a payment in a prior step of the proposal. This payment must +// A reference to a payment in a prior step of the proposal. This payment must // belong to the wallet. message PriorStepOutput { uint32 stepIndex = 1; uint32 paymentIndex = 2; } -// A reference a change output from a prior step of the proposal. +// A reference to a change or ephemeral output from a prior step of the proposal. message PriorStepChange { uint32 stepIndex = 1; uint32 changeIndex = 2; @@ -112,26 +112,29 @@ enum FeeRule { // The proposed change outputs and fee value. message TransactionBalance { - // A list of change output values. - // - // Each `ChangeValue` for the transparent value pool must be consumed by - // a subsequent step. These represent ephemeral outputs that will each be - // given a unique t-address. + // A list of change or ephemeral output values. repeated ChangeValue proposedChange = 1; // The fee to be paid by the proposed transaction, in zatoshis. uint64 feeRequired = 2; } -// A proposed change output. If the transparent value pool is selected, -// the `memo` field must be null. +// A proposed change or ephemeral output. If the transparent value pool is +// selected, the `memo` field must be null. +// +// When the `isEphemeral` field of a `ChangeValue` is set, it represents +// an ephemeral output, which must be spent by a subsequent step. This is +// only supported for transparent outputs. Each ephemeral output will be +// given a unique t-address. message ChangeValue { - // The value of a change output to be created, in zatoshis. + // The value of a change or ephemeral output to be created, in zatoshis. uint64 value = 1; - // The value pool in which the change output should be created. + // The value pool in which the change or ephemeral output should be created. ValuePool valuePool = 2; - // The optional memo that should be associated with the newly created change output. - // Memos must not be present for transparent change outputs. + // The optional memo that should be associated with the newly created output. + // Memos must not be present for transparent outputs. MemoBytes memo = 3; + // Whether this is to be an ephemeral output. + bool isEphemeral = 4; } // An object wrapper for memo bytes, to facilitate representing the diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index dc4d47f90..77e6387dc 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -52,7 +52,7 @@ use crate::{ decrypt_transaction, fees::{self, DustOutputPolicy}, keys::UnifiedSpendingKey, - proposal::{Proposal, ProposalError, Step, StepOutputIndex}, + proposal::{Proposal, Step, StepOutputIndex}, wallet::{Note, OvkPolicy, Recipient}, zip321::{self, Payment}, PoolType, ShieldedProtocol, @@ -74,7 +74,11 @@ use zip32::Scope; #[cfg(feature = "transparent-inputs")] use { - crate::{fees::ChangeValue, proposal::StepOutput, wallet::TransparentAddressMetadata}, + crate::{ + fees::ChangeValue, + proposal::{ProposalError, StepOutput}, + wallet::TransparentAddressMetadata, + }, input_selection::ShieldingSelector, std::collections::HashMap, zcash_keys::encoding::AddressCodec, @@ -624,8 +628,10 @@ where step_results.push((step, step_result)); } - // Change step outputs represent ephemeral outputs that must be referenced exactly once. - // (We don't support transparent change.) + // Ephemeral outputs must be referenced exactly once. Currently this is all + // transparent outputs using `StepOutputIndex::Change`. + // TODO: if we support transparent change, this will need to be updated to + // not require it to be referenced by a later step. #[cfg(feature = "transparent-inputs")] if unused_transparent_outputs .into_keys() @@ -670,33 +676,40 @@ where ParamsT: consensus::Parameters + Clone, FeeRuleT: FeeRule, { - #[cfg(feature = "transparent-inputs")] + #[allow(unused_variables)] let step_index = prior_step_results.len(); - // Spending shielded outputs of prior multi-step transaction steps (either payments or change) - // is not supported. + // We only support spending transparent payments or ephemeral outputs from a prior step. // - // TODO: Maybe support this 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. + // 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. + #[cfg(feature = "transparent-inputs")] for input_ref in proposal_step.prior_step_inputs() { - let prior_pool = prior_step_results + let supported = prior_step_results .get(input_ref.step_index()) .and_then(|(prior_step, _)| match input_ref.output_index() { - StepOutputIndex::Payment(i) => prior_step.payment_pools().get(&i).cloned(), - StepOutputIndex::Change(i) => prior_step - .balance() - .proposed_change() - .get(i) - .map(|change| change.output_pool()), + StepOutputIndex::Payment(i) => prior_step + .payment_pools() + .get(&i) + .map(|&pool| pool == PoolType::TRANSPARENT), + StepOutputIndex::Change(i) => { + prior_step.balance().proposed_change().get(i).map(|change| { + change.is_ephemeral() && change.output_pool() == PoolType::TRANSPARENT + }) + } }) .ok_or(Error::Proposal(ProposalError::ReferenceError(*input_ref)))?; - // Return an error on trying to spend a prior shielded output. - if matches!(prior_pool, PoolType::Shielded(_)) { + // Return an error on trying to spend a prior shielded output or non-ephemeral change output. + if !supported { return Err(Error::ProposalNotSupported); } } + #[cfg(not(feature = "transparent-inputs"))] + if !proposal_step.prior_step_inputs().is_empty() { + return Err(Error::ProposalNotSupported); + } let account_id = wallet_db .get_account_for_ufvk(&usk.to_unified_full_viewing_key()) 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 776dc9869..6a8ece442 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -602,7 +602,7 @@ where // The ephemeral output should always be at the last change index. assert_eq!( *balance.proposed_change().last().expect("nonempty"), - ChangeValue::transparent(ephemeral_output_amounts[0]) + ChangeValue::ephemeral_transparent(ephemeral_output_amounts[0]) ); let ephemeral_stepoutput = StepOutput::new( 0, diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 3bf99f9fc..7e5e966a5 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -27,28 +27,44 @@ pub struct ChangeValue { output_pool: PoolType, value: NonNegativeAmount, memo: Option, + is_ephemeral: bool, } impl ChangeValue { - /// Constructs a new change value from its constituent parts. + /// Constructs a new change or ephemeral value from its constituent parts. + /// + /// Currently, the only supported combinations are change sent to a shielded + /// pool, or (if the "transparent-inputs" feature is enabled) an ephemeral + /// output to the transparent pool. pub fn new( output_pool: PoolType, value: NonNegativeAmount, memo: Option, + is_ephemeral: bool, ) -> Option { - (matches!(output_pool, PoolType::Shielded(_)) || memo.is_none()).then_some(Self { + match output_pool { + PoolType::Shielded(_) => !is_ephemeral, + #[cfg(feature = "transparent-inputs")] + PoolType::Transparent => is_ephemeral && memo.is_none(), + #[cfg(not(feature = "transparent-inputs"))] + PoolType::Transparent => false, + } + .then_some(Self { output_pool, value, memo, + is_ephemeral, }) } - /// Constructs a new change value that will be created as a transparent output. - pub fn transparent(value: NonNegativeAmount) -> Self { + /// Constructs a new ephemeral transparent output value. + #[cfg(feature = "transparent-inputs")] + pub fn ephemeral_transparent(value: NonNegativeAmount) -> Self { Self { output_pool: PoolType::TRANSPARENT, value, memo: None, + is_ephemeral: true, } } @@ -62,6 +78,7 @@ impl ChangeValue { output_pool: PoolType::Shielded(protocol), value, memo, + is_ephemeral: false, } } @@ -76,20 +93,25 @@ impl ChangeValue { Self::shielded(ShieldedProtocol::Orchard, value, memo) } - /// Returns the pool to which the change output should be sent. + /// Returns the pool to which the change or ephemeral output should be sent. pub fn output_pool(&self) -> PoolType { self.output_pool } - /// Returns the value of the change output to be created, in zatoshis. + /// Returns the value of the change or ephemeral output to be created, in zatoshis. pub fn value(&self) -> NonNegativeAmount { self.value } - /// Returns the memo to be associated with the change output. + /// Returns the memo to be associated with the output. pub fn memo(&self) -> Option<&MemoBytes> { self.memo.as_ref() } + + /// Whether this is to be an ephemeral output. + pub fn is_ephemeral(&self) -> bool { + self.is_ephemeral + } } /// The amount of change and fees required to make a transaction's inputs and diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index b77811abf..1d40d4966 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -400,7 +400,7 @@ where change.extend( ephemeral_output_amounts .iter() - .map(|&amount| ChangeValue::transparent(amount)), + .map(|&amount| ChangeValue::ephemeral_transparent(amount)), ); TransactionBalance::new(change, fee).map_err(|_| overflow()) diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 915d1d39e..f90967ae8 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -335,7 +335,7 @@ pub const PROPOSAL_SER_V1: u32 = 1; /// representation. #[derive(Debug, Clone)] pub enum ProposalDecodingError { - /// The encoded proposal contained no steps + /// The encoded proposal contained no steps. NoSteps, /// The ZIP 321 transaction request URI was invalid. Zip321(Zip321Error), @@ -366,6 +366,8 @@ pub enum ProposalDecodingError { TransparentMemo, /// Change outputs to the specified pool are not supported. InvalidChangeRecipient(PoolType), + /// Ephemeral outputs to the specified pool are not supported. + InvalidEphemeralRecipient(PoolType), } impl From for ProposalDecodingError { @@ -424,6 +426,11 @@ impl Display for ProposalDecodingError { "Change outputs to the {} pool are not supported.", pool_type ), + ProposalDecodingError::InvalidEphemeralRecipient(pool_type) => write!( + f, + "Ephemeral outputs to the {} pool are not supported.", + pool_type + ), } } } @@ -572,6 +579,7 @@ impl proposal::Proposal { memo: change.memo().map(|memo_bytes| proposal::MemoBytes { value: memo_bytes.as_slice().to_vec(), }), + is_ephemeral: change.is_ephemeral(), }) .collect(), fee_required: step.balance().fee_required().into(), @@ -662,7 +670,7 @@ impl proposal::Proposal { PoolType::Transparent => { #[cfg(not(feature = "transparent-inputs"))] return Err(ProposalDecodingError::ValuePoolNotSupported( - 1, + out.value_pool, )); #[cfg(feature = "transparent-inputs")] @@ -751,18 +759,27 @@ impl proposal::Proposal { .map_err(ProposalDecodingError::MemoInvalid) }) .transpose()?; - match cv.pool_type()? { - PoolType::Shielded(ShieldedProtocol::Sapling) => { + match (cv.pool_type()?, cv.is_ephemeral) { + (PoolType::Shielded(ShieldedProtocol::Sapling), false) => { Ok(ChangeValue::sapling(value, memo)) } #[cfg(feature = "orchard")] - PoolType::Shielded(ShieldedProtocol::Orchard) => { + (PoolType::Shielded(ShieldedProtocol::Orchard), false) => { Ok(ChangeValue::orchard(value, memo)) } - PoolType::Transparent if memo.is_some() => { + (PoolType::Transparent, _) if memo.is_some() => { Err(ProposalDecodingError::TransparentMemo) } - t => Err(ProposalDecodingError::InvalidChangeRecipient(t)), + #[cfg(feature = "transparent-inputs")] + (PoolType::Transparent, true) => { + Ok(ChangeValue::ephemeral_transparent(value)) + } + (pool, false) => { + Err(ProposalDecodingError::InvalidChangeRecipient(pool)) + } + (pool, true) => { + Err(ProposalDecodingError::InvalidEphemeralRecipient(pool)) + } } }) .collect::, _>>()?, diff --git a/zcash_client_backend/src/proto/proposal.rs b/zcash_client_backend/src/proto/proposal.rs index be8da848a..1ea321afc 100644 --- a/zcash_client_backend/src/proto/proposal.rs +++ b/zcash_client_backend/src/proto/proposal.rs @@ -72,7 +72,7 @@ pub struct ReceivedOutput { #[prost(uint64, tag = "4")] pub value: u64, } -/// A reference a payment in a prior step of the proposal. This payment must +/// A reference to a payment in a prior step of the proposal. This payment must /// belong to the wallet. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] @@ -82,7 +82,7 @@ pub struct PriorStepOutput { #[prost(uint32, tag = "2")] pub payment_index: u32, } -/// A reference a change output from a prior step of the proposal. +/// A reference to a change or ephemeral output from a prior step of the proposal. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct PriorStepChange { @@ -115,32 +115,36 @@ pub mod proposed_input { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct TransactionBalance { - /// A list of change output values. - /// - /// Each `ChangeValue` for the transparent value pool must be consumed by - /// a subsequent step. These represent ephemeral outputs that will each be - /// given a unique t-address. + /// A list of change or ephemeral output values. #[prost(message, repeated, tag = "1")] pub proposed_change: ::prost::alloc::vec::Vec, /// The fee to be paid by the proposed transaction, in zatoshis. #[prost(uint64, tag = "2")] pub fee_required: u64, } -/// A proposed change output. If the transparent value pool is selected, -/// the `memo` field must be null. +/// A proposed change or ephemeral output. If the transparent value pool is +/// selected, the `memo` field must be null. +/// +/// When the `isEphemeral` field of a `ChangeValue` is set, it represents +/// an ephemeral output, which must be spent by a subsequent step. This is +/// only supported for transparent outputs. Each ephemeral output will be +/// given a unique t-address. #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ChangeValue { - /// The value of a change output to be created, in zatoshis. + /// The value of a change or ephemeral output to be created, in zatoshis. #[prost(uint64, tag = "1")] pub value: u64, - /// The value pool in which the change output should be created. + /// The value pool in which the change or ephemeral output should be created. #[prost(enumeration = "ValuePool", tag = "2")] pub value_pool: i32, - /// The optional memo that should be associated with the newly created change output. - /// Memos must not be present for transparent change outputs. + /// The optional memo that should be associated with the newly created output. + /// Memos must not be present for transparent outputs. #[prost(message, optional, tag = "3")] pub memo: ::core::option::Option, + /// Whether this is to be an ephemeral output. + #[prost(bool, tag = "4")] + pub is_ephemeral: bool, } /// An object wrapper for memo bytes, to facilitate representing the /// `change_memo == None` case. diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 17dd6dfeb..cb0292bd2 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -302,7 +302,6 @@ pub(crate) fn send_single_step_proposed_transfer() { #[cfg(feature = "transparent-inputs")] pub(crate) fn send_multi_step_proposed_transfer() { - use zcash_address::ZcashAddress; use zcash_client_backend::fees::ChangeValue; let mut st = TestBuilder::new() @@ -313,12 +312,6 @@ pub(crate) fn send_multi_step_proposed_transfer() { let account = st.test_account().cloned().unwrap(); let dfvk = T::test_account_fvk(&st); - let fee_rule = StandardFeeRule::Zip317; - let input_selector = GreedyInputSelector::new( - standard::SingleOutputChangeStrategy::new(fee_rule, None, T::SHIELDED_PROTOCOL), - DustOutputPolicy::default(), - ); - let add_funds = |st: &mut TestState<_>, value| { let (h, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); st.scan_cached_blocks(h, 1); @@ -353,34 +346,37 @@ pub(crate) fn send_multi_step_proposed_transfer() { TransparentAddress::PublicKeyHash(data) => Address::Tex(data), _ => unreachable!(), }; - let request = zip321::TransactionRequest::new(vec![Payment::without_memo( - tex_addr.to_zcash_address(&st.network()), - amount, - )]) - .unwrap(); + // As of this commit, change memos are not correctly handled in ZIP 320 proposals. + let change_memo = None; + // We use `st.propose_standard_transfer` here in order to also test round-trip + // serialization of the proposal. let proposal = st - .propose_transfer( + .propose_standard_transfer::( account.account_id(), - &input_selector, - request, + StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), + &tex_addr, + amount, + None, + change_memo.clone(), + T::SHIELDED_PROTOCOL, ) .unwrap(); let steps: Vec<_> = proposal.steps().iter().cloned().collect(); assert_eq!(steps.len(), 2); + assert_eq!(steps[0].balance().fee_required(), expected_step0_fee); + assert_eq!(steps[1].balance().fee_required(), expected_step1_fee); assert_eq!( steps[0].balance().proposed_change(), [ - ChangeValue::shielded(T::SHIELDED_PROTOCOL, expected_step0_change, None), - ChangeValue::transparent((amount + expected_step1_fee).unwrap()), + ChangeValue::shielded(T::SHIELDED_PROTOCOL, expected_step0_change, change_memo), + ChangeValue::ephemeral_transparent((amount + expected_step1_fee).unwrap()), ] ); - assert_eq!(steps[0].balance().fee_required(), expected_step0_fee); assert_eq!(steps[1].balance().proposed_change(), []); - assert_eq!(steps[1].balance().fee_required(), expected_step1_fee); let create_proposed_result = st.create_proposed_transactions::( account.usk(), @@ -451,19 +447,24 @@ pub(crate) fn send_multi_step_proposed_transfer() { assert!(ephemeral0 != ephemeral1); add_funds(&mut st, value); - let request = zip321::TransactionRequest::new(vec![Payment::without_memo( - ZcashAddress::try_from_encoded(&ephemeral0).expect("valid address"), - amount, - )]) - .unwrap(); + + let ephemeral_taddr = Address::decode(&st.wallet().params, &ephemeral0).expect("valid address"); + assert_matches!( + ephemeral_taddr, + Address::Transparent(TransparentAddress::PublicKeyHash(_)) + ); // Attempting to use the same address again should cause an error. let proposal = st - .propose_transfer( + .propose_standard_transfer::( account.account_id(), - &input_selector, - request, + StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), + &ephemeral_taddr, + amount, + None, + None, + T::SHIELDED_PROTOCOL, ) .unwrap();