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.

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

Update EIP-7685: Update based on latest discussions #8916

Closed
wants to merge 4 commits into from
Closed

Update EIP-7685: Update based on latest discussions #8916

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be6cf8e
Update based on latest changes
lucassaldanha Sep 30, 2024
959facf
Apply suggestions from code review
lucassaldanha Oct 1, 2024
008a695
Adding CL details
lucassaldanha Oct 1, 2024
565e681
Apply suggestions from code review
lucassaldanha Oct 1, 2024
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
66 changes: 31 additions & 35 deletions EIPS/eip-7685.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,64 +13,61 @@ created: 2024-04-14
## Abstract

This proposal defines a general purpose framework for storing contract-triggered
requests. It extends the execution header and body with a single field each to
store the request information. This inherently exposes the requests to the
consensus layer, which can then process each one.
requests. It extends the execution header with a single field as a commitment to
the request. Requests are later on exposed to the consensus layer via the
Engine API.

Requests are not stored on the execution block, only a commitment to the requests
is stored in the execution block.

## Motivation

The proliferation of smart contract controlled validators has caused there to be
a demand for additional EL triggered behaviors. By allowing these systems to
delegate administrative operations to their governing smart contracts, they can
avoid intermediaries needing to step in and ensure certain operations occur.
This creates a safer system for end users.
This creates a safer system for end users. By abstracting each individual request
details from the EL, adding new request types is simpler and does not require an
update on the execution block structure.

## Specification

### Execution Layer

#### Request
#### Requests

Let `request_data` be the output of a system call to a Request Smart Contract.

A `request` consists of a `request_type` prepended to an opaque byte array
`request_data`:
`request_data` consists of two parts: `request_type` (single byte) and
`request_list_ssz` (ssz list encoding of many requests) as following:

```
request = request_type ++ request_data
request_data = request_type ++ request_list_ssz
Copy link
Contributor

Choose a reason for hiding this comment

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

It might worth mentioning a special case for an empty list, where request_data = requests_type, to make the spec more explicit

```

Let `requests` be the list of all `request` objects in the block in ascending
order by type. For example:
Let `requests` be the concatenation of each `request_data` returned from each request
contract:

```
[0x00_request_0, 0x01_request_0, 0x01_request_1, 0x02_request_0, ...]
requests = request_data ++ request_data ++ ... ++ request_data
```

The ordering of requests within a type is to be defined by each request type.

#### Block structure
Note: each `request_data` represents a different list of requests from a particular
smart contract, prepended by their type. They must be ordered by their first byte
(the `request_type`).

The block body is appended with a list of requests. RLP encoding of the extended
block body structure is computed as follows:

```python
block_body_rlp = rlp([
field_0,
...,
# Latest block body field before `requests`
field_n,
[request_0, ..., request_k],
])
```
The ordering of requests within a type is to be defined by each request type and it
is the smart contract responsibility to choose.

#### Block Header

Extend the header with a new 32 byte value `requests_hash`:

```python
def compute_requests_hash(list):
return keccak256(rlp.encode([rlp.encode(req) for req in list]))
def compute_requests_hash(requests):
return sha256(requests_data)

block.header.requests_root = compute_requests_hash(block.body.requests)
block.header.requests_hash = compute_requests_hash(requests)
```

### Consensus Layer
Expand All @@ -80,11 +77,11 @@ EL request.

## Rationale

### Opaque byte array rather than an RLP array
### Ssz byte array rather than an RLP array

By having the bytes of `request_data` array from second byte on be opaque bytes, rather
than an RLP (or other encoding) list, we can support different encoding formats for the
request payload in the future such as SSZ, LEB128, or a fixed width format.
Because the EL does not need to handle `request_data` or decode it. We can make let
the smart contract decide what encoding to use. For existing requests they are going
to use SSZ as requests are consumed by the CL.
Comment on lines -85 to +121
Copy link
Contributor

@etan-status etan-status Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any benefit for using SSZ here. The CL consumes this data via JSON (engine API), and uses a different data structure internally (ExecutionRequests) with strong typing.

What I find useful is the grouping by type, as in, having separate lists per request type. However, they can be a regular RLP array.

0x00_requests = 0x00_request_0 + 0x00_request_1 + 0x00_request_2   # without request type prefixes
0x01_requests = 0x01_request_0 + 0x01_request_1 + 0x01_request_2   # without request type prefixes
0x02_requests = 0x02_request_0 + 0x02_request_1 + 0x02_request_2   # without request type prefixes
requests_hash = keccak256(rlp([0x00_requests, 0x01_requests, 0x02_requests]))

Engine would have a list of the concatenated requests:

{
    requests: [
        "0x...... 0x00_requests",
        "0x...... 0x01_requests",
        "0x...... 0x02_requests"
    ]
}

Each of the individual requests items coincides with the encoding for List[XyzRequest, AnyLimit] in SSZ, as long as the request data per item is constant sized. This is regardless of what the list limit actually is (SSZ doesn't encode the list length but computes it from the data length). Therefore, the CL can efficiently parse each of the request lists, and enables the EL to treat the data as opaque, while avoiding the introduction of random SSZ List types that are EL-only.

If the goal is a requests_root SSZ commitment in the EL, the root should match the hash_tree_root(beacon_block.body.execution_requests) to have any advantage. The advantage would be that in newPayload, instead of sending the full requests, the CL could just send the requests_root (self-computed), reducing the amount of JSON encoding that's necessary for block validation, and reducing the number of hashes that the EL has to do to validate newPayload. In such a scenario, the EL cannot treat the request content as opaque, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Encoding of a new request list is supposed to be implemented as a part of the corresponding smart contract, like it is done today already for withdrawals and consolidations. So, SSZ is the encoding that we assume can be implemented in a smart contract even if a request data will be of a dynamic size in the future. Given that request encoding becomes opaque for EL clients which is nice as any new request doesn’t require any work on EL side besides adding a new requests smart contract.

The benefit of requestsHash = sha256(0x00_requests || 0x01_requests || 0x02_requests) is that CL can already send requestsHash to EL in the Engine API instead of sending request data, and this approach is easy to implement without RLP.

Copy link
Contributor

Choose a reason for hiding this comment

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

sha256(0x00_requests || 0x01_requests || 0x02_requests) is not the same as hash_tree_root(beacon_block.body.execution_requests).

Copy link
Contributor Author

@lucassaldanha lucassaldanha Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sha256(0x00_requests || 0x01_requests || 0x02_requests) is not the same as hash_tree_root(beacon_block.body.execution_requests).

It is not the same. But it is trivial for CL to calculate it. And we avoid sending back the whole execution requests object that the EL does not care about. Not saying it can't be done though. I think there was a suggestion about it in the Discord thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any benefit for using SSZ here. The CL consumes this data via JSON (engine API), and uses a different data structure internally (ExecutionRequests) with strong typing.

The benefit is that each request list can be decoded to a list of corresponding request types. Each one of the lists of requests inExecutionRequest has its container type defined, so we can directly decode this list into a ssz list with container elements of the corresponding request type (DepositRequest, ConsolidationRequest and WithdrawalRequest).

Copy link
Contributor

@etan-status etan-status Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it may be trivial for the CL given that CL already uses SHA256 for SSZ hashes, but it is a second hash over the request data, so if the request data is large it can be expensive; but if the data is small, the optimization may be premature? Realistically, the extra requests data tops out around 4k when JSON encoded?

on ETH R&D Discord we have also identified a hash collision problem. imagine that all request types consist of just a single uint8 for these examples:

example 1:

  • type 0 -> 0x01, 0x02, 0x02, 0x02
  • type 1 -> none
  • type 2 -> none

example 2

  • type 0 -> none
  • type 1 -> 0x02
  • type 2 -> 0x02, 0x01, 0x02

both hash as sha256(0x00, 0x01, 0x02, 0x02, 0x02, 0x01, 0x02). So, if one were to swap out example 1 and example 2 in a beacon block, the EL would deem both as VALID.

I'm not sure how easy it is to construct a collision with the initial 3 types and their limits, but as I understand the idea of this request bus is to be generic and extensible, and at the very least the security section of the EIP should address the hashing method, and should prove that collisions are not possible at least for the proposed request types.

in my opinion, the bandwith savings on newPayload are not large enough to warrant introducing an experimental hashing method. it feels off to me that we are fine with exchanging the full request data via libp2p among beacon nodes, which is an inefficient link where latency matters, but then focus on optimizing a local connection where it is deemd fine to waste 50% of data on encoding binary data as hex strings. I understand that it is ugly to send unnecessary data to the EL, but we can also address the optimization in a future fork with alternatives such as SSZ block headers.

if we want to go with a special purpose hash just for requests, from a CL perspective I think it should be SHA256 based, and exclude MPT, RLP, and keccak (so the current proposal would satisfy that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PSA: more on this convo on https://discord.com/channels/595666850260713488/1288408367068610585


### Request source and validity

Expand All @@ -110,8 +107,7 @@ The authors' recommendations on source and validity of requests are:
### Ordering

The ordering across types is ascending by type. This is to simplify the process
of verifying that all requests which were committed to in `requests_root` were
found in the block.
of verifying that all requests which were committed to in `requests_hash` match.

An alternative could be to order by when the request was generated within the
block. Since it's expected that many requests will be accumulated at the end of
Expand Down
Loading