Implement `suggest_scan_ranges` and updated chain validation. by nuttycom · Pull Request #872 · zcash/librustzcash

nuttycom · 2023-07-07T00:07:08Z

No description provided.

codecov · 2023-07-07T19:41:27Z

Codecov Report

Patch coverage: 72.45% and project coverage change: +0.27 🎉

Comparison is base (4d5dc28) 69.85% compared to head (6bddc46) 70.12%.

❗ Current head 6bddc46 differs from pull request most recent head c7b308b. Consider uploading reports for the commit c7b308b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #872      +/-   ##
==========================================
+ Coverage   69.85%   70.12%   +0.27%     
==========================================
  Files         127      129       +2     
  Lines       12006    12480     +474     
==========================================
+ Hits         8387     8752     +365     
- Misses       3619     3728     +109

Impacted Files	Coverage Δ
zcash_client_backend/src/data_api.rs	`40.00% <0.00%> (-0.68%)`	⬇️
zcash_client_backend/src/data_api/chain.rs	`56.62% <ø> (ø)`
zcash_client_backend/src/proto/service.rs	`0.00% <0.00%> (ø)`
zcash_client_backend/src/scanning.rs	`42.40% <0.00%> (-2.85%)`	⬇️
zcash_client_sqlite/src/wallet/init.rs	`53.57% <0.00%> (-0.65%)`	⬇️
zcash_primitives/src/sapling/tree.rs	`89.13% <0.00%> (-6.22%)`	⬇️
zcash_client_sqlite/src/wallet/commitment_tree.rs	`93.68% <37.50%> (-1.30%)`	⬇️
...te/src/wallet/init/migrations/shardtree_support.rs	`55.20% <48.71%> (+1.98%)`	⬆️
zcash_client_sqlite/src/chain.rs	`52.42% <50.00%> (-0.10%)`	⬇️
zcash_client_backend/src/data_api/scanning.rs	`82.85% <82.85%> (ø)`
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

str4d · 2023-07-08T01:30:17Z

+        );
+        insert_into_cache(&db_cache, &cb);
+
+        for i in 1..=10 {


This should be an exclusive end, or the 11th block should either be scanned, or expected not to be scanned (and noted as such somewhere).

This implements a priority queue backed by the wallet database for scan range ordering. The scan queue is updated on each call to `put_blocks` or to `update_chain_tip`.

str4d

Flushing my review comments.

str4d · 2023-07-10T20:29:34Z

    string orchardTree = 6; // orchard commitment tree state
 }

+enum ShieldedProtocol {


I confirmed that this matches the canonical state of service.proto as of zcash/lightwalletd@ab90099.

daira · 2023-07-18T17:57:52Z

+        use super::RangeOrdering::*;
+        // Equal
+        assert_eq!(RangeOrdering::cmp(&(0..1), &(0..1)), Equal);
+


Suggested change

assert_eq!(RangeOrdering::cmp(&(1..1), &(1..1)), Equal);

This is technically needed to cover all the cases, but it's non-blocking.

daira

Reviewed with blocking comments (the questions about ordering of inputs to ScanRange::from_parts).

daira · 2023-07-18T18:25:23Z

+                    LeftFirstOverlap => {
+                        let split_point = left.span().end;
+                        if split_point > to_insert.block_range().start {
+                            Self::from_split(*left, *right, to_insert, split_point)


I think we should pay attention to the check warnings saying that some of these cases are not covered by tests. If you disagree, I won't block on it.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

Previously it would panic due to constructing invalid `ScanRange`s.

daira

utACK

Co-authored-by: Daira Emma Hopwood <daira@jacaranda.org>

ebfull

ACK and tested. I haven't thoroughly reviewed some parts but those appear to have been given plenty of attention by others.

daira

utACK; remaining suggestions are non-blocking.

…14868de..d978256a2 d978256a2 Merge pull request #1 from zcash/compact_tx_transparent 7eeb82e7c Merge pull request #4 from zcash/add_changelog a95359dc9 Apply suggestions from code review 592b637a8 Add transparent data to the `CompactBlock` format. 9d1fb2c41 Add a CHANGELOG.md that documents the evolution of the light client protocol. 180717dfa Merge pull request #3 from zcash/merge_librustzcash_history 450bd4181 Merge the history of the .proto files from `librustzcash` for complete history preservation. a4859d11d Move protobuf files into place for use in `zcash/lightwallet-protocol` 2e66cdd9e Update zcash_client_backend/proto/service.proto eda012519 fix comment f838d10ad Add gRPC LightdInfo Donation Address db12c0415 Merge pull request #1473 from nuttycom/wallet/enrichment_queue 698feba96 Apply suggestions from code review 20ce57ab3 zcash_client_backend: Add `block_height` argument to `decrypt_and_store_transaction` a6dea1da8 Merge pull request #1482 from zancas/doc_tweak 4d2d45fc9 fix incorrect doc-comment e826f4740 update CompactBlock doc-comment, to cover non-Sapling shielded notes, and addresses e9a6c00bf Various documentation improvements 988bc7214 Merge pull request #872 from nuttycom/feature/pre_dag_sync-suggest_scan_ranges 58d07d469 Implement `suggest_scan_ranges` and `update_chain_tip` a9222b338 Address comments from code review. e20310857 Rename proto::compact::{BlockMetadata => ChainMetadata} ac63418c5 Reorganize Sapling and Orchard note commitment tree sizes in CompactBlock. 0fdca14f1 zcash_client_backend: Add note commitment tree sizes to `CompactBlock` serialization. 2a0c2b8b7 zcash_client_backend: Add gRPC bindings behind feature flag 1342f0480 zcash_client_backend: Address compact_formats.proto comments 68aa4e01b zcash_client_backend: Bring in latest `compact_formats.proto` e712eb1bc Add prevHash field to CompactBlock 440384c3e Build protobufs for compact formats git-subtree-dir: zcash_client_backend/lightwallet-protocol git-subtree-split: d978256a2bf33bb5f83846d8994e7377512f5f05

…14868de..23f0768ea 23f0768ea Release lightwallet-protocol v0.4.0 41156c767 Merge pull request #11 from zcash/feature/get_mempool_tx_pools 7c130e883 Add `lightwalletProtocolVersion` field to `LightdInfo` struct. edbb726d7 Apply suggestion from code review 38fddd73b Apply suggestions from code review 0250f2720 Add pool type filtering to `GetMempoolTx` argument. 54ccaadd5 Change semantics of pool-based pruning of compact transactions from "may prune" to "must prune". b0667ec99 Merge pull request #9 from zcash/2025-11-doc-TransparentAddressBlockFilter f3fea7bd4 doc: TransparentAddressBlockFilter doesn't include mempool a67dd323a Merge pull request #8 from zcash/2025-11-lightdinfo-upgrade-info 11da4b7e3 add next upgrade info to LightdInfo structure (GetLightdInfo) 42cd8f720 Transparent data docs update (#7) c0cf957ac Merge pull request #5 from zcash/2025-11-comments 912fc3609 Minor clarification in GetBlockRange documentation. 6b03f2cce Documentation (comments) only d978256a2 Merge pull request #1 from zcash/compact_tx_transparent 7eeb82e7c Merge pull request #4 from zcash/add_changelog a95359dc9 Apply suggestions from code review 592b637a8 Add transparent data to the `CompactBlock` format. 9d1fb2c41 Add a CHANGELOG.md that documents the evolution of the light client protocol. 180717dfa Merge pull request #3 from zcash/merge_librustzcash_history 450bd4181 Merge the history of the .proto files from `librustzcash` for complete history preservation. a4859d11d Move protobuf files into place for use in `zcash/lightwallet-protocol` 2e66cdd9e Update zcash_client_backend/proto/service.proto eda012519 fix comment f838d10ad Add gRPC LightdInfo Donation Address db12c0415 Merge pull request #1473 from nuttycom/wallet/enrichment_queue 698feba96 Apply suggestions from code review 20ce57ab3 zcash_client_backend: Add `block_height` argument to `decrypt_and_store_transaction` a6dea1da8 Merge pull request #1482 from zancas/doc_tweak 4d2d45fc9 fix incorrect doc-comment e826f4740 update CompactBlock doc-comment, to cover non-Sapling shielded notes, and addresses e9a6c00bf Various documentation improvements 988bc7214 Merge pull request #872 from nuttycom/feature/pre_dag_sync-suggest_scan_ranges 58d07d469 Implement `suggest_scan_ranges` and `update_chain_tip` a9222b338 Address comments from code review. e20310857 Rename proto::compact::{BlockMetadata => ChainMetadata} ac63418c5 Reorganize Sapling and Orchard note commitment tree sizes in CompactBlock. 0fdca14f1 zcash_client_backend: Add note commitment tree sizes to `CompactBlock` serialization. 2a0c2b8b7 zcash_client_backend: Add gRPC bindings behind feature flag 1342f0480 zcash_client_backend: Address compact_formats.proto comments 68aa4e01b zcash_client_backend: Bring in latest `compact_formats.proto` e712eb1bc Add prevHash field to CompactBlock 440384c3e Build protobufs for compact formats git-subtree-dir: zcash_client_backend/lightwallet-protocol git-subtree-split: 23f0768ea4471b63285f3c0e9b6fbb361674aa2b

nuttycom force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch 5 times, most recently from 12e182d to 86a4af7 Compare July 7, 2023 19:22

nuttycom changed the title ~~WIP: Implement suggest_scan_ranges and updated chain validation.~~ Implement suggest_scan_ranges and updated chain validation. Jul 7, 2023

nuttycom marked this pull request as ready for review July 7, 2023 19:22

nuttycom force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from 86a4af7 to ea5faa0 Compare July 7, 2023 19:40

nuttycom force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch 7 times, most recently from 4d775aa to c5a8609 Compare July 7, 2023 23:51

str4d reviewed Jul 8, 2023

View reviewed changes

Comment thread zcash_client_backend/src/data_api/chain.rs Outdated

Comment thread zcash_client_backend/src/data_api/chain.rs

nuttycom force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch 2 times, most recently from a7b59bb to caea5da Compare July 8, 2023 01:09

str4d reviewed Jul 8, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/scanning.rs Outdated

nuttycom added 3 commits July 7, 2023 20:13

Implement suggest_scan_ranges and update_chain_tip

6fa0b46

This implements a priority queue backed by the wallet database for scan range ordering. The scan queue is updated on each call to `put_blocks` or to `update_chain_tip`.

Fix an error in dominance calculation.

6db4355

Add a test for scan_complete and update_chain_tip

352e1c7

nuttycom force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from caea5da to 352e1c7 Compare July 8, 2023 02:13

daira reviewed Jul 10, 2023

View reviewed changes

Comment thread zcash_client_backend/CHANGELOG.md

daira reviewed Jul 10, 2023

View reviewed changes

Comment thread zcash_client_backend/proto/service.proto

str4d reviewed Jul 10, 2023

View reviewed changes

daira reviewed Jul 10, 2023

View reviewed changes

Comment thread zcash_client_backend/src/data_api/scanning.rs Outdated

daira reviewed Jul 10, 2023

View reviewed changes

Comment thread zcash_client_backend/src/data_api/scanning.rs Outdated

daira reviewed Jul 18, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/scanning.rs Outdated

daira reviewed Jul 18, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/scanning.rs Outdated

str4d force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from 89d5661 to fa8b135 Compare July 18, 2023 18:54

daira reviewed Jul 18, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/scanning.rs

str4d force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from fa8b135 to 1a5bc86 Compare July 18, 2023 19:04

str4d and others added 4 commits July 19, 2023 14:54

Documentation updates, fixes, and cleanups

a87dca0

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

Migrate to latest shardtree to improve performance

98ea08e

Do nothing if update_chain_tip is given a tip lower than we can use

359ff55

Previously it would panic due to constructing invalid `ScanRange`s.

Add test for join_nonoverlapping.

e16aa41

str4d force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from 2cb284f to e16aa41 Compare July 19, 2023 14:54

daira reviewed Jul 19, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet/scanning.rs

daira previously approved these changes Jul 19, 2023

View reviewed changes

daira mentioned this pull request Jul 19, 2023

Update comment in protobuf file zcash/lightwalletd#446

Closed

daira reviewed Jul 19, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/wallet.rs Outdated

daira reviewed Jul 19, 2023

View reviewed changes

Comment thread zcash_client_sqlite/src/lib.rs Outdated

str4d dismissed daira’s stale review via f679e56 July 19, 2023 16:02

Rename VALIDATION_DEPTH constant to VERIFY_LOOKAHEAD

c7b308b

Co-authored-by: Daira Emma Hopwood <daira@jacaranda.org>

str4d force-pushed the feature/pre_dag_sync-suggest_scan_ranges branch from f679e56 to c7b308b Compare July 19, 2023 16:03

ebfull approved these changes Jul 19, 2023

View reviewed changes

daira approved these changes Jul 19, 2023

View reviewed changes

str4d merged commit f422188 into zcash:main Jul 19, 2023

This was referenced Jul 19, 2023

Fix spent nullifier detection for non-linear scanning #876

Closed

zcash_client_sqlite: Improve performance of shardtree database migration #877

Closed

nuttycom deleted the feature/pre_dag_sync-suggest_scan_ranges branch July 31, 2023 22:08

nuttycom mentioned this pull request Aug 16, 2023

Extract "scan arbitrary block range" function from scan_cached_blocks #728

Closed

Conversation

nuttycom commented Jul 7, 2023

Uh oh!

codecov Bot commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

str4d Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

str4d Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daira Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

daira Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ebfull left a comment

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jul 7, 2023 •

edited

Loading

daira Jul 18, 2023 •

edited

Loading

daira Jul 18, 2023 •

edited

Loading