-
Notifications
You must be signed in to change notification settings - Fork 252
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
zcash_client_sqlite: Fix errors in progress computation. #1564
Conversation
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.
Codecov ReportAttention: Patch coverage is
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. |
zcash_client_sqlite/src/wallet.rs
Outdated
// 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))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scan progress denominator is guaranteed to always be nonzero, so adding the numerators and denominators always results in a valid fractional value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK with comments.
// Get the starting note commitment tree size from the wallet birthday, or failing that | ||
// from the blocks table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"or failing that from the blocks table" seems wrong now; get_tree_size_near
doesn't use the blocks
table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a56de09
to
108e953
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 108e953. My remaining comment feedback is non-blocking.
cd1b13b
to
279479c
Compare
force-pushed to address comments from code review and prepare for the hotfix release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 279479c
1f711e0
into
zcash:hotfix/zcash_client_sqlite-0.12.x
/// not be treated as authoritative note counts. The denominator of this ratio is | ||
/// guaranteed to be nonzero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-hoc ACK 279479c. There is a minor inaccuracy in a comment, I think (https://github.com/zcash/librustzcash/pull/1564/files#r1797495621).
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.