Skip to content

Engine API subset for the Merge Interop #74

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.

Sign up for GitHub

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

Merged
lightclient merged 14 commits into from
Sep 24, 2021
Merged

Engine API subset for the Merge Interop #74

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bfe33a9
Add Engine API subset for the Merge interop
mkalinin Sep 20, 2021
e77a47a
Engine API: Add bit length to QUANTITY fields
mkalinin Sep 20, 2021
76b3eb3
Clarify which rules adhere when executing a payload
mkalinin Sep 22, 2021
60ab636
Note QUANTITY is big-endian
mkalinin Sep 22, 2021
0843b8c
Add better error handling
mkalinin Sep 22, 2021
146f632
Be more explicit about field encoding
mkalinin Sep 22, 2021
44bab2e
fix spelling errors
lightclient Sep 22, 2021
5fd60ac
Apply suggestions from code review
mkalinin Sep 22, 2021
86aed25
Remove linking to particular sync spec
mkalinin Sep 23, 2021
4d3807d
Polish encoding clarification
mkalinin Sep 23, 2021
fd116bf
Add verbosity to error handling
mkalinin Sep 23, 2021
4b36a27
Apply suggestions from code review
mkalinin Sep 24, 2021
9bca6ed
Apply more suggestion from the review
mkalinin Sep 24, 2021
ab4ec22
Relax the requirement to respond with SYNCING
mkalinin Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/engine/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,10 @@
# Engine JSON-RPC API

The Engine JSON-RPC API is a collection of methods that all execution clients implement.
This interface allows the communication between consensus and execution layers of the two-component post-Merge Ethereum Client.

## Merge Interop

Documentation of the subset of the Engine API methods for the Merge Interop:
- [Specification](./interop/specification.md)
- Schema *TBD*
130 changes: 130 additions & 0 deletions src/engine/interop/specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
# Engine API. Interop edition

This document specifies a subset of the Engine API methods that are required to be implemented for the Merge interop.

*Note*: Sync API design is yet a draft and considered optional for the interop.

## Underlying protocol

This specification is based on [Ethereum JSON-RPC API](https://eth.wiki/json-rpc/API) and inherits the following properties of this standard:
* Supported communication protocols (HTTP and WebSocket)
* Message format and encoding notation
* [Error codes improvement proposal](https://eth.wiki/json-rpc/json-rpc-error-codes-improvement-proposal)

Client software **MUST** expose Engine API at a port independent from JSON-RPC API. The default port for the Engine API is 8550 for HTTP and 8551 for WebSocket.

Comment on lines +14 to +15
Copy link

@garyschulte garyschulte Sep 23, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Member

@lightclient lightclient Sep 23, 2021

Choose a reason for hiding this comment

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

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?

## Structures

### ExecutionPayload

This structure maps on the [`ExecutionPayload`](https://github.com/ethereum/consensus-specs/blob/dev/specs/merge/beacon-chain.md#ExecutionPayload) structure of the beacon chain spec. The fields are encoded as follows:
- `parentHash`: `DATA`, 32 Bytes
- `coinbase`: `DATA`, 20 Bytes
Copy link
Member

@lightclient lightclient Sep 23, 2021

Choose a reason for hiding this comment

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

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

Thoughts?

Copy link
Contributor Author

@mkalinin mkalinin Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@djrtwo djrtwo Sep 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@djrtwo djrtwo Sep 24, 2021

Choose a reason for hiding this comment

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

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"

Copy link

@MicahZoltu MicahZoltu Sep 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link

@MicahZoltu MicahZoltu Sep 24, 2021

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@mkalinin mkalinin Sep 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@djrtwo djrtwo Sep 24, 2021

Choose a reason for hiding this comment

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

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

- `stateRoot`: `DATA`, 32 Bytes
- `receiptRoot`: `DATA`, 32 Bytes
- `logsBloom`: `DATA`, 256 Bytes
- `random`: `DATA`, 32 Bytes
- `blockNumber`: `QUANTITY`
- `gasLimit`: `QUANTITY`
- `gasUsed`: `QUANTITY`
- `timestamp`: `QUANTITY`
- `extraData`: `DATA`
- `baseFeePerGas`: `QUANTITY`
- `blockHash`: `DATA`, 32 Bytes
- `transactions`: `Array of DATA` - Array of transaction objects, each object is a byte list (`DATA`) representing `TransactionType || TransactionPayload` or `LegacyTransaction` as defined in [EIP-2718](https://eips.ethereum.org/EIPS/eip-2718)

## Core

### engine_preparePayload

#### Parameters
1. `Object` - The payload attributes:
- `parentHash`: `DATA`, 32 Bytes - hash of the parent block
- `timestamp`: `QUANTITY` - value for the `timestamp` field of the new payload
- `random`: `DATA`, 32 Bytes - value for the `random` field of the new payload
- `feeRecipient`: `DATA`, 20 Bytes - suggested value for the `coinbase` field of the new payload

#### Returns
1. `payloadId`: `QUANTITY` - identifier of the payload building process

#### Specification

1. Given provided field value set client software **SHOULD** build the initial version of the payload which has an empty transaction set.

2. Client software **SHOULD** start the process of updating the payload. The strategy of this process is implementation dependent. The default strategy would be to keep the transaction set up-to-date with the state of local mempool.

3. Client software **SHOULD** stop the updating process either by finishing to serve the [**`engine_getPayload`**](#engine_getPayload) call with the same `payloadId` value or when [`SECONDS_PER_SLOT`](https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#time-parameters-1) (currently set to 12 in the Mainnet configuration) seconds have passed since the point in time identified by the `timestamp` parameter.

4. Client software **MUST** set the payload field values according to the set of parameters passed in the call to this method with exception for the `feeRecipient`. The `coinbase` field value **MAY** stem from what is specified by the `feeRecipient` parameter.

### engine_getPayload

#### Parameters
1. `payloadId`: `QUANTITY` - Identifier of the payload building process

#### Returns
`Object|Error` - Either instance of [`ExecutionPayload`](#ExecutionPayload) or an error

#### Specification

1. Given the `payloadId` client software **MUST** respond with the most recent version of the payload that is available in the corresponding building process at the time of receiving the call.

2. The call **MUST** be responded with error code `2` (`Action not allowed`) if a building process identified by the `payloadId` doesn't exist.
Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mkalinin mkalinin Sep 20, 2021

Choose a reason for hiding this comment

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

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?

Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mkalinin mkalinin Sep 22, 2021

Choose a reason for hiding this comment

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

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


3. Client software **MAY** stop the corresponding building process after serving this call.
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Sep 23, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mkalinin mkalinin Sep 23, 2021

Choose a reason for hiding this comment

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

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.


### engine_executePayload

#### Parameters
1. `Object` - Instance of [`ExecutionPayload`](#ExecutionPayload)

#### Returns
`Object` - Response object:
1. `status`: `String` - the result of the payload execution:
- `VALID` - given payload is valid
- `INVALID` - given payload is invalid
- `SYNCING` - sync process is in progress

#### Specification

1. Client software **MUST** validate the payload according to the execution environment rule set defined in the [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) and respond with the validation result.
Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

Could this link to the rules within the specification?

Copy link
Contributor Author

@mkalinin mkalinin Sep 20, 2021

Choose a reason for hiding this comment

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

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?

Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mkalinin mkalinin Sep 22, 2021

Choose a reason for hiding this comment

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

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.


2. Client software **MUST** defer persisting a valid payload until the corresponding [**`engine_consensusValidated`**](#engine_consensusValidated) message deems the payload valid with respect to the proof-of-stake consensus rules.

3. Client software **MUST** discard the payload if it's deemed invalid.

4. The call **MUST** be responded with `SYNCING` status while the sync process is in progress and thus the execution cannot yet be validated.
Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mkalinin mkalinin Sep 20, 2021

Choose a reason for hiding this comment

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

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.

Copy link

@mcdee mcdee Sep 20, 2021

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

@mkalinin mkalinin Sep 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@djrtwo djrtwo Sep 22, 2021

Choose a reason for hiding this comment

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

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


5. In the case when the parent block is unknown, client sofware **MUST** pull the block from the network and take one of the following actions depending on the parent block properties:
- If the parent block is a PoW block as per [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) definition then all missing dependencies of the payload **MUST** be pulled from the network and validated accordingly. The call **MUST** be responded according to the validity of the payload and the chain of its ancestors.
- If the parent block is a PoS block as per [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) definition then the call **MUST** be responded with `SYNCING` status and sync process **SHOULD** be initiated as it is specified in the [merge-sync.md](https://github.com/fjl/p2p-drafts/blob/master/merge-sync/merge-sync.md) document.

### engine_consensusValidated

#### Parameters
1. `Object` - Payload validity status with respect to the consensus rules:
- `blockHash`: `DATA`, 32 Bytes - block hash value of the payload
- `status`: `String: VALID|INVALID` - result of the payload validation with respect to the proof-of-stake consensus rules

#### Returns
none

#### Specification

1. The call of this method with `VALID` status maps on the `POS_CONSENSUS_VALIDATED` event of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) and **MUST** be processed according to the specification defined in the EIP.

2. If the status in this call is `INVALID` the payload **MUST** be discarded disregarding its validity with respect to the execution environment rules.

### engine_forkchoiceUpdated

#### Parameters
1. `Object` - The state of the fork choice:
- `headBlockHash`: `DATA`, 32 Bytes - block hash of the head of the canonical chain
- `finalizedBlockHash`: `DATA`, 32 Bytes - block hash of the most recent finalized block

#### Returns
none
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Sep 20, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mkalinin mkalinin Sep 20, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mkalinin mkalinin Sep 20, 2021

Choose a reason for hiding this comment

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

cc @fjl

Copy link
Contributor Author

@mkalinin mkalinin Sep 23, 2021

Choose a reason for hiding this comment

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

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.


#### Specification

This method call maps on the `POS_FORKCHOICE_UPDATED` event of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) and **MUST** be processed according to the specification defined in the EIP.