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

Engine API: engine_forkchoiceUpdated refinement #109

Merged
merged 6 commits into from
Nov 4, 2021

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Nov 4, 2021

Refines engine_forkchoiceUpdated spec as per feedback by @MariusVanDerWijden:

  • Adds and utilizes ForkchoiceState structure to be able to envelop the fork choice information in one argument. Proposed change regroups the parameter set of this method in a way that it has two subsets, forkchoiceState and payloadAttributes. Maintaining the parameter set of this method within two subsets does make sense since the semantics of forkchoiceUpdated method is twofold, and there is a parameter subset per each part of the semantics that can be updated independently.
  • Adds payloadId to the response. It is natural if EL is responsible for the computation of build process identifiers as the process is initiated by EL. It also gets rid of the complexity related to maintaining the exact algorithm of the computation in the spec which involves versioning and collision resistance.

UPD re: payloadId type DATA vs QUANTITY. QUANTITY is semantically more strict than plain DATA hence this updated keeps the type of the payloadId field as DATA to avoid any additional restrictions caused by the semantics of the field type. One may argue that the length should rather be 32 Bytes then.

TODO

cc @lightclient @djrtwo @protolambda

Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Nice changes, the API will be easier to implement with grouping of params and the return of an 8 byte payload ID.

This also alleviates my concerns (even if far fetched) of previous 8 byte payload IDs, as it's the EL, not the CL, that determines what to use.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Returning payloadId will certainly make the interaction simpler.

src/engine/specification.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

excellent! a couple of minor nits

src/engine/specification.md Outdated Show resolved Hide resolved
src/engine/specification.md Outdated Show resolved Hide resolved
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
@lightclient lightclient merged commit 89070c4 into ethereum:main Nov 4, 2021
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