Implement EIP-7873 and EIP-5920 #1180
Conversation
gurukamath
force-pushed
the
eips/osaka/eip-7873
branch
from
4411f3c to
99e3a93
Compare
| ) | ||
| push(evm.stack, U256(1)) | ||
| except AssertionError: | ||
| # TODO: This behavior has not been |
There was a problem hiding this comment.
@SamWilsn had mentioned that we want to avoid TODOs directly in our code. I think the comment is good and explanitory, but should we spin the TODO bit off into an issue and track it that way?
| to: Union[Bytes0, Address] | ||
| value: U256 | ||
| data: Bytes | ||
| access_list: Tuple[Tuple[Address, Tuple[Bytes32, ...]], ...] |
There was a problem hiding this comment.
Just a heads up, this will end up needing to be refactored from Tuple[Address, Tuple[Bytes32, ...]] to Access after Osaka merges the upstream changes. Just noting this, since this branch is behind upstream changes to master/prague that include this.
There was a problem hiding this comment.
Thanks. Yeah. There will be a larger re-basing effort on all our EOF related branches soon
| raise InvalidTransaction( | ||
| "Type 5 tx with zero-length init code" | ||
| ) | ||
| if len(init_code) > 2 * MAX_CODE_SIZE: |
There was a problem hiding this comment.
I think there should either be a comment referencing the EIP definition for MAX_INITCODE_SIZE, or MAX_INITCODE_SIZE should just be defined in the same place as MAX_CODE_SIZE and referenced from there. IMO this would be more readable that way.
| """ | ||
| preimage = b"\xff" + address + salt | ||
| computed_address = keccak256(preimage) | ||
| canonical_address = computed_address[-20:] |
There was a problem hiding this comment.
The EIP specifies keccak256(0xff || sender32 || salt)[12:]. This form is equivalent, but I think it would be more readable to anyone who isn't a python native if it's [12:] ... though it dovetails well with the next line. More of a "to consider" than "defect".
| @@ -121,6 +122,8 @@ def encode_receipt(tx: Transaction, receipt: Receipt) -> Union[Bytes, Receipt]: | |||
| return b"\x03" + rlp.encode(receipt) | |||
| elif isinstance(tx, SetCodeTransaction): | |||
| return b"\x04" + rlp.encode(receipt) | |||
| elif isinstance(tx, EofInitCodeTransaction): | |||
| return b"\x05" + rlp.encode(receipt) | |||
There was a problem hiding this comment.
Hi @gurukamath, I want to SFI the PAY EIP for Fusaka, but by the process https://eips.ethereum.org/EIPS/eip-7723#scheduled-for-inclusion we need it both implemented here and we need tests in execution-spec-tests (done here: ethereum/execution-spec-tests#1509). Great to see you already started on this! I have only checked the PAY opcode (and would also suggest to split this in an individual PR for PAY, and keep the TXCREATE stuff also in an individual PR)
I also see you added stack items here, which are not specced yet, but I have them in this PR for the EIP spec: ethereum/EIPs#9590. Would also love if you could take a look there and leave some feedback 😄 👍
| try: | ||
| to = to_address_without_mask(pop(evm.stack)) | ||
| except ValueError as e: | ||
| raise ExceptionalHalt from e |
There was a problem hiding this comment.
This error cannot happen in EVM. The to_address_without_mask will only throw if the value is larger than an uint256 and pushing such value is not possible in EVM. I think what was intended to do here is to check for the bytes20/uint160 case, so it should be verified that the stack value fits in an uint160 or bytes20. If this is not the case then throw (EVM should internally run like the INVALID opcode)
There was a problem hiding this comment.
(The error cannot happen in EVM because in EVM it is impossible to push items larger than uint256 to stack)
There was a problem hiding this comment.
I think the name given to the variable is slightly misleading MAX_ADDRESS_U256 but it is in fact checking for address that have more than 20 bytes. b"\xff" * 20. I will change the variable name to something more appropriate. Thanks for pointing this out.
There was a problem hiding this comment.
Ah right, I did not check that definition 😄 Since ethereum addresses are currently all 160 bits / 20 bytes MAX_ADDRESS would currently be appropriate, but could of course also use MAX_ADDRESS_U160 or MAX_ADDRESS_B20 👍
| to, | ||
| value, | ||
| ) | ||
| push(evm.stack, U256(1)) |
There was a problem hiding this comment.
Items being pushed to stack is not part of current spec (April 15) https://github.com/ethereum/EIPs/blob/5fae72b226be42a043e75c682d13d3eb889da17b/EIPS/eip-5920.md
An item being pushed is part of this proposed PR for the EIP: ethereum/EIPs#9590
Note: it pushes 0 to stack if it is successful, and nonzero (1) in case it fails (so the other way around as of here). Motivation is in the EIP but this allows to add extra failure error codes in the future.
There was a problem hiding this comment.
The PR got changed and it now pushes 1 to stack on success and 0 on failure! So, we can say that EELS was ahead of the spec 😄 👍 The PR got merged.
| @@ -1271,3 +1379,58 @@ def return_contract(evm: Evm) -> None: | |||
|
|
|||
| # PROGRAM COUNTER | |||
| evm.running = False | |||
|
|
|||
|
|
|||
| def pay(evm: Evm) -> None: | |||
There was a problem hiding this comment.
This is not part of current spec (April 15) https://github.com/ethereum/EIPs/blob/5fae72b226be42a043e75c682d13d3eb889da17b/EIPS/eip-5920.md but this should be rejected in the STATIC frame (when previously STATICCALL has been used in the call stack). Part of ethereum/EIPs#9590
|
@jochem-brouwer Thanks for taking the time to look at this. We don't yet have an "official" Osaka branch since we are working on a couple of small re-factors before freezing the Pectra code. Once the osaka branch is up, I will rebase this commit on there and make the necessary adjustments as per the latest spec. Will keep an eye on ethereum/EIPs#9590 |
gurukamath
force-pushed
the
eips/osaka/eip-7873
branch
from
99e3a93 to
aa8f57c
Compare
|
Closing since EOF is not a part of Osaka. Raised a separate PR for EIP-5920 |
(closes #1173 )
(closes #1181 )
What was wrong?
The EOF implementation lacks the EIP-7673 (TX Create) and EIP-5920 (PAY Opcode)
Related to Issue #1173
Related to Issue #1181
How was it fixed?
Implement the relevant EIPs.