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

shardtree: Significantly rework handling of checkpoint depths and truncation operations. #115

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

nuttycom
Copy link
Contributor

No description provided.

@nuttycom nuttycom force-pushed the shardtree_rewind_to branch 2 times, most recently from 5583ba8 to 8189418 Compare September 26, 2024 22:48
}

fn rewind(&mut self, depth: usize) -> bool {
if self.checkpoints.len() > depth {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be valid to rewind by exactly the number of checkpoints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depth is treated as a (reverse-ordered) 0-based index, so it must be less than the length.

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 0c01af9 (the first commit), flushing comments.

incrementalmerkletree-testing/src/lib.rs Outdated Show resolved Hide resolved
incrementalmerkletree-testing/CHANGELOG.md Outdated Show resolved Hide resolved
incrementalmerkletree-testing/src/lib.rs Show resolved Hide resolved
incrementalmerkletree-testing/src/lib.rs Outdated Show resolved Hide resolved
incrementalmerkletree-testing/src/lib.rs Outdated Show resolved Hide resolved
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 8189418.

shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Show resolved Hide resolved
shardtree/src/store/memory.rs Show resolved Hide resolved
shardtree/src/lib.rs Show resolved Hide resolved
shardtree/src/lib.rs Show resolved Hide resolved
The previous semantics of the `rewind` operation would remove the last
checkpoint, if any, but would not further modify the tree. However,
these semantics are error prone - if you rewind to a checkpoint, you are
not able to rewind to the same checkpoint again; also, in practice, it
doesn't make sense to shift the location of a checkpoint in the note
commitment tree. This change alters `rewind` to (a) take an explicit
checkpoint depth, with depth `0` meaning that any state added since the
last checkpoint should be discarded, and (b) only allow a rewind
operation to succeed if a checkpoint actually exists at the specified
depth.
@nuttycom
Copy link
Contributor Author

force-pushed to address comments from code review.

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 9a77e51 after doc fixes

incrementalmerkletree-testing/src/lib.rs Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
shardtree/CHANGELOG.md Outdated Show resolved Hide resolved
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.

Re-utACK 3f59900

@nuttycom nuttycom merged commit ffe4234 into zcash:main Sep 27, 2024
8 checks passed
@nuttycom nuttycom deleted the shardtree_rewind_to branch September 27, 2024 17:09
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.

3 participants