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

Check attestation shuffling when producing blocks #900

Merged
merged 6 commits into from
Apr 20, 2020

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Mar 10, 2020

Issue Addressed

Closes #845

Proposed Changes

During block production, use the fork choice block DAG to check that the implied shuffling for an attestation is the same as the state on which the block is being produced. This ensures that the signature check that was done upon inserting into the op pool remains valid, and prevents attestations from forks with different shufflings from appearing on chain with seemingly invalid signatures.

Additional Info

To avoid having the op pool contain a reference to fork choice, the get_attestations function takes a filtering closure. I thought this was quite neat (and functional 😏)!

TODO

  • Tests for different fork lengths
  • Unit tests for attestation_shuffling_is_compatible
  • Consider combining the checks for attestations sharing a beacon block root. There could be up to 128 attestations, but likely only a handful of unique roots that need checking.

@michaelsproul michaelsproul added the work-in-progress PR is a work-in-progress label Mar 10, 2020
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 16, 2020
@michaelsproul michaelsproul marked this pull request as ready for review March 16, 2020 05:39
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

I didn't go through the tests in detail but the main logic looks good to me!

Squerge at will.

beacon_node/beacon_chain/src/beacon_chain.rs Show resolved Hide resolved
beacon_node/beacon_chain/src/beacon_chain.rs Show resolved Hide resolved
@paulhauner paulhauner added ready-to-squerge and removed ready-for-review The code is ready for review labels Apr 19, 2020
Relying on the block root alone could have caused us to wrongly include an attestation from the
current epoch if the root was initially checked against the previous epoch.
@michaelsproul michaelsproul merged commit 50ef0d7 into master Apr 20, 2020
@michaelsproul michaelsproul deleted the block-production-fix branch April 20, 2020 02:34
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.

Forks longer than two epochs cause invalid blocks to be produced
2 participants