Merge pull request #1570 from pacu/feature/transparent-balance

[#1411] Refactor AccountBalance to use Balance for transparent funds

zcash_client_backend/CHANGELOG.md

@@ -11,18 +11,37 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api::AccountSource::key_derivation`
 
 ### Changed
+- `zcash_client_backend::data_api::AccountBalance`: Refactored to use `Balance`
+  for transparent funds (issue #1411). It now has an `unshielded_balance()`
+  method that returns `Balance`, allowing the unshielded spendable, unshielded
+  pending change, and unshielded pending non-change values to be tracked
+  separately.
 - `zcash_client_backend::data_api::WalletRead`:
   - The `create_account`, `import_account_hd`, and `import_account_ufvk`
     methods now each take additional `account_name` and `key_source` arguments.
     These allow the wallet backend to store additional metadata that is useful
     to applications managing these accounts.
 - `zcash_client_backend::data_api::AccountSource`:
-  - Both the `Derived` and `Importants` now have an additional `key_source`
-    property that is used to convey application-specific key source metadata.
+  - Both `Derived` and `Imported` alternatives of `AccountSource` now have an
+    additional `key_source` field that is used to convey application-specific
+    key source metadata.
   - The `Copy` impl for this type has been removed.
 - `zcash_client_backend::data_api::Account` has an additional `name` method
   that returns the human-readable name of the account, if any.
 
+### Deprecated
+- `AccountBalance::unshielded`. Instead use `unshielded_balance` which
+  provides a `Balance` value. Its `total()` method can be used to obtain the
+  total of transparent funds.
+
+### Removed
+- `zcash_client_backend::AccountBalance::add_unshielded_value`. Instead use
+  `AccountBalance::with_unshielded_balance_mut` with a closure that calls
+  the appropriate `add_*_value` method(s) of `Balance` on its argument.
+  Note that the appropriate method(s) depend on whether the funds are
+  spendable, pending change, or pending non-change (previously, only the
+  total unshielded value was tracked).
+
 ## [0.15.0] - 2024-11-14
 
 ### Added

zcash_client_backend/src/data_api.rs

@@ -231,14 +231,8 @@ pub struct AccountBalance {
     /// The value of unspent Orchard outputs belonging to the account.
     orchard_balance: Balance,
 
-    /// The value of all unspent transparent outputs belonging to the account, irrespective of
-    /// confirmation depth.
-    ///
-    /// Unshielded balances are not subject to confirmation-depth constraints, because the only
-    /// possible operation on a transparent balance is to shield it, it is possible to create a
-    /// zero-conf transaction to perform that shielding, and the resulting shielded notes will be
-    /// subject to normal confirmation rules.
-    unshielded: NonNegativeAmount,
+    /// The value of all unspent transparent outputs belonging to the account.
+    unshielded_balance: Balance,
 }
 
 impl AccountBalance {
@@ -246,12 +240,14 @@ impl AccountBalance {
     pub const ZERO: Self = Self {
         sapling_balance: Balance::ZERO,
         orchard_balance: Balance::ZERO,
-        unshielded: NonNegativeAmount::ZERO,
+        unshielded_balance: Balance::ZERO,
     };
 
     fn check_total(&self) -> Result<NonNegativeAmount, BalanceError> {
-        (self.sapling_balance.total() + self.orchard_balance.total() + self.unshielded)
-            .ok_or(BalanceError::Overflow)
+        (self.sapling_balance.total()
+            + self.orchard_balance.total()
+            + self.unshielded_balance.total())
+        .ok_or(BalanceError::Overflow)
     }
 
     /// Returns the [`Balance`] of Sapling funds in the account.
@@ -259,7 +255,7 @@ impl AccountBalance {
         &self.sapling_balance
     }
 
-    /// Provides a `mutable reference to the [`Balance`] of Sapling funds in the account
+    /// Provides a mutable reference to the [`Balance`] of Sapling funds in the account
     /// to the specified callback, checking invariants after the callback's action has been
     /// evaluated.
     pub fn with_sapling_balance_mut<A, E: From<BalanceError>>(
@@ -276,7 +272,7 @@ impl AccountBalance {
         &self.orchard_balance
     }
 
-    /// Provides a `mutable reference to the [`Balance`] of Orchard funds in the account
+    /// Provides a mutable reference to the [`Balance`] of Orchard funds in the account
     /// to the specified callback, checking invariants after the callback's action has been
     /// evaluated.
     pub fn with_orchard_balance_mut<A, E: From<BalanceError>>(
@@ -289,22 +285,36 @@ impl AccountBalance {
     }
 
     /// Returns the total value of unspent transparent transaction outputs belonging to the wallet.
+    #[deprecated(
+        note = "this function is deprecated. Please use [`AccountBalance::unshielded_balance`] instead."
+    )]
     pub fn unshielded(&self) -> NonNegativeAmount {
-        self.unshielded
+        self.unshielded_balance.total()
     }
 
-    /// Adds the specified value to the unshielded total, checking for overflow of
-    /// the total account balance.
-    pub fn add_unshielded_value(&mut self, value: NonNegativeAmount) -> Result<(), BalanceError> {
-        self.unshielded = (self.unshielded + value).ok_or(BalanceError::Overflow)?;
+    /// Returns the [`Balance`] of unshielded funds in the account.
+    pub fn unshielded_balance(&self) -> &Balance {
+        &self.unshielded_balance
+    }
+
+    /// Provides a mutable reference to the [`Balance`] of transparent funds in the account
+    /// to the specified callback, checking invariants after the callback's action has been
+    /// evaluated.
+    pub fn with_unshielded_balance_mut<A, E: From<BalanceError>>(
+        &mut self,
+        f: impl FnOnce(&mut Balance) -> Result<A, E>,
+    ) -> Result<A, E> {
+        let result = f(&mut self.unshielded_balance)?;
         self.check_total()?;
-        Ok(())
+        Ok(result)
     }
 
     /// Returns the total value of funds belonging to the account.
     pub fn total(&self) -> NonNegativeAmount {
-        (self.sapling_balance.total() + self.orchard_balance.total() + self.unshielded)
-            .expect("Account balance cannot overflow MAX_MONEY")
+        (self.sapling_balance.total()
+            + self.orchard_balance.total()
+            + self.unshielded_balance.total())
+        .expect("Account balance cannot overflow MAX_MONEY")
     }
 
     /// Returns the total value of shielded (Sapling and Orchard) funds that may immediately be
@@ -1254,8 +1264,9 @@ pub trait WalletRead {
     /// or `Ok(None)` if the wallet has no initialized accounts.
     fn get_wallet_birthday(&self) -> Result<Option<BlockHeight>, Self::Error>;
 
-    /// Returns the wallet balances and sync status for an account given the specified minimum
-    /// number of confirmations, or `Ok(None)` if the wallet has no balance data available.
+    /// Returns a [`WalletSummary`] that represents the sync status, and the wallet balances
+    /// given the specified minimum number of confirmations for all accounts known to the
+    /// wallet; or `Ok(None)` if the wallet has no summary data available.
     fn get_wallet_summary(
         &self,
         min_confirmations: u32,

zcash_client_backend/src/data_api/testing/transparent.rs

@@ -4,7 +4,7 @@ use crate::{
             AddressType, DataStoreFactory, ShieldedProtocol, TestBuilder, TestCache, TestState,
         },
         wallet::input_selection::GreedyInputSelector,
-        Account as _, InputSource, WalletRead, WalletWrite,
+        Account as _, Balance, InputSource, WalletRead, WalletWrite,
     },
     fees::{standard, DustOutputPolicy, StandardFeeRule},
     wallet::WalletTransparentOutput,
@@ -13,8 +13,76 @@ use assert_matches::assert_matches;
 use sapling::zip32::ExtendedSpendingKey;
 use zcash_primitives::{
     block::BlockHash,
+    legacy::TransparentAddress,
     transaction::components::{amount::NonNegativeAmount, OutPoint, TxOut},
 };
+use zcash_protocol::local_consensus::LocalNetwork;
+
+use super::TestAccount;
+
+/// Checks whether the transparent balance of the given test `account` is as `expected`
+/// considering the `min_confirmations`. It is assumed that zero or one `min_confirmations`
+/// are treated the same, and so this function also checks the other case when
+/// `min_confirmations` is 0 or 1.
+fn check_balance<DSF>(
+    st: &TestState<impl TestCache, <DSF as DataStoreFactory>::DataStore, LocalNetwork>,
+    account: &TestAccount<<DSF as DataStoreFactory>::Account>,
+    taddr: &TransparentAddress,
+    min_confirmations: u32,
+    expected: &Balance,
+) where
+    DSF: DataStoreFactory,
+{
+    // Check the wallet summary returns the expected transparent balance.
+    let summary = st
+        .wallet()
+        .get_wallet_summary(min_confirmations)
+        .unwrap()
+        .unwrap();
+    let balance = summary.account_balances().get(&account.id()).unwrap();
+
+    #[allow(deprecated)]
+    let old_unshielded_value = balance.unshielded();
+    assert_eq!(old_unshielded_value, expected.total());
+    assert_eq!(balance.unshielded_balance(), expected);
+
+    // Check the older APIs for consistency.
+    let mempool_height = st.wallet().chain_height().unwrap().unwrap() + 1;
+    assert_eq!(
+        st.wallet()
+            .get_transparent_balances(account.id(), mempool_height)
+            .unwrap()
+            .get(taddr)
+            .cloned()
+            .unwrap_or(NonNegativeAmount::ZERO),
+        expected.total(),
+    );
+    assert_eq!(
+        st.wallet()
+            .get_spendable_transparent_outputs(taddr, mempool_height, min_confirmations)
+            .unwrap()
+            .into_iter()
+            .map(|utxo| utxo.value())
+            .sum::<Option<NonNegativeAmount>>(),
+        Some(expected.spendable_value()),
+    );
+
+    // we currently treat min_confirmations the same regardless they are 0 (zero confirmations)
+    // or 1 (one block confirmation). We will check if this assumption holds until it's no
+    // longer made. If zero and one [`min_confirmations`] are treated differently in the future,
+    // this check should then be removed.
+    if min_confirmations == 0 || min_confirmations == 1 {
+        assert_eq!(
+            st.wallet()
+                .get_spendable_transparent_outputs(taddr, mempool_height, 1 - min_confirmations)
+                .unwrap()
+                .into_iter()
+                .map(|utxo| utxo.value())
+                .sum::<Option<NonNegativeAmount>>(),
+            Some(expected.spendable_value()),
+        );
+    }
+}
 
 pub fn put_received_transparent_utxo<DSF>(dsf: DSF)
 where
@@ -136,46 +204,8 @@ where
     }
     st.scan_cached_blocks(start_height, 10);
 
-    let check_balance = |st: &TestState<_, DSF::DataStore, _>, min_confirmations: u32, expected| {
-        // Check the wallet summary returns the expected transparent balance.
-        let summary = st
-            .wallet()
-            .get_wallet_summary(min_confirmations)
-            .unwrap()
-            .unwrap();
-        let balance = summary.account_balances().get(&account.id()).unwrap();
-        // TODO: in the future, we will distinguish between available and total
-        // balance according to `min_confirmations`
-        assert_eq!(balance.unshielded(), expected);
-
-        // Check the older APIs for consistency.
-        let mempool_height = st.wallet().chain_height().unwrap().unwrap() + 1;
-        assert_eq!(
-            st.wallet()
-                .get_transparent_balances(account.id(), mempool_height)
-                .unwrap()
-                .get(taddr)
-                .cloned()
-                .unwrap_or(NonNegativeAmount::ZERO),
-            expected,
-        );
-        assert_eq!(
-            st.wallet()
-                .get_spendable_transparent_outputs(taddr, mempool_height, 0)
-                .unwrap()
-                .into_iter()
-                .map(|utxo| utxo.value())
-                .sum::<Option<NonNegativeAmount>>(),
-            Some(expected),
-        );
-    };
-
     // The wallet starts out with zero balance.
-    // TODO: Once we have refactored `get_wallet_summary` to distinguish between available
-    // and total balance, we should perform additional checks against available balance;
-    // we use minconf 0 here because all transparent funds are considered shieldable,
-    // irrespective of confirmation depth.
-    check_balance(&st, 0, NonNegativeAmount::ZERO);
+    check_balance::<DSF>(&st, &account, taddr, 0, &Balance::ZERO);
 
     // Create a fake transparent output.
     let value = NonNegativeAmount::from_u64(100000).unwrap();
@@ -192,7 +222,12 @@ where
         .unwrap();
 
     // The wallet should detect the balance as available
-    check_balance(&st, 0, value);
+    let mut zero_or_one_conf_value = Balance::ZERO;
+
+    // add the spendable value to the expected balance
+    zero_or_one_conf_value.add_spendable_value(value).unwrap();
+
+    check_balance::<DSF>(&st, &account, taddr, 0, &zero_or_one_conf_value);
 
     // Shield the output.
     let input_selector = GreedyInputSelector::new();
@@ -216,14 +251,14 @@ where
 
     // The wallet should have zero transparent balance, because the shielding
     // transaction can be mined.
-    check_balance(&st, 0, NonNegativeAmount::ZERO);
+    check_balance::<DSF>(&st, &account, taddr, 0, &Balance::ZERO);
 
     // Mine the shielding transaction.
     let (mined_height, _) = st.generate_next_block_including(txid);
     st.scan_cached_blocks(mined_height, 1);
 
     // The wallet should still have zero transparent balance.
-    check_balance(&st, 0, NonNegativeAmount::ZERO);
+    check_balance::<DSF>(&st, &account, taddr, 0, &Balance::ZERO);
 
     // Unmine the shielding transaction via a reorg.
     st.wallet_mut()
@@ -232,7 +267,7 @@ where
     assert_eq!(st.wallet().chain_height().unwrap(), Some(mined_height - 1));
 
     // The wallet should still have zero transparent balance.
-    check_balance(&st, 0, NonNegativeAmount::ZERO);
+    check_balance::<DSF>(&st, &account, taddr, 0, &Balance::ZERO);
 
     // Expire the shielding transaction.
     let expiry_height = st
@@ -243,5 +278,91 @@ where
         .expiry_height();
     st.wallet_mut().update_chain_tip(expiry_height).unwrap();
 
-    check_balance(&st, 0, value);
+    check_balance::<DSF>(&st, &account, taddr, 0, &zero_or_one_conf_value);
+}
+
+/// This test attempts to verify that transparent funds spendability is
+/// accounted for properly given the different minimum confirmations values
+/// that can be set when querying for balances.
+pub fn transparent_balance_spendability<DSF>(dsf: DSF, cache: impl TestCache)
+where
+    DSF: DataStoreFactory,
+{
+    let mut st = TestBuilder::new()
+        .with_data_store_factory(dsf)
+        .with_block_cache(cache)
+        .with_account_from_sapling_activation(BlockHash([0; 32]))
+        .build();
+
+    let account = st.test_account().cloned().unwrap();
+    let uaddr = st
+        .wallet()
+        .get_current_address(account.id())
+        .unwrap()
+        .unwrap();
+    let taddr = uaddr.transparent().unwrap();
+
+    // Initialize the wallet with chain data that has no shielded notes for us.
+    let not_our_key = ExtendedSpendingKey::master(&[]).to_diversifiable_full_viewing_key();
+    let not_our_value = NonNegativeAmount::const_from_u64(10000);
+    let (start_height, _, _) =
+        st.generate_next_block(&not_our_key, AddressType::DefaultExternal, not_our_value);
+    for _ in 1..10 {
+        st.generate_next_block(&not_our_key, AddressType::DefaultExternal, not_our_value);
+    }
+    st.scan_cached_blocks(start_height, 10);
+
+    // The wallet starts out with zero balance.
+    check_balance::<DSF>(
+        &st as &TestState<_, DSF::DataStore, _>,
+        &account,
+        taddr,
+        0,
+        &Balance::ZERO,
+    );
+
+    // Create a fake transparent output.
+    let value = NonNegativeAmount::from_u64(100000).unwrap();
+    let txout = TxOut {
+        value,
+        script_pubkey: taddr.script(),
+    };
+
+    // Pretend the output was received in the chain tip.
+    let height = st.wallet().chain_height().unwrap().unwrap();
+    let utxo = WalletTransparentOutput::from_parts(OutPoint::fake(), txout, Some(height)).unwrap();
+    st.wallet_mut()
+        .put_received_transparent_utxo(&utxo)
+        .unwrap();
+
+    // The wallet should detect the balance as available
+    let mut zero_or_one_conf_value = Balance::ZERO;
+
+    // add the spendable value to the expected balance
+    zero_or_one_conf_value.add_spendable_value(value).unwrap();
+
+    check_balance::<DSF>(&st, &account, taddr, 0, &zero_or_one_conf_value);
+
+    // now if we increase the number of confirmations our spendable balance should
+    // be zero and the total balance equal to `value`
+    let mut not_confirmed_yet_value = Balance::ZERO;
+
+    not_confirmed_yet_value
+        .add_pending_spendable_value(value)
+        .unwrap();
+
+    check_balance::<DSF>(&st, &account, taddr, 2, &not_confirmed_yet_value);
+
+    // Add one extra block
+    st.generate_empty_block();
+
+    // Scan that block
+    st.scan_cached_blocks(height, 1);
+
+    // now we generate one more block and the balance should be the same as when the
+    // check_balance function was called with zero or one confirmation.
+    st.generate_empty_block();
+    st.scan_cached_blocks(height + 1, 1);
+
+    check_balance::<DSF>(&st, &account, taddr, 2, &zero_or_one_conf_value);
 }

zcash_client_sqlite/CHANGELOG.md

@@ -43,6 +43,18 @@ and this library adheres to Rust's notion of
 - Migrated to `rusqlite 0.32`.
 - `error::SqliteClientError` has additional variant `NoteFilterInvalid`
 
+### Fixed
+- `zcash_client_sqlite::WalletDb`'s implementation of
+  `zcash_client_backend::data_api::WalletRead::get_wallet_summary` has been
+  fixed to take account of `min_confirmations` for transparent balances.
+  (Previously, it would treat transparent balances as though
+  `min_confirmations` were `1` even if it was set to a higher value.)
+  Note that this implementation treats `min_confirmations == 0` the same
+  as `min_confirmations == 1` for both shielded and transparent TXOs.
+  It also does not currently distinguish between pending change and
+  non-change; the pending value is all counted as non-change (issue
+  [#1592](https://github.com/zcash/librustzcash/issues/1592)).
+
 ## [0.12.2] - 2024-10-21
 
 ### Fixed

zcash_client_sqlite/src/wallet.rs

@@ -1327,7 +1327,8 @@ impl ProgressEstimator for SubtreeProgressEstimator {
 /// that they are not yet spendable, or for which it is not yet possible to construct witnesses.
 ///
 /// `min_confirmations` can be 0, but that case is currently treated identically to
-/// `min_confirmations == 1` for shielded notes. This behaviour may change in the future.
+/// `min_confirmations == 1` for both shielded and transparent TXOs. This behaviour
+/// may change in the future.
 #[tracing::instrument(skip(tx, params, progress))]
 pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
     tx: &rusqlite::Transaction,
@@ -1578,7 +1579,12 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
     drop(sapling_trace);
 
     #[cfg(feature = "transparent-inputs")]
-    transparent::add_transparent_account_balances(tx, chain_tip_height + 1, &mut account_balances)?;
+    transparent::add_transparent_account_balances(
+        tx,
+        chain_tip_height + 1,
+        min_confirmations,
+        &mut account_balances,
+    )?;
 
     // The approach used here for Sapling and Orchard subtree indexing was a quick hack
     // that has not yet been replaced. TODO: Make less hacky.

zcash_client_sqlite/src/wallet/transparent.rs

@@ -407,16 +407,59 @@ pub(crate) fn get_transparent_balances<P: consensus::Parameters>(
 pub(crate) fn add_transparent_account_balances(
     conn: &rusqlite::Connection,
     mempool_height: BlockHeight,
+    min_confirmations: u32,
     account_balances: &mut HashMap<AccountUuid, AccountBalance>,
 ) -> Result<(), SqliteClientError> {
-    let mut stmt_account_balances = conn.prepare(
+    // TODO (#1592): Ability to distinguish between Transparent pending change and pending non-change
+    let mut stmt_account_spendable_balances = conn.prepare(
         "SELECT a.uuid, SUM(u.value_zat)
          FROM transparent_received_outputs u
          JOIN accounts a ON a.id = u.account_id
          JOIN transactions t ON t.id_tx = u.transaction_id
-         -- the transaction that created the output is mined or is definitely unexpired
+         -- the transaction that created the output is mined and with enough confirmations
          WHERE (
             t.mined_height < :mempool_height -- tx is mined
+            AND :mempool_height - t.mined_height >= :min_confirmations -- has at least min_confirmations
+         )
+         -- and the received txo is unspent
+         AND u.id NOT IN (
+           SELECT transparent_received_output_id
+           FROM transparent_received_output_spends txo_spends
+           JOIN transactions tx
+             ON tx.id_tx = txo_spends.transaction_id
+           WHERE tx.mined_height IS NOT NULL -- the spending tx is mined
+           OR tx.expiry_height = 0 -- the spending tx will not expire
+           OR tx.expiry_height >= :mempool_height -- the spending tx is unexpired
+         )
+         GROUP BY a.uuid",
+    )?;
+    let mut rows = stmt_account_spendable_balances.query(named_params![
+        ":mempool_height": u32::from(mempool_height),
+        ":min_confirmations": min_confirmations,
+    ])?;
+
+    while let Some(row) = rows.next()? {
+        let account = AccountUuid(row.get(0)?);
+        let raw_value = row.get(1)?;
+        let value = NonNegativeAmount::from_nonnegative_i64(raw_value).map_err(|_| {
+            SqliteClientError::CorruptedData(format!("Negative UTXO value {:?}", raw_value))
+        })?;
+
+        account_balances
+            .entry(account)
+            .or_insert(AccountBalance::ZERO)
+            .with_unshielded_balance_mut(|bal| bal.add_spendable_value(value))?;
+    }
+
+    let mut stmt_account_unconfirmed_balances = conn.prepare(
+        "SELECT a.uuid, SUM(u.value_zat)
+         FROM transparent_received_outputs u
+         JOIN accounts a ON a.id = u.account_id
+         JOIN transactions t ON t.id_tx = u.transaction_id
+         -- the transaction that created the output is mined with not enough confirmations or is definitely unexpired
+         WHERE (
+            t.mined_height < :mempool_height
+            AND :mempool_height - t.mined_height < :min_confirmations -- tx is mined but not confirmed
             OR t.expiry_height = 0 -- tx will not expire
             OR t.expiry_height >= :mempool_height
          )
@@ -432,8 +475,11 @@ pub(crate) fn add_transparent_account_balances(
          )
          GROUP BY a.uuid",
     )?;
-    let mut rows = stmt_account_balances
-        .query(named_params![":mempool_height": u32::from(mempool_height),])?;
+
+    let mut rows = stmt_account_unconfirmed_balances.query(named_params![
+        ":mempool_height": u32::from(mempool_height),
+        ":min_confirmations": min_confirmations,
+    ])?;
 
     while let Some(row) = rows.next()? {
         let account = AccountUuid(row.get(0)?);
@@ -445,7 +491,7 @@ pub(crate) fn add_transparent_account_balances(
         account_balances
             .entry(account)
             .or_insert(AccountBalance::ZERO)
-            .add_unshielded_value(value)?;
+            .with_unshielded_balance_mut(|bal| bal.add_pending_spendable_value(value))?;
     }
     Ok(())
 }
@@ -870,4 +916,12 @@ mod tests {
             BlockCache::new(),
         );
     }
+
+    #[test]
+    fn transparent_balance_spendability() {
+        zcash_client_backend::data_api::testing::transparent::transparent_balance_spendability(
+            TestDbFactory::default(),
+            BlockCache::new(),
+        );
+    }
 }