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

Fix gas expectations in the semantics tests #12225

Closed
ekpyron opened this issue Nov 3, 2021 · 7 comments · Fixed by #12282
Closed

Fix gas expectations in the semantics tests #12225

ekpyron opened this issue Nov 3, 2021 · 7 comments · Fixed by #12282

Comments

@ekpyron
Copy link
Member

ekpyron commented Nov 3, 2021

In #11974 I noticed that the tests seem to pass no matter what legacyOptimized gas costs were used for the deposit contract.
The expectation only got handled correctly with a value for --enforce-gas-cost-min-value 1 in isoltest and also soltest doesn't seem to fail on wrong expectation.

@hrkrshnn
Copy link
Member

hrkrshnn commented Nov 3, 2021

Maybe related: #11600

@ekpyron
Copy link
Member Author

ekpyron commented Nov 3, 2021

Maybe related: #11600

It indeed also was the constructor call in my case, so maybe a duplicate - but the conditions under which it happens seem a bit fuzzy.

@cameel
Copy link
Member

cameel commented Nov 3, 2021

Which tests was that? #11974 has a lot of them :)

--enforce-gas-cost-min-value defaults to 100000 so maybe that is the issue? Do we want to lower it? Or make soltest not ignore it and instead have isoltest remove the directive if it's below min value?

@ekpyron
Copy link
Member Author

ekpyron commented Nov 3, 2021

As I said: it was the constructor for the deposit contract.
The gas expectations for that seem to be incorrect on develop even.

Try running build/test/tools/isoltest -t sem*/ext*/deposit* --enforce-gas-cost-min-value 1 --optimize and/or
build/test/tools/isoltest -t sem*/ext*/deposit* --enforce-gas-cost-min-value 1 on develop.

Which means it's not only isoltest, but also soltest, since CI doesn't report the mismatch.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 3, 2021

Also in both cases the value is way beyond 100000.

@cameel
Copy link
Member

cameel commented Nov 3, 2021

ok, in that case it does sound like a bug.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 3, 2021

Actually it's also not only legacyOptimized, but also ìrOptimized (just didn't notice it since you need to update legacy before it's even run).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants