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

Propose restrictions for jump. #2663

Closed
wants to merge 1 commit into from
Closed

Propose restrictions for jump. #2663

wants to merge 1 commit into from

Conversation

chriseth
Copy link
Contributor

This is the specification for the restrictions on the jump and jumpi opcode outlined in https://ethereum-magicians.org/t/eip-2315-simple-subroutines-for-the-evm-analysis/4229 (this is not the EIP-2315 discussion itself, but a discussion of a restriction of it).

This specification was requested by @gcolvin as grounds for a discussion.

@chriseth chriseth force-pushed the restrictJumps branch 2 times, most recently from adfa979 to 354832f Compare May 20, 2020 09:02
@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

@gcolvin
Copy link
Contributor

gcolvin commented May 22, 2020

This proposal amounts to a syntactic restriction on subroutine layout which can only be enforced at runtime. Compilers must still generate code to allow for this possibility, so it's not clear that it gains much. Further, for optimizing compilers and compilers of unusual languages this behavior can be a feature, no a bug.

EIP-615 is the kind of path that results from consistently enforcing syntactic constraints at deploy time -- it would require a versioning scheme, which this proposal does not.

On the other hand, this might be the only such change that can be checked at runtime -- further analysis would help. That is a good reason to get this in.

Overall, I oppose this proposal.

I’d support a proposal that limited constraints to those jumps that are recognizably static— i.e. are preceded by the push of a constant. A compiler could detect that at deploy time and take advantage of it.

Inelectably polymorphic code would need to be handled with if-else chains.

@chriseth
Copy link
Contributor Author

@gcolvin can you explain why you are opposed to reverting on jumping across subroutines but not opposed to reverting on "running into a subroutine"?

@gcolvin gcolvin requested a review from holiman May 27, 2020 15:24
@gcolvin
Copy link
Contributor

gcolvin commented May 27, 2020

@chriseth

  1. Code that can execute BEGINSUB directly can always be checked for at deploy time.
  2. Code that can "jump into a subroutine" usually cannot be checked for at deploy time.
  3. Code that can "jump into a subroutine" can support code that violates typical calling conventions, such as multiple entry points and various optimizations.

Again, EIP-2315 is meant as minimal extension to allow for subroutine calls, not -- like EIP-615 -- an extension that enforces a system of syntactic constraints. It is intended as an efficient target for compilers to EVM bytecode, not an easy source for compiling EVM bytecode to machine code.

@sorpaas
I'm regretting introducing BEGINSUB, and wonder if it should just be removed as you suggested.

@holiman
Copy link
Contributor

holiman commented May 27, 2020

@gcolvin I see you tagged me for a review. So I'm on the other end -- I think this is a good change. I was opposed to 615 because my opinion was that it created more problems than it solved, whereas this adds a (hopefully) useful constraint which so far seems to be possible to get away with, without introducing an attack-vector.

The problem with "analysing static jumps" is that there are no static jumps, and naively saying PUSHX XX; JUMP is a static jump doesn't work, imo. An optimizer may well use e..g a DUP inside a tight loop to 'rewind' to the jump target.

It's a lot better to be able to say, without deep stack-state-machine-analysis: "This subroutine EITHER loops correctly OR exits with error". With unbounded jumps, you wind up with "This subroutine EITHER loops correctly, or does, well, something entirely different, who knows... ".

@holiman
Copy link
Contributor

holiman commented May 27, 2020

I'm unsure if I should "approve" it though, which would make automerger merge it. I don't want to approve it just based on my own opinion...

@gcolvin
Copy link
Contributor

gcolvin commented Jun 18, 2020

naively saying PUSHX XX; JUMP is a static jump doesn't work, imo

Good point, thanks.

I'm unsure if I should "approve" it though, which would make automerger merge it. I don't want to approve it just based on my own opinion...

No, please don't. I remain opposed, so I'd just have to merge another PR to take it back out ;)

@gcolvin
Copy link
Contributor

gcolvin commented Jul 1, 2020

@holiman

It's a lot better to be able to say, without deep stack-state-machine-analysis: "This subroutine EITHER loops correctly OR exits with error". With unbounded jumps, you wind up with "This subroutine EITHER loops correctly, or does, well, something entirely different, who knows... ".

Thinking of it this way is more convincing - it is a useful runtime property. It comes at a price -- those entirely different things include useful idioms and optimizations like multiple entry points, coroutines, alternative calling conventions, and tail call elimination.

And what concerns me more and more is complexity of specification. BEGINSUB adds enough complexity to the Yellow Paper that I almost want to remove it, and I'm not sure how to describe this restriction as an exceptional halting condition of the machine state.

@github-actions
Copy link

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Oct 27, 2020
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

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

Successfully merging this pull request may close these issues.

5 participants