Skip to content
This repository has been archived by the owner on Jul 11, 2019. It is now read-only.

Out of gas in implementation will not "propagate" to proxy #51

Closed
frangio opened this issue Apr 19, 2018 · 5 comments
Closed

Out of gas in implementation will not "propagate" to proxy #51

frangio opened this issue Apr 19, 2018 · 5 comments
Assignees
Labels
kind:bug Bug! kind:research Research on a topic before implementing it
Milestone

Comments

@frangio
Copy link
Contributor

frangio commented Apr 19, 2018

The Proxy contract calls delegatecall to some other implementation which may run out of gas. In that case, the proxy will revert. It will probably still have gas left because the delegatecall (like normal call) only forwards a fraction of the gas left to the callee (see EIP-150). Therefore, there is some information lost for the caller of the proxy: they cannot see that the call ran out of gas.

I think this is part of a bigger problem of the platform which is that there is no way of knowing whether a transaction failed because of running out of gas or some other reason.

We want to consider making the proxy "run out of gas" if the delegatecall failed and it consumed all available gas, so as to propagate this information upwards the call chain. It's not clear if or how this could be done.

@maraoz maraoz added this to the v0.2.0 milestone Apr 19, 2018
@maraoz maraoz added kind:research Research on a topic before implementing it kind:bug Bug! labels Apr 19, 2018
@facuspagnuolo facuspagnuolo modified the milestones: Backlog, Sprint #2, v2.0.0 Jun 5, 2018
@spalladino spalladino self-assigned this Jul 2, 2018
@spalladino
Copy link
Contributor

spalladino commented Jul 3, 2018

I pushed a potential solution to https://github.com/zeppelinos/zos-lib/tree/feature/out-of-gas-errors. The idea would be to change the delegate code in the proxy to the following:

  function _delegate(address implementation) internal {
    assembly {
      // Copy msg.data. We take full control of memory in this inline assembly
      // block because it will not return to Solidity code. We overwrite the
      // Solidity scratch pad at memory position 0.
      calldatacopy(0, 0, calldatasize)

      // Store how much gas we have available
      let availablegas := gas

      // Call the implementation.
      // out and outsize are 0 because we don't know the size yet.
      let result := delegatecall(gas, implementation, 0, calldatasize, 0, 0)

      // Assume we are out of gas if we have under 1/64 of gas remaining
      // TODO: Fine-tune this formula to account for additional cost of delegatecall itself
      let outofgas := lt(gas, div(availablegas, 64))

      // Copy the returned data.
      returndatacopy(0, 0, returndatasize)

      switch result
      // delegatecall returns 0 on error.
      case 0 { 
        switch outofgas
        case 0 { revert(0, returndatasize) }
        case 1 { for { } 1 { } { } }
      }
      default { 
        return(0, returndatasize) 
      }
    }
  }

The idea is that, if the delegatecall failed, and the gas left is less than 1/64 of the original gas available, then simulate an out-of-gas in the proxy itself. Note throwing an invalid opcode would not work, as it would consume all gas, but would cause the node running the tx to return an invalid opcode instead of an out of gas error.


That said, I'd suggest against implementing this feature. Returning out-of-gas from a call is an added feature from the node, not part of the EVM itself, and is lost among regular calls. For instance, given this code:

contract Gassy {
    uint256 i;
    function gassy() public {
        while(true) { i = i + 1; }
    }
    function throws() public {
        i = 10;
        assert(false);
    }
}

contract ManualProxy {
    Gassy gassy;    
    function ManualProxy() {
        gassy = new Gassy();
    }    
    function callGassy() {
        gassy.gassy();
    }    
    function callThrow() {
        gassy.throws();
    }
}

Given an instance gassy of the contract Gassy, and an instance proxy of ManualProxy:

  • Calling gassy.gassy() returns an out-of-gas error
  • Calling gassy.throws() returns an invalid opcode error
  • Calling proxy.gassy() returns a revert
  • Calling proxy.trhwos() returns a revert

This means that regular calls completely miss the out of gas or invalid opcode errors, since Solidity itself only checks the return value of a CALL and reverts if it failed (see here).

As such, I recommend not trying to add a feature not even present in Solidity, and keep the same behaviour of just reverting if the nested call failed, as any custom solution may be prone to errors. Also, this solutions bubbles both out-of-gas and invalid-opcode errors as out-of-gas errors, since it cannot distinguish between the two (see this test).

The only argument for implementing this would be to make the proxy as transparent as possible, from the point of view of a client interacting with it. I'd appreciate any comments here @frangio @facuspagnuolo.

@frangio
Copy link
Contributor Author

frangio commented Jul 4, 2018

Great analysis, @spalladino.

Unless someone shows a realistic example where it's a problem, I think I'm almost convinced that we should not implement this.

Some remaining questions I'd like to see answered:

  • Does not propagating out-of-gas affect a node's gas estimation in any way?
  • (This one is some math that may be trivial...) Suppose the logic contract consumes all 63/64ths of the available gas, and the proxy (as it normally would) consumes some of the remaining 1/64th. If a caller of the proxy does the same check @spalladino implemented, does everything look normal to it? I.e. does it see that there is more than 1/64th of the gas available? Consider that the caller would be doing integer arithmetic.

@facuspagnuolo
Copy link
Contributor

Thanks for the job @spalladino! I do agree with your conclusion, I don't see the point to simulate OOG errors when even Solidity is not doing it.

Nit comment regarding the solution, I'd try to perform the outofgas calculation inside the first case 0 scenario if possible.

@spalladino
Copy link
Contributor

Nit comment regarding the solution, I'd try to perform the outofgas calculation inside the first case 0 scenario if possible.

I wanted to avoid the gas from returndatacopy from interfering in the calculation, that's why I added it right after the delegatecall.

@spalladino
Copy link
Contributor

Closing as discussed. Will keep the code here though for future reference.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind:bug Bug! kind:research Research on a topic before implementing it
Projects
None yet
Development

No branches or pull requests

4 participants