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

"require" helper in assembly #7317

Closed
axic opened this issue Aug 28, 2019 · 10 comments
Closed

"require" helper in assembly #7317

axic opened this issue Aug 28, 2019 · 10 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. stale The issue/PR was marked as stale because it has been open for too long.

Comments

@axic
Copy link
Member

axic commented Aug 28, 2019

It would be nice to start introducing helper functions in assembly. This was previously discussed in #474 and #1319.

A lot of contracts/libraries deal with calling precompiles directly to avoid cost overheads. They do this with if iszero(staticcall(..)) { revert(0, 0) }. It would be nice to support require and/or assert:

  • require(expr) -> if iszero(expr) { revert(0, 0) }
  • assert(expr) -> if iszero(expr) { invalid }
@chriseth
Copy link
Contributor

chriseth commented Sep 3, 2019

Same argument I always give: Hidden library functions are hard to debug or verify. It would be much better to support importing assembly functions by source. Then it is easier to check what actually happens.

@ekpyron
Copy link
Member

ekpyron commented Sep 3, 2019

I actually think require and assert might be an exception here, though, and it might be beneficial to have them as primitive - that way we or someone could implement something like the SMTChecker for Yul that just uses them as assumptions and verification targets (ping @leonardoalt), right? We would translate require to require and assert to assert and change our reverts on a case-by-case basis, e.g. ABI decoding reverts would be require...

@ekpyron
Copy link
Member

ekpyron commented Sep 3, 2019

So for these particular two it might actually make verifying easier, if we add them...

@chriseth
Copy link
Contributor

chriseth commented Sep 3, 2019

I don't see the difference to checking whether invalid is reachable or not.

@ekpyron
Copy link
Member

ekpyron commented Sep 3, 2019

As verification target maybe, yes, but for assumptions?
I'm not sure revert is used only in places where it's conceptually a require...

@chriseth
Copy link
Contributor

chriseth commented Sep 3, 2019

The (one?) purpose of require/assert is that you can do bytecode-level verification.

@ekpyron
Copy link
Member

ekpyron commented Apr 3, 2020

@MicahZoltu brought this up in the chat again and there is actually another good argument for having this for requires with message.

The argument so far was that one can always just define a function, but for reverts with reason this becomes a problem. Say we define

function require(condition, message, length) {
  if iszero(condition) {
    mstore(0, <sig>)
    mstore(4, 0x20)
    mstore(0x24, length)
    mstore(0x44, message)
    revert(0, add(0x44, length))
  }
}

And then use it several times, the optimizer likely won't inline this. This results in the message being pushed on the stack needlessly when the condition is true.

Since we need builtins with mixed "required literal" and arbitrary arguments anyways for the immutables, this might be quite a good reason to introduce a require builtin...
So far I suggested to just use if condition { revertWithReason("msg", 3) } with a custom defined function revertWithReason, but that's (1) less readable and (2) inconvenient and error prone in that it requires the length to be explicitly stated.
A builtin require(condition, <requiredLiteralReasonString>) would solve those problems and could potentially conveniently allow reason strings longer than 32-bytes transparently.

@chriseth
Copy link
Contributor

chriseth commented Apr 6, 2020

I don't think we should introduce a builtin function only to save 3 gas.

@ekpyron
Copy link
Member

ekpyron commented Apr 6, 2020

Hm, true - for some reason I was thinking about the data gas being expensive, but that's paid either way...
But there's still the difference between jumping to the function and back or not, since if it isn't inlined the condition is only checked after the call, not inside... but that actually sounds much more like something we could fine-tune the optimizer for...

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

Hi everyone! This issue has been closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Feb 2, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

No branches or pull requests

5 participants