eip7685: Pass execution_requests_hash to is_valid_block_hash

[mkalinin]

Replaces execution_requests with the execution_requests_hash in the call to is_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

[ralexstokes]

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

[mkalinin]

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.

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 requestsHash. So, the CL needs to at least encode requests and then send them encoded to the EL, i.e. send [deposit_bytes, withdrawal_bytes, consolidation_bytes] — from that concatenating and computing the hash is an optimisation which is quite straightforward to do. Then EL would just need to incorporate the hash into the blockHash validation process.

[mkalinin]

Updated the PR to modify is_valid_block_hash and revert changes made to notify_new_payload

[ppopth]

I quite agree with @ralexstokes that the benefit is very minimal.

[ralexstokes]

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

[mkalinin]

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

[lucassaldanha]

LGTM

[g11tech]

lgtm

[potuz]

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)

[jtraglia]

@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]

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

[mkalinin]

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

[arnetheduck]

I thought we wanted to get rid of these custom one-off hashing schemes that are just a mess to maintain over time?

[jtraglia]

Closing. We have reached a consensus to send the full requests.