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

simulators/ethereum/engine: Add ForkchoiceUpdated with Invalid Payload Attributes Test #527

Merged
merged 2 commits into from
May 5, 2022

Conversation

marioevz
Copy link
Member

This PR adds test cases where a ForkchoiceUpdated directive is sent to the EL with invalid payload attributes, timestamp==0.

The expected behavior remains undecided with two options proposed, as per @mkalinin:

Option 1

  1. Check headBlock is known and there is no missing data, if not respond with SYNCING
  2. Check headBlock is VALID, if not respond with INVALID
  3. Apply forkchoiceState
  4. Check payloadAttributes, if invalid respond with error: code: Invalid payload attributes
  5. Start payload build process and respond with VALID

Option 2

  1. Check headBlock is known and there is no missing data, if not respond with SYNCING
  2. Check payloadAttributes (requires a lookup to database to get a timestamp of yet to be applied headBlock), if invalid respond with error: code: Invalid payload attributes
  3. Check headBlock is VALID, if not respond with INVALID
  4. Start payload build process and respond with VALID

Currently in this test case Option 2 check is implemented but can be changed as necessary.

Current outcome per client:

  • Geth: Error but forkchoice is applied
  • Nethermind: Error and forkchoice is rolled back
  • Erigon: No error on any timestamp value
  • EthJS: No error on any timestamp value

cc @MariusVanDerWijden @MarekM25

@mkalinin
Copy link

Thanks a lot for making this scenario into tests! The spec work is pending. I need to think more about this case in terms of the spec, which option to go with. It's on the list and probably will be done this week

@MariusVanDerWijden
Copy link
Member

I think Option 1 makes the most sense, we want to apply the fork choice even if we are unable to produce a payload (How that might happen is irrelevant imo).
And if we get payload params we should make sure that invalid timestamps produce an error imo

@mkalinin
Copy link

The corresponding PR to the spec ethereum/execution-apis#211

@marioevz
Copy link
Member Author

I have updated to the test case to match the option in the updated spec.
Let me know if this is ok.

@marioevz marioevz marked this pull request as ready for review May 3, 2022 17:48
@holiman holiman merged commit 98779cb into ethereum:master May 5, 2022
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.

4 participants