zcash_client_backend: Return scan and restore progress as a single value.

[nuttycom]

In zcash_client_backend 0.14, the semantics of the WalletSummary::scan_progress method changed silently, which could lead to unwary users getting bad results for scan progress, in particular, an indication that scanning is complete when there is still a substantial amount of recovery still needed.

In addition, the type of the WalletSummary::scan_progress and recovery_progress methods incorrectly make the state where recovery progress information is available, but scan progress information is not available, representable.

This PR introduces a breaking API change that is intended to reduce the possibility of incorrect usage, and correctly represent the possible states of scanning and recovery.

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.64%. Comparing base (dd51c2a) to head (634e43e).
Report is 58 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1562      +/-   ##
==========================================
+ Coverage   61.63%   61.64%   +0.01%     
==========================================
  Files         148      148              
  Lines       18836    18829       -7     
==========================================
- Hits        11610    11608       -2     
+ Misses       7226     7221       -5     

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

[str4d]

utACK 44c834e after doc fix

[str4d]

As I noted in a pairing, if we combine this API change with a migration that ensures every wallet has a recover-until height (and potentially also the other change where we slowly ratchet the recover-until height over time), then we could make this new API non-optional (as then a lack of recover-until height could be treated the same way as a lack of information for scan progress: making the entire WalletSummary struct None).

Fine to leave this as-is in this PR, but it does affect when we include this in a release.

[nuttycom]

I'm undecided as to whether we should make the recover-until height non-optional. If we want to have a separate height where we split the range, I wonder whether we should have a different column for it in the database? Right now, the recovery height stores the height at which recovery from seed was initiated, and if we later mutate that height, then we'll lose that information. Also, I need to think more about the interaction between updating that height (or recording a separate height at which we want to split scanning) and the feature I want to add where we track where we've inserted frontiers to have better information about what we actually need to scan to make notes spendable.

[daira]

I'm skeptical about adding a migration for this. Once the old wallets that didn't have the recovery height set have been recovered, there is no further benefit to setting it, right? And a None value will be treated in the UI as 100% recovered, which is then correct. So the problem will fix itself, if it hasn't already, for all of those users. The slight improvement in API elegance isn't worth a database migration.

[str4d]

The point of setting it at all in a migration, would be for subsequently changing it over time for all users.

If a user has a fully synced wallet (whether or not recovered from seed), and then their wallet is offline for 6 months, when it comes back online we inherently have two scan ranges: the "make my existing funds spendable" range, and the "catch up on 6 months of history" range. Treating the latter as "recovery" is a way of distinguishing this slower process from the steady-state sync process which we want to be fast and responsive.

Meanwhile, if a user is always keeping their wallet synced, then when they open their app the "scanning" percentage is always going from 99.3% or something to 100%, and provides almost no utility to the user. From this perspective, moving the recover-until height forwards would be an internal recognition of the range that would be counted as recovery if the user lost their phone right now and had to do so on a new phone. And it has the nice effect that the scan progress is always over a small range and will have more useful progress updates.

[nuttycom]

The point of setting it at all in a migration, would be for subsequently changing it over time for all users.

If we want to update the recover_until_height value, then I think that creating an additional column is still the right approach here, in the following fashion:

• Add a recovery_init_height that is an optional column, and has essentially the same semantics as the existing recover_until_height: it is not set for new wallets, but is set when a user begins recovery from seed. This allows us to distinguish between wallets that have been recovered from seed and those that have not; additionally, it allows the user to track exactly when they started using a given incarnation of the wallet - this sort of history is useful for e.g. when you're trying to figure out when a certain thing happened for tax reporting purposes.
• recover_until_height becomes a required column, and it is initially set to either recovery_init_height for wallets initialized via recovery from seed, or set to the wallet birthday height for new wallets, and it is then updated according to whatever rules we decide on.

What I'm not sure about is, should we perhaps just use some other height value that is already known to the wallet for the recover-until height for progress computation? In the "jumps forward 6 months" use case, we would probably want to set the recover-until height to previous_shard_end_height + 1. But, we could already do that without adding another column or storing additional state.

One possibility along these lines is to set the wallet recovery window as:

let recovery_window = wallet_birthday_height..(max(recover_until_height, Some(previous_shard_end_height + 1), Some(fully_scanned_height + 1)).unwrap())

This doesn't require the addition of a new column or the recover_until_height column to be updated in a way that destroys the history data I want to preserve, and I think it gets us all the features we want from that boundary. And if a shard is too "big", we could even throw chain_tip_height - 100 into that max expression.

[daira]

utACK modulo @str4d's comments.

[str4d]

utACK 634e43e