zcash_client_backend: Allow proposer to specify fallback change pool.

In the event that the pool to which change should be sent cannot automatically be determined based upon the inputs and outputs of a transaction, it is up to the caller to specify where change should be sent.
nuttycom · Feb 16, 2024 · fcbfdaa · fcbfdaa
1 parent daf88a1
commit fcbfdaa
Show file tree

Hide file tree

Showing 16 changed files with 254 additions and 101 deletions.
diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md
@@ -154,8 +154,9 @@ and this library adheres to Rust's notion of
       the database identifiers for its contained notes by universally quantifying
       the `NoteRef` type parameter.
     - It returns a `NonEmpty<TxId>` instead of a single `TxId` value.
-  - `wallet::create_spend_to_address` now takes an additional `change_memo`
-    argument. It also returns its result as a `NonEmpty<TxId>` instead of a
+  - `wallet::create_spend_to_address` now takes additional `change_memo` and
+    `fallback_change_pool` arguments. It also returns its result as a
+    `NonEmpty<TxId>` instead of a
     single `TxId`.
   - `wallet::spend` returns its result as a `NonEmpty<TxId>` instead of a
     single `TxId`.
@@ -226,9 +227,10 @@ and this library adheres to Rust's notion of
     now also provides the output pool to which change should be sent and an
     optional memo to be associated with the change output.
   - `ChangeError` has a new `BundleError` variant.
-  - `fixed::SingleOutputChangeStrategy::new` and
-    `zip317::SingleOutputChangeStrategy::new` each now accept an additional
-    `change_memo` argument.
+  - `fixed::SingleOutputChangeStrategy::new`,
+    `zip317::SingleOutputChangeStrategy::new`, and
+    `standard::SingleOutputChangeStrategy::new` each now accept additional
+    `change_memo` and `fallback_change_pool` arguments.
 - `zcash_client_backend::wallet`:
   - The fields of `ReceivedSaplingNote` are now private. Use
     `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods

diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs
@@ -27,8 +27,7 @@ use crate::{
     decrypt_transaction,
     fees::{self, DustOutputPolicy},
     keys::UnifiedSpendingKey,
-    proposal::ProposalError,
-    proposal::{self, Proposal},
+    proposal::{self, Proposal, ProposalError},
     wallet::{Note, OvkPolicy, Recipient},
     zip321::{self, Payment},
     PoolType, ShieldedProtocol,
@@ -210,6 +209,7 @@ pub fn create_spend_to_address<DbT, ParamsT>(
     ovk_policy: OvkPolicy,
     min_confirmations: NonZeroU32,
     change_memo: Option<MemoBytes>,
+    fallback_change_pool: ShieldedProtocol,
 ) -> Result<
     NonEmpty<TxId>,
     Error<
@@ -240,6 +240,7 @@ where
         amount,
         memo,
         change_memo,
+        fallback_change_pool,
     )?;
 
     create_proposed_transactions(
@@ -423,6 +424,8 @@ where
 /// * `amount`: The amount to send.
 /// * `memo`: A memo to be included in the output to the recipient.
 /// * `change_memo`: A memo to be included in any change output that is created.
+/// * `fallback_change_pool`: The shielded pool to which change should be sent if
+///   automatic change pool determination fails.
 #[allow(clippy::too_many_arguments)]
 #[allow(clippy::type_complexity)]
 pub fn propose_standard_transfer_to_address<DbT, ParamsT, CommitmentTreeErrT>(
@@ -435,6 +438,7 @@ pub fn propose_standard_transfer_to_address<DbT, ParamsT, CommitmentTreeErrT>(
     amount: NonNegativeAmount,
     memo: Option<MemoBytes>,
     change_memo: Option<MemoBytes>,
+    fallback_change_pool: ShieldedProtocol,
 ) -> Result<
     Proposal<StandardFeeRule, DbT::NoteRef>,
     Error<
@@ -461,7 +465,11 @@ where
         "It should not be possible for this to violate ZIP 321 request construction invariants.",
     );
 
-    let change_strategy = fees::standard::SingleOutputChangeStrategy::new(fee_rule, change_memo);
+    let change_strategy = fees::standard::SingleOutputChangeStrategy::new(
+        fee_rule,
+        change_memo,
+        fallback_change_pool,
+    );
     let input_selector =
         GreedyInputSelector::<DbT, _>::new(change_strategy, DustOutputPolicy::default());
 
@@ -644,73 +652,83 @@ where
         .map_err(Error::DataSource)?
         .ok_or(Error::KeyNotRecognized)?;
 
-    let (sapling_anchor, sapling_inputs) = proposal_step.shielded_inputs().map_or_else(
-        || Ok((sapling::Anchor::empty_tree(), vec![])),
-        |inputs| {
-            wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| {
-                let anchor = sapling_tree
-                    .root_at_checkpoint_id(&inputs.anchor_height())?
-                    .into();
+    let (sapling_anchor, sapling_inputs) =
+        if proposal_step.involves(PoolType::Shielded(ShieldedProtocol::Sapling)) {
+            proposal_step.shielded_inputs().map_or_else(
+                || Ok((Some(sapling::Anchor::empty_tree()), vec![])),
+                |inputs| {
+                    wallet_db.with_sapling_tree_mut::<_, _, Error<_, _, _, _>>(|sapling_tree| {
+                        let anchor = sapling_tree
+                            .root_at_checkpoint_id(&inputs.anchor_height())?
+                            .into();
+
+                        let sapling_inputs = inputs
+                            .notes()
+                            .iter()
+                            .filter_map(|selected| match selected.note() {
+                                Note::Sapling(note) => {
+                                    let key = match selected.spending_key_scope() {
+                                        Scope::External => usk.sapling().clone(),
+                                        Scope::Internal => usk.sapling().derive_internal(),
+                                    };
+
+                                    sapling_tree
+                                        .witness_at_checkpoint_id_caching(
+                                            selected.note_commitment_tree_position(),
+                                            &inputs.anchor_height(),
+                                        )
+                                        .map(|merkle_path| Some((key, note, merkle_path)))
+                                        .map_err(Error::from)
+                                        .transpose()
+                                }
+                                #[cfg(feature = "orchard")]
+                                Note::Orchard(_) => None,
+                            })
+                            .collect::<Result<Vec<_>, Error<_, _, _, _>>>()?;
 
-                let sapling_inputs = inputs
-                    .notes()
-                    .iter()
-                    .filter_map(|selected| match selected.note() {
-                        Note::Sapling(note) => {
-                            let key = match selected.spending_key_scope() {
-                                Scope::External => usk.sapling().clone(),
-                                Scope::Internal => usk.sapling().derive_internal(),
-                            };
-
-                            sapling_tree
-                                .witness_at_checkpoint_id_caching(
-                                    selected.note_commitment_tree_position(),
-                                    &inputs.anchor_height(),
-                                )
-                                .map(|merkle_path| Some((key, note, merkle_path)))
-                                .map_err(Error::from)
-                                .transpose()
-                        }
-                        #[cfg(feature = "orchard")]
-                        Note::Orchard(_) => None,
+                        Ok((Some(anchor), sapling_inputs))
                     })
-                    .collect::<Result<Vec<_>, Error<_, _, _, _>>>()?;
-
-                Ok((anchor, sapling_inputs))
-            })
-        },
-    )?;
+                },
+            )?
+        } else {
+            (None, vec![])
+        };
 
     #[cfg(feature = "orchard")]
-    let (orchard_anchor, orchard_inputs) = proposal_step.shielded_inputs().map_or_else(
-        || Ok((Some(orchard::Anchor::empty_tree()), vec![])),
-        |inputs| {
-            wallet_db.with_orchard_tree_mut::<_, _, Error<_, _, _, _>>(|orchard_tree| {
-                let anchor = orchard_tree
-                    .root_at_checkpoint_id(&inputs.anchor_height())?
-                    .into();
-
-                let orchard_inputs = inputs
-                    .notes()
-                    .iter()
-                    .filter_map(|selected| match selected.note() {
-                        #[cfg(feature = "orchard")]
-                        Note::Orchard(note) => orchard_tree
-                            .witness_at_checkpoint_id_caching(
-                                selected.note_commitment_tree_position(),
-                                &inputs.anchor_height(),
-                            )
-                            .map(|merkle_path| Some((note, merkle_path)))
-                            .map_err(Error::from)
-                            .transpose(),
-                        Note::Sapling(_) => None,
-                    })
-                    .collect::<Result<Vec<_>, Error<_, _, _, _>>>()?;
+    let (orchard_anchor, orchard_inputs) =
+        if proposal_step.involves(PoolType::Shielded(ShieldedProtocol::Orchard)) {
+            proposal_step.shielded_inputs().map_or_else(
+                || Ok((Some(orchard::Anchor::empty_tree()), vec![])),
+                |inputs| {
+                    wallet_db.with_orchard_tree_mut::<_, _, Error<_, _, _, _>>(|orchard_tree| {
+                        let anchor = orchard_tree
+                            .root_at_checkpoint_id(&inputs.anchor_height())?
+                            .into();
+
+                        let orchard_inputs = inputs
+                            .notes()
+                            .iter()
+                            .filter_map(|selected| match selected.note() {
+                                #[cfg(feature = "orchard")]
+                                Note::Orchard(note) => orchard_tree
+                                    .witness_at_checkpoint_id_caching(
+                                        selected.note_commitment_tree_position(),
+                                        &inputs.anchor_height(),
+                                    )
+                                    .map(|merkle_path| Some((note, merkle_path)))
+                                    .map_err(Error::from)
+                                    .transpose(),
+                                Note::Sapling(_) => None,
+                            })
+                            .collect::<Result<Vec<_>, Error<_, _, _, _>>>()?;
 
-                Ok((Some(anchor), orchard_inputs))
-            })
-        },
-    )?;
+                        Ok((Some(anchor), orchard_inputs))
+                    })
+                },
+            )?
+        } else {
+            (None, vec![])
+        };
     #[cfg(not(feature = "orchard"))]
     let orchard_anchor = None;
 
@@ -720,7 +738,7 @@ where
         params.clone(),
         min_target_height,
         BuildConfig::Standard {
-            sapling_anchor: Some(sapling_anchor),
+            sapling_anchor,
             orchard_anchor,
         },
     );
@@ -984,7 +1002,6 @@ where
                     Some(memo),
                 ))
             }
-            #[cfg(zcash_unstable = "orchard")]
             ShieldedProtocol::Orchard => {
                 #[cfg(not(feature = "orchard"))]
                 return Err(Error::UnsupportedChangeType(PoolType::Shielded(

diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs
@@ -455,9 +455,9 @@ where
                 Err(other) => return Err(other.into()),
             }
 
-            #[cfg(not(zcash_unstable = "orchard"))]
+            #[cfg(not(feature = "orchard"))]
             let selectable_pools = &[ShieldedProtocol::Sapling];
-            #[cfg(zcash_unstable = "orchard")]
+            #[cfg(feature = "orchard")]
             let selectable_pools = &[ShieldedProtocol::Sapling, ShieldedProtocol::Orchard];
 
             shielded_inputs = wallet_db

diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs
@@ -34,6 +34,7 @@ pub(crate) fn single_change_output_balance<
     dust_output_policy: &DustOutputPolicy,
     default_dust_threshold: NonNegativeAmount,
     change_memo: Option<MemoBytes>,
+    _fallback_change_pool: ShieldedProtocol,
 ) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
 where
     E: From<F::Error> + From<BalanceError>,
@@ -86,7 +87,6 @@ where
 
     // TODO: implement a less naive strategy for selecting the pool to which change will be sent.
     #[cfg(feature = "orchard")]
-    #[allow(clippy::if_same_then_else)]
     let (change_pool, sapling_change, orchard_change) =
         if orchard_in.is_positive() || orchard_out.is_positive() {
             // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs
@@ -96,8 +96,12 @@ where
             // Sapling outputs, so that we avoid pool-crossing.
             (ShieldedProtocol::Sapling, 1, 0)
         } else {
-            // For all other transactions, send change to Orchard.
-            (ShieldedProtocol::Orchard, 0, 1)
+            // This is a fully-transparent transaction, so the caller gets to decide
+            // where to shield change.
+            match _fallback_change_pool {
+                ShieldedProtocol::Orchard => (_fallback_change_pool, 0, 1),
+                ShieldedProtocol::Sapling => (_fallback_change_pool, 1, 0),
+            }
         };
     #[cfg(not(feature = "orchard"))]
     let (change_pool, sapling_change) = (ShieldedProtocol::Sapling, 1);

diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs
@@ -9,6 +9,8 @@ use zcash_primitives::{
     },
 };
 
+use crate::ShieldedProtocol;
+
 use super::{
     common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy,
     DustOutputPolicy, TransactionBalance,
@@ -22,15 +24,21 @@ use super::orchard as orchard_fees;
 pub struct SingleOutputChangeStrategy {
     fee_rule: FixedFeeRule,
     change_memo: Option<MemoBytes>,
+    fallback_change_pool: ShieldedProtocol,
 }
 
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule
     /// and change memo.
-    pub fn new(fee_rule: FixedFeeRule, change_memo: Option<MemoBytes>) -> Self {
+    pub fn new(
+        fee_rule: FixedFeeRule,
+        change_memo: Option<MemoBytes>,
+        fallback_change_pool: ShieldedProtocol,
+    ) -> Self {
         Self {
             fee_rule,
             change_memo,
+            fallback_change_pool,
         }
     }
 }
@@ -65,6 +73,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
             dust_output_policy,
             self.fee_rule().fixed_fee(),
             self.change_memo.clone(),
+            self.fallback_change_pool,
         )
     }
 }
@@ -89,13 +98,15 @@ mod tests {
             tests::{TestSaplingInput, TestTransparentInput},
             ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy,
         },
+        ShieldedProtocol,
     };
 
     #[test]
     fn change_without_dust() {
         #[allow(deprecated)]
         let fee_rule = FixedFeeRule::standard();
-        let change_strategy = SingleOutputChangeStrategy::new(fee_rule, None);
+        let change_strategy =
+            SingleOutputChangeStrategy::new(fee_rule, None, ShieldedProtocol::Sapling);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(
@@ -136,7 +147,8 @@ mod tests {
     fn dust_change() {
         #[allow(deprecated)]
         let fee_rule = FixedFeeRule::standard();
-        let change_strategy = SingleOutputChangeStrategy::new(fee_rule, None);
+        let change_strategy =
+            SingleOutputChangeStrategy::new(fee_rule, None, ShieldedProtocol::Sapling);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(