-
Notifications
You must be signed in to change notification settings - Fork 358
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
[EVM-Equivalence-YUL] Fix compiler tester bugs #527
Merged
jrchatruc
merged 22 commits into
matter-labs:evm-equivalence-yul
from
lambdaclass:fix-compiler-tester-bugs
Jul 5, 2024
Merged
[EVM-Equivalence-YUL] Fix compiler tester bugs #527
jrchatruc
merged 22 commits into
matter-labs:evm-equivalence-yul
from
lambdaclass:fix-compiler-tester-bugs
Jul 5, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrchatruc
requested review from
vladbochok and
StanislavBreadless
as code owners
June 19, 2024 14:59
jrchatruc
merged commit Jul 5, 2024
e730132
into
matter-labs:evm-equivalence-yul
15 of 16 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What ❔
This PR fixes bugs found using era-compiler-tester, most of them related to overflow checks.
A full description of the bugs and their fixes below.
Empty Return Data
We were not returning gas on reverts, which made a lot of tests fail on a check by the
compiler-tester
after execution where it tried to retrieve gas used from the return data and it was empty. We added gas on reverts so now those errors are no longer there.mcopy
Some tests fail because compiling them with the
solc
version used by the project, they internally generatemcopy
opcodes, which we do not support because we target Shanghai, not Cancun. We added a basic implementation ofmcopy
that makes tests pass, though it's very inefficient since there is nomcopy
opcode on the zkEVM.The
mcopy
tests are the following:returndatacopy
with size less than expectedSome solidity programs (especially those using
try/catch
statements) pass a certainsize
when performing a call, but if execution reverts the actual return data size can be smaller thansize
. We were not handling that correctly when copying the return data; now the behaviour correctly mimics the EVM one: when saving the return data from a call we copy the minimum amount of bytes between the one requested and the return data size. You can check out thegeth
code for that here.Tests that were failing because of this were:
Truncated addresses
The following test
failed because we were not truncating addresses to
bytes20
before calling contracts on the call opcodes.mstore8/mstore
and return data too longThese two tests try to store a value to a memory location far exceeding the maximum the interpreter allows (
0x1101e0
).This test:
fails because the return data tries to allocate more memory than is possible on the interpreter.
These three tests are the only ones that are not yet fixed, but we believe in the context of the interpreter it makes sense that they do not pass. It might be a good idea to skip them.
Overflow checks
All the other tests failed because of missing overflow checks on opcodes. They were fixed. Note that for those opcodes their implementation becomes a bit more expensive. For the special case of
keccak
opcode, we had to add some overflow checks before actually callingkeccak
only to be able to insert the gas left on reverts, because the opcode does not do it on its own.Why ❔
Checklist