-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EIP-7702: 7702 validity #8845
Conversation
✅ All reviewers have approved. |
My stance on this is:
Side question for this PR, why not use u8 for |
Agree sanity limits need to exist, I just think we should keep them minimal and inline with existing types.
All of these things should be checked, they just shouldn't affect the transactions validity.
We bound |
```python | ||
assert auth.chain_id < 2**256 | ||
assert auth.nonce < 2**64 | ||
assert len(auth.address) == 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be len(auth.address)<=20
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, address is a fixed length item.
Co-authored-by: Mario Vega <[email protected]>
Check (in the txpool and in the main code) that an EIP-7702 transaction is [valid](https://eips.ethereum.org/EIPS/eip-7702#set-code-transaction), namely that: - `to != nil` - `len(authorization_list) != 0` - `authorization.chain_id` is `uint256` - `authorization.nonce` is `uint64` - `authorization.address` is `bytes20` - `authorization.y_parity == 0 || authorization.y_parity == 1` - `authorization.r` is `uint256` - `authorization.s` is `uint256` and `authorization.s <= secp256k1n/2` This PR doesn't implement ethereum/EIPs#8865 or ethereum/EIPs#8845
Check (in the txpool and in the main code) that an EIP-7702 transaction is [valid](https://eips.ethereum.org/EIPS/eip-7702#set-code-transaction), namely that: - `to != nil` - `len(authorization_list) != 0` - `authorization.chain_id` is `uint256` - `authorization.nonce` is `uint64` - `authorization.address` is `bytes20` - `authorization.y_parity == 0 || authorization.y_parity == 1` - `authorization.r` is `uint256` - `authorization.s` is `uint256` and `authorization.s <= secp256k1n/2` This PR doesn't implement ethereum/EIPs#8865 or ethereum/EIPs#8845
Seems like there was good support for this in ACDE 196. Still need to look more into constraining the |
|
||
```python | ||
assert auth.chain_id < 2**256 | ||
assert auth.nonce < 2**64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a slight nit about this. (Sorry, just noticed after-merge). By https://eips.ethereum.org/EIPS/eip-2681 should this not be 2**64 - 1
? Or is this checkout outside tx validity, but instead inside EVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but specific to the tx sender. The idea here is to only give numeric boundaries here and leave interpretation to other subsystems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but specific to the tx sender.
The nonce of authority is also incremented during delegation setting, so it is important to check nonce < 2**64 - 1
here.
I've made this change in #8905
Implement ethereum/EIPs#8845, which is part of [pectra-devnet-4](https://notes.ethereum.org/@ethpandaops/pectra-devnet-4)
Repost from #8840 as it should not have automatically merged.