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: unified list of opaque requests #577

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

fjl
Copy link
Collaborator

@fjl fjl commented Sep 4, 2024

This is a simplification/alternative to #565.

The purpose of EIP-7685 is creating a generic mechanism for relaying requests from EL to CL.
However, as it is defined, the EL has to have knowledge of the internal encoding of request types.

The current spec also requires us to implement four different encoding formats for each request object
across the stack. Here is an illustration for EIP-7251 consolidation requests:

We parse the raw return value encoding from the contract into a list. In the EL block, we store in them in an intermediate RLP-based format that isn't used anywhere else. For the engine API, we return the requests as a JSON object containing type-specific fields. Within the CL, requests are encoded into SSZ as part of the beacon block.

An important observation is that the EIP-7002 and EIP-7251 contracts already return a valid SSZ representation of their respective request objects. If we could just treat these objects as an opaque bytes container in the EL, our lives would be much simplified. The deposit contract is an outlier here because it returns ABI encoded events, but we can deal with that transformation in the EL as a legacy one-off.

So my proposal is returning the requests as opaque hex bytes, similar to transactions. On the CL side, the request types can be distinguished by the first byte in each encoded object. The remaining bytes are a valid SSZ payload for the request as defined by the CL spec. Less type definitions and format conversions for everyone.

@ralexstokes
Copy link
Member

ideally the outputs of the "system" contracts would be little-endian so that the outputs are indeed SSZ (I would guess they are big-endian at the moment), but otherwise I think this is a nice simplification!

@mkalinin
Copy link
Collaborator

mkalinin commented Sep 5, 2024

The simplification makes sense to me! We can update 7002 contract with the logic converting the amount to little endian replicating the to_little_endian_64 function from the deposit contract.

It might also make sense to represent type in little endian as then it will be compatible with SSZ Union definition

src/engine/prague.md Outdated Show resolved Hide resolved
src/engine/prague.md Outdated Show resolved Hide resolved
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>
@fjl
Copy link
Collaborator Author

fjl commented Sep 5, 2024

It might also make sense to represent type in little endian as then it will be compatible with SSZ Union definition.

The type is just one byte, so there is no endianness to consider AFAIK.

@fjl
Copy link
Collaborator Author

fjl commented Sep 5, 2024

Contract update here: lightclient/sys-asm#20

fjl and others added 2 commits September 5, 2024 23:44
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>
@tersec
Copy link
Contributor

tersec commented Sep 10, 2024

Whatever encoding one uses, they still come out of three different system request contracts. It's unclear to me why they should end up in a single list at any point, aside from the encodings used (JSON, RLP, SSZ, or anything else). That is,

On the CL side, the request types can be distinguished by the first byte in each encoded object.

doesn't seem related to the stated goals of:

Less type definitions and format conversions for everyone.

For CLs, sure, of course they're not going to use these interchangeably, but that was already obvious/stipulated. The ELs don't really either, in any intrinsic way. They have to gather them from various logs. Why combine them just for the CLs to immediately to have to disentangle this and create separate lists again?

https://eips.ethereum.org/EIPS/eip-7685#ordering even already requires:

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.

So the EL already has to either get them in this more or less sorted order or sort them.

Meanwhile, this PR doesn't actually convey this requirement, so the CL has to sort them again, just in case the EL does something pathologically strange with the ordering it provides.

@fjl
Copy link
Collaborator Author

fjl commented Sep 10, 2024

The main reason for the ELs to group all requests into a single list is to avoid modifying the block body all the time. It is very annoying for us to add stuff in blocks, and I think it's not strictly necessary. From the perspective of the EL, all requests are similar because they are just 'stuff for the CL to process'. So we want them in a single list of opaque objects that we do not have to otherwise care about. It really does simplify it for the EL.

I understand you may feel different about it on the CL side, since the requests have a meaning there, and you want to put them into their own dedicated lists in the end.

@lucassaldanha
Copy link
Contributor

They have to gather them from various logs

@tersec imho the point this change makes is the fact that a "Request" is ultimately a domain entity that the CL cares about. And its existence in the execution block is a mere limitation of our existing design. If we had an easy way to send requests to CL w/o using the execution block as "transport", it would be even better (it was considered but ended up being harder than we were willing to deal with).

As mentioned before, deposits are a bit special, but for other requests like consolidations or withdrawals, the EL barely has to do anything to them. They are already pulled from the contracts in the right format and order. So just putting them into the block w/o worrying about their meaning makes sense to simplify the EL logic. Even more so as more and more requests are added in the future. It means less changes in the execution block and the engine API spec as well.

So the gist of this from my point of view is:

  • easier to add new requests in the future;
  • not too complicated for CL to parse the data anyway;

@lucassaldanha
Copy link
Contributor

CL has to sort them again, just in case the EL does something pathologically strange with the ordering it provides

I am not sure if I understand this point. Why would the CL need to reorder the list? The order is defined by the underlying request "origin", I.e. the contract where requests are being pulled from. In my understanding the CL has to do is to process the requests preserving the order within their type.

@tersec
Copy link
Contributor

tersec commented Sep 11, 2024

CL has to sort them again, just in case the EL does something pathologically strange with the ordering it provides

I am not sure if I understand this point. Why would the CL need to reorder the list? The order is defined by the underlying request "origin", I.e. the contract where requests are being pulled from. In my understanding the CL has to do is to process the requests preserving the order within their type.

Nothing in this PR, in engine API terms, enforced from https://eips.ethereum.org/EIPS/eip-7685#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.

It only requires that https://eips.ethereum.org/EIPS/eip-7685#request

request = request_type ++ request_data

It does reference EIP-7685 but specifically to pull in this, not in more general terms.

Therefore, as stated, it's unclear from the text of the proposed engine API changes that they can correctly assume that an EL has not randomly sorted or shuffled the requests in, yes, some pathological way.

@tersec
Copy link
Contributor

tersec commented Sep 11, 2024

They have to gather them from various logs

@tersec imho the point this change makes is the fact that a "Request" is ultimately a domain entity that the CL cares about. And its existence in the execution block is a mere limitation of our existing design. If we had an easy way to send requests to CL w/o using the execution block as "transport", it would be even better (it was considered but ended up being harder than we were willing to deal with).

As mentioned before, deposits are a bit special, but for other requests like consolidations or withdrawals, the EL barely has to do anything to them. They are already pulled from the contracts in the right format and order. So just putting them into the block w/o worrying about their meaning makes sense to simplify the EL logic. Even more so as more and more requests are added in the future. It means less changes in the execution block and the engine API spec as well.

So the gist of this from my point of view is:

* easier to add new requests in the future;

* not too complicated for CL to parse the data anyway;

Yes, "requests" are CL domain entities.

However, even if the format from each deposit contract is more or less the same (helped by aligning one of the remaining contracts with SSZ), and the EL is merely conveying the system contract logs with as little further processing as it can imagine, it starts with

  • a condition where there aren't any single combined lists of all requests (which aren't directly useful to any entity in the system, CL or EL), because they come from separate sources (even if massaged so that the EL doesn't have to care at all about the internals of the format, it does have to know what system contract address it's interacting with); to
  • this intermediate format which is a single flattened list with extra structure and flexibility which according to https://eips.ethereum.org/EIPS/eip-7685#ordering isn't actually valid; which
  • CLs have to then split again, despite the EL having gotten them from different system contracts to begin with.

One issue here transcends per se parsing or reconstruction issues, but rather having the list of

[0x00_request_0, 0x01_request_0, 0x01_request_1, 0x02_request_0, ...]

fed back to the CL via the engine API has false flexibility and opens various edge cases which nothing in the EIP or this PR defines. For example, what if:

  • they are pathologically interleaved (e.g., request type a, then request type b, then request type a again), is this something a CL should reject, and treat it as effectively an invalid block? If so, that needs to be spelled out explicitly, otherwise it's a consensus issue waiting to happen the details of the reconstruction, and there's some risk that the most liberal-to-reconstruct method will win by default;
  • there are invalid entries (e.g., invalid request types, something that the EL does have visibility into, even if it treats the SSZ as 100% a black box), is that skipped, or? Again, these would benefit from being engine API and consensus rules, e.g., maybe they mean the block is invalid.

A root cause of both of these is the proposed engine API representation is overly flexible. One alternative which retains the properties described as useful for the EL is

[[request_0 for type 0], [request_0 for type 1, request_1 for type 1], [request_0 for type 2]]

Where

  • the groupings are already in the list, and the contents can be still viewed as as much black box payloads for the CL as with this PR's approach; and
  • the arguably-invalid-but-never-stated-as-such-for-engine-API cases listed above are simply not possible to represent, so don't need to be specially detected.

@fjl
Copy link
Collaborator Author

fjl commented Sep 12, 2024

I have added some more text @tersec PTAL.

@tersec
Copy link
Contributor

tersec commented Sep 14, 2024

I have added some more text @tersec PTAL.

This resolves those particular, specific concerns, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants