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: Ensure that target and anchor heights are relative to the chain tip. #896

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

nuttycom
Copy link
Contributor

Prior to the scan-before-sync changes, the wallet was able to assume that the maximum scanned block height at the time of the spend was within a few blocks of the chain tip. However, under linear scanning after the spend-before-sync changes this invariant no longer holds, resulting in a situation where in linear sync conditions the wallet could attempt to create transactions with already-past expiry heights.

This change separates the notion of "chain tip" from "max scanned height", relying upon the scan_queue table to maintain the wallet's view of the consensus chain height and using information from the blocks table only in situations where the latest and/or earliest scanned height is required.

As part of this change, the WalletRead interface is also modified to disambiguate these concepts.

@nuttycom nuttycom marked this pull request as ready for review August 14, 2023 16:27
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Patch coverage: 62.16% and project coverage change: -0.04% ⚠️

Comparison is base (d25f2bd) 70.73% compared to head (69b4d55) 70.70%.

❗ Current head 69b4d55 differs from pull request most recent head dee4385. Consider uploading reports for the commit dee4385 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
- Coverage   70.73%   70.70%   -0.04%     
==========================================
  Files         131      131              
  Lines       12728    12742      +14     
==========================================
+ Hits         9003     9009       +6     
- Misses       3725     3733       +8     
Files Changed Coverage Δ
zcash_client_backend/src/data_api.rs 36.20% <0.00%> (-7.98%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 86.55% <0.00%> (+0.72%) ⬆️
zcash_client_sqlite/src/chain.rs 49.58% <ø> (ø)
zcash_client_sqlite/src/wallet/sapling.rs 80.15% <ø> (ø)
zcash_client_sqlite/src/wallet/scanning.rs 91.63% <ø> (ø)
zcash_client_sqlite/src/wallet.rs 79.02% <72.72%> (-0.27%) ⬇️
zcash_client_sqlite/src/lib.rs 67.56% <77.77%> (+0.06%) ⬆️

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

@nuttycom nuttycom force-pushed the chain_tip_target_height branch 2 times, most recently from af33c93 to f7080a5 Compare August 14, 2023 19:47
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

I only reviewed the new commit in this PR (f7080a5).

zcash_client_backend/src/data_api.rs Show resolved Hide resolved
zcash_client_backend/CHANGELOG.md Show resolved Hide resolved
zcash_client_backend/src/data_api.rs Show resolved Hide resolved
zcash_client_sqlite/src/wallet/sapling.rs Show resolved Hide resolved
@nuttycom nuttycom force-pushed the chain_tip_target_height branch 2 times, most recently from 67029d6 to 69b4d55 Compare August 15, 2023 15:26
…ve to the chain tip.

Prior to the scan-before-sync changes, the wallet was able to assume
that the maximum scanned block height at the time of the spend was
within a few blocks of the chain tip. However, under linear scanning
after the spend-before-sync changes this invariant no longer holds,
resulting in a situation where in linear sync conditions the wallet
could attempt to create transactions with already-past expiry heights.

This change separates the notion of "chain tip" from "max scanned
height", relying upon the `scan_queue` table to maintain the wallet's
view of the consensus chain height and using information from the
`blocks` table only in situations where the latest and/or earliest
scanned height is required.

As part of this change, the `WalletRead` interface is also modified to
disambiguate these concepts.
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK dee4385

@str4d str4d merged commit 104577c into zcash:main Aug 17, 2023
9 of 10 checks passed
@nuttycom nuttycom deleted the chain_tip_target_height branch August 17, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants