[Do not merge] Check that CI test failures show backtraces

.github/workflows/ci.yml

@@ -64,17 +64,19 @@ jobs:
       - name: Run tests
         run: >
           cargo test
-          --release
           --workspace
           ${{ steps.prepare.outputs.feature-flags }}
+        env:
+          RUST_BACKTRACE: 1
       - name: Run slow tests
         run: >
           cargo test
-          --release
           --workspace
           ${{ steps.prepare.outputs.feature-flags }}
           --features expensive-tests
           -- --ignored
+        env:
+          RUST_BACKTRACE: 1
       - name: Verify working directory is clean
         run: git diff --exit-code
 

Cargo.toml

@@ -160,5 +160,22 @@ lto = true
 panic = 'abort'
 codegen-units = 1
 
+[profile.test]
+# Since we have many computationally expensive tests, this changes the test profile to
+# compile with optimizations by default, but keep full debug info.
+#
+# This differs from the release profile in the following ways:
+# - it does not set `lto = true`, which increases compile times without substantially
+#   speeding up tests;
+# - it does not set `codegen-units = 1`, which increases compile times and is only
+#   useful to improve determinism of release builds;
+# - it does not set `panic = 'abort'`, which is in any case ignored for tests.
+#
+# To get results as close as possible to a release build, use `cargo test --release`.
+# To speed up compilation and avoid optimizations potentially resulting in lower-quality
+# debug info, use `cargo test --profile=dev`.
+opt-level = 3
+debug = true
+
 [workspace.lints.rust]
 unexpected_cfgs = { level = "warn", check-cfg = ['cfg(zcash_unstable, values("zfuture"))'] }

zcash_client_backend/CHANGELOG.md

@@ -28,6 +28,9 @@ and this library adheres to Rust's notion of
 - MSRV is now 1.77.0.
 - Migrated to `arti-client 0.23`.
 - `zcash_client_backend::data_api`:
+  - `AccountBalance` was refactored to use `Balance` for transparent funds (issue #1411).
+    `AccountBalance` now has an `unshielded_balance()` that uses `Balance` and replaces the
+    (now deleted) `unshielded` non-negative amount.
   - `InputSource` has an added method `get_wallet_metadata`
   - `error::Error` has additional variant `Error::Change`. This necessitates
     the addition of two type parameters to the `Error` type,
@@ -79,6 +82,15 @@ and this library adheres to Rust's notion of
 
 ### Removed
 - `zcash_client_backend::data_api`:
+  - `AccountBalance::unshielded`. `AccountBalance` no longer provides the `unshielded`
+    method returning a `NonNegativeAmount` for the total of transparent funds.
+    Instead use `unshielded_balance` which provides a `Balance` value.
+  - `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).
   - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have
     been removed. Use `WalletSummary::progress` instead.
   - `testing::input_selector` use explicit `InputSelector` constructors

zcash_client_backend/src/data_api.rs

@@ -231,27 +231,23 @@ 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 {
     /// The [`Balance`] value having zero values for all its fields.
     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.
@@ -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()
+    }
+
+    /// Returns the [`Balance`] of unshielded funds in the account.
+    pub fn unshielded_balance(&self) -> &Balance {
+        &self.unshielded_balance
     }
 
-    /// 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)?;
+    /// 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
@@ -1028,8 +1038,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,