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

[RFC] Engine API: do not bump method's version upon datatype change #376

Closed
mkalinin opened this issue Feb 8, 2023 · 6 comments
Closed

Comments

@mkalinin
Copy link
Collaborator

mkalinin commented Feb 8, 2023

Existing Engine API specification states that a method version must be updated whenever behaviour or parameter set of the method has changed, including the case when a datatype version is advanced, e.g. a switch from ExecutionPayloadV2 to ExecutionPayloadV3 causes the newPayload version bump. This approach makes sense as whatever change to a method specification is considered as a change to its semantics.

The problem of the above approach is that in some cases when datatype is updated method's behaviour remains unchanged, i.e. datatype change only affects the underlying block processing routines. For instance, newPayload itself does not use newly added withdrawals field and defer it processing to downstream handlers.

Thus, we may consider avoid bumping method's version if there is no change in its behaviour and instead just extend already existing method to work with newly added datatype. In other words, explicitly say that newPayloadV2 is modified in Cancun and the only modification is support of ExecutionPayloadV3 datatype in addition to already supported V1 and V2.

@g11tech
Copy link
Contributor

g11tech commented Feb 13, 2023

100% support this (lodestar/ethereumjs), will reduce unnecessary lines adding version methods which just pipe down the data to the underlying processing fns.

Currently our versions are backward fork compatible (lower version data types can be passed/fetched from higher versions), and this would make versions forward fork compatible as well like how all beacon apis work.

Also would be useful to cleanly determine api methods to select via getCapabilities.

@siladu
Copy link
Contributor

siladu commented Apr 12, 2023

Also would be useful to cleanly determine api methods to select via getCapabilities.

This made me wonder, in getCapabilities, how would the CL tell that newPayloadV2 supports ExecutionPayloadV3?

Isn't the point of getCapabilities to enable API changes without relying on a fork as a way to synchronize? So if we wanted an out-of-fork change to the data type rather than the behaviour, we'd need some other way of synchronizing thus making getCapabilities redundant I think

@rkapka
Copy link

rkapka commented Apr 12, 2023

In terms of code simplicity, it will make Prysm's code a little cleaner, but nothing significant.

@mkalinin
Copy link
Collaborator Author

The problem of the above approach is that in some cases when datatype is updated method's behaviour remains unchanged, i.e. datatype change only affects the underlying block processing routines

This statement misses one thing. With each newly added field an EL client gets a responsibility to at least validate the presence of this field, and maybe run some other related validations. The more forks with data structures change is supported the more validations and related complexity EL clients have to deal with in the logic of the same method handler.

After deeper thinking on that I would like to propose the change in the opposite direction to the original one and bring Engine API to 1-1 mapping between method version and a version of data type this method supports, i.e. make newPayloadV3 support only ExecutionPayloadV3 and so on. Originally, supporting several versions by a single method was proposed because of it being convenient to CL client but apparently not every CL client utilises this feature. 1-1 mapping would improve spec clarity as well.

cc @MariusVanDerWijden

@tbenr
Copy link

tbenr commented May 29, 2023

@mkalinin
from teku perspective, we always have called the latest version. Before exchange capability endpoint the selection was based on current CL highest supported milestone (ie we started using capella's versions as soon as we scheduled capella fork). With the support for exchange capability endpoint we introduced a runtime selection, which always select the latest compatible version.

The good news is that the runtime method selection is a nice separate module so we can implement a "param driven" version of it which selects method based on data type.

So if the change simplifies things on spec and\or EL side (and I personally see the point) we can do the switch.

@mkalinin
Copy link
Collaborator Author

Finally, the decision is to get back and stay with 1-1 relationships between structure and method versions since Cancun #420

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

No branches or pull requests

6 participants
@mkalinin @siladu @tbenr @rkapka @g11tech and others