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

REVERT instruction #206

Merged
merged 17 commits into from Dec 8, 2017
Merged

REVERT instruction #206

merged 17 commits into from Dec 8, 2017

Conversation

@axic
Copy link
Member

@axic axic commented Feb 6, 2017

Replaces #140.

@@ -0,0 +1,47 @@
## Preamble

EIP: <to be assigned>

This comment has been minimized.

@axic

axic Feb 6, 2017
Author Member

I assume we did assign 140 for this, given issue 140 was accepted.

This comment has been minimized.

@Souptacular

Souptacular Feb 21, 2017
Member

You can either assign the issue or PR number. In this case it would be better to assign the issue # since that is what it has been referred to most widely.

@axic
Copy link
Member Author

@axic axic commented Feb 6, 2017

I wonder if it would make sense adjusting CALL (and CALLCODE, DELEGATECALL) to indicate a REVERT.

Currently, CALL (& co) will return 1 on success and 0 in in case it failed. Perhaps code 2 could be returned in case of REVERT. Downside is that Solidity contracts would not be backwards compatible.

Likely it would make sense to take this into consideration when designing the successor of CALL and not touching the current one.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 7, 2017

For the sake of backwards compatibility, anything that reverts state changes should signal a "failure" to the caller. Since we cannot distinguish the two cases where the callee did provide a return value and where it didn't, I would propose to postpone that feature until we have a means to signal this e.g. in a new call opcode: If the caller used the CALL opcode and the callee provided failure data, the data is discarded.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 7, 2017

Returning 0 from a CALL that ends in a REVERT seems reasonable, though it would be easier to check returned data if contracts had a way to detect the length of returned data; at present they could at least zero the return memory area before calling (or we could HF to specify CALL does that).

@axic
Copy link
Member Author

@axic axic commented Feb 7, 2017

I agree it is better to make this low impact by not touching CALL at the moment. External callers (i.e. users of the RPC) will still be able distinguish and retrieve and error message.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 7, 2017

If we add it without any return support initially, then it'll be even more complex to add it later. Shouldn't we try and solve this once-and-for-all?

@axic
Copy link
Member Author

@axic axic commented Feb 7, 2017

I mean that CALL wouldn't differentiate and would ignore the return value. However, the return value would still be available through external calls, such as a transaction or anything invoked via an RPC.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 7, 2017

Wouldn't it make more sense to have CALL copy the return value, though? Does Solidity currently make any assumptions about the return memory area in case of a failed call?

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 7, 2017

@Arachnid if revert writes to the same memory area as return then the caller has to be able to distinguish the two cases. Since the ABI does not have an unused bit, I don't see a good way to put the distinction into the data. The only other option (apart from a new call opcode) I see is returning 2 in case of reversion, but that would signal a success condition to older code, which is probably not what you would want.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 7, 2017

@Arachnid if revert writes to the same memory area as return then the caller has to be able to distinguish the two cases. Since the ABI does not have an unused bit, I don't see a good way to put the distinction into the data.

If CALL returns 0, the data area contains any return data from a REVERT. If it returns 1, the data area contains any return data from a normal return. Both have the issue that you can't directly determine the return data length, but that's orthogonal.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 7, 2017

That's not correct: If CALL returns 0, the data area either contains the data from the revert or whatever was there before (because it was an OOG).

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 7, 2017

Right; just like if call returns 1 - the data area either contains the return data, or whatever was there before (if the return length was 0).

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 8, 2017

@Arachnid the difference is that currently, with contracts adhering to the ABI, you always know the type of the contents from the type of the contract.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 8, 2017

@chriseth That could still be the case - just extend the ABI spec to specify an ABI definition for error returns, too. The default would be 'null'.

@VoR0220
Copy link

@VoR0220 VoR0220 commented Feb 8, 2017

@chriseth there is a good way around this by just creating a new type in the ABI, that type being the "error" type. I fully agree with @Arachnid here. An error type has been sorely needed and missed here and has created much confusion for newcomers as to why the only error they ever get with their solidity code is "invalid jump dest.".

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 9, 2017

@Arachnid @VoR0220 of course that can be done but we still need a flag to indicate whether the data is an error or regular return data.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 9, 2017

@chriseth But we already have that flag - the return code of CALL. If it's zero, the return data - if any - follows the error ABI. If it's nonzero, the return data - if any - follows the return ABI.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 9, 2017

@Arachnid sure, but we have three situations instead of two: OOG, revert, success. There is currently no way to distinguish between OOG and revert, so new code that wants to read the revert error code has to clear the output area beforehand. In any case, it is really messy and complicated and thus I would propose to move the feature of actually reading the error data (not supplying it) to a new call opcode.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 9, 2017

But what I'm saying is that the issue of "was any data returned" is orthogonal to returning the data; we already have this problem (and the need to zero out the data before calling) with regular returns, where the called contract could return 0 bytes of data and the caller wouldn't know.

It makes far more sense to me to just return error data using the existing mechanism, and then add a 'RETURNDATALEN' (or similar) opcode afterwards.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 9, 2017

Sure, but I would say that something like "returndatalen" should be a feature of the call opcode. I don't think it is a nice design if you can check the size of the returned data at any time after the call. This is another word of state the VM (and all related tools) have to keep.

@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 9, 2017

I agree - a better CALL opcode would be an improvement. But I don't see why that means that CALL shouldn't return error data with the introduction of REVERT. We shouldn't make it impossible just because the current design is not ideal.

@vbuterin
Copy link
Contributor

@vbuterin vbuterin commented Feb 9, 2017

Looks good to me.

@Souptacular Souptacular mentioned this pull request Feb 10, 2017
0 of 3 tasks complete
@pirapira pirapira mentioned this pull request Feb 10, 2017
12 of 12 tasks complete
@Arachnid
Copy link
Collaborator

@Arachnid Arachnid commented Feb 13, 2017

Update post all-core-devs meeting: The consensus was that REVERT should take the same arguments as RETURN, and CALL should return data provided to REVERT, with another EIP to be opened on improving handling of return data from CALL and other related opcodes.

@pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Oct 18, 2017

@Souptacular

Can this be merged now that Byzantium is live?

@pipermerriam pipermerriam mentioned this pull request Oct 20, 2017
9 of 9 tasks complete
@pirapira pirapira mentioned this pull request Oct 23, 2017
0 of 10 tasks complete

EIP: 140
Title: REVERT instruction in the Ethereum Virtual Machine
Author: Alex Beregszaszi, Nikolai Mushegian (nikolai@nexusdev.us)

This comment has been minimized.

@pirapira

pirapira Nov 17, 2017
Member

@chriseth do you want to be listed as an author for your paragraphs added in efd2333 ?

Would that be ok @axic?


## Motivation

Currently this is not possible. There are two practical ways to revert a transaction from within a contract: running out of gas or executing an invalid instruction. Both of these options will consume all remaining gas. Additionally, reverting a transaction means that all changes, including LOGs, are lost and there is no way to convey a reason for aborting a transaction.

This comment has been minimized.

@pirapira

pirapira Nov 17, 2017
Member

reverting a transaction

aborting a transaction

We are here talking about something smaller than a transaction. Perhaps "reverting an EVM execution" and "aborting an execution"?


## Specification

The `REVERT` instruction is introduced at `0xfd`. It expects two stack items, the top item is the `memory_offset` followed by `memory_length`. It does not return anything because it stops execution.

This comment has been minimized.

@pirapira

pirapira Nov 17, 2017
Member

return anything

is ambiguous. It might be talking about return data to the caller, or returned word on top of the stack.

Perhaps It does not produce any stack elements because it stops execution?

should:
- return `726576657274206d657373616765` as `REVERT` data,
- the storage at key `0x0` should be left as unset and
- use 20024 gas in total.

This comment has been minimized.

@pirapira

pirapira Nov 17, 2017
Member

20024 here should be interpreted as decimal while 726576657274206d657373616765 might be hex.

Would it hurt to add 0x in front of the hex?

Copy link
Member

@pirapira pirapira left a comment

I left some small comments. After a week of silence, I'll apply those cosmetic changes myself.

@frangio
Copy link
Contributor

@frangio frangio commented Nov 17, 2017

The "Backwards Compatibility" section seems to imply that REVERT is treated as an invalid opcode for a contract created before the Byzantium hard fork.

Is this interpretation correct?

@pirapira
Copy link
Member

@pirapira pirapira commented Nov 20, 2017

@frangio you are correct with how you interpret the section. I'll leave a comment there.


## Backwards Compatibility

This change has no effect on contracts created in the past.

This comment has been minimized.

@pirapira

pirapira Nov 20, 2017
Member

This is not true if those contracts contain 0xfd as an instruction.

This comment has been minimized.

@pirapira

pirapira Nov 20, 2017
Member

I'll add unless they contain 0xfd as an instruction after a week of silence.

This comment has been minimized.

@frangio

frangio Nov 21, 2017
Contributor

Okay, that is a pretty important difference! IIRC Solidity had been emitting 0xfd opcodes several months before Byzantium, so there were many contracts out there whose behavior changed after being created.

This comment has been minimized.

@maurelian

maurelian Nov 30, 2017
Contributor

1 week of silence. :)

This comment has been minimized.

@pirapira

pirapira Nov 30, 2017
Member

Added.

pirapira added 3 commits Nov 30, 2017
@pirapira
Copy link
Member

@pirapira pirapira commented Nov 30, 2017

I think this EIP should say

If block.number >= BYZANTIUM_FORK_BLKNUM

I wait for another week and apply the change.

pirapira and others added 3 commits Dec 1, 2017
@axic axic changed the title Draft EIP for the REVERT opcode REVERT instruction Dec 5, 2017
Copy link
Member

@pirapira pirapira left a comment

Looks good to me.

@pirapira pirapira merged commit 75edcfd into ethereum:master Dec 8, 2017
@pirapira pirapira deleted the axic:revert branch Dec 8, 2017
@cleanunicorn
Copy link

@cleanunicorn cleanunicorn commented Mar 6, 2018

If REVERT is called with a message where is this returned?
Will this be returned in eth_getTransactionReceipt for that transaction?

@pirapira
Copy link
Member

@pirapira pirapira commented Mar 6, 2018

The message is returned to the caller if the caller is also a program in EVM.

Transaction receipts do not contain the REVERTed messages (that was once considered).

@holgerd77 holgerd77 mentioned this pull request Jun 25, 2020
15 of 15 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.