Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zcash_client_sqlite: Fix errors in progress computation. #1564

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions supply-chain/imports.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ audited_as = "0.13.0"
version = "0.12.0"
audited_as = "0.11.2"

[[unpublished.zcash_client_sqlite]]
version = "0.12.1"
audited_as = "0.12.0"

[[unpublished.zcash_keys]]
version = "0.4.0"
audited_as = "0.3.0"
Expand Down Expand Up @@ -270,6 +274,13 @@ user-id = 169181
user-login = "nuttycom"
user-name = "Kris Nuttycombe"

[[publisher.zcash_client_sqlite]]
version = "0.12.0"
when = "2024-10-04"
user-id = 169181
user-login = "nuttycom"
user-name = "Kris Nuttycombe"

[[publisher.zcash_encoding]]
version = "0.2.0"
when = "2022-10-19"
Expand Down Expand Up @@ -346,6 +357,13 @@ user-id = 169181
user-login = "nuttycom"
user-name = "Kris Nuttycombe"

[[publisher.zip321]]
version = "0.2.0"
when = "2024-10-04"
user-id = 169181
user-login = "nuttycom"
user-name = "Kris Nuttycombe"

[[audits.bytecode-alliance.wildcard-audits.bumpalo]]
who = "Nick Fitzgerald <fitzgen@gmail.com>"
criteria = "safe-to-deploy"
Expand Down
16 changes: 10 additions & 6 deletions zcash_client_backend/src/data_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,10 +534,11 @@ impl<AccountId: Eq + Hash> WalletSummary<AccountId> {
/// 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.
/// not be treated as authoritative note counts. The denominator of this ratio is
/// guaranteed to be nonzero.
Comment on lines +537 to +538
Copy link
Contributor

@daira daira Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

///
/// Returns `None` if the wallet is unable to determine the size of the note
/// commitment tree.
/// Returns `None` if the wallet has insufficient information to be able to determine
/// scan progress.
pub fn scan_progress(&self) -> Option<Ratio<u64>> {
self.scan_progress
}
Expand All @@ -554,10 +555,12 @@ impl<AccountId: Eq + Hash> WalletSummary<AccountId> {
/// 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.
/// 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 is unable to determine the size of the note
/// commitment tree.
/// 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<Ratio<u64>> {
self.recovery_progress
}
Expand Down Expand Up @@ -1270,6 +1273,7 @@ pub trait WalletTest: InputSource + WalletRead {
///
/// This type is opaque, and exists for use by tests defined in this crate.
#[cfg(any(test, feature = "test-dependencies"))]
#[allow(dead_code)]
#[derive(Clone, Debug)]
pub struct OutputOfSentTx {
value: NonNegativeAmount,
Expand Down
16 changes: 6 additions & 10 deletions zcash_client_backend/src/data_api/testing/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,20 +881,16 @@ pub fn spend_fails_on_unverified_notes<T: ShieldedPoolTester>(
NonNegativeAmount::ZERO
);

// The account is configured without a recover-until height, so is by definition
// fully recovered, and we count 1 per pool for both numerator and denominator.
let fully_recovered = {
let n = 1;
#[cfg(feature = "orchard")]
let n = n * 2;
Some(Ratio::new(n, n))
};
// If none of the wallet's accounts have a recover-until height, then there
// is no recovery phase for the wallet, and therefore the denominator in the
// resulting ratio (the number of notes in the recovery range) is zero.
let no_recovery = Some(Ratio::new(0, 0));

// Wallet is fully scanned
let summary = st.get_wallet_summary(1);
assert_eq!(
summary.as_ref().and_then(|s| s.recovery_progress()),
fully_recovered,
no_recovery,
);
assert_eq!(
summary.and_then(|s| s.scan_progress()),
Expand All @@ -915,7 +911,7 @@ pub fn spend_fails_on_unverified_notes<T: ShieldedPoolTester>(
let summary = st.get_wallet_summary(1);
assert_eq!(
summary.as_ref().and_then(|s| s.recovery_progress()),
fully_recovered
no_recovery
);
assert_eq!(
summary.and_then(|s| s.scan_progress()),
Expand Down
10 changes: 10 additions & 0 deletions zcash_client_sqlite/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ and this library adheres to Rust's notion of

## [Unreleased]

## [0.12.1] - 2024-10-10

### Fixed
- An error in scan progress computation was fixed. As part of this fix, wallet
summary information is now only returned in the case that some note
commitment tree size information can be determined, either from subtree root
download or from downloaded block data. NOTE: The recovery progress ratio may
be present as `0:0` in the case that the recovery range contains no notes;
this was not adequately documented in the previous release.

## [0.12.0] - 2024-10-04

### Added
Expand Down
2 changes: 1 addition & 1 deletion zcash_client_sqlite/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "zcash_client_sqlite"
description = "An SQLite-based Zcash light client"
version = "0.12.0"
version = "0.12.1"
authors = [
"Jack Grigg <jack@z.cash>",
"Kris Nuttycombe <kris@electriccoin.co>"
Expand Down
4 changes: 2 additions & 2 deletions zcash_client_sqlite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ pub mod error;
pub mod wallet;
use wallet::{
commitment_tree::{self, put_shard_roots},
SubtreeScanProgress,
SubtreeProgressEstimator,
};

#[cfg(test)]
Expand Down Expand Up @@ -461,7 +461,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
&self.conn.borrow().unchecked_transaction()?,
&self.params,
min_confirmations,
&SubtreeScanProgress,
&SubtreeProgressEstimator,
)
}

Expand Down
Loading
Loading