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

Validate that the attestation bitlist size matches the committee size #1693

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Apr 29, 2020

PR Description

When creating an indexed attestation we need to ensure that the aggregation bits are the same size as the committee. There are four places this can occur:

  • When a BeaconState contains invalid PendingAttestation waiting processing at the end of the epoch. This will never occur "naturally" but could occur if an invalid start state is provided.
  • When an attestation is received via gossip. We need to create the indexed attestation as part of validating it.
  • As part of running fork choice rule on received attestations. Partial validation was in place here - if the bitlist was too short the attestation was rejected, but if it was too large it was processed.
  • When an attestation is received as part of a block. Validation is already in place here.

An explicit precondition check has been added in get_attesting_indices so that it's now impossible to create an indexed attestation if the bitlist size is incorrect. All of the above cases now will reject the attestation.

The gossip attestation validator also performs an explicit check so it can return a standard INVALID result instead of throwing an IllegalStateException. Fork choice now catches InvalidArgumentException when creating the indexed attestation instead of IndexOutOfBoundsException (which is what was thrown if the bitlist was too short). The other cases already expect and handle the precondition check exception.

Fixed Issue(s)

fixes #1685
covers at least part of #1686

Copy link
Contributor

@cemozerr cemozerr left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants