Skip to content

Check attestation shuffling when producing blocks#900

Merged
michaelsproul merged 6 commits intomasterfrom
block-production-fix
Apr 20, 2020
Merged

Check attestation shuffling when producing blocks#900
michaelsproul merged 6 commits intomasterfrom
block-production-fix

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.

@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