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

Add and verify weak-subjectivity-checkpoint flag #7256

Merged
merged 10 commits into from
Sep 17, 2020
Merged

Add and verify weak-subjectivity-checkpoint flag #7256

merged 10 commits into from
Sep 17, 2020

Conversation

terencechain
Copy link
Member

What type of PR is this?

Feature

What does this PR do? Why is it needed?
This PR implemented a CLI flag to take in weak subjectivity checkpoint. As discussed with teams and in the reference (below). The CLI input is in block_root:epoch_number format, where block_root is the root of a block at epoch_number is the epoch of the block.

It also added a helper to validate and parse the input string + tests to go with it. The results are inserted to initial sync service struct but not used at the moment. It will be used in the subsequent PRs

Reference: https://github.com/ethereum/eth2.0-specs/blob/weak-subjectivity-guide/specs/phase0/weak-subjectivity.md#weak-subjectivity-sync-procedure

@terencechain terencechain requested a review from a team as a code owner September 16, 2020 23:36
@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #7256 into master will increase coverage by 1.54%.
The diff coverage is 69.71%.

@@            Coverage Diff             @@
##           master    #7256      +/-   ##
==========================================
+ Coverage   60.07%   61.62%   +1.54%     
==========================================
  Files         323      418      +95     
  Lines       27422    33111    +5689     
==========================================
+ Hits        16473    20403    +3930     
- Misses       8733     9777    +1044     
- Partials     2216     2931     +715     

}

// Get the epoch number from input string.
epoch, err := strconv.ParseUint(s[1], 10, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

what if len is < 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll add a check

},
{
name: "Correct input #1",
input: "0x010203:987",
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt the block root be 32 bytes. Lets add a check for it.

s := strings.Split(wsp, ":")
if len(s) != 2 {
return nil, 0, errors.New("bad format string")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe output why it is not well formatted, showing them the expected format. Perhaps that should also be part of the flag usage if not specified already

Copy link
Member Author

Choose a reason for hiding this comment

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

it's part of the flag usage. I'll reiterate in the error again

@rauljordan rauljordan merged commit dcdf5d0 into master Sep 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the wsp-input branch September 17, 2020 19:32
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