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.

Sign up for GitHub

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

Jump to bottom

Migrations & data storage for pre-DAG-sync #831

Merged
merged 27 commits into from
Jul 4, 2023
Merged

Migrations & data storage for pre-DAG-sync #831

merged 27 commits into from
Jul 4, 2023

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented May 2, 2023 •
edited
Loading

Based on #857

@nuttycom nuttycom force-pushed the feature/pre_dag_sync branch 5 times, most recently from b9c1782 to 58412a1 Compare May 9, 2023 02:03
@nuttycom nuttycom force-pushed the feature/pre_dag_sync branch 8 times, most recently from 59d712d to cea2917 Compare May 24, 2023 18:47
@nuttycom nuttycom force-pushed the feature/pre_dag_sync branch 2 times, most recently from e7bf6e1 to 785dd37 Compare May 25, 2023 23:45
daira
daira reviewed Jun 5, 2023
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Only cursory review so far.

@nuttycom nuttycom force-pushed the feature/pre_dag_sync branch 4 times, most recently from 0a75a07 to e5c1386 Compare June 13, 2023 17:20
@codecov
Copy link

codecov bot commented Jun 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 71.45% and project coverage change: +0.47 🎉

Comparison is base (fa67d9a) 69.75% compared to head (c13c8c6) 70.23%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
+ Coverage   69.75%   70.23%   +0.47%     
==========================================
  Files         124      127       +3     
  Lines       11618    11848     +230     
==========================================
+ Hits         8104     8321     +217     
- Misses       3514     3527      +13
Impacted Files Coverage Δ
zcash_client_backend/src/data_api/chain/error.rs 0.00% <0.00%> (-21.43%) ⬇️
zcash_client_sqlite/src/error.rs 0.00% <0.00%> (ø)
...rc/wallet/init/migrations/add_transaction_views.rs 61.40% <ø> (+2.08%) ⬆️
...llet/init/migrations/received_notes_nullable_nf.rs 71.42% <ø> (+4.76%) ⬆️
...e/src/wallet/init/migrations/v_transactions_net.rs 73.33% <ø> (+4.58%) ⬆️
zcash_client_backend/src/data_api/error.rs 8.77% <16.66%> (+0.92%) ⬆️
zcash_client_sqlite/src/wallet/init.rs 54.21% <25.00%> (-17.72%) ⬇️
zcash_client_backend/src/scanning.rs 42.23% <25.71%> (ø)
zcash_client_sqlite/src/chain.rs 52.52% <33.33%> (+1.04%) ⬆️
...te/src/wallet/init/migrations/shardtree_support.rs 53.22% <53.22%> (ø)
... and 14 more

... and 47 files with indirect coverage changes

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

@nuttycom
Use ruqlite::Rows::mapped to allow collect
95745dd
@nuttycom
Ensure that checkpoints are ordered by position when querying for pru…
cd939f9 
…ning.
@nuttycom
Only store z->t transaction data once, not once per Sapling output.
70497a2
@nuttycom
Handle parsing of the not-present CommitmentTree sentinel.
8625e9a
@nuttycom
Use NonZeroU32 for all min_confirmations values.
e225a54
@nuttycom
Remove zcash_client_backend::data_api::chain::validate_chain
77b6380 
Local chain validation will be performed internal to
`scan_cached_blocks`, and as handling of chain reorgs will need to
change to support out-of-order scanning, the `validate_chain` method
will be superfluous. It is removed in advance of other changes in order
to avoid updating it to reflect the forthcoming changes.
@nuttycom
Copy link
Contributor Author

nuttycom commented Jul 2, 2023 •
edited
Loading

force-pushed to address comments from code review.

@nuttycom nuttycom requested a review from str4d July 2, 2023 02:06
@str4d str4d mentioned this pull request Jul 3, 2023
Merged
2 tasks
@nuttycom
Move chain continuity checks into scan_block_with_runner
e3aafda 
In preparation for out-of-order range-based scanning, it is necessary
to ensure that the size of the Sapling note commitment tree is carried
along through the scan process and that stored blocks are always
persisted with the updated note commitment tree size.
@nuttycom
Use valid serialized CommitmentTree values for migration tests.
09a0096
@nuttycom
Rename zcash_client_backend::welding_rig to `zcash_client_backend::…
42ed6ba 
…scanning`
@nuttycom
Copy link
Contributor Author

nuttycom commented Jul 3, 2023

force-pushed to fix an off-by-one error and fix cargo fmt errors.

nuttycom
nuttycom commented Jul 3, 2023
View reviewed changes
zcash_client_backend/src/data_api/chain.rs
},
)?;
block_source.with_blocks::<_, DbT::Error>(scan_from, limit, |block: CompactBlock| {
add_block_to_runner(params, block, &mut batch_runner);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It occurs to me that we should actually do the continuity check here, because there's no reason to add a block to the runner or continue the scan if the continuity check fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change in the forthcoming range-suggestions PR.

daira
daira reviewed Jul 3, 2023
View reviewed changes
zcash_client_backend/src/data_api.rs
};

pub struct MockWalletDb {
pub network: Network,
pub sapling_tree: ShardTree<
MemoryShardStore<sapling::Node, BlockHeight>,
{ SAPLING_SHARD_HEIGHT * 2 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ SAPLING_SHARD_HEIGHT * 2 },
{ sapling::NOTE_COMMITMENT_TREE_DEPTH },

str4d
str4d requested changes Jul 3, 2023
View reviewed changes
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.

Reviewed as of c363e71.

Cargo.toml Show resolved Hide resolved
Cargo.toml
[patch.crates-io]
incrementalmerkletree = { git = "https://github.com/zcash/incrementalmerkletree.git", rev = "082109deacf8611ee7917732e19b56158bda96d5" }
shardtree = { git = "https://github.com/zcash/incrementalmerkletree.git", rev = "082109deacf8611ee7917732e19b56158bda96d5" }
orchard = { git = "https://github.com/zcash/orchard.git", rev = "5da41a6bbb44290e353ee4b38bcafe37ffe79ce8" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I verified that this is the latest commit in zcash/orchard#398.

zcash_client_backend/src/data_api.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/chain.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/chain.rs Show resolved Hide resolved
zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet.rs
Comment on lines +899 to +901
if expected_hash != block_hash {
return Err(SqliteClientError::BlockConflict(block_height));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminding @daira to write hir comment.

zcash_client_sqlite/src/wallet/init.rs Outdated Show resolved Hide resolved
zcash_client_backend/proto/compact_formats.proto Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs Outdated Show resolved Hide resolved
@daira
Copy link
Contributor

daira commented Jul 3, 2023 •
edited
Loading

I would like to finish my review before this merges (which I should be able to do tonight).

Edit: had a terrible migraine. It's fine that this merged, I'll finish my review post-hoc anyway though.

str4d
str4d approved these changes Jul 4, 2023
View reviewed changes
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 c13c8c6

@str4d
Copy link
Contributor

str4d commented Jul 4, 2023

I'm using c13c8c6 in zcash/lightwalletd#440; please do not rebase that commit.

@nuttycom nuttycom merged commit d8148f9 into zcash:main Jul 4, 2023
@nuttycom nuttycom deleted the feature/pre_dag_sync branch July 7, 2023 00:08
@str4d str4d mentioned this pull request Jul 12, 2023
Closed
@str4d str4d mentioned this pull request Aug 9, 2023
Closed
This was referenced Aug 16, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira left review comments

@str4d str4d str4d approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nuttycom @daira @str4d