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-4788: set nonce of beacon root history address to nonzero #7431

Closed
wants to merge 3 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jul 31, 2023

This is a not-yet approved specification-change for EIP 4788. I put it up for discussion on next ACD.

@holiman holiman requested a review from eth-bot as a code owner July 31, 2023 17:46
@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Jul 31, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 31, 2023

File EIPS/eip-4788.md

Requires 1 more reviewers from @adietrichs, @djrtwo, @ralexstokes

@eth-bot eth-bot changed the title eips/eip-4788: set nonce of beacon root history address to nonzero Update EIP-4788: set nonce of beacon root history address to nonzero Jul 31, 2023
@eth-bot eth-bot added the a-review Waiting on author to review label Jul 31, 2023
EIPS/eip-4788.md Outdated Show resolved Hide resolved
EIPS/eip-4788.md Outdated Show resolved Hide resolved
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

this works and should alleviate the issues people have had testing this

one downside is that we will need to remember to do something like this for each stateful precompile we decide to deploy in the future (e.g. EIP-7002)

which suggests a more general solution (update EIP-161 to exclude precompiles?) would be less of a foot gun in the future

but I am fine w/ this change to move things along if everyone else is satisfied w/ it

@jochem-brouwer
Copy link
Member

this works and should alleviate the issues people have had testing this

one downside is that we will need to remember to do something like this for each stateful precompile we decide to deploy in the future (e.g. EIP-7002)

which suggests a more general solution (update EIP-161 to exclude precompiles?) would be less of a foot gun in the future

but I am fine w/ this change to move things along if everyone else is satisfied w/ it

@ralexstokes I do not think this is a good idea. This means that the weird pre-Spurious Dragon behavior prior to EIP 161 (state trie clearing) that it is possible to create empty accounts in the trie, now gets back. This now means that for a precompile, it is possible to create an empty account in the trie at that precompile address. This empty account is thus an account with no code, no balance and no nonce (per EIP 161 storage does not matter). This has the weird logic that if you request an account over RPC, it can return the empty account, but this empty account can or cannot exist in the trie. I do not think we should return this. (If you would request a proof of the existence of these empty-but-in-trie account or the existence of this empty-but-not-in-trie account, you would get different proofs).

I would more be in favor to do a one-time upgrade of all precompiles (as defined by a precompile range EIP, for instance https://eips.ethereum.org/EIPS/eip-1352) where we just set the nonce of these precompiles to 1. We now should never (i.e. in the reasonable future) have the problem that we "forget" to set the nonce to 1 for a precompile. We have had testnets in the past (I think Baikal?) where we forgot to set the nonce to nonzero in this genesis. If we would create this EIP, we can just make it mandatory that upon activation of this EIP, the nonce is set to nonzero and the account can thus never be empty/dead regarding the precompiles. This would also reduce the size of genesis files for testnets.

write the parent beacon root provided in the block header into the storage of the contract at `HISTORY_STORAGE_ADDRESS`.

- Write the parent beacon root provided in the block header into the storage of the contract at `HISTORY_STORAGE_ADDRESS`.
- If the `HISTORY_STORAGE_ADDRESS` has `nonce==0`, set `nonce` to `1`. This is to prevent touch-delete rules from deleting the account from the state.
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just set the account to some clear state without conditions, so we can say

  • Set the HISTORY_STORAGE_ADDRESS's nonce to 1 and balance to 0

@holiman
Copy link
Contributor Author

holiman commented Aug 3, 2023

Empty:

An account is considered empty when it has no code and zero nonce and zero balance.

If we make the account hold code, then the change in this PR becomes moot.

yperbasis added a commit to erigontech/erigon that referenced this pull request Aug 23, 2023
#8055)

[devnet-8](https://notes.ethereum.org/@ethpandaops/dencun-devnet-8)
doesn't include ethereum/EIPs#7431 since it's
rendered obsolete by upcoming ethereum/EIPs#7456
(to be implemented by PR #8038).

This reverts PR #7952.
AskAlexSharov pushed a commit to erigontech/erigon that referenced this pull request Sep 6, 2023
@holiman holiman closed this Oct 10, 2023
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-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants