ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.48k stars 5.81k forks source link

Incorrect handling of revert when `viaIR = true` #14244

Open barakman opened 1 year ago

barakman commented 1 year ago

Description

Calling a function which takes no input arguments, under a scenario in which the function should revert with a message:

Environment

I have tested this problem separately with hardhat versions 2.12.7 and 2.14.0. The reproduction below uses the 'truffle5' plugin, but it also works without it.

Steps to Reproduce

pragma solidity 0.8.19;

contract MyContract {
    bool private alreadyCalled;

    function initialize() external {
        require(!alreadyCalled, "already called");
        alreadyCalled = true;
    }
}
const MyContract = artifacts.require("MyContract");

contract("MyContract", () => {
    it("Test", async () => {
        const myContract = await MyContract.new();

        await myContract.initialize();
        try {
            await myContract.initialize();
        } catch (error) {
            console.log(error.message);
        }
    });
});

Outcome:

+---------+-------+-----------------------------------------------------------------------------------------+
| HardHat | viaIR | Printout                                                                                |
+---------+-------+-----------------------------------------------------------------------------------------+
| 2.12.7  | false | VM Exception while processing transaction: reverted with reason string 'already called' |
+---------+-------+-----------------------------------------------------------------------------------------+
| 2.12.7  | true  | Transaction reverted: function was called with incorrect parameters                     |
+---------+-------+-----------------------------------------------------------------------------------------+
| 2.14.0  | false | VM Exception while processing transaction: reverted with reason string 'already called' |
+---------+-------+-----------------------------------------------------------------------------------------+
| 2.14.0  | true  | Transaction reverted and Hardhat couldn't infer the reason                              |
+---------+-------+-----------------------------------------------------------------------------------------+

Strangely enough, adding a console.log inside the contract function solves the problem.

This is possibly a HardHat issue, so I shall post it there too.

Please find the project configuration below.

File package.json:

{
    "scripts": {
        "build": "hardhat compile",
        "test": "hardhat test --bail"
    },
    "devDependencies": {
        "@nomiclabs/hardhat-truffle5": "2.0.7",
        "@nomiclabs/hardhat-web3": "2.0.0",
        "hardhat": "2.12.7"
    }
}

File hardhat.config.js:

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

module.exports = {
    solidity: {
        version: "0.8.19",
        settings: {
            viaIR: true,
            optimizer: {
                enabled: true,
                runs: 200
            }
        }
    },
    paths: {
        sources: "./project/contracts",
        tests: "./project/tests",
        cache: "./project/cache",
        artifacts: "./project/artifacts"
    }
};

Thanks :)

hrkrshnn commented 1 year ago

@barakman could you please confirm with a different framework? The viaIR in Hardhat is a bit flaky at times, especially with revert messages.

cameel commented 1 year ago

This looks like https://github.com/NomicFoundation/hardhat/issues/2453 and https://github.com/NomicFoundation/hardhat/issues/3750. See especially https://github.com/NomicFoundation/hardhat/issues/3750#issuecomment-1462483527.

The tl;dr is that you should be running your tests with optimizer disabled if you need Hardhat to properly detect revert reasons when using viaIR: true. It's because Hardhat uses heuristics to detect those reasons and they're not reliable with optimized code generated by the the IR pipeline.

Not sure about other frameworks but I would not be suprised if they either have similar limitations or just don't support checking revert reasons. This will only really be solved properly when we get the ethdebug format specified and implemented in the compiler so that debuggers and frameworks can stop relying on heuristics.

SevenSwen commented 1 year ago

We managed to avoid a similar problem when calling populateTransaction: https://github.com/1inch/limit-order-protocol/blob/master/test/RangeAmountCalculator.js#L23

hardhat 2.14.0 / @nomiclabs/hardhat-ethers

Before using populateTransaction we had received "without reason" error too.