Engine API subset for the Merge Interop

[mkalinin]

Introduces Engine API subset for the Merge Interop

[mcdee]

This isn't an obvious response. Could we add a new error code 4 (Unknown ID) to make this error explicit?

[mkalinin]

I was not sure what is the rule of thumb of adding new error codes and just picked the one that already exists with the following description:

Should be used when some action is not allowed, e.g. preventing an action, while another depending action is processing on, like sending again when a confirmation popup is shown to the user

Do you know if each custom error code supposed to be used only in one place or semantically similar places of the client software?

[mcdee]

It's not so much "not allowed" as "not possible" (due to the ID being unknown), so although the best of the current options it isn't a great answer.

I don't know what the process is to add to error codes, but I would certainly try to do so rather than overload an existing error code.

[mkalinin]

I've added 4: Missing resource and used it here and in a couple of other places.

[mcdee]

Could this link to the rules within the specification?

[mkalinin]

These rules are written throughout the specification section of the EIP. Did you mean to list sections of the EIP that are related to the payload execution?

[mcdee]

Either that or have a consolidated list. Given the validation is a "must", it should be clear what the validation rules are.

[mkalinin]

Updated to:

1. Client software MUST validate the payload according to the execution environment rule set with modifications to this rule set definted in the Block Validity section of EIP-3675 and respond with the validation result.

[mcdee]

I would assume that in this situation the payload is discarded by the execution engine, but this should be made clear.

[mkalinin]

I would defer the decision of whether to keep or discard the payload in this case to the execution client. As if the execution client is being synced then it rather store all the payloads and then prune the storage upon receiving finalized block as current sync proposal depends on finalized checkpoints and in advance it is unknown which one is already finalized or will be finalized soon. Also, execution clients might want to store blocks of the finalized chain and prune the others.

[mcdee]

I think my concern here is that the caller ends up in limbo at current regarding the state of the payload. It may be discarded, it may be kept until the node is synced and then either integrated in to the chain or dropped, it may be kept only if the node is nearly synced and then broadcast if integrated in to the chain, etc. Seems like the safest and clearest option would be for the node to discard the payload if it cannot process it immediately.

(Could storing the blocks be a potential DoS vector, where a malicious attacker would flood a syncing node with future payloads the node needs to hold around in case they are used?)

[mkalinin]

Consensus client should verify the beacon block and send the corresponding consensusValidated message disregarding the sync status of the execution client. If consensusValidated deems the payload as invalid then it must be discarded as it is stated in the specification to the method below. During the sync all non-canonical forks may still be pruned upon receiving forkchoiceUpdated. I think execution client should be free to decide how to handle these calls while in a sync mode and rely on consensus client in sorting out invalid blocks.

[djrtwo]

Right, it is valuable to continue to pass these blocks into the execution-engine even during SYNCING because the execution engine (depending on sync method) will often need more current blocks to be able to sufficiently finish sync.

Eventually, the beacon chain will pass in a payload that is VALID or INVALID instead of SYNCING which is a signal that sync is completed and payloads are being fully validated

[MariusVanDerWijden]

I've started updating our el implementation of it to this spec here: ethereum/go-ethereum#23607

[MariusVanDerWijden]

What if the el engine does not have the finalizedBlockHash in db?
Previously SetHead returned Success:False for that

[mkalinin]

Good question. I think then the sync process should be initiated (if it hasn't been previously, this could be the first message after a start either with a fresh state or after a long period of being out) and the corresponding SYNCING response should be sent back. WDYT?

[mkalinin]

cc @fjl

[mkalinin]

I was thinking about the design of the forkchoiceUpdated for a little bit. IMO, it should be designed as a notification hence no response is expected. The same should be applicable to the consensusValidated.

I also think that we better use executePayload as the only method related to sync process if we want sync to piggy back on the core methods and responses to them. A slight improvement here could be an introduction of another type of response to the executePayload which provides more detail on the sync process.

[djrtwo]

Right, it is valuable to continue to pass these blocks into the execution-engine even during SYNCING because the execution engine (depending on sync method) will often need more current blocks to be able to sufficiently finish sync.

Eventually, the beacon chain will pass in a payload that is VALID or INVALID instead of SYNCING which is a signal that sync is completed and payloads are being fully validated

[MariusVanDerWijden]

Should the engine respond to getPayload twice if called twice with the same payloadID?

[mkalinin]

It should respond with error if the process doesn't exist. So, if engine kills the process after serving the getPayload for the first time then it should respond with error, if engine relies on the expiration by timeout and doesn't kill the process on getPayload then it may respond as usual. One call for this method should be pretty much enough for the consensus client unless there is an edge case we're overlooking.

[mkalinin]

SHOULD verb should be used instead of a MUST for the same reason as described above

[garyschulte]

should the 8550/1 port only serve engine endpoints or are we thinking it will offer a dedicated channel for the consensus engine to fetch from the necessary eth_* endpoints as well?

[lightclient]

Is your main concern that a client serving normal traffic on 8545 might become overloaded with traffic potentially causing consensus-critical requests to be dropped/delayed?

[lightclient]

Unrelated to the api - is there a concern with the beacon node asking the execution engine to prepare a payload with a timestamp that would be considered invalid under old consensus rules?

[mkalinin]

The timestamp field of the payload is set to the slot.timestamp and this condition is enforced by validation conditions on the beacon chain, so the timestamp value is enforced to be aligned with the timestamp of a slot. Beacon chain has strict time restrictions and validations in the fork choice and gossip. So, I think that execution clients should blindly follow the consensus clients with respect to the timestamp validation, i.e. don't validate timestamps at all to avoid edge case bugs.

[mkalinin]

This is a good point, btw! Thanks for raising it 👍

[lightclient]

Suggested change

	- `coinbase`: `DATA`, 20 Bytes
	- `feeRecipient`: `DATA`, 20 Bytes

Thoughts?

[mkalinin]

I am not opposed to this renaming. If we accept this then the respective change must be done to the beacon chain spec. The feeRecipient name deviates from the geth codebase which names this field as Coinbase, it also deviates from JSON-RPC API which names the same field as miner (in the PoS the latter name doesn't make any sense and we might want to update the JSON-RPC API).

cc @MicahZoltu @djrtwo

[djrtwo]

I'm mixed. Seems easier to not go in and adjust it on all clients but... don't have a dog in this race

[djrtwo]

Actually, we state "The coinbase field value MAY deviate from what is specified by the feeRecipient parameter." in another place. So we would need to be clearer here maybe "payload.feeRecipient field value.... from what is specified by the feeRecipient call parameter"

[MicahZoltu]

I have always hated Coinbase as it is totally non-descriptive. The only thing we know for sure about this address is that it is the address that the consensus client is suggesting should receive fees. Because of this, I favor suggestedFeeRecipient to make it very clear to the reader that this is just a suggestion.

[MicahZoltu]

Sorry, I thought this was for the API. I agree with feeRecipient here, and I think the API method should have suggestedFeeRecipient. That will address @djrtwo's concern about name collisions, and also provide the most descriptive variable names possible (important for specifications IMO).

[mkalinin]

I'd suggest we continue this discussion in a separate issue/PR or discord

[djrtwo]

Yeah, let's take it there. I think the terms are fine for this initial interop release.

[djrtwo]

Looks good to me!

Still a few qustions to answer but this will serve us well for interop