zcash_client_backend: Fix note selection & add more cross-pool tests.

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -394,34 +394,80 @@ where
         // of funds selected is strictly increasing. The loop will either return a successful
         // result or the wallet will eventually run out of funds to select.
         loop {
+            let sapling_input_total = shielded_inputs
+                .iter()
+                .filter(|i| matches!(i.note(), Note::Sapling(_)))
+                .map(|i| i.note().value())
+                .sum::<Option<NonNegativeAmount>>()
+                .ok_or(BalanceError::Overflow)?;
+
+            #[cfg(feature = "orchard")]
+            let orchard_input_total = shielded_inputs
+                .iter()
+                .filter(|i| matches!(i.note(), Note::Orchard(_)))
+                .map(|i| i.note().value())
+                .sum::<Option<NonNegativeAmount>>()
+                .ok_or(BalanceError::Overflow)?;
+            #[cfg(not(feature = "orchard"))]
+            let orchard_input_total = NonNegativeAmount::ZERO;
+
+            let sapling_inputs =
+                if sapling_outputs.is_empty() && orchard_input_total >= amount_required {
+                    // Avoid selecting Sapling inputs if we don't have Sapling outputs and the value is
+                    // fully covered by Orchard inputs.
+                    #[cfg(feature = "orchard")]
+                    {
+                        shielded_inputs = shielded_inputs
+                            .into_iter()
+                            .filter(|i| matches!(i.note(), Note::Orchard(_)))
+                            .collect();
+                    }
+                    vec![]
+                } else {
+                    shielded_inputs
+                        .iter()
+                        .filter_map(|i| match i.note() {
+                            Note::Sapling(n) => Some((*i.internal_note_id(), n.value())),
+                            #[cfg(feature = "orchard")]
+                            Note::Orchard(_) => None,
+                        })
+                        .collect::<Vec<_>>()
+                };
+
+            #[cfg(feature = "orchard")]
+            let orchard_inputs =
+                if orchard_outputs.is_empty() && sapling_input_total >= amount_required {
+                    // Avoid selecting Orchard inputs if we don't have Orchard outputs and the value is
+                    // fully covered by Sapling inputs.
+                    shielded_inputs = shielded_inputs
+                        .into_iter()
+                        .filter(|i| matches!(i.note(), Note::Sapling(_)))
+                        .collect();
+                    vec![]
+                } else {
+                    shielded_inputs
+                        .iter()
+                        .filter_map(|i| match i.note() {
+                            Note::Sapling(_) => None,
+                            Note::Orchard(n) => Some((*i.internal_note_id(), n.value())),
+                        })
+                        .collect::<Vec<_>>()
+                };
+
             let balance = self.change_strategy.compute_balance(
                 params,
                 target_height,
                 &Vec::<WalletTransparentOutput>::new(),
                 &transparent_outputs,
                 &(
                     ::sapling::builder::BundleType::DEFAULT,
-                    &shielded_inputs
-                        .iter()
-                        .cloned()
-                        .filter_map(|i| match i.note() {
-                            Note::Sapling(n) => Some((*i.internal_note_id(), n.value())),
-                            #[cfg(feature = "orchard")]
-                            Note::Orchard(_) => None,
-                        })
-                        .collect::<Vec<_>>()[..],
+                    &sapling_inputs[..],
                     &sapling_outputs[..],
                 ),
                 #[cfg(feature = "orchard")]
                 &(
                     ::orchard::builder::BundleType::DEFAULT,
-                    &shielded_inputs
-                        .iter()
-                        .filter_map(|i| match i.note() {
-                            Note::Sapling(_) => None,
-                            Note::Orchard(n) => Some((*i.internal_note_id(), n.value())),
-                        })
-                        .collect::<Vec<_>>()[..],
+                    &orchard_inputs[..],
                     &orchard_outputs[..],
                 ),
                 &self.dust_output_policy,

zcash_client_sqlite/src/testing/pool.rs

@@ -1406,7 +1406,7 @@ pub(crate) fn checkpoint_gaps<T: ShieldedPoolTester>() {
 }
 
 #[cfg(feature = "orchard")]
-pub(crate) fn cross_pool_exchange<P0: ShieldedPoolTester, P1: ShieldedPoolTester>() {
+pub(crate) fn pool_crossing_required<P0: ShieldedPoolTester, P1: ShieldedPoolTester>() {
     let mut st = TestBuilder::new()
         .with_block_cache()
         .with_test_account(|params| AccountBirthday::from_activation(params, NetworkUpgrade::Nu5))
@@ -1419,12 +1419,11 @@ pub(crate) fn cross_pool_exchange<P0: ShieldedPoolTester, P1: ShieldedPoolTester
     let p1_fvk = P1::test_account_fvk(&st);
     let p1_to = P1::fvk_default_address(&p1_fvk);
 
-    let note_value = NonNegativeAmount::const_from_u64(300000);
+    let note_value = NonNegativeAmount::const_from_u64(350000);
     st.generate_next_block(&p0_fvk, AddressType::DefaultExternal, note_value);
-    st.generate_next_block(&p1_fvk, AddressType::DefaultExternal, note_value);
     st.scan_cached_blocks(birthday.height(), 2);
 
-    let initial_balance = (note_value * 2).unwrap();
+    let initial_balance = note_value;
     assert_eq!(st.get_total_balance(account), initial_balance);
     assert_eq!(st.get_spendable_balance(account, 1), initial_balance);
 
@@ -1489,6 +1488,90 @@ pub(crate) fn cross_pool_exchange<P0: ShieldedPoolTester, P1: ShieldedPoolTester
     );
 }
 
+#[cfg(feature = "orchard")]
+pub(crate) fn fully_funded_fully_private<P0: ShieldedPoolTester, P1: ShieldedPoolTester>() {
+    let mut st = TestBuilder::new()
+        .with_block_cache()
+        .with_test_account(|params| AccountBirthday::from_activation(params, NetworkUpgrade::Nu5))
+        .build();
+
+    let (account, usk, birthday) = st.test_account().unwrap();
+
+    let p0_fvk = P0::test_account_fvk(&st);
+
+    let p1_fvk = P1::test_account_fvk(&st);
+    let p1_to = P1::fvk_default_address(&p1_fvk);
+
+    let note_value = NonNegativeAmount::const_from_u64(350000);
+    st.generate_next_block(&p0_fvk, AddressType::DefaultExternal, note_value);
+    st.generate_next_block(&p1_fvk, AddressType::DefaultExternal, note_value);
+    st.scan_cached_blocks(birthday.height(), 2);
+
+    let initial_balance = (note_value * 2).unwrap();
+    assert_eq!(st.get_total_balance(account), initial_balance);
+    assert_eq!(st.get_spendable_balance(account, 1), initial_balance);
+
+    let transfer_amount = NonNegativeAmount::const_from_u64(200000);
+    let p0_to_p1 = zip321::TransactionRequest::new(vec![Payment {
+        recipient_address: p1_to,
+        amount: transfer_amount,
+        memo: None,
+        label: None,
+        message: None,
+        other_params: vec![],
+    }])
+    .unwrap();
+
+    let fee_rule = StandardFeeRule::Zip317;
+    let input_selector = GreedyInputSelector::new(
+        // We set the default change output pool to P0, because we want to verify later that
+        // change is actually sent to P1 (as the transaction is fully fundable from P1).
+        standard::SingleOutputChangeStrategy::new(fee_rule, None, P0::SHIELDED_PROTOCOL),
+        DustOutputPolicy::default(),
+    );
+    let proposal0 = st
+        .propose_transfer(
+            account,
+            &input_selector,
+            p0_to_p1,
+            NonZeroU32::new(1).unwrap(),
+        )
+        .unwrap();
+
+    let _min_target_height = proposal0.min_target_height();
+    assert_eq!(proposal0.steps().len(), 1);
+    let step0 = &proposal0.steps().head;
+
+    // We expect 2 logical actions, since either pool can pay the full balance required
+    // and note selection should choose the fully-private path.
+    let expected_fee = NonNegativeAmount::const_from_u64(10000);
+    assert_eq!(step0.balance().fee_required(), expected_fee);
+
+    let expected_change = (note_value - transfer_amount - expected_fee).unwrap();
+    let proposed_change = step0.balance().proposed_change();
+    assert_eq!(proposed_change.len(), 1);
+    let change_output = proposed_change.get(0).unwrap();
+    // Since this is a cross-pool transfer, change will be sent to the preferred pool.
+    assert_eq!(change_output.output_pool(), P1::SHIELDED_PROTOCOL);
+    assert_eq!(change_output.value(), expected_change);
+
+    let create_proposed_result =
+        st.create_proposed_transactions::<Infallible, _>(&usk, OvkPolicy::Sender, &proposal0);
+    assert_matches!(&create_proposed_result, Ok(txids) if txids.len() == 1);
+
+    let (h, _) = st.generate_next_block_including(create_proposed_result.unwrap()[0]);
+    st.scan_cached_blocks(h, 1);
+
+    assert_eq!(
+        st.get_total_balance(account),
+        (initial_balance - expected_fee).unwrap()
+    );
+    assert_eq!(
+        st.get_spendable_balance(account, 1),
+        (initial_balance - expected_fee).unwrap()
+    );
+}
+
 pub(crate) fn valid_chain_states<T: ShieldedPoolTester>() {
     let mut st = TestBuilder::new()
         .with_block_cache()

zcash_client_sqlite/src/wallet/orchard.rs

@@ -604,9 +604,16 @@ pub(crate) mod tests {
     }
 
     #[test]
-    fn cross_pool_exchange() {
+    fn pool_crossing_required() {
         use crate::wallet::sapling::tests::SaplingPoolTester;
 
-        testing::pool::cross_pool_exchange::<OrchardPoolTester, SaplingPoolTester>()
+        testing::pool::pool_crossing_required::<OrchardPoolTester, SaplingPoolTester>()
+    }
+
+    #[test]
+    fn fully_funded_fully_private() {
+        use crate::wallet::sapling::tests::SaplingPoolTester;
+
+        testing::pool::fully_funded_fully_private::<OrchardPoolTester, SaplingPoolTester>()
     }
 }

zcash_client_sqlite/src/wallet/sapling.rs

@@ -607,9 +607,17 @@ pub(crate) mod tests {
 
     #[test]
     #[cfg(feature = "orchard")]
-    fn cross_pool_exchange() {
+    fn pool_crossing_required() {
         use crate::wallet::orchard::tests::OrchardPoolTester;
 
-        testing::pool::cross_pool_exchange::<SaplingPoolTester, OrchardPoolTester>()
+        testing::pool::pool_crossing_required::<SaplingPoolTester, OrchardPoolTester>()
+    }
+
+    #[test]
+    #[cfg(feature = "orchard")]
+    fn fully_funded_fully_private() {
+        use crate::wallet::orchard::tests::OrchardPoolTester;
+
+        testing::pool::fully_funded_fully_private::<SaplingPoolTester, OrchardPoolTester>()
     }
 }