Revert reasons not detected in external calls with viaIR: true and optimizer enabled

[cameel]

If a contract compiled with optimization and viaIR: true calls an external function that reverts, to.be.revertedWith() matcher does not detect the right revert reason.

This does not happen:

• with viaIR: false,
• without optimization,
• on solc < 0.8.7,
• when the revert happens inside the same contract..

I saw cases where to.be.revertedWith() thinks here was no reason given at all (see run 994880 in ethereum/solidity#12736): EDIT: this one was actually an unrelated problem caused by a bug in the compiler. Only the one below is relevant here.

AssertionError: Expected transaction to be reverted with ERC20: invalid-permit, but other exception was thrown: Error: Transaction reverted without a reason string

and ones where it's misdetected as something else (see repro below):

AssertionError: Expected transaction to be reverted with ABC, but other exception was thrown: Error: Transaction reverted: function was called with incorrect parameters

This might be caused by the same thing as #2115 but the effects are a bit different so I'm reporting as a separate issue.

How to reproduce

Run this in the shell:

mkdir test/ contracts/

cat <<EOF > hardhat.config.js
require("@nomiclabs/hardhat-waffle");

module.exports = {
    solidity: {
        compilers: [{
            version: "0.8.12",
            settings: {
                viaIR: true,
                optimizer: { enabled: true },
            },
        }]
    }
};
EOF

cat <<EOF > test/RevertTest.js
const { expect } = require("chai");
const { ethers } = require("hardhat");

it("D should revert with 'ABC'", async () => {
    const C = await ethers.getContractFactory("C");
    const D = await ethers.getContractFactory("D");
    const c = await C.deploy();
    const d = await D.deploy();

    await expect(d.testRevert(c.address)).to.be.revertedWith("ABC");
});
EOF

cat <<EOF > contracts/RevertTest.sol
contract C {
    function f() public {
        revert("ABC");
    }
}

contract D {
    function testRevert(C c) public {
        c.f();
    }
}
EOF

npm install hardhat @nomiclabs/hardhat-waffle chai
npx hardhat test

Test output:

  1) D should revert with 'ABC'

  0 passing (687ms)
  1 failing

  1) D should revert with 'ABC':
     AssertionError: Expected transaction to be reverted with ABC, but other exception was thrown: Error: Transaction reverted: function was called with incorrect parameters

[cameel]

Here's another case of this, though it's a bit different. This one fails only with the latest compiler built from develop branch (so should fail with latest nightly too). The thing is, if you remove current() or increment() function, it starts behaving like the other one, e.g. fails on versions >= 0.8.7.

I found it while debugging a revert with viaIR in OpenZeppelin. It initially looked like something else or like a bug in their @openzepeplin/test-helpers library, but after reducing it to a small example, I think it's actually just another instance of this problem.

How to reproduce

Here's a repro showing the problem both with expectRevert() from @openzepeplin/test-helpers and with expect().to.be.revertedWith.

No failure on 0.8.12

mkdir test/ contracts/

cat <<EOF > hardhat.config.js
require("@nomiclabs/hardhat-waffle");
require('@nomiclabs/hardhat-truffle5');

module.exports = {
    solidity: {
        compilers: [{
            version: "0.8.12",
            settings: {
                viaIR: true,
                optimizer: { enabled: true },
            },
        }]
    }
};
EOF

cat <<EOF > test/CountersTest.js
const { expectRevert } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');
const CountersImpl = artifacts.require('CountersImpl');

it('expectRevert', async function () {
  const counter = await CountersImpl.new();
  await expectRevert(counter.decrement(), 'Counter: decrement overflow');
});

it('expect().to.be.revertedWith', async function () {
  const counter = await CountersImpl.new();
  await expect(counter.decrement()).to.be.revertedWith('Counter: decrement overflow');
});
EOF

cat <<EOF > contracts/CountersImpl.sol
contract CountersImpl {
    function current() public {}
    function increment() public {}
    function decrement() public {
        require(false, "Counter: decrement overflow");
    }
}
EOF

npm install hardhat @nomiclabs/hardhat-waffle chai @openzeppelin/test-helpers @nomiclabs/hardhat-truffle5
npx hardhat test

Output:

  ✔ expectRevert (528ms)
  ✔ expect().to.be.revertedWith

  2 passing (569ms)

Without current()

If you replace the contract with this (i.e. same thing but with current() removed):

contract CountersImpl {
    function increment() public {}
    function decrement() public {
        require(false, "Counter: decrement overflow");
    }
}

you get

  1) expectRevert
  2) expect().to.be.revertedWith

  0 passing (548ms)
  2 failing

  1) expectRevert:

      Wrong kind of exception received
      + expected - actual

      -Transaction reverted: function was called with incorrect parameters
      +Counter: decrement overflow

      at expectException (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at expectRevert (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:75:3)
      at Context.<anonymous> (test/CountersTest.js:7:3)

  2) expect().to.be.revertedWith:
     AssertionError: Expected transaction to be reverted with Counter: decrement overflow, but other exception was thrown: Error: Transaction reverted: function was called with incorrect parameters

despite the fact that current() is not used anywhere.

Similarly, if you run with the binary built by our CI today and configure hardhat.config.js to use it:

require("@nomiclabs/hardhat-waffle");
require('@nomiclabs/hardhat-truffle5');

module.exports = {
    solidity: {
        compilers: [{
            version: "0.8.13",
            settings: {
                viaIR: true,
                optimizer: { enabled: true },
            },
        }]
    }
};

const {TASK_COMPILE_SOLIDITY_GET_SOLC_BUILD} = require('hardhat/builtin-tasks/task-names');
const assert = require('assert');

subtask(TASK_COMPILE_SOLIDITY_GET_SOLC_BUILD, async (args, hre, runSuper) => {
    assert(args.solcVersion == '0.8.13', 'Unexpected solc version: ' + args.solcVersion)

    return {
        compilerPath: '/tmp/solc',
        isSolcJs: false,
        version: args.solcVersion,
        longVersion: '0.8.13-ci.2022.3.4+commit.198b7053.Linux.g++'
    }
})

you get the same failure:

  1) expectRevert
  2) expect().to.be.revertedWith

  0 passing (532ms)
  2 failing

  1) expectRevert:

      Wrong kind of exception received
      + expected - actual

      -Transaction reverted: function was called with incorrect parameters
      +Counter: decrement overflow

      at expectException (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at expectRevert (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:75:3)
      at Context.<anonymous> (test/CountersTest.js:7:3)

  2) expect().to.be.revertedWith:
     AssertionError: Expected transaction to be reverted with Counter: decrement overflow, but other exception was thrown: Error: Transaction reverted: function was called with incorrect parameters

[robinxb]

Also, when testing on Rinkeby, the issue happens.

It is find when testing with local node

Envs:

• ERC1967Proxy (UUPS)
• sol 0.8.2
• optmise on
• require(false, "Reason")

    await expect(
      proxy.ShoudFail()
    ).to.be.revertedWith("My error");

result with

      AssertionError: Expected transaction to be reverted
      + expected - actual

      -Transaction NOT reverted.
      +Transaction reverted.

[github-actions]

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

[cameel]

Still relevant.

[github-actions]

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

[cameel]

Still relevant.

[github-actions]

This issue was marked as stale because it didn't have any activity in the last 30 days. If you think it's still relevant, please leave a comment indicating so. Otherwise, it will be closed in 7 days.

[cameel]

Still relevant.

[Marenz]

fyi, this happens to more and more tests. We now have an additional 58 failing tests in zeppelin with our latest development version.

[0xkarmacoma]

I am getting hit by this, I have a hardhat test on this branch that passes with viaIR: false but fails with viaIR: true

[sashaaldrick]

This happened to me on the most recent hardhat version, 2.11.2 with solidity 0.8.17 and I have a test with fails with viaIR: true but passes with viaIR: false.

[thedavidmeister]

i have a similar experience, failures that only appear with viaIR enabled

[fvictorio]

Hi folks, we are working on some having basic support for this, but please keep in mind that we won't be able to have a really good support until this issue is fixed. The reason is that Hardhat relies on non-optimized code for a lot of its heuristics, and viaIR doesn't really work right now with the optimizer disabled.

[0xkarmacoma]

@fvictorio can you give us some insight into why heuristics are needed at all? The data returned by REVERT is deterministic

[fvictorio]

@karmacoma-eth you are right that for revert reasons, things should just work. I was referring more to general support for viaIR compiled code.

In this particular case, what's happening is that we still run some heuristics when an error has revert data, which is probably a mistake (at least if the revert data is an Error(string)). And some of those heuristics make assumptions that are broken by viaIR, which makes some things stop working.

I think we'll have a (probably beta) release with basic support for viaIR next week.

[cameel]

@fvictorio It's now possible to disable all Yul Optimizer steps and have only the stack-related optimizations enabled (see ethereum/solidity#12533 (comment)). Would that be good enough to keep heuristics working?

[fvictorio]

Hey @cameel, sorry for not responding before. I haven't been able to work on this during the last weeks, but this is something we want to get fixed and released.

I opened #3365 to have a single issue tracking this effort, so I'm going to close this one in favor of that.