Skip to content
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: add several clarifications to align spec with tests #8906

Closed
wants to merge 5 commits into from

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Sep 25, 2024

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Sep 25, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 25, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Sep 25, 2024
@gumb0 gumb0 force-pushed the eip7702-clarifications branch 3 times, most recently from cdb63b6 to f50bc21 Compare September 25, 2024 11:02
@gumb0 gumb0 marked this pull request as ready for review September 25, 2024 11:02
@gumb0 gumb0 requested a review from eth-bot as a code owner September 25, 2024 11:02

For example, `EXTCODESIZE` would return the size of the code pointed to by `address` instead of `23` which would represent the delegation designation. `CALL` would similarly load the code from `address` and execute it in the context of `authority`.

In case a delegation designator points to a precompile address, retrieved code is considered empty and `CALL`, `CALLCODE`, `STATICCALL`, `DELEGATECALL` instructions targeting this account will execute empty code, i.e. succeed with no execution given enough gas.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally would like to change this rule to the opposite, i.e. executing precompiles, but this is what tests currently expect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the argument for doing this? IMO that conflates the trie structure and it's values with the convenience feature of the EVM of having "precompiles" at specific addresses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument would be being consistent in how if execution is targeting certain address, it behaves the same regardless if it was 7702-delegated.

I.e. currently there is a special case "If execution targets a precompile, don't execute it if target was delegated to".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I just don't see 7702 operating at that level. To me it's more about reading the trie and setting certain values in the trie. It's then, separately, the job of the EVM to execute the code it's provided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on where the boundary between EVM and the rest of the client lies in the particular implementation. If EVM is in charge of resolving precompile addresses, it's natural to do the same resolution for 7702 case. If precompile resolution is outside of EVM and EVM gets only code, then it could be more natural to give it empty code in this case.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@eth-bot eth-bot enabled auto-merge (squash) October 8, 2024 17:43
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Reviewers Have Approved; Performing Automatic Merge...

@lightclient
Copy link
Member

shoot now this has merge conflicts

@lightclient
Copy link
Member

replaced with #8940 so i could resolve the merge conflicts. thanks for this

@lightclient lightclient closed this Oct 8, 2024
auto-merge was automatically disabled October 8, 2024 17:52

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants