Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
matheusaaguiar opened this issue Jun 27, 2024 · 10 comments · Fixed by #5467
Assignees

Comments

@matheusaaguiar
Copy link

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):

{
"type": "REVERT_ERROR",
"sourceReference": {
"contract": "C",
"file": "c.sol",
"function": "m2",
"line": 6
}

File c.sol:

pragma solidity ^0.8.0;
contract C {
modifier m2(bool b) {
require(b);
_;
}
function test(bool b) m1(b) m2(b) public {
revert();
}
modifier m1(bool b) {
_;
}
}

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
Copy link
Member

alcuadrado commented Jun 28, 2024

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
Copy link
Author

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:

  • modifiers/call-message/multiple-modifiers-require/
  • modifiers/create-message/multiple-modifiers-require/
  • revert-without-message/within-receive/between-statements
  • revert-without-message/within-receive/no-other-statements
  • revert-without-message/within-receive/statement-after
  • revert-without-message/within-receive/statement-before

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

I opened #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
Copy link
Member

fvictorio commented Jul 2, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants