-
Notifications
You must be signed in to change notification settings - Fork 162
feat(benchmark): add log opcode worst case #1745
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
Conversation
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.
This test looks fine to me, it tests all LOG opcodes and runs those all as much as possible. However, I think we should also add these situations:
- Log data length is nonzero
- Log data length + data is nonzero (here we have to initialize the memory to something non-zero)
- The topics are nonzero
I am not sure because I am not deep into zkEVM, is calculating the logsBloom
part of the scope of the EVM (and would thus add extra logic/cycles) @jsign? If this is indeed part of it (either logsBloom
or calculating receiptTrie
or both) then those tests should be added for zkEVM (can be done in a different PR).
Those tests are also nice benchmark tests for non-zkEVM also 😄 👍
Will not explicitly approve for now.
Thanks for comment, I do not know |
Yep, calculating |
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.
Looking good! Reg @jochem-brouwer suggestion to cover non-zero scenarios (which I agree are useful), I'll let you decide if those might be worked in this PR or in a separate one -- for now leaving an approval if it is the latter.
Just left one comment for your consideration.
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.
LGTM, just one suggestion that is in line with the comments from previous reviewers, thanks!
For the mypy error, I think if you rebase to main it should be gone, I just fixed that in another PR. |
ff3440e
to
d542a92
Compare
d542a92
to
3853835
Compare
@jsign @jochem-brouwer I’ve updated the test case and added the following scenarios:
Could you please help me review again? And i am asking why the CI is failing |
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.
Thanks! Just some minor comments.
I think we can move this new test function into a new file, perhaps tests/zkevm/test_worst_opcode.py
?
We can start by moving only this new function for now in this PR, I have an idea on how to fix the coverage script and we can move the rest once that's fixed.
c4a0cdc
to
980342e
Compare
@marioevz Thanks for review. My original problem for evmone coverage report is that the CI never ends, but after I rebase again, the issue changed. After checking the coverage report, the root cause is from BLS signature, which has nothing to do with this benchmark. I move the test case to @jsign @jochem-brouwer could you please also review this benchmark, let me know if I miss any cases! |
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.
Looks good, I left some comments for your consideration.
1510245
to
7fa06ab
Compare
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.
Fixed the coverage issue by rebasing on top of main again, so we can address @jsign comments and then proceed to merge.
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.
LGTM, I think there's only one easy detail left but leaving it approved.
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.
LGTM, thanks!
Just some minor nit-picks.
Thanks @marioevz |
🗒️ Description
Create Benchmark for
LOG
OpcodeThis PR introduces a benchmark for the
LOG
opcode in the EVM. TheLOG
operation includes multiple fields:offset
,size
, and up toN
topics depending on the variant (LOG0 through LOG4).Gas Cost Calculation
Using
LOG0
as an example, the gas cost for theLOG
operation is computed as:375
dynamic_gas = 375 * topic_count + 8 * size + memory_expansion_cost
To minimize gas costs:
PUSH0
(which costs 2 bytes) for pushing values to the stack, as it’s cheaper than other operation likePUSH1
(3 bytes with an immediate) orDUP1
(3 bytes).Using
PUSH0
is also more space-efficient, since it avoids including immediate values and reduces bytecode size.Benchmark Bytecode Pattern
A minimal loopable pattern for benchmarking LOGN looks like this:
Using
PUSH0
is also more space-efficient, since it avoids including immediate values and reduces bytecode size. This makes it preferable overDUP1
for setting up the stack for topics or memory offset/size.🔗 Related Issues
Issue #1689
✅ Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.