search

NomicFoundation / hardhat

Hardhat is a development environment to compile, deploy, test, and debug your Ethereum software.
https://hardhat.org
Other
7k stars 1.35k forks source link

Stack trace tests expectations failing upon introducing new optimization in the Solidity compiler #5443

Closed matheusaaguiar closed 3 days ago

matheusaaguiar commented 1 week ago

I am working on this PR which improves and introduces new optimizations to the Solidity compiler and I have run into some failing stack traces tests. For context, we run hardhat's tests (among other external tools) in our CI for better coverage. The failing tests' log can be seen here. I am convinced that those failures are false positives and due to a mismatch between expectations and different generated code which is not accounted for. In short, with the changes introduced by the aforementioned PR, the compiler can produce further optimized code which produces different stack traces from the expected in the failing tests. See the log above. In the particular case of the first failure in that log, for example, the call to function test with param value false is expected to produce a revert error from a require function (line 6 of the c.sol file in the same folder): https://github.com/NomicFoundation/hardhat/blob/53c422c042a527930cae6b5d8cb748bbdbe51b44/packages/hardhat-core/test/internal/hardhat-network/stack-traces/test-files/0_8/revert-without-message/modifiers/call-message/multiple-modifiers-require/test.json#L21-L28

File c.sol: https://github.com/NomicFoundation/hardhat/blob/53c422c042a527930cae6b5d8cb748bbdbe51b44/packages/hardhat-core/test/internal/hardhat-network/stack-traces/test-files/0_8/revert-without-message/modifiers/call-message/multiple-modifiers-require/c.sol#L1-L18

However, with the optimizations introduced by the PR, the code for that is optimized out (which makes sense since test always reverts). I explained how that happens in detail in this comment. The other cases are similar situations where there is a mismatch between the expected stack trace and the new code generated by this PR. I am not sure if it is possible to change those expectations and how that could be done. What is the preferred way to handle this inconsistency?

Sorry if this is lacking more information. Please feel free to request anything I might have forgotten.

alcuadrado commented 1 week ago

Thanks for opening this issue. I see how this test can be problematic. Would it still fail if we removed the unconditional revert() from the function?

Can you provide a list of the failing cases?

Ideally, we change the tests so that they cover both versions. But if we need to, we can specify that some tests should only be run with certain versions of solc.

matheusaaguiar commented 1 week ago

Would it still fail if we removed the unconditional revert() from the function?

No, in that case, the pattern which triggers the new optimization method is no longer present and then it works as expected.

Can you provide a list of the failing cases?

Starting from this point: packages/hardhat-core/test/internal/hardhat-network/stack-traces/test-files/0_8/revert-without-message/ These are the failing tests:

Ideally, we change the tests so that they cover both versions. But if we need to, we can specify that some tests should only be run with certain versions of solc.

The new optimizations will affect only versions after it is introduced, so it could be covered in a specific set of tests.

alcuadrado commented 6 days ago

Thanks @matheusaaguiar! I think our tests shouldn't unconditionally revert() and expect things not to be optimized out.

@fvictorio should we fix these tests here or in the EDR repository?

@matheusaaguiar, we moved our network simulation to Rust and now lives in this repository. The tracing logic isn't fully merged, but we are working on it. Your integration with our test suite will need to be updated. We can provide guidance about it.

matheusaaguiar commented 4 days ago

Your integration with our test suite will need to be updated. We can provide guidance about it.

Thanks for letting us know, @alcuadrado. We will look into that and appreciate any guidance.

fvictorio commented 3 days ago

I opened https://github.com/NomicFoundation/hardhat/pull/5467 which fixes those six test cases. But when I ran this locally (using this build), I got 28 more failures, most of them related to console logs. I don't know why those don't happen in Circle though.

I then added 0.8.25 to the test suite, and got those same 28 failures. So I think it's unrelated to the 0.8.27 optimizations, and should be fixed as part of adding support for 0.8.25 and 0.8.26.

@fvictorio should we fix these tests here or in the EDR repository?

Let's do both for now, it's the simplest thing. After the solc CI moves to using EDR, we can stop updating the tests in the Hardhat repo.

fvictorio commented 3 days ago

I don't know why those don't happen in Circle though.

Ok, I think the reason is this:

https://github.com/ethereum/solidity/blob/8c6fed720367825718ffeed01218fb3756a796aa/.circleci/config.yml#L1440

Using shanghai as the target hardfork makes those extra tests fail (I only checked 1 of the 28, but it seems like a reasonable explanation, since it's the only difference in how the tests are executed).

matheusaaguiar commented 2 days ago

Thanks everyone! When is the new network simulation expected to be deployed? Are there any available resources/guides that we can check in preparation for this change?

matheusaaguiar commented 2 days ago

I don't know why those don't happen in Circle though.

Ok, I think the reason is this:

https://github.com/ethereum/solidity/blob/8c6fed720367825718ffeed01218fb3756a796aa/.circleci/config.yml#L1440

Using shanghai as the target hardfork makes those extra tests fail (I only checked 1 of the 28, but it seems like a reasonable explanation, since it's the only difference in how the tests are executed).

That's a bit confusing, since we have to set shanghai there in order to not have those 28 tests fail (assuming they are the same 28 here). Maybe something different in the settings run locally?

fvictorio commented 2 days ago

That's a bit confusing, since we have to set shanghai there in order to not have those 28 tests fail (assuming they are the same 28 here). Maybe something different in the settings run locally?

Sorry, maybe I said it backwards. But I meant exactly that: that our tests don't pass with solc 0.8.26 by default. I was surprised that your CI hadn't catched that, and then I saw it was changing the hardfork, which explains it. When we add support for 0.8.26/0.8.27 those tests should pass, and hopefully you can remove that sed workaround.

matheusaaguiar commented 2 days ago

Ah sorry, actually I had that backwards :sweat_smile: You were right from the beginning, shangai makes the tests fail and we have to set it to cancun.