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

Verify GenesisValidatorRoot Matches the One in DB on Slashing Protection Import #7864

Merged
merged 14 commits into from
Nov 20, 2020

Conversation

shayzluf
Copy link
Contributor

@shayzluf shayzluf commented Nov 19, 2020

Builds off #7855
Feature

What does this PR do? Why is it needed?
Genesis validator root is being used for signing attestation and block proposals
making sure beacon node use the same genesis validator root as the one that is stored in the slashing protection db makes sure we are on the right chain.

Which issues(s) does this PR fix?

Fixes #

Other notes for review

@shayzluf shayzluf requested a review from a team as a code owner November 19, 2020 22:53
@shayzluf shayzluf marked this pull request as draft November 19, 2020 22:55
@shayzluf shayzluf changed the base branch from master to val-genvalroot November 20, 2020 00:35
@rauljordan rauljordan changed the title Save genesis on wait chainstart Verify GenesisValidatorRoot Matches the One in DB on Slashing Protection Import Nov 20, 2020
if err != nil {
return fmt.Errorf("%#x is not a valid root: %v", interchangeJSON.Metadata.GenesisValidatorsRoot, err)
}
if err = validatorDB.SaveGenesisValidatorsRoot(ctx, gvr[:]); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect, we should instead fetch the genesis validators root and compare it. We should not save what we get from the JSON file like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rauljordan save detects difference and returns error. will change to fetch

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but the name is misleading though, we should just do db.GenesisValidatorsRoot and do a simple comparison

@shayzluf shayzluf changed the base branch from val-genvalroot to master November 20, 2020 20:06
@shayzluf shayzluf marked this pull request as ready for review November 20, 2020 20:06
@rauljordan rauljordan added OK to merge Ready For Review A pull request ready for code review labels Nov 20, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit 3fb78ff into master Nov 20, 2020
@delete-merged-branch delete-merged-branch bot deleted the save_genesis_on_wait_chainstart branch November 20, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants