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

Option to specify opcode for require/assert exceptions #2586

Closed
maurelian opened this issue Jul 18, 2017 · 14 comments
Closed

Option to specify opcode for require/assert exceptions #2586

maurelian opened this issue Jul 18, 2017 · 14 comments

Comments

@maurelian
Copy link
Contributor

It appears to me that further separation of concerns between require() and assert() would be helpful. From the documentation:

Note that assert-style exceptions consume all gas available to the call, while require-style exceptions will not consume any gas starting from the Metropolis release.

This is of course a reference to ethereum/EIPs#140, and the new REVERT opcode. In this case the choice between the two is at a lower level than the source code.

The difference between the two is that assert should only be used for internal errors and require should be used to check external conditions (invalid inputs or errors in external components).

This is a completely distinct use case, which I believe is a static analysis concern), above the source code level.

I'm not sure what if anything the advantages of an invalid opcode exception would be, but I think it would be helpful to be able to throw an internal error AND have the benefits of REVERT (gas refunds + pass a return value).

It's possible the issue title is overly prescriptive, there might be a better way to resolve this.

@axic
Copy link
Member

axic commented Jul 18, 2017

The documentation may need some rewording/refactoring (that particular block is quite chunky), but the idea is the following:

Insert hard assertions into the code before critical parts (such as overflows, transfers, etc.). Additionally, guard the code with require for input parameters and return values of critical calls. If everything is catered for with require, the assertions should never be hit.

This can be easily detected via outside testing tools (i.e. truffle/dapple/etc.). Once ethereum/interfaces#1 or ethereum/interfaces#8 is implemented, the execution result (success, failure or revert) should be easily distinguishible.

Furthermore, analyzers should be able to point to unchecked conditions and perhaps in the future assertions could be optimised out for conditions already covered by require.

@GNSPS
Copy link
Contributor

GNSPS commented Jul 18, 2017

Big 👍 for this.

Personally, I understand and value how this is supposed to ease the development of formal verification tools for Solidity code but it appears to me it would make more sense to use the 0xfd opcode in all the exceptions by default.

Maybe there is some obvious reason I'm missing as to why this is engineered this way?

EDIT: Just missed the answer above while typing.

@axic is it possible that this is going to promote some waste of gas? I get where you're coming from but this is a huge trade-off. Maybe this should be done at the protocol level? Something like making REVERT taking a byte param to distinguish between different cases?

@maurelian
Copy link
Contributor Author

maurelian commented Jul 18, 2017

Insert hard assertions into the code before critical parts (such as overflows, transfers, etc.). Additionally, guard the code with require for input parameters and return values of critical calls. If everything is catered for with require, the assertions should never be hit.

This is quite helpful, thank you. Would it be accurate to say then that if tests are causing assert(false) and thus an invalid opcode error, then you're doing something wrong?

@chriseth
Copy link
Contributor

chriseth commented Jul 25, 2017

Yes, if anything can cause assert(false), your contract has a bug. This is actually what it says in the documentation - but perhaps it is not clear enough?

@maurelian
Copy link
Contributor Author

Looking again, this is clear "If this is possible, this means there is a bug in your contract you should fix.", though not as emphatic as I think it should be for such an important distinction.

Generally, from the perspective contract designer, it feels difficult to differentiate between the exact instances when one should be:

  1. "checking for internal errors" (assert)
  2. "checking external conditions (invalid inputs or errors in external components)" (require)

@maurelian
Copy link
Contributor Author

maurelian commented Jul 25, 2017

Rather than just go back and forth, how does this look? The main change is expanding on the cases which should use revert and assert, and IMO more precise language.

#2631

@chriseth
Copy link
Contributor

Can this be closed now?

@maurelian
Copy link
Contributor Author

FWIW, there is still a lot of confusion in the field about require vs. assert, but I think this is enough for now.

Maybe you could tweet one of your solidity tips. :)

@chriseth
Copy link
Contributor

Not sure if the tweet will help, but tried it anyway :-)
We should add some example use-cases to the documentation.

@GNSPS
Copy link
Contributor

GNSPS commented Jul 27, 2017

Yes, I think that would be really valuable!

@maurelian
Copy link
Contributor Author

@chriseth

We should add some example use-cases to the documentation.

What about guarding against overflow/underflow as in this case?

https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/math/SafeMath.sol#L11

@chriseth
Copy link
Contributor

chriseth commented Jul 28, 2017

@maurelian we are currently working on a static analyzer for that: #2538

@GNSPS
Copy link
Contributor

GNSPS commented Jul 28, 2017

Can you just clarify that, for current popular use of safeMath which is to be first line of defense against overflows, the correct construct to use would be require since this condition MAY be false given some input?

(Sorry for being a pin in the ass, just trying to get this really right 😄)

@chriseth
Copy link
Contributor

@GNSPS my opinion is that you should not just throw an exception on overflow, because the fact that at overflow was possible usually means that you did not think of some of the edge cases, and furthermore, throwing an exception might forever stall some process. For example, take the following code (from ENS) - perhaps not the best example:

    function setBalance(uint newValue, bool throwOnFailure) onlyRegistrar onlyActive {
        // Check if it has enough balance to set the value
        if (value < newValue) throw;
        value = newValue;
        // Send the difference to the owner
        if (!owner.send(this.balance - newValue) && throwOnFailure) throw;
}

If you just checked for underflow in this.balance - newValue and throw on failure, it might be that you never get your money back. It is much better to use formal methods to prove that underflow can never happen in this code path. Indeed, this.balance should always be equal to value and thus newValue will be at most this.balance because of the check in the first line of the function. If you forget to put this check, the "safeMath" method will lock up your money (at least in this function), while the assertion checking method will notify you about your error at compile-time.

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

No branches or pull requests

4 participants