ACL: remove ACLOracle canPerform() gas limit

[willjgriff]

This PR will enable the 1Hive Oracles to function as expected since the Istanbul hard fork. Unfortunately our Oracles canPerform() functions can not be guaranteed to execute under the gas limit of 30000.

Relevant functions in the Oracles can be seen here:
https://github.com/1Hive/token-oracle/blob/master/contracts/TokenBalanceOracle.sol#L59
https://github.com/1Hive/dandelion-voting-app/blob/master/contracts/DandelionVoting.sol#L252

I've replaced the Oracle canPerform() staticcall gas limit of 30000 with gasleft() minus some amount for returning an error (preventing a revert if the staticcall runs out of gas). As suggested by @sohkai.

I've observed that if the staticcall reverts with an oog after being given all of gasleft() it still seems to reserve about 100000 gas, as can be seen with a gasleft() directly after the oog staticcall (tested on remix). The function doesn't revert and returns false. I've still included the error gas allowance but it may not be necessary.

I had to extract testRevertOracle and testStateModifyingOracle from the testOracle test because when the staticcall reverts it consumes all the available gas, leaving very little to none for the remaining tests in the same transaction.

[welcome]

Thanks for opening this pull request! Someone will review it soon 🔍

[sohkai]

I've observed that if the staticcall reverts with an oog after being given all of gasleft() it still seems to reserve about 100000 gas, as can be seen with a gasleft() directly after the oog staticcall (tested on remix)

I think this is due to the 63/64 gas forward change from EIP-150; if you test with a lower starting gas amount, you most likely will get a different value.

I'm not super sure what value we should use here, but perhaps we should also decrease the saved amount to ~50k (save at least 1-2 SSTOREs after the ACL call)?

[sohkai]

A few notes from our call with CD.

The goal of this change is to minimize the changeset between the currently deployed and new ACL contracts. In the future, we will want to evaluate whether hard set amounts make sense and whether we should move them to be dynamic (both the gas buffer as well as the 1/64 assumption).

[sohkai]

As mentioned in the call, we should use gasleft() once and then do operations on the same cached value afterwards to avoid any potential underflows.

[willjgriff]

In the most recent commit, as well as doing the changes requested, I have had to remove the sig variable to prevent a stack too deep error. I couldn't inline the oracleCanPerformGas calculation as it uses a storage variable which can't be accessed in assembly. Any alternative suggestions are welcome. I also had to move the StateModifyingOracle test to a JS and remove it from the Solidity tests as this Oracle consumes all the gas it's given as if it's gone OOG.

[bingen]

I think this is due to the 63/64 gas forward change from EIP-150; if you test with a lower starting gas amount, you most likely will get a different value.

Good catch I forgot about this! Trying to recall it I was reading that EIP and got confused about this:

if compustate.gas < gas + extra_gas:
            gas = min(gas, max_call_gas(compustate.gas - extra_gas))

If that if condition holds, then compustate.gas - extra_gas < gas, and max_call_gas(compustate.gas - extra_gas) <= compustate.gas - extra_gas, therefore the min operator inside is not needed, right? What am I missing here? (Not a big deal though, it would just be unnecessary)

[bingen]

Overall looks good to me. Thanks @willjgriff for all this work!
I left a slight modification proposal inline.

[bingen]

I would refactor all this as:

        uint256 gasLeftBeforeCall = gasleft();

        // I don't think we even need this, but if we do I would convert it to a require:
        // require(gasLeftBeforeCall > ERROR_GAS_ALLOWANCE, ERROR_ORACLE_OOG);
        assembly {
            ok := staticcall(gasLeftBeforeCall, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
        }

        if (!ok) {
            uint256 gasLeftAfterCall = gasLeft();
            require(gasLeftAfterCall > gasLeftBeforeCall / 64 && gasLeftAfterCall > ERROR_GAS_ALLOWANCE), ERROR_ORACLE_OOG);
            return false;
        }

And we can get rid of the max function below. I haven't checked, but as Solidity doesn't have inline functions we may save some gas this way. Also, I think if we split the require in 2 (instead oh using && inside) it's slightly cheaper.

[willjgriff]

If we use all the available gas in the staticcall is there a chance there won't be enough left over for returning the error (even with the 1/64th rule)? After testing I can't find a gas limit where that's the case (eg just a revert without the error message) but it wasn't exhaustive. Also, due to a stack too deep error I can't add any more variables eg gasLeftAfterCall unless I remove something eg oracleCanPerformGas. So I can do this if you think the second require statement can always be reached. Eg the answer to my question is no.

[bingen]

If we use all the available gas in the staticcall is there a chance there won't be enough left over for returning the error (even with the 1/64th rule)?

In that case we could leave the commented require(gasLeftBeforeCall > ... check, with the proper amount to make sure that 1/64 of that amount is enough.
Anyway, even if that happen, what would be the problem? It would just revert, right? So compared to what would happen later in require(gasLeftAfterCall >... we would just miss the error message? Or am I missing something?

[willjgriff]

Yes we would just miss the error message. @sohkai do you have a preference?

I would like to remove the max() function but I can't unless I remove the oracleCanPerformGas variable (or some unrelated variable somewhere) which would mean passing all of the gas to the function.

[sohkai]

Not necessarily affecting the plans, but interesting to know that gas may be changing in the future: EIP-2489 deprecates gas and always returns UINT256_MAX.

We use gas in a few other important places (DelegateProxy and CallsScriptExecutor) that would be affected, so I'm not totally concerned about this case, especially since we're already covering the 63/64 case.

[sohkai]

@willjgriff I've pushed a few commits. You'll notice I've reverted most of the logic around reserving gas, and just opted to send all the remaining gas to the oracle.

First of all, sorry for making you run around in a few circles in hopes that we could make this mechanism work and provide a better debugging message.

I've thought about it for a while now, and came to the conclusion that trying to save gas is relatively futile.

While executing the revert statement is light on gas—usually lower than 1000 gas, placing the call gas requirement to safely execute the revert to be 40-60k with the 63/64 gas forward rule (it's only really in trouble inside the 2300 gas stipends)—in actual debugging we are likely to suspect out of gas issues through the gas used in a transaction as well as specific traces (parity's is much more helpful than geth's in this manner). In particular, tools like Tenderly also help us debug which parts of a transaction may have used up a lot of gas, and it should be fairly obvious if the oracle took up all the required gas.

I started thinking about just leaving the 1/64 check, but then also started suspecting if this was really required (as I mention above, we have other heuristics to suspect out of gas scenarios). Hardcoding 64 may also not be the best idea in an environment that has no future guarantees, and I started specifically weighing whether the enhanced debugging experience would be better than potentially asking all organizations to upgrade their ACLs if a future hardfork moved this value (especially lower, e.g. 32).

[sohkai]

Note that in these cases that are now not throwing due to sending all gas (truffle by default is using 6.9M gas), it may be interesting to experiment with sending a transaction with lower gas values to see if they start failing and adding some tests around this.

We could also add some assertions for the gas used in these transactions to double check that the oracle is indeed eating up 63/64th of the gas available.

[coveralls]

Coverage decreased (-0.001%) to 99.115% when pulling 3882538 on 1Hive:acl-remove-oracle-limit into fb93dee on aragon:next.

[sohkai]

Just updated the tests a bit, reorganizing them and adding a few more checks :).

Got caught up with us giving account[0] permissions and re-using it in tests. Generally, for anything permissions-based, you want to avoid testing with account[0] since it's the default account sending transactions.

[sohkai]

This seems like quite a lot of gas, since I initially only assumed that we would be reverting under very low gas constraints.

However, in these tests, we are using a full organization, with proxies and all, and this adds a lot of overhead. We revert here because we run out of gas after the SLOAD for the ANY_ADDR's permission params (if we flip the order that hasPermission() checks entities such that it checks the ANY_ADDR before the specific who, we can lower this LOW_GAS value).

[welcome]

Congrats on merging your first pull request! Aragon is proud of you 🦅