Revert reasons not detected with via-ir enabled

[r0qs]

Version of Hardhat

2.13.0

What happened?

Although a basic support to viaIR was added in #3365, and #2453 was closed in favor of it, the problem mentioned by this comment: #2453 (comment) still not fixed. So I'm opening this issue here so we can keep track of the problem.

It now throws the following error: Transaction reverted and Hardhat couldn't infer the reason as shown below:

Compiled 2 Solidity files successfully


  1) expectRevert
  2) expect().to.be.revertedWith
  3) [current] expectRevert
  4) [current] expect().to.be.revertedWith

  0 passing (860ms)
  4 failing

  1) expectRevert:

      Wrong kind of exception received
      + expected - actual

      -Transaction reverted and Hardhat couldn't infer the reason.
      +Counter: decrement overflow
      
      at expectException (/home/roqs/node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at runNextTicks (node:internal/process/task_queues:65:3)
      at listOnTimeout (node:internal/timers:528:9)
      at processTimers (node:internal/timers:502:7)
      at expectRevert (/home/roqs/node_modules/@openzeppelin/test-helpers/src/expectRevert.js:75:3)
      at Context.<anonymous> (test/CountersTest.js:8:3)

  2) expect().to.be.revertedWith:
     AssertionError: Expected transaction to be reverted with "Counter: decrement overflow", but other reason was found: ""
  

  3) [current] expectRevert:

      Wrong kind of exception received
      + expected - actual

      -Transaction reverted and Hardhat couldn't infer the reason.
      +Counter: decrement overflow
      
      at expectException (/home/roqs/node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at runNextTicks (node:internal/process/task_queues:65:3)
      at listOnTimeout (node:internal/timers:528:9)
      at processTimers (node:internal/timers:502:7)
      at expectRevert (/home/roqs/node_modules/@openzeppelin/test-helpers/src/expectRevert.js:75:3)
      at Context.<anonymous> (test/CountersTest.js:18:3)

  4) [current] expect().to.be.revertedWith:
     AssertionError: Expected transaction to be reverted with "Counter: decrement overflow", but other reason was found: ""
  

Minimal reproduction steps

Run the script below adapted from #2453 (comment), changing the solidity compiler version to 0.8.19 and 0.8.12 to see the errors. Using the current latest solc version (0.8.19) all tests will fail as in the example above. Using the version 0.8.12 only the two tests that has current() defined will pass as mentioned in the comment on the issue #2453.

#!/bin/bash

SOLC_VERSION=0.8.19
SOLC_BINARY=solc-static-$SOLC_VERSION
TEST_DIR=hardhat-viair-test
mkdir -p $TEST_DIR/{test,contracts}

(
cd $TEST_DIR || exit

wget -c "https://github.com/ethereum/solidity/releases/download/v$SOLC_VERSION/solc-static-linux" -O "$SOLC_BINARY"
chmod +x "$SOLC_BINARY"

BINARY_PATH="$(pwd)/$SOLC_BINARY"
SOLC_VERSION_LONG=$("$BINARY_PATH" --version | tail -n 1 | sed -n -E 's/^Version: (.*)$/\1/p')

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

module.exports = {
    solidity: {
        compilers: [{
            version: "$SOLC_VERSION",
            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 == '$SOLC_VERSION', 'Unexpected solc version: ' + args.solcVersion)

    return {
        compilerPath: '$BINARY_PATH',
        isSolcJs: false,
        version: args.solcVersion,
        longVersion: '$SOLC_VERSION_LONG'
    }
})
EOF

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

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');
});

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

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

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

cat <<EOF > contracts/CountersImpl.sol
contract CountersImpl {
    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
)

Search terms

No response

[fvictorio]

Hey @r0qs, does this still happen if you set minimal optimizer steps, as explained here?

[cameel]

Good point. I suspect it might be working with minimal optimizer and if it does, it should not be an issue for regular users.

Our situation is a bit special, because being able to run tests with optimizer enabled in our CI is still useful. Especially for benchmarking the optimizer. And vast majority of tests do pass with optimization, we just have to manually disable the select ones that fail. We'll need to think what to do about it on our side going forward. I expected that they'll just work then you add support for via IR, but I should have realized that that's going to be the case only for unoptimized code :)

By the way, what is Hardhat's behavior going to be when a user does try to run tests with optimizer anyway? Will you leave it as is and just recommend not to do it? Or will you block it? Or maybe something in between, e.g. detect and skip misbehaving tests or ignore the optimizer flag and run unoptimized?

[fvictorio]

By the way, what is Hardhat's behavior going to be when a user does try to run tests with optimizer anyway? Will you leave it as is and just recommend not to do it?

Probably that, yes. We wouldn't block it because several other things continue to work, and people might prefer running their tests that way (and losing some Hardhat features).

[r0qs]

Thanks @fvictorio, adding only the UnusedPruner step to optimizerSteps fixes this issue indeed.

However, there are still some other cases in our external tests that use hardhat and still failing with the same error even when using only the suggested optimizer step. So maybe we can keep this issue open until we fix it all?

I couldn't find a minimal reproducible example yet, I will update this issue if I find some. But you can check some of the failed cases here: https://app.circleci.com/pipelines/github/r0qs/solidity/319/workflows/9c536705-b7d6-4e5f-958a-831d97a27288

For instance, this specific test from openzeppelin: https://app.circleci.com/pipelines/github/r0qs/solidity/319/workflows/9c536705-b7d6-4e5f-958a-831d97a27288/jobs/9088?invite=true#step-104-5087

1) Contract: TransparentUpgradeableProxy
       transparent proxy
         proxy admin cannot call delegated functions:

      Wrong kind of exception received
      + expected - actual

      -Transaction reverted and Hardhat couldn't infer the reason.
      +TransparentUpgradeableProxy: admin cannot fallback to proxy target
      
      at expectException (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:20:30)
      at runNextTicks (node:internal/process/task_queues:60:5)
      at processTimers (node:internal/timers:508:9)
      at expectRevert (node_modules/@openzeppelin/test-helpers/src/expectRevert.js:75:3)
      at Context.<anonymous> (test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js:327:7)

You can see the compiler settings used to run the tests here: https://app.circleci.com/pipelines/github/r0qs/solidity/319/workflows/9c536705-b7d6-4e5f-958a-831d97a27288/jobs/9088?invite=true#step-104-30

Settings preset: ir-optimize-evm+yul
Settings: {evmVersion: 'paris', viaIR: true,  optimizer: {enabled: true, details: {yul: true, yulDetails: { optimizerSteps: 'u' }}}}
EVM version: paris
Compiler version: 0.8.20
Compiler version (full): 0.8.20-ci.2023.3.10+commit.27beaa72.Linux.g++

[fvictorio]

Hey folks. First, sorry for taking so long to re-visit this, and thanks for the great reproduction info.

I feel like this is no longer happening, although I'm not 100% sure. This is what I tried:

• In a sample project, I tried again the repro given by @cameel in Revert reasons not detected in external calls with viaIR: true and optimizer enabled #2453 (comment). I should note that I used our new chai matchers, not hardhat-waffle, but I believe this shouldn't be relevant. (If it is, that would be a different issue.)
• I cloned the OpenZeppelin repo, upgraded Hardhat, configured the compiler with these settings, and ran the .TransparentUpgradeableProxy tests. They passed.

Hopefully I'm not missing anything here. If you have some other example of your CI failing because of Hardhat's viaIR support, please let me know.

(I'm going to tentatively close this issue for bookkeeping reasons, but I'll re-open it, or open a new one, if there is another example of this problem.)