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: Bound precompile storage #7178

Merged
merged 13 commits into from
Jul 5, 2023

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented Jun 13, 2023

No description provided.

@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Jun 13, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jun 13, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title Bound 4788 storage Update EIP-4788: Bound 4788 storage Jun 13, 2023
@ralexstokes ralexstokes changed the title Update EIP-4788: Bound 4788 storage Update EIP-4788: Bound precompile storage Jun 13, 2023
@ralexstokes ralexstokes force-pushed the bound-4788-storage branch 2 times, most recently from deb13c6 to 4c6e3cb Compare June 13, 2023 20:13
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice! a bit strange, but I like it

EIPS/eip-4788.md Show resolved Hide resolved
EIPS/eip-4788.md Outdated Show resolved Hide resolved
EIPS/eip-4788.md Outdated
```

If there is no timestamp stored at the given root, the opcode follows the existing EVM semantics of `sload` returning `0`.
Alongside the existing gas for calling the precompile, there is an additional gas cost of `G_beacon_root` to reflect the two (2) implicit `SLOAD`s from
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean "existing gas"? Do you mean CALL costs? I would be a bit more specific.

Also, is the cost of the SLOAD sufficient because it's dominant or should we estimate the other operations (adds, assignments, etc)

Also, this could be priced cheaper than SLOAD if we just assume these two buffers are held in memory. That said, diverging from treating it just like native code/storage is probably not great for stateless and other things

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean CALL costs? I would be a bit more specific.

yes, the idea is that as a user I need to CALL the precompile and I want to say on top of whatever existing costs to get to this point in the execution, let's add in some extra gas to reflect the SLOADs, ill make another pass on the language

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, is the cost of the SLOAD sufficient because it's dominant or should we estimate the other operations (adds, assignments, etc)

I have not done a rigorous analysis but I think we can safely say the state reads will be the dominant cost here; otherwise there are just a few native operations that should relatively be free

priced cheaper than SLOAD if we just assume these two buffers are held in memory

I wouldn't want to make this assumption unless we got consensus from all EL devs that this was to be expected

EIPS/eip-4788.md Outdated Show resolved Hide resolved
EIPS/eip-4788.md Outdated Show resolved Hide resolved
EIPS/eip-4788.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the t-erc label Jun 22, 2023
@eth-bot eth-bot changed the title Update EIP-4788: Bound precompile storage CI: Bound precompile storage Jun 22, 2023
@eth-bot eth-bot added a-review Waiting on author to review e-consensus Waiting on editor consensus labels Jun 22, 2023
@github-actions github-actions bot removed the t-erc label Jun 22, 2023
@ralexstokes ralexstokes changed the title CI: Bound precompile storage Update EIP-4788: Bound precompile storage Jun 22, 2023
@eth-bot eth-bot removed the a-review Waiting on author to review label Jun 22, 2023
EIPS/eip-4788.md Outdated Show resolved Hide resolved
EIPS/eip-4788.md Outdated Show resolved Hide resolved
@jochem-brouwer
Copy link
Member

Also I just realized this (regarding testnets):

There is a super ugly edge case here, in which you do not pre-fund the 0x0B address in genesis. In that case, the balance is 0, the nonce is zero, and the code is the empty code hash. The storage however, will be non-empty starting at the fork block. However, this still means that the account is empty as per https://eips.ethereum.org/EIPS/eip-158. This means that if someone calls into 0x0B on the testnet (but does not fund it), then this should cleanup the account after the tx, thus wiping the storage. Not sure if it should be mentioned anywhere in the EIP, since I think most (all?) testnets pre-fund the precompile addresses with 1 wei, but I could imagine this gets messed up in testing setups if they set it up privately 🤔 (It is super weird if you call with tx1 into the contract to get timestamp X, and then call in the same block again into the contract again with timestamp X, and now get a zero result while it was nonzero before 🤔 )

@ralexstokes
Copy link
Member Author

yeah it sounds like the working knowledge is to pre-fund precompiles to avoid this behavior (cc @MariusVanDerWijden )

I can add a note to the EIP

@ralexstokes ralexstokes marked this pull request as ready for review July 5, 2023 21:31
@ralexstokes ralexstokes requested a review from eth-bot as a code owner July 5, 2023 21:31
@lightclient lightclient closed this Jul 5, 2023
@lightclient lightclient reopened this Jul 5, 2023
@eth-bot eth-bot enabled auto-merge (squash) July 5, 2023 21:45
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 d05d4b3 into ethereum:master Jul 5, 2023
@ralexstokes ralexstokes deleted the bound-4788-storage branch July 5, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal e-consensus Waiting on editor consensus s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants