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-3540: Move to Review #8152

Merged
merged 24 commits into from
Apr 16, 2024
Merged

Conversation

pdobacz
Copy link
Contributor

@pdobacz pdobacz commented Jan 31, 2024

First step to update to be in sync with Megaspec.

Opening as draft first to get a round of reviews on these changes, then update at least the EIP-7480 and add the CREATE3/CREATE4 EIP to keep references working. (EDIT: both done, CREATE3/4 EIP is EIP-7620)

Let's focus the first round of reviews on overall structure and scope of this doc, and also any potential issues with the spec itself, which may come to mind when looking through these changes -- if anything looks suspicious here please report.

Note that many of the removed portions concern contract creation which will be described from ground up in a new EIP (current specification is inside the Megaspec is EIP-7620).

@github-actions github-actions bot added c-update Modifies an existing proposal s-stagnant This EIP is Stagnant t-core labels Jan 31, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 31, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Jan 31, 2024
@eth-bot eth-bot changed the title Update EIP-3540 to current EOF Megaspec Update EIP-3540: Update EIP-3540 to current EOF Megaspec Jan 31, 2024
@pdobacz pdobacz changed the title Update EIP-3540: Update EIP-3540 to current EOF Megaspec Update EIP-3540: match current EOF Megaspec Jan 31, 2024
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 31, 2024
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
For an EOF contract:
- Execution starts at the first byte of code section 0, and `pc` is set to 0.
- `pc` is scoped to the executing code section
- `CODESIZE`, `CODECOPY`, `EXTCODESIZE`, `EXTCODECOPY`, `EXTCODEHASH`, `GAS` are rejected by validation in EOF contracts, with no replacements
Copy link
Member

Choose a reason for hiding this comment

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

I think we should either remove this, or have a full list of deprecated instructions. But it should be in EIP-3670, so maybe remove.

Copy link
Contributor Author

@pdobacz pdobacz Feb 5, 2024

Choose a reason for hiding this comment

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

After some deliberation, I decided to have the deprecations accompany the replacement definitions wherever possible, and here list all the deprecations without replacements. The EIPs also kinda followed this model so far, so retaining this should give a smaller diff. But mostly, I think it reads easier, if the legacy and EOF logical counterparts are nearby.

But that's not a strong opinion, I spent quite a lot of time juggling these alternatives, all have pros and cons

Co-authored-by: Andrei Maiboroda <[email protected]>
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 6, 2024
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Mar 7, 2024
EIPS/eip-3540.md Outdated Show resolved Hide resolved
@pdobacz pdobacz marked this pull request as draft March 13, 2024 16:30
@pdobacz pdobacz requested review from axic and gumb0 March 14, 2024 12:52
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
@pdobacz pdobacz requested a review from gumb0 April 15, 2024 10:00
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Show resolved Hide resolved
EIPS/eip-3540.md Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated Show resolved Hide resolved
EIPS/eip-3540.md Outdated

(*Remark:* Due to [EIP-4750](./eip-4750.md), `JUMP` and `JUMPI` are disabled and therefore are not discussed in relation to EOF.)
- Execution starts at the first byte of code section 0, and PC is set to 0.
- `PC` is scoped to the executing code section
Copy link
Member

Choose a reason for hiding this comment

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

Probably this also can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, but isn't it a remark on how the context variable PC works, rather than opcode? I never was really sure. If you think opcode - let's remove. Actually, I think EIP-4750 elaborates enough on how PC works as a context variable, so I guess we can remove anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I thought so too at first, but it's next to the list of what happens to other opcodes, so very much looks like about opcode too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

088f5e2 a final check on this pls. I think this leaves PC out completely as irrelevant, making 3540 define how EOF would have worked without CALLF/RETF (only 0th code section executes), which I think is coherent.

@pdobacz pdobacz marked this pull request as ready for review April 16, 2024 12:40
@lightclient lightclient reopened this Apr 16, 2024
@lightclient
Copy link
Member

When you mark a draft ready, it often doesn't fully trigger the EIP bot.

@eth-bot eth-bot enabled auto-merge (squash) April 16, 2024 12:58
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...

@eth-bot eth-bot merged commit 5a043e4 into ethereum:master Apr 16, 2024
26 of 27 checks passed
@pdobacz pdobacz deleted the update-eip3540 branch April 16, 2024 13:24
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.

8 participants