Revert due to overflow misdetected as CONTRACT_TOO_LARGE_ERROR on a contract compiled with viaIR: true

[cameel]

I'm trying to run OpenZeppelin tests using the new, experimental IR code generator of the Solidity compiler (viaIR: true) and I'm running into failures that do not happen when using the default code generator. I tracked it down to Hardhat's stack trace module misdetecting a revert caused by an integer overflow as CONTRACT_TOO_LARGE_ERROR.

I think that the most likely cause is that some the current heuristics make too strong assumptions about the output from the default code generator and need to be updated to handle the new one as well. I cannot also exclude the possibility that it's actually some kind of problem with compiler's output. Please check it out and let me know if there's actually something that needs to be fixed in the compiler.

Repro

Just OpenZeppelin tests with version pragmas stripped from contracts and custom compiler settings:

git clone https://github.com/OpenZeppelin/openzeppelin-contracts.git
cd openzeppelin-contracts

cat <<EOF >> hardhat.config.js
module.exports = {
    solidity: {
        version: '0.8.10',
        settings: {
            viaIR: true,
            optimizer: {
                enabled: true,
                runs: 200,
            },
        },
    },
    networks: {
        hardhat: {
            blockGasLimit: 10000000,
            allowUnlimitedContractSize: false,
        },
    },
};
EOF

npm install
find . test -name '*.sol' -type f -print0 | xargs -0 sed -i -E -e 's/pragma solidity [^;]+;/pragma solidity *;/'
npm test test/token/ERC20/extensions/ERC20FlashMint.test.js

Note: ERC20FlashMint.test.js is not the only test that fails - 57 out of 751 tests fail if you run the full test suite - but it's the one I investigated so far.

Output

Compiling 219 files with 0.8.10
Compilation finished successfully


  Contract: ERC20FlashMint
    maxFlashLoan
      ✓ token match
      ✓ token mismatch
    flashFee
      ✓ token match
      ✓ token mismatch (44ms)
    flashLoan
      ✓ success (63ms)
      ✓ missing return value (51ms)
      ✓ missing approval (39ms)
      ✓ unavailable funds (51ms)
(node:181971) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 60)
(node:181971) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
      1) more than maxFlashLoan


  8 passing (22s)
  1 failing

/tmp/openzeppelin-contracts/node_modules/hardhat/internal/hardhat-network/stack-traces/solidity-errors.js:108
    return new SolidityCallSite(sourceReference.file.sourceName, sourceReference.contract, sourceReference.function !== undefined
                                                ^

TypeError: Cannot read property 'file' of undefined
(Use `node --trace-uncaught ...` to show where the exception was thrown)

The failing test case

    it ('more than maxFlashLoan', async function () {
      const receiver = await ERC3156FlashBorrowerMock.new(true, true);
      const data = this.token.contract.methods.transfer(other, 10).encodeABI();
      // _mint overflow reverts using a panic code. No reason string.
      await expectRevert.unspecified(this.token.flashLoan(receiver.address, this.token.address, MAX_UINT256, data));
    });

[zoeyTM]

Hey @cameel , thanks for opening a ticket!

At first glance this does seem like a bug. I'm admittedly not extremely familiar with this part of our codebase yet, so let me look into this more later this week and talk with the team before I say anything for certain.

[Severno]

I had the same problem. In my case, it was because I was getting a negative value when I subtracted two numbers.

    function vote(uint256 _proposalId, uint256 _amount)
        external
        payable
        proposalInProgress(_proposalId)
        proposalExist(_proposalId)
        proposalIsOpened(_proposalId)
    {
        require(
            balances[msg.sender] - _amount > 0,
            "DAO: You have not enought tokens deposited for voting"
        );

        _distributeTokensForVoting(_proposalId, _amount);

        emit Voted(_proposalId, msg.sender, _amount);
    }

So I change require to:

      require(
            balances[msg.sender] > _amount,
            "DAO: You have not enought tokens deposited for voting"
        );

See if you got a negative number somewhere too.
It may help!

[cameel]

No, it definitely overflows. The test sets amount to MAX_UINT256 and both amount and fee are uint256. Any non-zero fee will cause an overflow at line 72 in ERC20FlashMint.sol:

require(currentAllowance >= amount + fee, "ERC20FlashMint: allowance does not allow refund");

But your case is interesting nevertheless. Were you using the IR-based codegen (i.e. viaIR: true)? If not, then it means that it's not only the new codegen that breaks this heuristic and this bug should get a higher priority.

In any case, thanks for the workaround - might help some people who run into this and find this issue - but the point here is to fix the problem at its core. It's not even my own code, it's OpenZeppelin :) and with the current heuristic it will stop passing the tests once we make the new codegen the default and deprecate the old one (which should happen some time next year).

[zoeyTM]

Hi @cameel , sorry for the delayed reply here.

While this is definitely a bug, and there are likely others that exist revolving around IR-based codegen, the fixes to fully support viaIR: true would be nontrivial at best. Do you have an expected timeline on when IR codegen will become stable? That would greatly help us in figuring out where exactly to prioritize this issue within our own timeline.

For the time being I'll label and add to our backlog.

Thanks!

[cameel]

Getting the IR codegen out is actually one of the big milestones we have planned for Q1 2022 so you can expect this to happen soon. It's already feature-complete and we're at the stage where we're testing it with bigger projects and tweaking the optimizer to ensure that it's as efficient or better than the current codegen. It likely won't become the default for some time yet - we need better support on the tooling side for that to happen.

This bug report is actually one of the things that came out of that testing. We have a test suite that compiles selected external projects and runs their tests as a canary to detect any unintended breaking changes in the compiler that would affect them. I'm currently adding viaIR runs for these projects and this bug is for example a blocker for the OpenZeppelin test. Most of the projects we want to test have switched to Hardhat recently so we have a bit of a chicken and egg problem here :)

[alcuadrado]

Thanks for the info @cameel! This gives us enough insights to start planning around it.

Something that would help to know: How much do you expect the code generation to change before it gets stable? And what about between it being stable and default?

We could start implementing this as an experimental feature, without any stability guarantee until it's the default. WDYT?

[cameel]

Great!

I don't think code generation will change very much. Most of the work now is on improving the optimizer, fixing bugs and dealing with "stack too deep" in the Yul-EVM transform. All the structure is already there and we're working within that structure rather than still extending it.

We actually consider it pretty stable already. It has no show-stopper bugs. No guarantees yet but, honestly, the remaining work is more about making it more efficient than making it work at all. From what I have seen so far compiling various projects, the only real problem is the "stack too deep" error. It does happen more frequently and some projects cannot compile via IR at all due to this at the moment (see ethereum/solidity#12343). But the fix for that is our new Yul->EVM transform and dealing with it only requires some incremental tweaks. Other than that, in projects that did not have this problem I haven't detected any breakage or tests failing with the new codegen and still passing with the old one. Except for the ones where viaIR breaks heuristics of course.

We'd like to see it as default as soon as possible but I think we'll have to wait for the tooling to catch up. Otherwise that will be a bad experience for users. We'll definitely need to coordinate that somehow. This would be a good topic to discuss on the tooling channel.

Making it experimental sounds good to me. It's still marked as experimental in the compiler as well. We're not yet at the point where users actually want that in production so we really only need enough support to be able to test it in a more complete configuration with both the compiler and the framework.

[ekpyron]

I have little knowledge about how the hardhat heuristics for these cases works in general. Is there any documentation about it or can you quickly explain it?

I'm asking on the one hand because, while via-IR-codegen is still not the default, it is no longer experimental, i.e. it's considered "stable". On the other hand, while IR code generation is not likely to change significantly anymore at this point, depending on how these heuristics work, I could very well imagine that they generally cannot be reproduced reliably for optimized code generated via IR: the optimizer has significantly more freedom in inlining and moving statements around, eliminating memory operations, dynamically generating stack layouts, moving variables to memory at any point, etc. pp.

So I could imagine that the only viable and really reliable way to "fix" these heuristics would for the solidity compiler to emit specific debug data that can be used to replace the existing heuristics.

On the other hand, since I'm not entirely sure how these heuristics currently work, this is more of a guess...

[ekpyron]

Just to emphasize this again: we're basically waiting for tooling like hardhat to tell us what specific debugging information they need from the solidity compiler to avoid these kinds of issues and make life easier for everybody :-).
So in case you're trying to fix the present and future via-IR issues with hardhat heuristics, please reach out to us on this :-).

[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.

[clemsos]

I am getting a similar error but the viaIR isnt enabled in my hardhat config. Any idea how to debug the root cause?
Many thanks !

/home/unlock/node_modules/hardhat/internal/hardhat-network/stack-traces/solidity-errors.js:108
    return new SolidityCallSite(sourceReference.file.sourceName, sourceReference.contract, sourceReference.function !== undefined

TypeError: Cannot read properties of undefined (reading 'sourceName')

[alcuadrado]

Hey @clemsos, are you using the latest version? I believe @fvictorio fixed that.

[clemsos]

yes it seems fixed now

[fvictorio]

Closing this in favor of #3365.