Update EIP-7910: Add Fork Id to response, spec BPOs, add "last"#9989
Conversation
|
✅ All reviewers have approved. |
lightclient
left a comment
There was a problem hiding this comment.
Generally LGTM - but I think this should be in execution-apis not an EIP
c6dc822 to
1854d73
Compare
|
The commit 1854d73 (as a parent of 887dd78) contains errors. |
| "DEPOSIT_CONTRACT_ADDRESS": "0x00000000219ab540356cbb839cbe05303d7705fa", | ||
| "HISTORY_STORAGE_ADDRESS": "0x0000f90827f1c53a10cb7a02335b175320002935", | ||
| "WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS": "0x00000961ef480eb55e80d19ad83579a64c007002" | ||
| "0x00000000219ab540356cbb839cbe05303d7705fa": "DEPOSIT_CONTRACT_ADDRESS", |
There was a problem hiding this comment.
sort by name feels more natural for "random" addresses. Think we should leave this by name, and leave precompile by address.
|
|
||
| The "currentHash" and "nextHash" members contain the hash of the current configuration and the next configuration, respectively, or null if the next configuration is also null. | ||
|
|
||
| The "currentForkId" and "nextForkId" members contain the `FORK_HASH` value as specified in [EIP-6122](./eip-6122.md) of the current configuration and the next configuration, respectively, or null if the next configuration is also null. |
There was a problem hiding this comment.
This would cause a fork to have two IDs, one where there is no next fork, and one where there is. The whole response was intended to be hashed.
So I'm not in favor of this as it stands.
There was a problem hiding this comment.
Re-reading this... that is outside the hashed json, so this is perfectly fine.
| #### `systemContracts` | ||
|
|
||
| A JSON object representing system-level contracts relevant to the fork, as introduced in their defining EIPs. Keys are the contract names (e.g., `BEACON_ROOTS_ADDRESS`) from the first EIP where they appeared, sorted alphabetically. Values are 20-byte addresses in `0x`-prefixed hexadecimal form, with leading zeros preserved. Omitted for forks before Cancun. | ||
| A JSON object representing system-level contracts relevant to the fork, as introduced in their defining EIPs. Keys are 20-byte addresses in `0x`-prefixed hexadecimal form, with leading zeros preserved, sorted alphabetically. Omitted for forks before Cancun. Values are the contract names (e.g., `BEACON_ROOTS_ADDRESS`) from the first EIP where they appeared. |
There was a problem hiding this comment.
I'm also against this too. What is more important is what the contract is (the name), so that should be the key.
| "0x00000961ef480eb55e80d19ad83579a64c007002": "WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS", | ||
| "0x0000bbddc7ce488642fb579f8b00f3a590007251": "CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS", | ||
| "0x0000f90827f1c53a10cb7a02335b175320002935": "HISTORY_STORAGE_ADDRESS", | ||
| "0x000f3df6d732807ef1319fb7b8bb8522d0beac02": "BEACON_ROOTS_ADDRESS" |
There was a problem hiding this comment.
putting the name as the value and not the key means we have to read in random order. The contract address is the configuration and thus should be the value.
|
@lightclient it can also live in execution specs, but there are semantics (such as how we hash a fork_config hash) that there really isn't room to spec in API definitions. Such semantics are more than just APIs. |
eth-bot
left a comment
There was a problem hiding this comment.
All Reviewers Have Approved; Performing Automatic Merge...
Fork ID
Adds Fork Id as members
currentForkIdandnextForkIdto theeth_configresponse, as specified in EIP-6122.Note that EIP-6122 only mentions
FORK_HASHas the current configured fork's Id, and the block/timestamp number as the sole piece of information for the next fork, but, in this PR,FORK_HASHis also calculated for the next fork in order to catch fork ID issues early.Blob Parameter Only Forks
BPO forks are now specified in the EIP.
lastmemberAdds
last,lastHashandlastForkIdmembers to represent the last configured fork know by the client.