-
Notifications
You must be signed in to change notification settings - Fork 151
test(zkevm): add coverage for CALLDATACOPY, CODECOPY, RETURNDATACOPY and MCOPY #1800
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
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.
These tests are fun, left some background notes and some suggestions because I think some tests are doing things they are not intended to.
In general, Op.MOD(Op.GAS, 7)
works only iff the difference of gas between each of these MOD
calls is not a multiple of 7. Otherwist the dst
is not dynamic and will be static. But not sure how to test/verify this, have to think about this. A solution for the attack loop could be to use both Op.MOD(Op.GAS, 7)
and Op.MOD(Op.GAS, 11)
(but this has the problem it still has a shorter cycle problem than we intend)
Co-authored-by: Jochem Brouwer <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Co-authored-by: Jochem Brouwer <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
@jochem-brouwer, I think we're ready for another review with many of the suggestions! To simplify your life re-reviewing, I added a comment on all new changes and resolved the old ones, so hopefully that helps. These opcodes are extremely tricky to get perfectly right (if we even know what that means 😄) , but I think we're in a good-ish place. |
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 went through all comments and (quickly) went over the file again - LGTM! Exciting tests juggling around with memory bytes 🤩
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
@marioevz, I think due to new branch protection rules I can't merge PRs anymore even with approvals (I'm fine with it!) -- so please take a look if all looks fine here for potential merging 🙏 |
🗒️ Description
This PR adds benchmark coverage for
CALLDATACOPY
,CODECOPY
,RETURNDATACOPY
andMCOPY
.Cycles:
🔗 Related Issues
#1690
✅ Checklist