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

Support reason string in assert #1686

Open
axic opened this issue Feb 13, 2017 · 44 comments

Comments

Projects
None yet
@axic
Copy link
Member

commented Feb 13, 2017

Depends on EIP 140 (REVERT) to be fully accepted.

assert(thing, "Thing is invalid");

would basically translate to

if (!thing) revert("Thing is invalid");

(Issue split off #1130.)

@axic axic added the enhancement label Feb 14, 2017

@maurelian

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

Should this issue be renamed to Support reason string in require()?

@axic

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

Yes, this refers to require and not assert. This issue was created before we settled on the require name, sorry for this confusion.

@maurelian

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

Thought that was why, I just wanted to clarify. Will there be any limitations on the data which can be given to revert(data) and require(bool, data)?

@axic

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

This is yet to be decided. I think revert might have a few overloaded versions, but the native one is revert(bytes) as that is how the opcode is implemented.

I hope there will be a way to either agree on an encoding scheme for error codes + messages or to include such a scheme in the ABI or Natspec. If this happens, it could have an error code and optional message.

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

Perhaps we should just do a quick and easy version now:

Error return data is either empty or is an ABI-encoding with the signature (uint,uint,string) where the first argument is always 1, the second argument can be used for an error code and the third argument for a reason string.

Future versions of this specification have to increment the version number.

This also forces the first 31 bytes to be zero which leaves ample room for further "selectors" (for example, the first four bytes could encode a (non-zero) function selector in the future).

@axic axic added feature and removed enhancement labels Oct 6, 2017

@axic axic changed the title Support reason string in assert() Support reason string in revert (and perhaps require / assert) Oct 11, 2017

@GriffGreen

This comment has been minimized.

Copy link

commented Oct 14, 2017

Very excited to follow this closely :-D, I am not sure how to contribute, but I'm going to try :-D

@axic

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2017

To support tracking of assert, we could consider pushing a "source location identifier" (to be defined) just before INVALID:

PUSH ....
INVALID

This way a debugger with having access to the sources could figure out which assertion terminated.

@awgneo

This comment has been minimized.

Copy link

commented Oct 31, 2017

@axic That is a genius idea and very forward thinking! +1000; Too often we are forced into printf-style debugging with solidity and while passing reason strings around is a good step, it still doesn't allow us to utilize modern debuggers with breakpoints, traces, watches, etc. Op-code additions are a serious thing, I imagine, and we should aim for the moon here to get it right.

@Quazia

This comment has been minimized.

Copy link

commented Nov 9, 2017

@axic +1 Yeah I think that would be just as if not more useful than the "message" or "reason" strings requested in various issues. How difficult would it be to implement "source location identifiers" and is there anything I can do towards making this a reality? This is the sort of thing that's going to save so much time the quicker it happens the better.

@axic

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2017

To support tracking of assert, we could consider pushing a "source location identifier" (to be defined) just before INVALID:

An idea regarding source location identifier is to reuse parts of the source mapping: <16bits source index><32bit offset><16bit length>, which should fit into 5 bytes including the PUSH opcode.

e.g. PUSH 0001000004200024 means source index 1, offset 0x42 and length 0x24.

The debugger then needs to look up the source mapping (emitted by the compiler) to find the actual code in source.

@veox

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@axic You mean offset 0x420? Or PUSH 00 01 00 00 00 42 00 24?..

EDIT: I'm not sure I understand how that fits into 5 bytes.

@axic

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Sorry, it was late. PUSH 0001 00000042 0024 is the correct one and it takes 9 bytes :)

@axic

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Perhaps a much better optimisations would be the following: insert a unique index for each assert and have a translation table (assertionMap) for source locations emitted by the compiler.

Then again, if debugging is needed, the sources could be recompiled with the same settings to figure out which assertion terminated.

This should only pose an overhead of 2 or 3 bytes per assertions assuming there are only <=2048 assertions in the input.

@Suhail

This comment has been minimized.

Copy link

commented Jan 2, 2018

+1

@fulldecent

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

-1 only on the implementation details.

My understanding of proposal is that ABI will be extended to include a mapping of <revertReturnValue => String>. And compiler will put a return value on the stack during crash. And the client can translate those strings if necessary to any end-user language.

I don't see why an additional value needs to be put on the stack before revert. This seems unnecessary.

Alternate proposal:

The client can already see where the program crashed by checking the program counter. Simply add a mapping of <program counter => String> for assertion operations having names. Or possibly <program counter => assert code> and <assert code => String> if you want a normal form database. This saves adding and additional code to programs. Also, you can add assertion language retroactively to existing contracts!

Of course the compiler will need to treat these special REVERT assembly instructions specially: you cannot optimize by merging code paths if it results in two distinct "colors" of REVERT instructions being merged.

@stevenroose

This comment has been minimized.

Copy link

commented Mar 12, 2018

@chriseth So what is the time schedule for this to be implemented?

I just refactored all our contracts from throw to require, revert and assert following this guide from ConsenSys and in there they seemed to imply that it would be supported once the syntax was rolled out. So I just assumed it was there, giving me a ton of Error: Wrong argument count for function call: 2 arguments given but expected 1. :)

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

@stevenroose it is under review and will hopefully go with the next release.

@stefek99

This comment has been minimized.

Copy link

commented Apr 10, 2018

Started writing Stack Overflow question.
Found the related: https://ethereum.stackexchange.com/a/30066/2524
Arrived here...

Totally...

Sometimes transaction execution fails.

Sometimes programmers put a lot of require, making sure everything works just fine.

If something fails, the error message is generic:

  • invalid opcode
  • revert

Is there a way to pass additional parameter to require with the message?

Docs: http://solidity.readthedocs.io/en/v0.4.21/control-structures.html?#error-handling-assert-require-revert-and-exceptions


Something like assert (now deprecated) or strictAssert accepting optional parameter message

https://nodejs.org/api/assert.html#assert_assert_strictequal_actual_expected_message
assert.strictEqual(actual, expected[, message])

@OTTTO

This comment has been minimized.

Copy link

commented Apr 10, 2018

@stefek99, it's being finalized RN
#3364 (comment)

@axic

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

Implemented support in Remix (but no ETA when that will be merged and released on the non-alpha version): ethereum/remix#760

@vsdigitall

This comment has been minimized.

Copy link

commented Apr 30, 2018

contract Example {
  function test (uint i) {
    require(i == 1, "ERROR_TEXT");
  }
}

How to read require/revert error string from failed transaction log? (ERROR_TEXT in example above)

@NachoPal

This comment has been minimized.

Copy link

commented May 2, 2018

+1 #1686 (comment)

@chiro-hiro

This comment has been minimized.

Copy link

commented Jun 7, 2018

It would be a great idea, I'm still stucking on debugging.

@chriseth chriseth changed the title Support reason string in revert (and perhaps require / assert) Support reason string in assert Jun 7, 2018

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

Changed the title again, I thought we had a different issue for require.

@fulldecent

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

Above, we were discussing if it returned strings should calculatable at runtime or if compiletime is good enough.

I still don't see why runtime is necessary. For-loops are an exceedingly rare commodity in this world.

Also, the ABI can't be spoofed, it is generatable by anybody that has access to the source code.

@chiro-hiro

This comment has been minimized.

Copy link

commented Jun 20, 2018

@fulldecent If require and assert would throw to us a reason, it could be helpful to develop and maintenance smart contracts. So, it might useful at runtime.

@roschler

This comment has been minimized.

Copy link

commented Jun 21, 2018

Is the revert() error message a Solidity thing or a Web3.js issue? The latter potentially being a problem with tool like the Web3.js library not being updated yet to retrieve the error code/msg return at the low-level Ethereum interface, and bubble it up back to the caller?

Also, can someone sort out the following rumors I've heard? Is an error code supported by revert() or an error message, or both? I read somewhere while scouring the Web that the error message idea was dropped because of concerns there could be DDOS-like attacks against the Ethereum network, from people spamming the network with method calls that would easily trigger a revert() and due to the much large byte handling issues involved with strings over a simple error byte return, could put a lot of stress on the network. Can someone shed some light on the actual facts here?

If strings error messages are still being considered, what effect does this have on the stipend as far as the error message possibly not being propagated because of a lack of gas?

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Incorrectly metered resource consumption ("DDOS") is not a concern for the language, but only for the VM, and I don't think it is an issue here. Currently, strings are implemented (note that this has been finalized for Solidity) as error messages with a possible extension to allow any ABI-encoded data.

Not sure what exactly you mean with "effect [...] on the stipend". This mechanism uses returndata which is mostly paid for by the caller.

@fulldecent

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@chiro-hiro Here is exactly how the alternate proposal works:

Contract code

pragma solidity ^0.4.21;
contract Math {
    function addPositiveNumbers(int addend, int otherAddend) external pure returns (int sum) {
        require(addend >= 0, "Parameter one must be positive");
        require(otherAddend >= 0, "Parameter two must be positive");
        sum = addend + otherAddend;
        assert(sum > addend, "I thought adding was monotonic!");
    }
}

Byte code

000000000000000011111111111111112222222222222222333333333333333344444444444444445555
123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234

somethingsomethingsomethingsomethingREVERTsomethingsomethingsomethingREVERTsomREVERT

ABI

[
  ...
  "revertStrings": {
    "0x25": "Parameter one must be positive",
    "0x46": "Parameter two must be positive",
    "0x4f": "I thought adding was monotonic!"
  },
  ...
]

Discussion

Yes, you do have access to the revert reasons at run time, not compile time. However at compile time there is no need to place english-language strings into the byte code.

Also my approach allows you to extend this concept by adding a file math-zh.po:

#: Math: addPositiveNumbers:4
msgid "Parameter one must be positive"
msgstr "参数一必须是正数"
msgid "Parameter two must be positive"
msgstr "参数二必须是正数"
msgid "I thought adding was monotonic!"
msgstr "我不知道怎么开发区块链软件!"
@chiro-hiro

This comment has been minimized.

Copy link

commented Jul 6, 2018

@fulldecent It's make scene, an offset of message could be better than put a string inside opcode. It's less annoying to deal with Unicode encoding.

@axic axic added language design :rage4: and removed in progress labels Jul 28, 2018

@devedse

This comment has been minimized.

Copy link

commented Jan 3, 2019

It seems this was added to the language but I can't find a way on obtaining the messages. Does anyone hasmore information about this?

E.g. see my test contract with source code on the Ropsten node:
https://ropsten.etherscan.io/address/0x8e663a720295c7518fba5ae3d4c4655dca4ddaa0

Last transaction failed but I can't find the message that should be written by it.
https://ropsten.etherscan.io/tx/0x8111735fcf2d2310e1a7a6d16419e5e35bfd410c43b456003f51ab88ee2328a8

@fulldecent @axic

@jamesmorgan

This comment has been minimized.

Copy link

commented Jan 5, 2019

@devedse with web3js or at least with the latest truffle (version 5) you can get the revert reason from the exception e.g.

try {
  await contract.myMethodCall();
} catch (error) {
  // error.reason now populated with an REVERT reason
  console.log("Failure reason", error.reason);
}
@xinbenlv

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

Oh so cool!

@devedse

This comment has been minimized.

Copy link

commented Jan 5, 2019

@jamesmorgan, I see, I assume however that you can't do this on transactions that have been executed on the blockchain though?

@simonDos

This comment has been minimized.

Copy link

commented Feb 27, 2019

@jamesmorgan I can see the reason field, but no matter what I do, it is always undefined.

@mscherer82

This comment has been minimized.

Copy link

commented Mar 19, 2019

Any progress in that? I still can't see revert messegas on ethereum testnet (while it works very fine on TRON). The error I got has this format:

Transaction has been reverted by the EVM:
{
  "blockHash": "0xcb70ea5a9ca990504cd3dcb5577c50f25d004d3e959f584a728906f63e019953",
  "blockNumber": 4057666,
  "contractAddress": null,
  "cumulativeGasUsed": 92226,
  "from": "0x143aa1f77937a3d5b81cbedd62584dddfbf0fe2a",
  "gasUsed": 66752,
  "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
  "status": false,
  "to": "0x7649b36218e573aad06112228f540aee670aaabb",
  "transactionHash": "0xc6e25f993de08f0f66f23ea77e7e49119d43698b21ff3273d8cc1999e3080e9c",
  "transactionIndex": 1,
  "events": {}
}
@chriseth

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@mscherer82 there is no indication why it should not work. Perhaps you are using a client that does not support it?

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Did you set the testnet to be byzantium-compatible?

@gluk64

This comment has been minimized.

Copy link

commented May 4, 2019

Here is a bash script to fetch revert reason:

https://ethereum.stackexchange.com/a/70391/3558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.