If a change memo is supplied, it should not be used in the second step

of a ZIP 320 proposal.

Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

zcash_client_backend/CHANGELOG.md

@@ -71,11 +71,12 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
     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
     shielded pool.
-  - `ChangeStrategy::compute_balance`: this trait method has two additional
+  - `ChangeStrategy::compute_balance`: this trait method has three additional
     parameters when the "transparent-inputs" feature is enabled. These are
-    used to specify amounts of additional transparent P2PKH inputs and outputs,
+    used to specify whether the change memo should be ignored, and the amounts
-    for which `InputView` or `OutputView` instances may not be available.
+    of additional transparent P2PKH inputs and outputs. Passing `false` for
-    Empty slices can be passed to obtain the previous behaviour.
+    `ignore_change_memo` and empty slices for the other two arguments will
+    retain the previous behaviour.
 - `zcash_client_backend::input_selection::GreedyInputSelectorError` has a
   new variant `UnsupportedTexAddress`.
 - `zcash_client_backend::proto::ProposalDecodingError` has a new variant

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -475,6 +475,8 @@ where
                         &orchard_fees::EmptyBundleView,
                         &self.dust_output_policy,
                         #[cfg(feature = "transparent-inputs")]
+                        true,
+                        #[cfg(feature = "transparent-inputs")]
                         &[NonNegativeAmount::ZERO],
                         #[cfg(feature = "transparent-inputs")]
                         &[],
@@ -494,6 +496,8 @@ where
                     &orchard_fees::EmptyBundleView,
                     &self.dust_output_policy,
                     #[cfg(feature = "transparent-inputs")]
+                    true,
+                    #[cfg(feature = "transparent-inputs")]
                     &[tr1_required_input_value],
                     #[cfg(feature = "transparent-inputs")]
                     &[],
@@ -574,6 +578,8 @@ where
                 ),
                 &self.dust_output_policy,
                 #[cfg(feature = "transparent-inputs")]
+                false,
+                #[cfg(feature = "transparent-inputs")]
                 &[],
                 #[cfg(feature = "transparent-inputs")]
                 &ephemeral_output_amounts,
@@ -764,6 +770,8 @@ where
             &orchard_fees::EmptyBundleView,
             &self.dust_output_policy,
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -785,6 +793,8 @@ where
                     &orchard_fees::EmptyBundleView,
                     &self.dust_output_policy,
                     #[cfg(feature = "transparent-inputs")]
+                    false,
+                    #[cfg(feature = "transparent-inputs")]
                     &[],
                     #[cfg(feature = "transparent-inputs")]
                     &[],

zcash_client_backend/src/fees.rs

@@ -356,6 +356,7 @@ pub trait ChangeStrategy {
         sapling: &impl sapling::BundleView<NoteRefT>,
         #[cfg(feature = "orchard")] orchard: &impl orchard::BundleView<NoteRefT>,
         dust_output_policy: &DustOutputPolicy,
+        #[cfg(feature = "transparent-inputs")] ignore_change_memo: bool,
         #[cfg(feature = "transparent-inputs")] ephemeral_input_amounts: &[NonNegativeAmount],
         #[cfg(feature = "transparent-inputs")] ephemeral_output_amounts: &[NonNegativeAmount],
     ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>>;

zcash_client_backend/src/fees/fixed.rs

@@ -66,9 +66,13 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
         sapling: &impl sapling_fees::BundleView<NoteRefT>,
         #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
         dust_output_policy: &DustOutputPolicy,
+        #[cfg(feature = "transparent-inputs")] ignore_change_memo: bool,
         #[cfg(feature = "transparent-inputs")] ephemeral_input_amounts: &[NonNegativeAmount],
         #[cfg(feature = "transparent-inputs")] ephemeral_output_amounts: &[NonNegativeAmount],
     ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
+        #[cfg(not(feature = "transparent-inputs"))]
+        let ignore_change_memo = false;
+
         single_change_output_balance(
             params,
             &self.fee_rule,
@@ -80,7 +84,11 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             orchard,
             dust_output_policy,
             self.fee_rule().fixed_fee(),
-            self.change_memo.clone(),
+            if ignore_change_memo {
+                None
+            } else {
+                self.change_memo.clone()
+            },
             self.fallback_change_pool,
             #[cfg(feature = "transparent-inputs")]
             ephemeral_input_amounts,
@@ -142,6 +150,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -191,6 +201,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],

zcash_client_backend/src/fees/standard.rs

@@ -68,6 +68,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
         sapling: &impl sapling_fees::BundleView<NoteRefT>,
         #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
         dust_output_policy: &DustOutputPolicy,
+        #[cfg(feature = "transparent-inputs")] ignore_change_memo: bool,
         #[cfg(feature = "transparent-inputs")] ephemeral_input_amounts: &[NonNegativeAmount],
         #[cfg(feature = "transparent-inputs")] ephemeral_output_amounts: &[NonNegativeAmount],
     ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
@@ -88,6 +89,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                 orchard,
                 dust_output_policy,
                 #[cfg(feature = "transparent-inputs")]
+                ignore_change_memo,
+                #[cfg(feature = "transparent-inputs")]
                 ephemeral_input_amounts,
                 #[cfg(feature = "transparent-inputs")]
                 ephemeral_output_amounts,
@@ -108,6 +111,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                 orchard,
                 dust_output_policy,
                 #[cfg(feature = "transparent-inputs")]
+                ignore_change_memo,
+                #[cfg(feature = "transparent-inputs")]
                 ephemeral_input_amounts,
                 #[cfg(feature = "transparent-inputs")]
                 ephemeral_output_amounts,
@@ -128,6 +133,8 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                 orchard,
                 dust_output_policy,
                 #[cfg(feature = "transparent-inputs")]
+                ignore_change_memo,
+                #[cfg(feature = "transparent-inputs")]
                 ephemeral_input_amounts,
                 #[cfg(feature = "transparent-inputs")]
                 ephemeral_output_amounts,

zcash_client_backend/src/fees/zip317.rs

@@ -68,6 +68,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
         sapling: &impl sapling_fees::BundleView<NoteRefT>,
         #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
         dust_output_policy: &DustOutputPolicy,
+        #[cfg(feature = "transparent-inputs")] ignore_change_memo: bool,
         #[cfg(feature = "transparent-inputs")] ephemeral_input_amounts: &[NonNegativeAmount],
         #[cfg(feature = "transparent-inputs")] ephemeral_output_amounts: &[NonNegativeAmount],
     ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
@@ -203,6 +204,9 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             }
         }
 
+        #[cfg(not(feature = "transparent-inputs"))]
+        let ignore_change_memo = false;
+
         single_change_output_balance(
             params,
             &self.fee_rule,
@@ -214,7 +218,11 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             orchard,
             dust_output_policy,
             self.fee_rule.marginal_fee(),
-            self.change_memo.clone(),
+            if ignore_change_memo {
+                None
+            } else {
+                self.change_memo.clone()
+            },
             self.fallback_change_pool,
             #[cfg(feature = "transparent-inputs")]
             ephemeral_input_amounts,
@@ -283,6 +291,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -330,6 +340,8 @@ mod tests {
             ),
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -386,6 +398,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             dust_output_policy,
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -433,6 +447,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -480,6 +496,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -533,6 +551,8 @@ mod tests {
                 Some(NonNegativeAmount::const_from_u64(1000)),
             ),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -597,6 +617,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             dust_output_policy,
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],
@@ -653,6 +675,8 @@ mod tests {
             &orchard_fees::EmptyBundleView,
             &DustOutputPolicy::default(),
             #[cfg(feature = "transparent-inputs")]
+            false,
+            #[cfg(feature = "transparent-inputs")]
             &[],
             #[cfg(feature = "transparent-inputs")]
             &[],

zcash_client_sqlite/src/testing/pool.rs

@@ -302,6 +302,8 @@ pub(crate) fn send_single_step_proposed_transfer<T: ShieldedPoolTester>() {
 
 #[cfg(feature = "transparent-inputs")]
 pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
+    use std::str::FromStr;
+
     use zcash_client_backend::fees::ChangeValue;
 
     let mut st = TestBuilder::new()
@@ -346,8 +348,7 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
             TransparentAddress::PublicKeyHash(data) => Address::Tex(data),
             _ => unreachable!(),
         };
-        // As of this commit, change memos are not correctly handled in ZIP 320 proposals.
+        let change_memo = Some(Memo::from_str("change").expect("valid memo").encode());
-        let change_memo = None;
 
         // We use `st.propose_standard_transfer` here in order to also test round-trip
         // serialization of the proposal.