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

ZIP-221/244 auth data commitment validation in checkpoint verifier #2633

Merged
merged 22 commits into from
Aug 23, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Aug 13, 2021

Motivation

ZIP-221 specifies a history tree to which each block must commit to. ZIP-244 expands this commitment to also include the authentication data commitment.

This adds this check to the checkpoint verifier, as a follow up to #2609

Specifications

See #2609

Designs

N/A

Solution

Since it would be difficult to add this logic directly to the checkpoint verifier, this adds the check to the finalized state (since, at the end, the checkpoint verifier commits to the finalized state). This is because we need the history tree, and the finalized state already has one.

Review

I think @teor2345 will want to review this.

There are no tests currently. I'll discuss a possible strategy in the chat, then we can decide if we add the test here or create a ticket for it.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Follow up to #2609

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I like the way this PR re-uses the existing check code.

I have one blocking question about a potential denial of service/hang.
And an optional suggestion for some documentation.

zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-state/src/service/finalized_state.rs Show resolved Hide resolved
Base automatically changed from zip221-validation to main August 17, 2021 14:49
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This code looks pretty good, and you're right, it's surprisingly simple.

I think we'll need to reset the initial_tip_hash as well as verifier_progress.

zebra-consensus/src/checkpoint.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Outdated Show resolved Hide resolved
@teor2345

This comment has been minimized.

teor2345
teor2345 previously approved these changes Aug 19, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we've made some good progress here, and the remaining work can be done later in other tickets.

I have a bunch of optional suggestions and questions, but feel free to merge with all or none of them done.

zebra-consensus/src/checkpoint.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/prop.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-chain/src/block/arbitrary.rs Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Another round of changes!

Yes I agree we can leave this as is, I've created the other tickets as mentioned in the comment.

I also fixed a bug (in partial_chain_strategy()) that was causing CI to fail.

zebra-chain/src/block/arbitrary.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/prop.rs Outdated Show resolved Hide resolved
zebra-consensus/src/checkpoint.rs Show resolved Hide resolved
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm good with this, just waiting on whatever we decide to do with the metrics.

Feel free to merge after you've done whatever you want there.

@conradoplg conradoplg enabled auto-merge (squash) August 23, 2021 13:51
@conradoplg conradoplg merged commit bc4194f into main Aug 23, 2021
@conradoplg conradoplg deleted the zip221-validation-checkpoint branch August 23, 2021 14:17
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