-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fetch the Correct Chain Head During AttestHead #1596
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1596 +/- ##
=======================================
Coverage 70.96% 70.96%
=======================================
Files 94 94
Lines 6695 6695
=======================================
Hits 4751 4751
Misses 1527 1527
Partials 417 417 |
validator/client/validator.go
Outdated
if v.assignment.AttesterSlot == slot { | ||
return pb.ValidatorRole_ATTESTER | ||
} else if v.assignment.ProposerSlot == slot { | ||
if v.assignment.ProposerSlot == slot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here since this is now an important distinction. Something like: A validator could be assigned to propose and attest in the same slot. Proposing takes priority.
Also, if a validator is selected to do both, should they do both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is part of #1565
Description
Write why you are making the changes in this pull request
When fetching the information necessary to attest to a beacon block, we need to fetch the chain head from the beacon node. Instead, we were fetching a block at the assigned slot for the attestation, which could be nil and is different than what the spec mentions.
Another problem is that sometimes attesters are assigned to the same slot as proposers in rare instances, and in those instances we should be prioritizing proposer assignments.
Additionally, there was a discrepancy in fetching the epoch and justified epoch boundary block roots. The reference issue in the spec is here
Write a summary of the changes you are making
This PR uses the ChainHead() function from the db to fetch the canonical head when it is time to attest. It also changes the logic of the if condition in assignments to prioritize proposers over attesters.