diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 769a7664c1..095f0e79f0 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -7,6 +7,16 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Added +- `zcash_client_backend::data_api`: + - `Progress` + - `WalletSummary::progress` + +### Removed +- `zcash_client_backend::data_api`: + - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have + been removed. Use `WalletSummary::progress` instead. + ## [0.14.0] - 2024-10-04 ### Added diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index ce743fc53c..38ac7abf2e 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -67,7 +67,6 @@ use incrementalmerkletree::{frontier::Frontier, Retention}; use nonempty::NonEmpty; use secrecy::SecretVec; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; -use zcash_keys::address::Address; use zip32::fingerprint::SeedFingerprint; use self::{ @@ -107,7 +106,7 @@ use { use ambassador::delegatable_trait; #[cfg(any(test, feature = "test-dependencies"))] -use zcash_primitives::consensus::NetworkUpgrade; +use {zcash_keys::address::Address, zcash_primitives::consensus::NetworkUpgrade}; pub mod chain; pub mod error; @@ -462,6 +461,51 @@ impl Ratio { } } +/// A type representing the progress the wallet has made toward detecting all of the funds +/// belonging to the wallet. +/// +/// The window over which progress is computed spans from the wallet's birthday to the current +/// chain tip. It is divided into two regions, the "Scan Window" which covers the region from the +/// wallet recovery height to the current chain tip, and the "Recovery Window" which covers the +/// range from the wallet birthday to the wallet recovery height. If no wallet recovery height is +/// available, the scan window will cover the entire range from the wallet birthday to the chain +/// tip. +/// +/// Progress for both scanning and recovery is represented in terms of the ratio between notes +/// scanned and the total number of notes added to the chain in the relevant window. This ratio +/// should only be used to compute progress percentages for display, and the numerator and +/// denominator should not be treated as authoritative note counts. In the case that there are no +/// notes in a given block range, the denominator of these values will be zero, so callers should always +/// use checked division when converting the resulting values to percentages. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Progress { + scan: Ratio, + recovery: Option>, +} + +impl Progress { + /// Constructs a new progress value from its constituent parts. + pub fn new(scan: Ratio, recovery: Option>) -> Self { + Self { scan, recovery } + } + + /// Returns the progress the wallet has made in scanning blocks for shielded notes belonging to + /// the wallet between the wallet recovery height (or the wallet birthday if no recovery height + /// is set) and the chain tip. + pub fn scan(&self) -> Ratio { + self.scan + } + + /// Returns the progress the wallet has made in scanning blocks for shielded notes belonging to + /// the wallet between the wallet birthday and the block height at which recovery from seed was + /// initiated. + /// + /// Returns `None` if no recovery height is set for the wallet. + pub fn recovery(&self) -> Option> { + self.recovery + } +} + /// A type representing the potentially-spendable value of unspent outputs in the wallet. /// /// The balances reported using this data structure may overestimate the total spendable value of @@ -475,8 +519,7 @@ pub struct WalletSummary { account_balances: HashMap, chain_tip_height: BlockHeight, fully_scanned_height: BlockHeight, - scan_progress: Option>, - recovery_progress: Option>, + progress: Progress, next_sapling_subtree_index: u64, #[cfg(feature = "orchard")] next_orchard_subtree_index: u64, @@ -488,8 +531,7 @@ impl WalletSummary { account_balances: HashMap, chain_tip_height: BlockHeight, fully_scanned_height: BlockHeight, - scan_progress: Option>, - recovery_progress: Option>, + progress: Progress, next_sapling_subtree_index: u64, #[cfg(feature = "orchard")] next_orchard_subtree_index: u64, ) -> Self { @@ -497,8 +539,7 @@ impl WalletSummary { account_balances, chain_tip_height, fully_scanned_height, - scan_progress, - recovery_progress, + progress, next_sapling_subtree_index, #[cfg(feature = "orchard")] next_orchard_subtree_index, @@ -527,42 +568,17 @@ impl WalletSummary { /// general usability, including the ability to spend existing funds that were /// previously spendable. /// - /// The window over which progress is computed spans from the wallet's recovery height - /// to the current chain tip. This may be adjusted in future updates to better match - /// the intended semantics. - /// - /// Progress is represented in terms of the ratio between notes scanned and the total - /// number of notes added to the chain in the relevant window. This ratio should only - /// be used to compute progress percentages, and the numerator and denominator should - /// not be treated as authoritative note counts. The denominator of this ratio is - /// guaranteed to be nonzero. - /// - /// Returns `None` if the wallet has insufficient information to be able to determine - /// scan progress. - pub fn scan_progress(&self) -> Option> { - self.scan_progress - } - - /// Returns the progress of recovering the wallet from seed. - /// - /// This progress metric is intended as an indicator of how close the wallet is to - /// having a complete history. - /// - /// The window over which progress is computed spans from the wallet birthday to the - /// wallet's recovery height. This may be adjusted in future updates to better match - /// the intended semantics. - /// - /// Progress is represented in terms of the ratio between notes scanned and the total - /// number of notes added to the chain in the relevant window. This ratio should only - /// be used to compute progress percentages, and the numerator and denominator should - /// not be treated as authoritative note counts. Note that both the numerator and the - /// denominator of this ratio may be zero in the case that there is no recovery range - /// that need be scanned. - /// - /// Returns `None` if the wallet has insufficient information to be able to determine - /// progress in scanning between the wallet birthday and the wallet recovery height. - pub fn recovery_progress(&self) -> Option> { - self.recovery_progress + /// The window over which progress is computed spans from the wallet's birthday to the current + /// chain tip. It is divided into two segments: a "recovery" segment, between the wallet + /// birthday and the recovery height (currently the height at which recovery from seed was + /// initiated, but how this boundary is computed may change in the future), and a "scan" + /// segment, between the recovery height and the current chain tip. + /// + /// When converting the ratios returned here to percentages, checked division must be used in + /// order to avoid divide-by-zero errors. A zero denominator in a returned ratio indicates that + /// there are no shielded notes to be scanned in the associated block range. + pub fn progress(&self) -> Progress { + self.progress } /// Returns the Sapling subtree index that should start the next range of subtree diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 3bf8715640..22a0a9a437 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -889,13 +889,10 @@ pub fn spend_fails_on_unverified_notes( // Wallet is fully scanned let summary = st.get_wallet_summary(1); assert_eq!( - summary.as_ref().and_then(|s| s.recovery_progress()), + summary.as_ref().and_then(|s| s.progress().recovery()), no_recovery, ); - assert_eq!( - summary.and_then(|s| s.scan_progress()), - Some(Ratio::new(1, 1)) - ); + assert_eq!(summary.map(|s| s.progress().scan()), Some(Ratio::new(1, 1))); // Add more funds to the wallet in a second note let (h2, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); @@ -910,13 +907,10 @@ pub fn spend_fails_on_unverified_notes( // Wallet is still fully scanned let summary = st.get_wallet_summary(1); assert_eq!( - summary.as_ref().and_then(|s| s.recovery_progress()), + summary.as_ref().and_then(|s| s.progress().recovery()), no_recovery ); - assert_eq!( - summary.and_then(|s| s.scan_progress()), - Some(Ratio::new(2, 2)) - ); + assert_eq!(summary.map(|s| s.progress().scan()), Some(Ratio::new(2, 2))); // Spend fails because there are insufficient verified notes let extsk2 = T::sk(&[0xf5; 32]); diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 388433b4f5..21977adad1 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -69,7 +69,7 @@ use rusqlite::{self, named_params, params, OptionalExtension}; use secrecy::{ExposeSecret, SecretVec}; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; use zcash_client_backend::data_api::{ - AccountPurpose, DecryptedTransaction, TransactionDataRequest, TransactionStatus, + AccountPurpose, DecryptedTransaction, Progress, TransactionDataRequest, TransactionStatus, }; use zip32::fingerprint::SeedFingerprint; @@ -804,26 +804,6 @@ pub(crate) fn get_derived_account( accounts.next().transpose() } -#[derive(Debug)] -pub(crate) struct Progress { - scan: Ratio, - recovery: Option>, -} - -impl Progress { - pub(crate) fn new(scan: Ratio, recovery: Option>) -> Self { - Self { scan, recovery } - } - - pub(crate) fn scan(&self) -> Ratio { - self.scan - } - - pub(crate) fn recovery(&self) -> Option> { - self.recovery - } -} - pub(crate) trait ProgressEstimator { fn sapling_scan_progress( &self, @@ -1598,8 +1578,7 @@ pub(crate) fn get_wallet_summary( account_balances, chain_tip_height, fully_scanned_height.unwrap_or(birthday_height - 1), - Some(progress.scan), - progress.recovery, + progress, next_sapling_subtree_index, #[cfg(feature = "orchard")] next_orchard_subtree_index, diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index 594c9c32ca..534656e3a8 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1297,15 +1297,17 @@ pub(crate) mod tests { // resulting ratio (the number of notes in the recovery range) is zero. let no_recovery = Some(Ratio::new(0, 0)); - // We have scan ranges and a subtree, but have scanned no blocks. + // We have scan ranges and a subtree, but have scanned no blocks. Given the number of + // blocks scanned in the previous subtree, we estimate the number of notes in the current + // subtree let summary = st.get_wallet_summary(1); assert_eq!( - summary.as_ref().and_then(|s| s.recovery_progress()), + summary.as_ref().and_then(|s| s.progress().recovery()), no_recovery, ); assert_matches!( - summary.and_then(|s| s.scan_progress()), - Some(progress) if progress.numerator() == &0 + summary.map(|s| s.progress().scan()), + Some(ratio) if *ratio.numerator() == 0 ); // Set up prior chain state. This simulates us having imported a wallet @@ -1345,7 +1347,7 @@ pub(crate) mod tests { assert_eq!(summary.as_ref().map(|s| T::next_subtree_index(s)), Some(0)); assert_eq!( - summary.as_ref().and_then(|s| s.recovery_progress()), + summary.as_ref().and_then(|s| s.progress().recovery()), no_recovery ); @@ -1357,7 +1359,7 @@ pub(crate) mod tests { let expected_denom = expected_denom * 2; let expected_denom = expected_denom + 1; assert_eq!( - summary.and_then(|s| s.scan_progress()), + summary.map(|s| s.progress().scan()), Some(Ratio::new(1, u64::from(expected_denom))) ); @@ -1450,7 +1452,7 @@ pub(crate) mod tests { / (max_scanned - (birthday.height() - 10))); let summary = st.get_wallet_summary(1); assert_eq!( - summary.and_then(|s| s.scan_progress()), + summary.map(|s| s.progress().scan()), Some(Ratio::new(1, u64::from(expected_denom))) ); }