eip7685: Pass execution_requests_hash to is_valid_block_hash#3950
eip7685: Pass execution_requests_hash to is_valid_block_hash#3950mkalinin wants to merge 10 commits into
execution_requests_hash to is_valid_block_hash#3950Conversation
ralexstokes
left a comment
There was a problem hiding this comment.
I'm wondering what value this brings the CL?
instead we could simply not apply this PR and leave the CL as-is
the CL then sends over the full ExecutionRequests over the Engine API and the EL can do whatever it wants from there, i.e. check conformance with the execution_requests_hash -- which it will have to do anyway AFAICT.
it seems like this PR is just an optimization to save some bytes across the Engine API and I'd rather prioritize keeping the spec code simple
Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>
According to the new design EL’s responsibility is get requests byte sequences from system smart contracts, order them by the first byte ascending and then concatenated in order to compute |
|
Updated the PR to modify |
|
I quite agree with @ralexstokes that the benefit is very minimal. |
| execution_payload, | ||
| parent_beacon_block_root, | ||
| execution_requests_hash): | ||
| execution_requests_list): |
There was a problem hiding this comment.
I would still prefer to pass execution_requests here, and if anything handle the serialization step in the engine API, rather than the consensus specs
There was a problem hiding this comment.
It’s not about serialization, the exact encoding is currently a part of the protocol and would have to be done even if there was other communication channel than engine API. Serialization of this array to JSON is happening in the engine API client
|
Please change the description of the PR as it's not passing the hash (which is what we should do given that the theoretical limit, disregarding block gas limit, is over 1.5 MB of data) |
This reverts commit 601631f.
|
@mkalinin I've reverted the commit which changed this to a list of bytes. During the testing call, we decided to pass the hash. This is originally how you had it. |
ralexstokes
left a comment
There was a problem hiding this comment.
I still stand by what I said above that I think this unnecessarily complicates the CL side of things for not much gain
That being said, assuming we fix the hash computation to reflect the latest EIP specification then I'm fine merging to unblock devnet-4
execution_requests_hash to is_valid_block_hash
| The computation algorithm is defined by [EIP-7685](https://eips.ethereum.org/EIPS/eip-7685). | ||
|
|
||
| ```python | ||
| def compute_execution_requests_hash(execution_requests: ExecutionRequests) -> Hash32: |
There was a problem hiding this comment.
Alternatively we can do:
def compute_execution_requests_hash(execution_requests: ExecutionRequests) -> Hash32:
request_list = [execution_requests.deposits, execution_requests.withdrawals, execution_requests.consolidations]
hash_list = []
for index, requests in enumerate(requests_list):
request_type = uint_to_bytes(uint8(index))
hash_list.append(hash(request_type + serialize(requests)))
return hash(b''.join(hash_list))This allows to get rid of explicit type bytes in the spec and easy to extend in the future by adding requests to the flat list used for commitment computation
There was a problem hiding this comment.
I thought we wanted to get rid of these custom one-off hashing schemes that are just a mess to maintain over time?
|
Closing. We have reached a consensus to send the full requests. |
Replaces
execution_requestswith theexecution_requests_hashin the call tois_valid_block_hash. The hash is computed as it is defined by EIP-7685.This changes is the part of EL triggered requests redesign, the corresponding Engine API change is in ethereum/execution-apis#591