-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Make Cancun the default EVM version #14907
Conversation
c4aab79
to
d92f540
Compare
Gas diff:
|
test/libsolidity/semanticTests/inlineAssembly/mcopy_as_identifier_pre_cancun.sol
Outdated
Show resolved
Hide resolved
// tstoreNotAllowedStaticCall() -> true | ||
// gas irOptimized: 98419720 | ||
// gas irOptimized code: 19000 | ||
// gas legacy: 98409086 | ||
// gas legacy code: 30000 | ||
// gas legacyOptimized: 98420962 | ||
// gas legacyOptimized code: 17800 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... the code cost here seems surprisingly low. I'd expect the deployment cost to be the biggest factor here. Given 200 gas per byte, this means code size of 85 bytes, 150 bytes and 89 bytes, respectively. So I guess it's plausible. Maybe it's just the other cost that seems off. How is this function eating almost 100 million gas? It does not loop or do anything that heavy except for the contract deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed that. I will try to check where exactly it is getting that high cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. This looks highly suspicious so it would be great if we knew what's happening and if something is broken.
But that's just in case. This is not a blocker for the PR itself. I think it's unlikely that it's caused by it and it's probably that we're just seeing the effects of something else here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that 100000000 * 63 / 64 = 98437500
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! And 100000000 is the gas limit in our tests:
solidity/test/ExecutionFramework.h
Line 275 in cc79c91
u256 const InitialGas = 100000000; |
So mystery solved. Looks like a failed staticcall
just eats all gas forwarded to it. I did not consider that this might happen in cases other than hitting an invalid opcode. Or is it simply because state-mutating opcodes are considered invalid within a staticcall
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it just hits an invalid opcode and thus eats up all gas forwarded to it. (The same should happen for an sstore)
968d203
to
1396e78
Compare
Overall, the PR looks fine and the only blocker are the failing external tests. |
1396e78
to
59b3530
Compare
|
8921505
to
c8551da
Compare
c8551da
to
a0d7615
Compare
a0d7615
to
a388ccb
Compare
Note that this now contains the workaround from #14933 so they'll go in together and CI should pass. |
Closes #14905.