Merge pull request #1333 from zcash/fix/single_pool_send_to_t

zcash_client_sqlite: Fix incorrect input selection filtering when sending to transparent.

zcash_client_backend/CHANGELOG.md

@@ -7,6 +7,13 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 
+## [0.12.1] - 2024-03-27
+
+### Fixed
+- This release fixes a problem in note selection when sending to a transparent
+  recipient, whereby available funds were being incorrectly excluded from 
+  input selection.
+
 ## [0.12.0] - 2024-03-25
 
 ### Added

zcash_client_backend/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "zcash_client_backend"
 description = "APIs for creating shielded Zcash light clients"
-version = "0.12.0"
+version = "0.12.1"
 authors = [
     "Jack Grigg <[email protected]>",
     "Kris Nuttycombe <[email protected]>"

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -394,20 +394,25 @@ 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 {
-            #[cfg(feature = "orchard")]
-            let (sapling_input_total, orchard_input_total) = (
-                shielded_inputs.sapling_value()?,
-                shielded_inputs.orchard_value()?,
-            );
-
             #[cfg(not(feature = "orchard"))]
-            let orchard_input_total = NonNegativeAmount::ZERO;
-
-            let use_sapling =
-                !(sapling_outputs.is_empty() && orchard_input_total >= amount_required);
+            let use_sapling = true;
             #[cfg(feature = "orchard")]
-            let use_orchard =
-                !(orchard_outputs.is_empty() && sapling_input_total >= amount_required);
+            let (use_sapling, use_orchard) = {
+                let (sapling_input_total, orchard_input_total) = (
+                    shielded_inputs.sapling_value()?,
+                    shielded_inputs.orchard_value()?,
+                );
+
+                // Use Sapling inputs if there are no Orchard outputs or there are not sufficient
+                // Orchard outputs to cover the amount required.
+                let use_sapling =
+                    orchard_outputs.is_empty() || amount_required > orchard_input_total;
+                // Use Orchard inputs if there are insufficient Sapling funds to cover the amount
+                // reqiuired.
+                let use_orchard = !use_sapling || amount_required > sapling_input_total;
+
+                (use_sapling, use_orchard)
+            };
 
             let sapling_inputs = if use_sapling {
                 shielded_inputs

zcash_client_sqlite/src/testing/pool.rs

@@ -1634,6 +1634,97 @@ pub(crate) fn fully_funded_fully_private<P0: ShieldedPoolTester, P1: ShieldedPoo
     );
 }
 
+#[cfg(feature = "orchard")]
+pub(crate) fn fully_funded_send_to_t<P0: ShieldedPoolTester, P1: ShieldedPoolTester>() {
+    let mut st = TestBuilder::new()
+        .with_block_cache()
+        .with_account_from_sapling_activation(BlockHash([0; 32])) // TODO: Allow for Orchard
+        // activation after Sapling
+        .build();
+
+    let account = st.test_account().cloned().unwrap();
+
+    let p0_fvk = P0::test_account_fvk(&st);
+    let p1_fvk = P1::test_account_fvk(&st);
+    let (p1_to, _) = account.usk().default_transparent_address();
+
+    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(account.birthday().height(), 2);
+
+    let initial_balance = (note_value * 2).unwrap();
+    assert_eq!(st.get_total_balance(account.account_id()), initial_balance);
+    assert_eq!(
+        st.get_spendable_balance(account.account_id(), 1),
+        initial_balance
+    );
+
+    let transfer_amount = NonNegativeAmount::const_from_u64(200000);
+    let p0_to_p1 = zip321::TransactionRequest::new(vec![Payment {
+        recipient_address: Address::Transparent(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.account_id(),
+            &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 3 logical actions, one for the transparent output and two for the source pool.
+    let expected_fee = NonNegativeAmount::const_from_u64(15000);
+    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 there are sufficient funds in either pool, change is kept in the same pool as
+    // the source note (the target pool), and does not necessarily follow preference order.
+    // The source note will always be sapling, as we spend Sapling funds preferentially.
+    assert_eq!(change_output.output_pool(), ShieldedProtocol::Sapling);
+    assert_eq!(change_output.value(), expected_change);
+
+    let create_proposed_result = st.create_proposed_transactions::<Infallible, _>(
+        account.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.account_id()),
+        (initial_balance - transfer_amount - expected_fee).unwrap()
+    );
+    assert_eq!(
+        st.get_spendable_balance(account.account_id(), 1),
+        (initial_balance - transfer_amount - expected_fee).unwrap()
+    );
+}
+
 #[cfg(feature = "orchard")]
 pub(crate) fn multi_pool_checkpoint<P0: ShieldedPoolTester, P1: ShieldedPoolTester>() {
     let mut st = TestBuilder::new()

zcash_client_sqlite/src/wallet/orchard.rs

@@ -656,6 +656,11 @@ pub(crate) mod tests {
         testing::pool::fully_funded_fully_private::<OrchardPoolTester, SaplingPoolTester>()
     }
 
+    #[test]
+    fn fully_funded_send_to_t() {
+        testing::pool::fully_funded_send_to_t::<OrchardPoolTester, SaplingPoolTester>()
+    }
+
     #[test]
     fn multi_pool_checkpoint() {
         testing::pool::multi_pool_checkpoint::<OrchardPoolTester, SaplingPoolTester>()

zcash_client_sqlite/src/wallet/sapling.rs

@@ -668,6 +668,14 @@ pub(crate) mod tests {
         testing::pool::fully_funded_fully_private::<SaplingPoolTester, OrchardPoolTester>()
     }
 
+    #[test]
+    #[cfg(feature = "orchard")]
+    fn fully_funded_send_to_t() {
+        use crate::wallet::orchard::tests::OrchardPoolTester;
+
+        testing::pool::fully_funded_send_to_t::<SaplingPoolTester, OrchardPoolTester>()
+    }
+
     #[test]
     #[cfg(feature = "orchard")]
     fn multi_pool_checkpoint() {