`estimateGas` reverts, but the transaction succeeds with a hardcoded `gasLimit` #664

platocrat · 2021-04-28T03:06:03Z

Describe the bug
In ChainLink's LinkToken integration tests, 3/7 tests fail. Those that do fail involve contract calls that should revert. However, instead of a revert message being returned, the familiar cannot estimate gas error is thrown.

To Reproduce
Steps to reproduce the behavior:

git clone https://github.com/smartcontractkit/LinkToken.git
git checkout feature/ovm-integration-upstream-changes
Start the docker containers under optimism/ops to start a local OE network
Back in the LinkToken project directory, run: yarn && yarn setup && yarn test:integration../test/v0.7/bridge/optimism/OVM_L2ERC20Gateway.test.ts
See the familiar error:

Error: cannot estimate gas; transaction may fail or may require manual gas limit

Expected behavior
A transaction that fails estimateGas should not succeed with a hardcoded gasLimit

Related issues
#474
#543

The text was updated successfully, but these errors were encountered:

transmissions11 · 2021-04-28T03:29:46Z

Hasn't there been an issue up for this for a while?

platocrat · 2021-04-28T03:39:47Z

Hasn't there been an issue up for this for a while?

yes. #474 is one related issue and #543 is perhaps another

platocrat · 2021-04-28T14:55:01Z

@K-Ho closing since this is a duplicate of #474

K-Ho · 2021-04-28T19:11:42Z

@platocrat The expected behavior listed here isn't actually right. Chainlink is getting UNPREDICTABLE_GAS_LIMIT when trying to estimateGas but the tx succeeds with a static gasLimit.

transmissions11 · 2021-04-28T19:24:49Z

@platocrat The expected behavior listed here isn't actually right. Chainlink is getting UNPREDICTABLE_GAS_LIMIT when trying to estimateGas but the tx succeeds with a static gasLimit.

@K-Ho I've noticed that txs always succeed with a static gas limit actually...

Here's a reproduction branch: https://github.com/Rari-Capital/nova/tree/gasLimitNoRevertRepro

platocrat · 2021-04-28T19:33:50Z

@K-Ho just to confirm that this really is the same issue that we see for contract calls that revert, here are the full error messages for the 2 Waffle-based tests that fail with this error:

I highlighted the code=UNPREDICTABLE_GAS_LIMIT field, which you can see appears in both error messages.

NOTE: I can confirm that specifying the gas limit to 8999999 for the calls in these two tests does makes the txs succeed.

snario · 2021-04-28T20:24:10Z

This is not a duplicate of #474. That issue describes “estimateGas does not provide the revert reason” and this issue describes: “estimateGas reverts, but the transaction succeeds with a hardcoded gasLimit”

tynes · 2021-04-30T03:28:30Z

I believe the problem is with simulateMessage(). It is the main difference between a regular transaction and eth_call, eth_estimateGas uses eth_call

platocrat · 2021-05-03T18:07:28Z

@tynes @snario I re-ran the integration test for the hardhat example in the monorepo and get an odd type error.

The error is below:

   Error: invalid BigNumber string (argument="value", value="auto", code=INVALID_ARGUMENT, version=bignumber/5.1.0)
      at Logger.makeError (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/logger/src.ts/index.ts:205:28)
      at Logger.throwError (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/logger/src.ts/index.ts:217:20)
      at Logger.throwArgumentError (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/logger/src.ts/index.ts:221:21)
      at Function.BigNumber.from (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/bignumber/src.ts/bignumber.ts:240:27)
      at JsonRpcProvider.provider.getGasPrice (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/packages/hardhat-ovm/src/index.ts:225:26)
      at Wallet.<anonymous> (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/abstract-signer/src.ts/index.ts:139:36)
      at step (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/abstract-signer/lib/index.js:48:23)
      at Object.next (/Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/abstract-signer/lib/index.js:29:53)
      at /Users/platocrat/Documents/GitHub/OptimismPBC/Optimism/optimism/node_modules/@ethersproject/abstract-signer/lib/index.js:23:71
      at new Promise (<anonymous>)

The error points to optimism/packages/hardhat-ovm/src/index.ts:225:26.
Note that this error is received after pulling and building @tynes's branch from ethereum-optimism/optimism/pull/721.

@K-Ho From this new error shown above, I believe ethereum-optimism/optimism/pull/721 should warrant a separate GH issue to document this error in hardhat-ovm
I also think we can conclude that the original issue here is resolved as per Kevin's response earlier:

@platocrat The expected behavior listed here isn't actually right. Chainlink is getting UNPREDICTABLE_GAS_LIMIT when trying to estimateGas but the tx succeeds with a static gasLimit.

gakonst · 2021-05-03T18:56:53Z

@platocrat this error you got is because there is no gasPrice: 0 parameter in your hardhat config here, whereas the examples now all use it.

platocrat · 2021-05-03T20:40:02Z

I'm confirming that this issue is fixed by ethereum-optimism/optimism/pull/721!!

gakonst · 2021-05-03T21:28:03Z

What's the exact fix by 721? Do you get proper revert messages?

platocrat · 2021-05-04T00:20:35Z

What's the exact fix by 721? Do you get proper revert messages?

The fix appears to be that we no longer need to specify a hardcoded gasLimit.

tynes · 2021-05-04T00:26:03Z

The fix appears to be that we no longer need to specify a hardcoded gasLimit.

This is the result of the fix

What's the exact fix by 721? Do you get proper revert messages?

The problem was that the interface between userland and the system contracts was not defined. ExecutionManager.run returned a different ABI encoded returndata compared to ExecutionManager.simulateMessage. The returndata was parsed by simply slicing bytes which worked when the returndata was a single type but then broke when it was multiple types.

eth_estimateGas calls eth_call under the hood a bunch of times, so the problem was also present in eth_call - basically the return value of the eth_call wouldn't be deserialized correctly by l2geth and it would interpret the returndata as indicating that the transaction reverted when it actually didn't. This would result in the message Error: cannot estimate gas; transaction may fail or may require manual gas limit because l2geth thought that the tx would revert

platocrat · 2021-05-04T01:34:07Z

@K-Ho @tynes @snario
Re-ran integration tests of ChainLink's old LinkToken branch feature/ovm-integration-upstream-changes using a newly fetched and rebuilt version of the optimism/ops containers.

The tests that are supposed to revert still revert with the same UNPREDICTABLE_GAS_LIMIT error:

Error: cannot estimate gas; transaction may fail or may require manual gas limit

I also confirmed that adding a hardcoded gasLimit of 8999999 is still a workaround.

Update

Running the tests again without specifying a hardcoded gasLimit nor gasPrice results in the UNPREDICTABLE_GAS_LIMIT error in all tests.
Adding a hardcoded gasPrice: 0 to all contract calls still results in all tests failing with the error above.
Finally, adding a hardcoded gasPrice: 0 and gasLimit: 8999999 to all contract calls results in the familiar error described in ethereum-optimism/optimism/issues/717 in 2/4 of the failing tests (the other 2 that fail are dependent on the first succeeding.

What's odd about point 3 is that only 3/7 tests pass, and of those that pass, they involve contract calls that revert, which is the opposite of what was happening previously.
Previously, 4/7 tests were passing, but those that were failing involved contract calls that revert.

K-Ho · 2021-05-04T03:13:37Z

@platocrat - are you still seeing this estimateGas error with the Waffle ERC20 Example tests? If so, I think the next step would be to comment out parts of execution until we nail down the exact section that causes estimateGas to fail

platocrat · 2021-05-04T22:39:08Z

Regarding #664 and #721:

l1-l2-deposit-withdrawal outside of monorepo (because of current issue with installing @eth-optimism/contracts as a dep when inside monorepo)
a. I ran the optimism/ops containers from the l2geth/fix-returndata branch (from pull/721) and ran the l1-l2-deposit-withdrawal script.
b. Without specifying gasPrice: 0 nor gasLimit: 8999999, none of the transactions in the example.js script will be submitted successfully.
c. Specifying only gasPrice: 0 in every contract call results in the errors thrown, as in issue/664, for every call
d. Specifying both gasPrice: 0 nor gasLimit: 8999999 results in the script working as expected, all transactions are submitted successfully
Waffle-ERC20-Example within monorepo
e. [Tests] As @K-Ho pointed out when running the Waffle tests in optimism/examples/waffle, all tests pass without any changes.
f. This means that we do NOT need to hardcode a gasPrice nor gasLimit anywhere
Truffle-ERC20-Example outside of monorepo (no concerning reason, just quicker for me -- I believe we can now finalize docs: add truffle example + ci #666 ):
g. [Tests] TL;DR is that tests fail ONLY if gasPrice: 0 is not set everywhere, including in all migrations scripts, when running tests
h. [Deployments] Running migrations is successful after settinggasPrice: 0 in migrations file(s).
optimism-tutorial within monorepo
i. [Tests] Will only pass if we set gasPrice: 0 in the hardhat.config.*
j. [Deployments] Successful even WITHOUT a hardcoded gasLimit and gasPrice

cc: @snario @gakonst @K-Ho @tynes

platocrat · 2021-05-10T19:28:51Z

@K-Ho @snario
Was able to repro this error in a couple of ChainLink's integration tests.

Repro case:

Git clone LinkToken: git clone https://github.com/smartcontractkit/LinkToken.git
Checkout branch: git checkout feature/oe-token
Install deps: yarn
Clean setup: yarn clean && yarn setup
Comment out line 8 in test/helpers/optimism/index.ts
Run tests: yarn test --network optimism ./test/v0.7/bridge/token/LinkTokenChild.test.ts
See error in 2/3 tests

smartcontracts · 2021-05-10T20:43:21Z

@platocrat I'll take a look at this in an hour or so.

smartcontracts · 2021-05-10T21:29:15Z

~~@platocrat I'm unable to reproduce this on v0.3.0-rc. We might've fixed the problem in that branch.~~

I was able to reproduce on v0.3.0-rc by commenting out this line: https://github.com/smartcontractkit/LinkToken/blob/8731d538f1290ac6bf6bdcd6a5aa92ae8a8125d5/test/helpers/optimism/index.ts#L8

platocrat · 2021-05-10T21:35:46Z

You need to comment out line 8 in LinkToken/test/helpers/optimism/index.ts

snario · 2021-05-10T21:48:43Z

The above ChainLink test failure seems unrelated to what this Github issue was created for. ChainLink is seeing #474 (reference) I believe — revert reasons not propagating up correctly.

smartcontracts · 2021-05-10T21:48:49Z

I was able to fix this problem by using v0.3.0 (now merged into master!) and changing the tests as follows:

     it('can NOT deposit without  access (gateway role)', async () => {
-      const depositTx = await l2Token.deposit(oe.l2Wallet.address, 100, h.optimism.TX_OVERRIDES_OE_BUG)
-      // TODO: fetch revert reason
-      // revert: LinkTokenChild: missing role
-      await h.txRevert(depositTx.wait())
+      await expect(
+        l2Token.deposit(oe.l2Wallet.address, 100, h.optimism.TX_OVERRIDES_OE_BUG)
+      ).to.be.revertedWith('No access')
     })
     it('owner can migrate to a new gateway', async () => {
@@ -221,10 +220,11 @@ describe(`LinkTokenChild ${Versions.v0_7}`, () => {
       const removeAccessTx = await l2Token.removeAccess(owner.address)
       await removeAccessTx.wait()
       // Owner deposit fails
-      const depositTx = await l2Token.deposit(owner.address, 100, h.optimism.TX_OVERRIDES_OE_BUG)
       // TODO: fetch revert reason
       // revert: LinkTokenChild: missing role
-      await h.txRevert(depositTx.wait())
+      await expect(
+        l2Token.deposit(owner.address, 100, h.optimism.TX_OVERRIDES_OE_BUG)
+      ).to.be.revertedWith('No access')
       // TODO: owner renounce ownership
     })

snario · 2021-05-10T21:54:01Z

I'm going to close this issue. If something from the above conversation is still unclear, let's open a new issue.

gakonst · 2021-05-11T07:12:08Z

Of note, Kelvin's approach above is the correct way to do it in JS. The way the tests were written before, they'd fail even in EVM mode because you're awaiting a failing transaction with an overridden gas limit.

K-Ho · 2021-05-12T18:41:54Z

re-opening this issue - here's Chainlink still seeing this issue appear:
This PR is green with static gasLimit: smartcontractkit/LinkToken#33
This PR is red with NO static gasLimit: smartcontractkit/LinkToken#37 (just this one change)
This is the failure: https://github.com/smartcontractkit/LinkToken/runs/2569267264?check_suite_focus=true (UNPREDICTABLE_GAS_LIMIT)

platocrat · 2021-05-12T19:56:08Z

I was able to repro smartcontractkit/LinkToken#37 locally.
Going to inspect further

platocrat · 2021-05-12T20:42:01Z

I resolved the UNPREDICTABLE_GAS_LIMIT errors in the two tests in smartcontractkit/LinkToken#37 using Kelvin's approach earlier above:

      it('can NOT deposit without  access (gateway role)', async () => {
-      const mintTx = await l2Token.mint(oe.l2Wallet.address, 100, optimism.TX_OVERRIDES_BUG)
-      // TODO: fetch revert reason
-      // revert: 'No access'
-      await h.txRevert(mintTx.wait())
+      const mintTx = l2Token.mint(oe.l2Wallet.address, 100, optimism.TX_OVERRIDES_BUG)
+      await expect(mintTx).to.be.revertedWith('No access')
})
      it('owner can migrate to a new gateway', async () => {
-      const mintTx = await l2Token.mint(owner.address, 100, optimism.TX_OVERRIDES_BUG)
-      // TODO: fetch revert reason
-      // revert: 'No access'
-      await h.txRevert(mintTx.wait())
+      const mintTx = l2Token.mint(owner.address, 100, optimism.TX_OVERRIDES_BUG)
+      await expect(mintTx).to.be.revertedWith('No access')
})

Note
The above changes were communicated to @krebernisak in the #optimism-chainlink channel in Synthetix's discord

snario · 2021-05-12T23:39:32Z

As @gakonst mentioned, it looks like the test was just written incorrectly.

platocrat added A-geth C-bug Category: bugs labels Apr 28, 2021

platocrat closed this as completed Apr 28, 2021

K-Ho reopened this Apr 28, 2021

snario changed the title ~~LinkToken: reverting contract calls return familiar cannot estimate gas error~~ estimateGas reverts, but the transaction succeeds with a hardcoded gasLimit Apr 28, 2021

gakonst mentioned this issue May 3, 2021

fix(hh-ovm): defaults to 0 gasPrice if none provided #734

Merged

This was referenced May 5, 2021

Update Geth to produce revert reasoning instead of UNPREDICTABLE_GAS_LIMIT #773

Closed

Truffle-based integration tests fail when requiring gas for transactions #543

Closed

smartcontracts added the S-confirmed Status: A confirmed bug label May 10, 2021

snario closed this as completed May 10, 2021

K-Ho reopened this May 12, 2021

snario closed this as completed May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`estimateGas` reverts, but the transaction succeeds with a hardcoded `gasLimit` #664

`estimateGas` reverts, but the transaction succeeds with a hardcoded `gasLimit` #664

platocrat commented Apr 28, 2021 •

edited by K-Ho

Loading

transmissions11 commented Apr 28, 2021

platocrat commented Apr 28, 2021

platocrat commented Apr 28, 2021 •

edited

Loading

K-Ho commented Apr 28, 2021

transmissions11 commented Apr 28, 2021 •

edited

Loading

platocrat commented Apr 28, 2021

snario commented Apr 28, 2021

tynes commented Apr 30, 2021

platocrat commented May 3, 2021 •

edited

Loading

gakonst commented May 3, 2021 •

edited

Loading

platocrat commented May 3, 2021

gakonst commented May 3, 2021

platocrat commented May 4, 2021

tynes commented May 4, 2021

platocrat commented May 4, 2021 •

edited

Loading

K-Ho commented May 4, 2021

platocrat commented May 4, 2021

platocrat commented May 10, 2021 •

edited

Loading

smartcontracts commented May 10, 2021

smartcontracts commented May 10, 2021 •

edited

Loading

platocrat commented May 10, 2021 •

edited

Loading

snario commented May 10, 2021

smartcontracts commented May 10, 2021

snario commented May 10, 2021

gakonst commented May 11, 2021

K-Ho commented May 12, 2021

platocrat commented May 12, 2021 •

edited

Loading

platocrat commented May 12, 2021 •

edited

Loading

snario commented May 12, 2021

estimateGas reverts, but the transaction succeeds with a hardcoded gasLimit #664

estimateGas reverts, but the transaction succeeds with a hardcoded gasLimit #664

Comments

platocrat commented Apr 28, 2021 • edited by K-Ho Loading

transmissions11 commented Apr 28, 2021

platocrat commented Apr 28, 2021

platocrat commented Apr 28, 2021 • edited Loading

K-Ho commented Apr 28, 2021

transmissions11 commented Apr 28, 2021 • edited Loading

platocrat commented Apr 28, 2021

snario commented Apr 28, 2021

tynes commented Apr 30, 2021

platocrat commented May 3, 2021 • edited Loading

gakonst commented May 3, 2021 • edited Loading

platocrat commented May 3, 2021

gakonst commented May 3, 2021

platocrat commented May 4, 2021

tynes commented May 4, 2021

platocrat commented May 4, 2021 • edited Loading

Update

K-Ho commented May 4, 2021

platocrat commented May 4, 2021

platocrat commented May 10, 2021 • edited Loading

smartcontracts commented May 10, 2021

smartcontracts commented May 10, 2021 • edited Loading

platocrat commented May 10, 2021 • edited Loading

snario commented May 10, 2021

smartcontracts commented May 10, 2021

snario commented May 10, 2021

gakonst commented May 11, 2021

K-Ho commented May 12, 2021

platocrat commented May 12, 2021 • edited Loading

platocrat commented May 12, 2021 • edited Loading

snario commented May 12, 2021

`estimateGas` reverts, but the transaction succeeds with a hardcoded `gasLimit` #664

`estimateGas` reverts, but the transaction succeeds with a hardcoded `gasLimit` #664

platocrat commented Apr 28, 2021 •

edited by K-Ho

Loading

platocrat commented Apr 28, 2021 •

edited

Loading

transmissions11 commented Apr 28, 2021 •

edited

Loading

platocrat commented May 3, 2021 •

edited

Loading

gakonst commented May 3, 2021 •

edited

Loading

platocrat commented May 4, 2021 •

edited

Loading

platocrat commented May 10, 2021 •

edited

Loading

smartcontracts commented May 10, 2021 •

edited

Loading

platocrat commented May 10, 2021 •

edited

Loading

platocrat commented May 12, 2021 •

edited

Loading

platocrat commented May 12, 2021 •

edited

Loading