-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat(pectra): Use EIP-7865 encoding for execution_requests #107
feat(pectra): Use EIP-7865 encoding for execution_requests #107
Conversation
} | ||
} | ||
}, | ||
"execution_requests": [ "0x00_TODO_DEPOSITS", "0x01_TODO_WITHDRAWLS", "0x02_TODO_CONSOLIDATIONS"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use the example value in ethereum/execution-apis#607 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, updated in 7955984 as well, thanks again!
types/electra/bid.yaml
Outdated
type: array | ||
items: | ||
$ref: '../../beacon-apis/types/primitive.yaml#/Bitvector' | ||
minItems: 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minItems
should be 0 in case all of the requests (deposit, withdrawal and consolidation) are empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right I was still thinking pre- ethereum/execution-apis#599, updated in 7955984 thanks!
description: "`ExecutionRequests` to include in block proposal." | ||
type: array | ||
items: | ||
$ref: '../../beacon-apis/types/primitive.yaml#/Bitvector' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if the Bitvector is a good semantic representation for the opaque field, although technically, it could work.
I would consider adding a Bytes
primitive type (similar to Bytes32) but unbounded in length.
Keen to have people weight in on this before committing to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny when writing this up I originally used Bytes
only to go look up the actual primitives and realize it didn't exist. :). So ya I'd be in favor of that change as well. I assume that'd be a separate PR on https://github.com/ethereum/beacon-apis/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so. It looks like builder-specs is importing beacon-api as a git submodule and referring to the types defined there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool I just sent ethereum/beacon-APIs#484 I guess if this is accepted I can update the submodule in this PR and point to that. Thanks for the suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding a Bytes primitive type (similar to Bytes32) but unbounded in length.
you can't have unbounded ssz types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we want to adopt this format on the CL side, this would mean we also have update the other apis on the beacon-api side to match the format.
SSZ is a native concept to the CL and embedding SSZ serialized data (as hex) inside JSON quite frankly is a horrible pattern.
Would be good if we can keep that hacky approach contained to the engine api
### `OpaqueExecutionRequests` | ||
|
||
```python | ||
class OpaqueExecutionRequests(List[Bitvector]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are not really opaque, just ssz encoded and 1-byte prefixed, anyone can decode the requests and read the data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're "semantically" opaque, EIP-7865 stipulates that each request type is allowed to decide its own encoding, it's just a coincidence that the current three are all SSZ in the same way that all EIP-2718 txs are currently RLP.
A requests object consists of a request_type prepended to an opaque byte array request_data. The request_data contains zero or more encoded request objects.
Each request type will defines its own requests object with its own request_data format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if the same wording is used on the engine-api side this is fine
When you say we "have to update the other apis" do you mean we'd be forced to because of some hard dependency, or just to maintain consistency? If there's a hard dependency then yes I agree we shouldn't do it, but since the builder spec sits in a grey zone between the EL and CL IMO it's ok for it to be less consistent with the rest of the CL APIs. |
There are apis which pass a Further more, I think we would also have to update BeaconBlockBody on the CL spec as otherwise the hash tree root of full block and blinded block would be different.
is it a grey zone? imo the builder spec is a pure CL spec extension |
Ah, thanks, I wasn't aware of these, and ya my reading is they would have to change as well. :( @lucassaldanha or @jtraglia can you confirm?
I guess what I meant by grey zone is that the block builders are usually custom ELs, for example |
Correct. These would need to change too. And I would advise against that. |
Ok, sounds like builders will have to go with the JSON encoding for their relay submissions afterall, thanks everyone for the feedback but I'll close this PR. |
You don't have to use JSON if we start adopting SSZ on the builder api, see #104. This would address any concerns related to performance / latency overhead. |
As discussed here on the Eth R&D Discord I think we should change to using the EIP-7865 encoding for
execution_requests
for the following reason:Since there didn't seem to be any strong objections to this I was encouraged to proceed with drafting a specs change, so here it is.
A couple more todos/questions:
0x_TODO
values for the encoded requests lists, ideally these should actually be decodable values (so someone can try converting them to the consensus-spec types as a code test) I just haven't had time to do that yet.get_execution_requests_list
function in the consensus-specs which properly explains the encoding process, I'll link to that as well.Bitvector
is the correct type to use in the spec for each entry, but I'm not terribly familiar with the CL specs so and happy to be corrected!