Engine API: Version -> Type Renaming for Static Data Structure Suffixes

[holgerd77]

Hi @mkalinin, here is the renaming PR for the static structures in the Engine API. Please wait with merging until tomorrow sometime during the morning, I would like to get/take in another round of feedback on this.

[rubo]

Could you please shed some light on why replacing V1 with Type1 is better? For me, it's more consistent having V1 everywhere but apparently, you have your reasons.

[holgerd77]

Could you please shed some light on why replacing V1 with Type1 is better? For me, it's more consistent having V1 everywhere but apparently, you have your reasons.

Reasoning is that we now with making the Engine API methods hardfork agnostic - see e.g. #355 - we have got two different semantics of what a "version" is in the Engine API. For methods it is an expansion of the functionality (the old functionality is still preserved), while for the static attribute types a now called "version" is just a new/different data type, not being compatible with the old one.

Adding these same "V1", "V2" suffixes on both methods binds them very strongly togehter, in a sense that one is inclined to think that a "WithdrawalV1" is likely associated with e.g. "ExecutionPayloadV1", which is not the case (used in "ExecutionPayloadV2").

So my assumption is that taking this apart on the naming side will help people on differentiation and better understand the API and not mix things up.

[holgerd77]

This is an alternative naming scheme which might be a still better option:

Copying over the argumentation from a chat message:

Another naming scheme which is somewhat also in the ring proposed by Marc from Lighthouse is to name the attributes "by hardfork".

I'll also give you a preview on the look and feel:

The more I have let this sink in the more I'm getting warm on this, I am also not seeing any drawbacks right now. This makes the API method versions instantly associable with its functionality scope just by looking at the signatures ("ah, V3, that's the one also taking in the Cancun payloads") and so this safes this mental mapping ("What was ExecutionPayloadV3 (or Type3) again?).

cc @ethDreamer

[siladu]

I like the clarification to help distinguish between method names and types as this has been a source of confusion in conversations: V1 method behaviour vs V1 data shape.

This PR still has the issue of numbers not aligning, e.g. V2 not aligning with Type1 despite Type1 being introduced in V2.

Yet another alternative (which I'm not completely sold on myself) could be to keep the types consistent with the versions, e.g. engine_newPayloadV2 has WithdrawalV2 type, so we're really saying "TypeAssociatedWithV2Method". There would be duplication for any types that don't change, but at least it's consistent.

I do really like ExecutionPayloadCancun, but as Mikhail said, the API may change separately to the fork boundary

[siladu]

typos? engine\_

[holgerd77]

Just to note: this is put on hold for now after taking some feedback from client developers not giving a clear picture yet.

So we can take this as PoC for one way of doing it (if we want to make changes).

Will keep this open for a couple of more days for awareness and eventual follow-up discussions or comments.

Otherwise I might close at some point.

[mkalinin]

I can see how the current way of distinction between different structures is confusing and personally not opposed to this change. I would give client developers more time to decide whether they do want it or not.

I think that type version should not strictly be incremental, allowing for ExecutionPayloadType6110 to exist and comply with the spec. This kind of type numbers are suitable for experimental features to discern them from a stream of hard fork versions.

[mkalinin]

Shouldn't we distinct method versions from structure types? I.e. shouldn't methods always be suffixed with VX?

[ethDreamer]

Another naming scheme which is somewhat also in the ring proposed by Marc from Lighthouse is to name the attributes "by hardfork".
I'll also give you a preview on the look and feel:
The more I have let this sink in the more I'm getting warm on this, I am also not seeing any drawbacks right now. This makes the API method versions instantly associable with its functionality scope just by looking at the signatures ("ah, V3, that's the one also taking in the Cancun payloads") and so this safes this mental mapping ("What was ExecutionPayloadV3 (or Type3) again?).

cc @ethDreamer

This was my initial thought yes. Still open to it although there are some drawbacks:

• Naming after hardforks works well for ExecutionPayload types, but may not necessarily work well for other types (PayloadAttributes for example)
• The engine_api is the point of contact between the execution & consensus layers, however the names of the hardforks are not the same on both layers. This forces us to either adopt one of them (e.g. Capella or Shanghai) or the hybrid version Shapella which seems worse :/

@siladu

Yet another alternative (which I'm not completely sold on myself) could be to keep the types consistent with the versions, e.g. engine_newPayloadV2 has WithdrawalV2 type, so we're really saying "TypeAssociatedWithV2Method". There would be duplication for any types that don't change, but at least it's consistent.

The only way to do this while maintaining the decoupling of methods from hard forks would be to design every new type to work with all hard forks. Thus instead of engine_newPayloadV3 taking either an ExecutionPayloadV1, ExecutionPayloadV2, or ExecutionPayloadV3 we would need to make ExecutionPayloadV3 flexible enough to encode a Merge, Shanghai, or Cancun block. This makes deserialization more difficult and destroys the nice 1:1 mapping between the hard forks and the ExecutionPayload object versions we obtained in #337 .

[siladu]

The only way to do this while maintaining the decoupling of methods from hard forks would be to design every new type to work with all hard forks. Thus instead of engine_newPayloadV3 taking either an ExecutionPayloadV1, ExecutionPayloadV2, or ExecutionPayloadV3 we would need to make ExecutionPayloadV3 flexible enough to encode a Merge, Shanghai, or Cancun block.

@ethDreamer

I think you could still achieve the same thing where newPayloadV3 takes ExecutionPayloadV1 | ExecutionPayloadV2 | ExecutionPayloadV3.
The only difference would be that the Vx number may sometimes be redundantly incremented. So say that there was no changes between ExecutionPayloadV3 and V4, then ExecutionPayloadV4 = ExecutionPayloadV3.

Let's say there's a change in behaviour in newPayloadV4, but not a change in the ExecutionPayload type, and also the change wasn't associated with a fork...

You'd have newPayloadV4 takes ExecutionPayloadV1 | ExecutionPayloadV2 | ExecutionPayloadV3 | ExecutionPayloadV4 where:

• ExecutionPayloadV1 MUST be used if the timestamp value is lower than the Shanghai timestamp,
• ExecutionPayloadV2 MUST be used if the timestamp value is greater or equal to the Shanghai and lower than the EIP-4844 activation timestamp,
• ExecutionPayloadV3 | ExecutionPayloadV4 MUST be used if the timestamp value is greater or equal to the EIP-4844 activation timestamp,

In the case of Withdrawals, you'd start at V2, so for newPayloadV2, ExecutionPayloadV2 is composed of WithdrawalsV2 instead of WithdrawalsV1.
Then for ExecutionPayloadV4, you'd have WithdrawalV4 = WithdrawalV3 = WithdrawalV2.

The advantage of this approach being that for the latest blocks, engine_methodVx uses TypeVx. This advantage is most clear I think when we introduce a new Type, e.g. for Withdrawal since using a V1 type within a V2 type is confusing IMO.

Main disadvantage is redundant type version increments :)

As I write this, the more typing of Types I have to do, the less convinced I am about this redundant version increment approach 😆

[holgerd77]

Stalled and no further follow-up, will close.