Update EIP-2935: bring up to date with sys contract impl #9144
Conversation
github-actions
bot
added
c-update
|
✅ All reviewers have approved. |
eth-bot
changed the title
| * If calldata is bigger than 2^64-1, revert. | ||
| * For any output outside the range of [block.number-`HISTORY_SERVE_WINDOW`, block.number-1] return 0. | ||
| * If calldata is not 32 bytes, revert. | ||
| * For any request outside the range of [block.number-`HISTORY_SERVE_WINDOW`, block.number-1], revert. |
There was a problem hiding this comment.
this is not the current BLOCKHASH (with window 256) behavior so we are actually diverging from the behavior ?
There was a problem hiding this comment.
mentioned this elsewhere, but I think this divergence is okay because i) it matches other system contracts and ii) it is pretty straightforward to create an adapter around this system contract to mimic the BLOCKHASH behavior when it is used to back the actual op.
There was a problem hiding this comment.
@lightclient could you please share a link to the audit? We're especially interested in figuring out the REVERT on out-of-bounds part.
The behavior of returning 0 was initially intended, as there was no ring buffer in initial versions of the spec, and any request for a block hash before the fork was to be 0. The size of the ring buffer might be extended in the future (especially if the block time changes). Now, this is not a big deal per se, as executing the system contract's bytecode becomes "legacy" with eip-7709. Still, it'd be nice to know why an out-of-bounds read returning 0 is a security problem in this particular case.
|
|
||
| // 0x57: set op - sstore the input to number-1 mod 8192 |
There was a problem hiding this comment.
any reason for removing the comments ?
There was a problem hiding this comment.
I removed all these comments because the assembled code comes from the command:
disease --code 0x$(geas src/consolidations/main.eas) | sed -E '/^[[:space:]]*$/d; s/^[[:space:]]*[0-9a-fA-F]+:[[:space:]]+//; s/[[:space:]]*# selector\(.*\)$//'
all the code info is in the sysasm: https://github.com/lightclient/sys-asm/blob/main/src/execution_hash/main.eas
I'd probably hold off on code comments in the eip until this is going to last call.
There was a problem hiding this comment.
I had a round of discussions and while we consider it less than ideal to pick a buffer size for no direct reason than matching a design choice in another contract, we won't block Prague for that, since it has limited impact provided the justification is written in the eip.
| // check if input > 8 byte value and revert if this isn't the case | ||
| // the check is performed by comparing the biggest 8 byte number with | ||
| // the call data, which is a right-padded 32 byte number. | ||
| push8 0xffffffffffffffff | ||
| push0 |
There was a problem hiding this comment.
| push0 | |
| // Check if input is requesting a block hash greater than current block number | |
| // minus 1. | |
| push0 |
| push2 0x1fff | ||
| dup2 |
There was a problem hiding this comment.
| push2 0x1fff | |
| dup2 | |
| // Check if the input is requesting a block hash before the earliest available | |
| // hash currently. Since we've verified that input <= number - 1, we know | |
| // there will be no overflow during the subtraction of number - input. | |
| push2 0x1fff | |
| dup2 |
| jumpi | ||
|
|
||
| // mod 8192 and sload | ||
| push2 0x1fff |
There was a problem hiding this comment.
| push2 0x1fff | |
| // Load the hash. | |
| push2 0x1fff |
|
|
||
| // 0x4b: return 0 | ||
| jumpdest | ||
| push0 | ||
| push0 |
There was a problem hiding this comment.
| push0 | |
| // Load into memory and return. | |
| push0 |
| push0 | ||
| mstore | ||
| push1 0x20 | ||
| push0 | ||
| return | ||
|
|
||
| // 0x53: revert | ||
| jumpdest | ||
| push0 |
There was a problem hiding this comment.
| push0 | |
| // Reverts current execution with no return data. | |
| push0 |
| jumpdest | ||
| push0 | ||
| push0 | ||
| revert | ||
|
|
||
| // 0x57: set op - sstore the input to number-1 mod 8192 | ||
| jumpdest |
There was a problem hiding this comment.
| jumpdest | |
| // sstore block hash | |
| jumpdest |
|
But @lightclient please address the change requests before merging |
The pectra-devnet-5 specs [1] includes an update to EIP-2935's system contract and its address [2]. [1]: https://notes.ethereum.org/@ethpandaops/pectra-devnet-5 [2]: ethereum/EIPs#9144
|
@lightclient there are some merge conflicts here i guess |
lightclient
force-pushed
the
2935-update
branch
from
41be7c3 to
7180020
Compare
The pectra-devnet-5 specs [1] includes an update to EIP-2935's system contract and its address [2]. [1]: https://notes.ethereum.org/@ethpandaops/pectra-devnet-5 [2]: ethereum/EIPs#9144
The pectra-devnet-5 specs [1] includes an update to EIP-2935's system contract and its address [2]. [1]: https://notes.ethereum.org/@ethpandaops/pectra-devnet-5 [2]: ethereum/EIPs#9144
The pectra-devnet-5 specs [1] includes an update to EIP-2935's system contract and its address [2]. [1]: https://notes.ethereum.org/@ethpandaops/pectra-devnet-5 [2]: ethereum/EIPs#9144
The [pectra-devnet-5 specs] includes an [update to EIP-2935]'s system contract and its address. To pass blockchain tests the #1094 is required, but we are going to merge this change first. [pectra-devnet-5 specs]: https://notes.ethereum.org/@ethpandaops/pectra-devnet-5 [update to EIP-2935]: ethereum/EIPs#9144
This makes a few updates based on audits of system contract code here: https://github.com/lightclient/sys-asm/blob/main/src/execution_hash/main.eas
getinput to be 32 bytes