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-2935: update mechanism via system call #8816

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Aug 20, 2024

Based on the discussion here: ethereum/go-ethereum#29465 (comment). I took the liberty to expand on block processing and the set/get functions.

@s1na s1na requested a review from eth-bot as a code owner August 20, 2024 12:57
@s1na s1na changed the title Eip2935/update mechanism Update EIP-2935: update mechanism via system call Aug 20, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 20, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Aug 20, 2024
lightclient
lightclient previously approved these changes Aug 20, 2024
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Aug 20, 2024
lightclient
lightclient previously approved these changes Aug 20, 2024
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated
state.insert_slot(HISTORY_STORAGE_ADDRESS, (block.number-1) % HISTORY_SERVE_WINDOW , block.parent.hash)
```

The first option is recommended until the Verkle fork, to stay consistent with [EIP-4788](./eip-4788.md) and to issues for misconfigured networks where this EIP is activated but history contract hasn't been deployed. The recommendation may be reconsidered at the Verkle fork to avoid working around attaching the history contract to every block's Verkle witness.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I fully get this.
"issues for misconfigured networks where this EIP is activated but history contract hasn't been deployed."
Why do we care about supporting misconfigured networks?

"The recommendation may be reconsidered at the Verkle fork to avoid working around attaching the history contract to every block's Verkle witness."

Do you mean that at verkle, we may want to directly update the state rather than access the system contract in order to avoid having to include the system contract to every block's verkle witness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we care about supporting misconfigured networks?

It's not about supporting misconfigured networks rather reducing damage in case contract is not deployed timely on a network. Calling the contract vs setting the store will behave differently if the contract is not deployed causing a network split. And this and 4788 are the only EIPs that in addition to activating the EIP someone needs to do an extra action of sending the transaction. Easy to miss or get wrong. Not using this as an argument but as an anecdote: a bug in geth's dev mode for 4788 until recently.

Do you mean that at verkle, we may want to directly update the state rather than access the system contract in order to avoid having to include the system contract to every block's verkle witness

This suggestion existed in the EIP (I believe put there by @gballet), I moved it to the rationale section as I believe it doesn't exist in the specification.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense concerning network split, thanks for explaining.

Concerning the last sentence, can we clarify the phrasing? Maybe it's just me, but there's a double negative in there that confused me, what does "avoid working around attaching" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think now it is simplified.

@SamWilsn
Copy link
Contributor

This just needs an author approval to go through.

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>
EIPS/eip-2935.md Outdated Show resolved Hide resolved
Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
@eth-bot eth-bot enabled auto-merge (squash) August 21, 2024 15:53
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 117933b into ethereum:master Aug 21, 2024
11 checks passed
```
At the start of processing any block where `block.timestamp >= FORK_TIMESTAMP` (ie. before processing any transactions), call to `HISTORY_STORAGE_ADDRESS` as `SYSTEM_ADDRESS` with the 32-byte input of `block.parent.hash`, a gas limit of `30_000_000`, and `0` value. This will trigger the `set()` routine of the history contract. This is a system operation following the same convention as [EIP-4788](./eip-4788.md) and therefore:

* the call must execute to completion
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean? Why even specify it? Should evm-implementors use 30MGas or some infinite supply? IMO this sentence only brings confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The system call semantics are directly copied from EIP-4788 for consistency since they should behave in the same manner. It applies to your next comment as well. I personally think it doesn't harm as long as the limit is high enough. Also makes me feel better when there is a limit even tho the contract is very simple and has been fully vetted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't get me wrong: I think the limit is good. That's why I think the sentence "must execute to completion" is bad, because it goes against the limit. Because if it must execute to completion, that implies infinite gas. If someone places an infinite loop there, then, without this sentence here, all clients would agree to burn 30M gas and done. With the sentence "must execute to completion", it can arguably be correct to loop forever.

* the call must execute to completion
* the call does not count against the block's gas limit
* the call does not follow the [EIP-1559](./eip-1559.md) burn semantics - no value should be transferred as part of the call
* if no code exists at `HISTORY_STORAGE_ADDRESS`, the call must fail silently
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have preferred something which shows that this is not a "new thing", rather just a clarification of existing semantics. Something like

if no code exists at HISTORY_STORAGE_ADDRESS, the call is a no-op, like any non-value-transferring call.

I also think it is incorrect to say that the call must fail. It's a no-op, it doesn't put a failure-symbol on any stack anywhere.

Copy link

@Jcpayvoice23 Jcpayvoice23 left a comment

Choose a reason for hiding this comment

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

My secretly apologies go out to everyone who has been there

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