-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
REVERT instruction #206
Conversation
EIPS/eip-draft_revert_opcode.md
Outdated
@@ -0,0 +1,47 @@ | |||
## Preamble | |||
|
|||
EIP: <to be assigned> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we did assign 140 for this, given issue 140 was accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I wonder if it would make sense adjusting Currently, Likely it would make sense to take this into consideration when designing the successor of |
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 |
Returning 0 from a |
I agree it is better to make this low impact by not touching |
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? |
I mean that |
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? |
@Arachnid if |
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. |
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). |
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). |
@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. |
@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'. |
@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 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. |
@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. |
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. |
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. |
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. |
Looks good to me. |
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. |
@frangio you are correct with how you interpret the section. I'll leave a comment there. |
EIPS/eip-140.md
Outdated
|
||
## Backwards Compatibility | ||
|
||
This change has no effect on contracts created in the past. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true if those contracts contain 0xfd
as an instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add unless they contain
0xfd as an instruction
after a week of silence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 week of silence. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
I think this EIP should say
I wait for another week and apply the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
If |
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). |
Replaces #140.