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 subset for the Merge Interop #74

Merged
merged 14 commits into from
Sep 24, 2021
Merged
3 changes: 3 additions & 0 deletions spellcheck.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ matrix:
lang: en
d: en_US
ignore-case: true
run-together: true
run-together-min: 2
run-together-limit: 256
dictionary:
wordlists:
- wordlist.txt
Expand Down
9 changes: 9 additions & 0 deletions src/engine/README.md
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 *in progress*
151 changes: 151 additions & 0 deletions src/engine/interop/specification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
# 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

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

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?

## Error codes

The list of error codes introduced by this specification can be found below.

| Code | Possible Return message | Description |
| - | - | - |
| 4 | Unknown header | Should be used when a call refers to the unknown header |
| 5 | Unknown payload | Should be used when the `payloadId` parameter of `engine_getPayload` call refers to a payload building process that is unavailable |

## Structures

Fields having `DATA` and `QUANTITY` types **MUST** be encoded according to the [HEX value encoding](https://eth.wiki/json-rpc/API#hex-value-encoding) section of Ethereum JSON-RPC API.

*Note:* Byte order of encoded value having `QUANTITY` type is big-endian.

### 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

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
Collaborator 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 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

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

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
Contributor

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
Contributor

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
Collaborator Author

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

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`, 64 Bits
- `gasLimit`: `QUANTITY`, 64 Bits
- `gasUsed`: `QUANTITY`, 64 Bits
- `timestamp`: `QUANTITY`, 64 Bits
- `extraData`: `DATA`, 0 to 32 Bytes
- `baseFeePerGas`: `QUANTITY`, 256 Bits
- `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)
lightclient marked this conversation as resolved.
Show resolved Hide resolved

## Core

### engine_preparePayload

#### Parameters
1. `Object` - The payload attributes:
- `parentHash`: `DATA`, 32 Bytes - hash of the parent block
- `timestamp`: `QUANTITY`, 64 Bits - value for the `timestamp` field of the new payload
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

- `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|Error`: `QUANTITY`, 64 Bits - 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** deviate from what is specified by the `feeRecipient` parameter.

5. Client software **SHOULD** respond with `2: Action not allowed` error if the sync process is in progress.
djrtwo marked this conversation as resolved.
Show resolved Hide resolved

6. Client software **SHOULD** respond with `4: Unknown block` error if the parent block is unknown.

### engine_getPayload

#### Parameters
1. `payloadId`: `QUANTITY`, 64 Bits - 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 `5: Unavailable payload` error if the building process identified by the `payloadId` doesn't exist.

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

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
Collaborator Author

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
lightclient marked this conversation as resolved.
Show resolved Hide resolved

#### Specification

1. Client software **MUST** validate the payload according to the execution environment rule set with modifications to this rule set defined in the [Block Validity](https://eips.ethereum.org/EIPS/eip-3675#block-validity) section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification) 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.
lightclient marked this conversation as resolved.
Show resolved Hide resolved

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

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
Collaborator Author

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

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
Collaborator Author

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

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 software **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 accordingly.

### 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 or an error

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

3. Client software **MUST** respond with `4: Unknown block` error if the payload identified by the `blockHash` is unknown.

### 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 or an error

#### Specification

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

2. Client software **MUST** respond with `4: Unknown block` error if the payload identified by either the `headBlockHash` or the `finalizedBlockHash` is unknown.
4 changes: 4 additions & 0 deletions wordlist.txt
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
apis
bytecode
eip
endian
enum
eth
ethereum
interop
json
mempool
npm
ommers
openrpc
params
pos
pre
pyspelling
rlp
Expand Down