Migrations & data storage for pre-DAG-sync#831
Merged
nuttycom merged 27 commits intozcash:mainfrom Jul 4, 2023
Merged
Conversation
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
5 times, most recently
from
b9c1782 to
58412a1
Compare
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
8 times, most recently
from
59d712d to
cea2917
Compare
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
2 times, most recently
from
e7bf6e1 to
785dd37
Compare
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
from
785dd37 to
a62a2e0
Compare
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
daira
reviewed
Jun 5, 2023
Contributor
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
Only cursory review so far.
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
4 times, most recently
from
0a75a07 to
e5c1386
Compare
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
from
e5c1386 to
cfd11f5
Compare
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
from
cfd11f5 to
5124572
Compare
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
force-pushed
the
feature/pre_dag_sync
branch
from
71ac466 to
c4e11e6
Compare
Collaborator
Author
|
force-pushed to address comments from code review. |
2 tasks
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
force-pushed
the
feature/pre_dag_sync
branch
from
b2e742d to
42ed6ba
Compare
Collaborator
Author
|
force-pushed to fix an off-by-one error and fix |
nuttycom
commented
Jul 3, 2023
| }, | ||
| )?; | ||
| block_source.with_blocks::<_, DbT::Error>(scan_from, limit, |block: CompactBlock| { | ||
| add_block_to_runner(params, block, &mut batch_runner); |
Collaborator
Author
There was a problem hiding this comment.
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.
Collaborator
Author
There was a problem hiding this comment.
I've made this change in the forthcoming range-suggestions PR.
daira
reviewed
Jul 3, 2023
| pub network: Network, | ||
| pub sapling_tree: ShardTree< | ||
| MemoryShardStore<sapling::Node, BlockHeight>, | ||
| { SAPLING_SHARD_HEIGHT * 2 }, |
Contributor
There was a problem hiding this comment.
Suggested change
| { SAPLING_SHARD_HEIGHT * 2 }, | |
| { sapling::NOTE_COMMITMENT_TREE_DEPTH }, |
str4d
requested changes
Jul 3, 2023
| [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" } |
Contributor
There was a problem hiding this comment.
I verified that this is the latest commit in zcash/orchard#398.
Comment on lines
+899
to
+901
| if expected_hash != block_hash { | ||
| return Err(SqliteClientError::BlockConflict(block_height)); | ||
| } |
zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs
Outdated
Show resolved
Hide resolved
Contributor
|
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. |
nuttycom
force-pushed
the
feature/pre_dag_sync
branch
from
e6676c6 to
c13c8c6
Compare
str4d
approved these changes
Jul 4, 2023
Contributor
|
I'm using c13c8c6 in zcash/lightwalletd#440; please do not rebase that commit. |
This was referenced Aug 16, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #857