-
Notifications
You must be signed in to change notification settings - Fork 169
feat(tests): add first few single-opcode test for state access in BloatNet #2040
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: Guillaume Ballet <[email protected]>
Signed-off-by: Guillaume Ballet <[email protected]>
Signed-off-by: Guillaume Ballet <[email protected]>
Signed-off-by: Guillaume Ballet <[email protected]>
Signed-off-by: Guillaume Ballet <[email protected]>
Signed-off-by: Guillaume Ballet <[email protected]> remove leftover single whitespace :|
b6cd62a
to
374e08a
Compare
Hello @gballet ! Thanks for adding this case. This is the issue tracker for bloatnet test cases, could you please help me (1) add the PR to the issue tracker PR description (like this) (2) link this PR to the issue, this would help us better track the progress, thank you! For benchmark test, we now add new cases under I also add some review below, please feel free to let me know if you have any issue! If you want some reference for benchmark test, maybe you can take a look at this This is a similar case for this benchmark! You can take a look at this structure! |
Signed-off-by: Guillaume Ballet <[email protected]>
Hey @LouisTsai-Csie thanks for the feedback.
I tried to do this, but this looks like it's very involved. I did my best effort but since I don't know what you're expecting, and also that I don't have all the time in the world, I'll leave it in your court to comment on that. #2064
if I do that, how do I run the test? it seems to ignore them after I moved it to the directory. I have pushed it to this PR for your consideration.
Thanks for the reference. |
@gballet Appologies. I forgot to link the issue tracker for you. We've created an issue tracker based on your documentation. I help you link this PR to the Also, it would be great if you can help me review if there is anything missing / wrong in our issue tracker! |
I'll need to have a closer look, but it seems fine as a first pass. Do you know what the problem is with moving my file to benchmarks? |
Our documentation is incomplete (I will fix them ASAP), for running the test, you will need to add a flag This is the command on our documentation:
But I would add some flag to run it:
Please let me know if there is anything unclear to you! |
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.
Hey @gballet. I did a first pass at this strictly just from setup. I didn't dig into the actual test case to make sure it's doing what we want it to do. I am going to take a deeper look at the logic.
I just wanted to add a bit more context on the
This allows us to test against the different gas limit values specified for the block, not transaction gas cap (1 = 1 Mgas). I am looking a bit deeper into the PR next but wanted to provide some better context. |
# with them. Only fill the block by a factor of SPEEDUP. | ||
SPEEDUP: int = 100 | ||
|
||
|
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.
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 yeah, good observation. The biggest question I suppose is whether those worst case scenarios are the same as the ones in the bloatnet doc here? If they are, maybe creating a marker on existing relevant tests, so we don't have to redefine them, would be a good approach. Then, instead of trying to run only the tests in this file, we could use a marker like -m bloatnet
and if we mark those existing, relevant, param
cases with this marker, they get included in all the tests you want to run. We could then mark this whole file (test_bloatnet.py
) with the appropriate marker for your use so that you get all the test cases relevant to you in one command, whether they are defined here or elsewhere.
Just a thought.
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.
Let's not conflate two different things together, the objectives are clearly different:
- BloatNet looks for the performance of regular execution
- the zk benchmarks are benchmarking zkevms, a widely different environment
Adding an extra coupling here is causing more work for no benefit, since these tests are maintained by different sets of people.
Regarding the worst case, the tests are doing different things since the code itself is different. The goal of the bloatnet test it to measure the sole performance of SSTORE
in client, whereas when I read the zkvm-specific code, it is doing extra stuff like jumps. It's normal, you wouldn't be able to load so much code as in our test inside a zkvm. But we can.
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.
@gballet, the tests in benchmark
are not specific to zkvms. (They are even used for PerfNet).
The test I linked are executing blocks where the full gas limit is used to do cold or warm reads, or to write slots to existent or non-existent storage slots. There's nothing specific to zkvms there, thus why I ask how these tests are different -- mainly to avoid duplication or explain better what different variant is trying to be benchmarked.
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 needs to be addressed before merging!
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 consider it has already been addressed by my first comment: it's not the same bytecode, not the same objectives (worst case vs perf), and not the same constraints. Ignacio's tests might not be specific to zkvms, but we do need something specific for us.
* refactor(tests): Proposed patch for bloatnet SSTORE tests * refactor(tests): Update tests from comments on PR PR: #1 Signed-off-by: fselmo <[email protected]> * Use parametrization of the value that is written to --------- Signed-off-by: fselmo <[email protected]> Co-authored-by: Guillaume Ballet <[email protected]>
Co-authored-by: felipe <[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.
There is some refactoring for the code, please help update them, thanks!
Let's wait for the answer of the execute
mode for bloatnet scenario.
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 comment that was making the test fail when using small --gas-benchmark-values
values.
Co-authored-by: θ‘δ½³θͺ Louis Tsai <[email protected]> Co-authored-by: Mario Vega <[email protected]>
ποΈ Description
Add a test required as part of the BloatNet effort. This is the
π Related Issues or PRs
Not an issue, but a test plan is described here.
β Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned@ported_from
marker.