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

Improve spec of MLOAD, MSTORE, and MSIZE. #775

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

acoglio
Copy link
Member

@acoglio acoglio commented Jun 3, 2020

This is in reference to Issue #767, which has all the relevant discussion.

This changes
old-mload-mstore
and
old-msize
to
new-mload-mstore
and
new-msize

This is in reference to Issue ethereum#767, which has all the relevant discussion.

ethereum#767
Paper.tex Outdated Show resolved Hide resolved
Co-authored-by: Daniel Kirchner <[email protected]>
@acoglio
Copy link
Member Author

acoglio commented Jun 4, 2020

@ekpyron Yes, definitely, thanks for catching it.

@acoglio
Copy link
Member Author

acoglio commented Jun 12, 2020

@nicksavers Does this look good to you too? If you confirm, you or I can merge it. Thanks.

@nicksavers
Copy link
Contributor

This limit has been a topic of discussion before.

There is EIP-1985: Sane limits for certain EVM parameters and Geth (and some others) have already implemented a different value for MSIZE. You are free to take those values for your interpreter or add your input in the discussion thread.

I suggest not to merge this until the EIP is Final.

@acoglio
Copy link
Member Author

acoglio commented Jun 15, 2020

@nicksavers Waiting for the EIP to be final is fine with me.

(We could still fix the overflow problem found by @ekpyron for the time being, even though it's a theoretical case, and then further revise things when the EIP is final. But perhaps we want to avoid too many revisions, especially if the EIP may be finalized soon(?).)

@ekpyron
Copy link
Member

ekpyron commented Jun 16, 2020

IMHO we should merge and fix this independently of that EIP.

@axic
Copy link
Member

axic commented Jun 18, 2020

@nicksavers I think this is independent from EIP-1985. This PR fixes an ambiguity (stack items are 256-bit wide so the offset can be at most 2^256-1), while EIP-1985 introduces stricter limits (which I think should definitely be applied to the YP once the EIP is accepted).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants