zcash_client_sqlite: Fix errors in progress computation. #1564

nuttycom · 2024-10-10T17:55:07Z

This ensures that we use recover_until_height as an exclusive end height, and makes sure that we are estimating tree size based upon consistent heights between the recovery and scan ranges.

In scan progress computation, `recover_until_height` was incorrectly being treated as an inclusive end, meaning that it was possible for a block's notes to be counted both within the recovery range and within the scanning range.

…uments.

codecov · 2024-10-10T18:14:49Z

Codecov Report

Attention: Patch coverage is 59.88701% with 71 lines in your changes missing coverage. Please review.

Project coverage is 61.65%. Comparing base (e57dc41) to head (279479c).
Report is 12 commits behind head on hotfix/zcash_client_sqlite-0.12.x.

Files with missing lines	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	58.95%	71 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##           hotfix/zcash_client_sqlite-0.12.x    #1564      +/-   ##
=====================================================================
+ Coverage                              61.63%   61.65%   +0.01%     
=====================================================================
  Files                                    148      148              
  Lines                                  18821    18832      +11     
=====================================================================
+ Hits                                   11601    11610       +9     
- Misses                                  7220     7222       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zcash_client_sqlite/src/wallet.rs

daira · 2024-10-10T19:01:48Z

zcash_client_sqlite/src/wallet.rs

+            // 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.
+            .unwrap_or_else(|| Some(Ratio::new(0, 0)));


This is fine for this API, but is an incompatible change for the SDK-level API. It's fine if we intend to explicitly handle 0:0 at the wallet level now; otherwise we should map that to 1:1 to emulate the old API.

We can cut an 0.13.0 as a hotfix if a breaking change is required. But I'm also inclined to not make this breaking change, unless there's a bug-related reason for it.

Currently the mapping to the existing API used by the SDK is done in zcash-light-client-ffi. That's where it makes most sense to do the mapping from 0:0 to 1:1, after adding the numerators and denominators, IMHO (i.e. in Electric-Coin-Company/zcash-light-client-ffi#156).

The scan progress denominator is guaranteed to always be nonzero, so adding the numerators and denominators always results in a valid fractional value.

In pairing with @str4d, we determined that there may have already been uncommon but possible corner cases where the recovery ratio could have had a zero denominator: this would occur in circumstances where no notes for a given shielded protocol exist in blocks between the wallet's birthday height and the wallet recovery height. This is actually a relatively easily observable situation on testnet, since most of the time there's no shielded testnet activity. As such, 0:0 is not an API-breaking change; we just hadn't previously thought about or documented it adequately.

zcash_client_sqlite/src/wallet.rs

str4d · 2024-10-10T21:17:04Z

zcash_client_sqlite/src/wallet.rs

+        let mut get_tree_size_near = |as_of: BlockHeight| {
+            stmt_start_tree_size
+                .query_row(
+                    named_params![":start_height": u32::from(birthday_height)],
+                    |row| row.get::<_, Option<u64>>(0),
+                )
+                .optional()
+                .map(|opt| opt.flatten())
+                .transpose()
+                .or_else(|| {
+                    conn.query_row(
+                        &format!(
+                            "SELECT MIN(shard_index)
+                    FROM {table_prefix}_tree_shards
+                    WHERE subtree_end_height >= :start_height
+                    OR subtree_end_height IS NULL",
+                        ),
+                        named_params! {
+                            ":start_height": u32::from(as_of),
+                        },
+                        |row| {
+                            let min_tree_size = row
+                                .get::<_, Option<u64>>(0)?
+                                .map(|min_idx| min_idx << shard_height);
+                            Ok(min_tree_size)
+                        },
+                    )
+                    .optional()
+                    .map(|opt| opt.flatten())
+                    .transpose()
+                })
+                .transpose()
+        };


I made the same comment to @nuttycom in mob today, that for this kind of highly-nested situation I find imperative code with early returns (?) clearer than functional code with type transposing (despite my general tendency to implement the latter, because in many cases it is great).

zcash_client_sqlite/src/wallet.rs

str4d

Reviewed a56de09. Changes to public API need to be discussed (as to whether we keep them, document them in the changelog, and release the hotfix as 0.13.0, or undo them and release the hotfix as 0.12.1).

In either case, this PR is missing a changelog entry documenting the fix.

zcash_client_sqlite/src/wallet.rs

daira

utACK with comments.

daira · 2024-10-10T22:12:22Z

zcash_client_sqlite/src/wallet.rs

        // Get the starting note commitment tree size from the wallet birthday, or failing that
        // from the blocks table.


"or failing that from the blocks table" seems wrong now; get_tree_size_near doesn't use the blocks table.

I was mistaken; stmt_start_tree_size does use the blocks table. size_from_subtree_roots doesn't though, so this comment is still not quite accurate (i.e. there is a case in which the tree size comes from another source than the blocks table).

We now compute the fallback tree size at the birthday height and the recover-until height separately in order to avoid the mixing of concerns.

…ss computation.

… tests.

str4d

utACK 108e953. My remaining comment feedback is non-blocking.

zcash_client_sqlite/src/wallet.rs

…ecovery_progress` methods.

nuttycom · 2024-10-11T02:32:11Z

force-pushed to address comments from code review and prepare for the hotfix release.

str4d

utACK 279479c

daira · 2024-10-11T23:29:16Z

zcash_client_backend/src/data_api.rs

+    /// not be treated as authoritative note counts. The denominator of this ratio is
+    /// guaranteed to be nonzero.


Is this correct?

daira

Post-hoc ACK 279479c. There is a minor inaccuracy in a comment, I think (https://github.com/zcash/librustzcash/pull/1564/files#r1797495621).

nuttycom added 2 commits October 10, 2024 11:52

zcash_client_sqlite: Minor refactoring of subtree_scan_progress arg…

acb0911

…uments.

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

str4d reviewed Oct 10, 2024

View reviewed changes

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

daira reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved

str4d requested changes Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

daira approved these changes Oct 10, 2024

View reviewed changes

daira reviewed Oct 10, 2024

View reviewed changes

nuttycom added 6 commits October 10, 2024 17:03

zcash_client_sqlite: Fix fallback tree size usage in scan progress.

c35c565

We now compute the fallback tree size at the birthday height and the recover-until height separately in order to avoid the mixing of concerns.

zcash_client_sqlite: Factor out tree size estimation from scan progre…

d89b04c

…ss computation.

Factor out minor internal code duplication in progress computation.

930f893

zcash_client_backend: Suppress dead code warning.

fa42b2e

Fix rustfmt and cargo vet errors.

f89b3ce

zcash_client_sqlite: Modify Progress::scan to be non-optional & fix…

e4c6a3d

… tests.

nuttycom force-pushed the hotfix/scan_progress branch from a56de09 to 108e953 Compare October 10, 2024 23:37

str4d reviewed Oct 10, 2024

View reviewed changes

zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved

nuttycom added 3 commits October 10, 2024 20:19

Apply code style suggestions from code review.

c108281

zcash_client_backend: Clarify documentation of scan_progress and `r…

370c83f

…ecovery_progress` methods.

Release zcash_client_sqlite version 0.12.1

279479c

nuttycom force-pushed the hotfix/scan_progress branch from cd1b13b to 279479c Compare October 11, 2024 02:31

str4d approved these changes Oct 11, 2024

View reviewed changes

nuttycom merged commit 1f711e0 into zcash:hotfix/zcash_client_sqlite-0.12.x Oct 11, 2024
26 of 27 checks passed

nuttycom deleted the hotfix/scan_progress branch October 11, 2024 14:49

daira reviewed Oct 11, 2024

View reviewed changes

This was referenced Oct 24, 2024

zcash_client_sqlite: Off-by-one in truncate_to_height #1263

Closed

zcash_client_sqlite: Fix off-by-one in scan estimation start height #945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zcash_client_sqlite: Fix errors in progress computation. #1564

zcash_client_sqlite: Fix errors in progress computation. #1564

nuttycom commented Oct 10, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

daira Oct 10, 2024

str4d Oct 10, 2024 •

edited

Loading

daira Oct 10, 2024 •

edited

Loading

nuttycom Oct 10, 2024

nuttycom Oct 11, 2024

str4d Oct 10, 2024

str4d left a comment

daira left a comment •

edited

Loading

daira Oct 10, 2024 •

edited

Loading

daira Oct 11, 2024 •

edited

Loading

str4d left a comment

nuttycom commented Oct 11, 2024

str4d left a comment

daira Oct 11, 2024 •

edited

Loading

daira left a comment •

edited

Loading

		// Get the starting note commitment tree size from the wallet birthday, or failing that
		// from the blocks table.

		/// not be treated as authoritative note counts. The denominator of this ratio is
		/// guaranteed to be nonzero.

zcash_client_sqlite: Fix errors in progress computation. #1564

zcash_client_sqlite: Fix errors in progress computation. #1564

Conversation

nuttycom commented Oct 10, 2024

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

daira Oct 10, 2024

Choose a reason for hiding this comment

str4d Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

daira Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

nuttycom Oct 10, 2024

Choose a reason for hiding this comment

nuttycom Oct 11, 2024

Choose a reason for hiding this comment

str4d Oct 10, 2024

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

daira Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom commented Oct 11, 2024

str4d left a comment

Choose a reason for hiding this comment

daira Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2024 •

edited

Loading

str4d Oct 10, 2024 •

edited

Loading

daira Oct 10, 2024 •

edited

Loading

daira left a comment •

edited

Loading

daira Oct 10, 2024 •

edited

Loading

daira Oct 11, 2024 •

edited

Loading

daira Oct 11, 2024 •

edited

Loading

daira left a comment •

edited

Loading