Skip to content

Test some jump scenarios + baseline; log resource utilization #1528

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

codygunton
Copy link
Contributor

@codygunton codygunton commented May 2, 2025

🗒️ Description

Add a JUMP-heavy workload to see how zkVMs handle this.

JUMP-heavy test

Uses 99.94% gas. Cycle counts from the Reth fork:

[crates/zkvm-benchmarks/host/src/main.rs:57:9] &reports = [
    WorkloadMetrics {
        name: "tests/zkevm/test_worst_compute.py::test_worst_jumps[fork_Cancun-blockchain_test-gas_limit_36000000]-1",
        total_num_cycles: 700410104,
        region_cycles: {
            "read_input": 250977884,
            "validation": 449428292,
            "verify-witness": 361229,
        },
    },
]

JUMPDEST-heavy test

Uses 99.999% gas. Cycle counts:

[crates/zkvm-benchmarks/host/src/main.rs:57:9] &reports = [
    WorkloadMetrics {
        name: "tests/zkevm/test_worst_compute.py::test_worst_jumps[fork_Cancun-blockchain_test-gas_limit_36000000]-1",
        total_num_cycles: 1053221216,
        region_cycles: {
            "read_input": 250977898,
            "verify-witness": 543282,
            "validation": 802239390,
        },
    },
]

🔗 Related Issues

N/A

✅ Checklist

  • All: Set appropriate labels for the changes. <- I seem to not have an abillity to set these, else I would use the same as chore(tests): add zkevm marker for bls g1 add tests #1514
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md. <- I don't see precedents for adding individual tests.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65. <- Just using ExecutionSpecsTransitionTool
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

Copy link
Collaborator

@jsign jsign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts!

Comment on lines 250 to 251
# Use Op.JUMPDEST + Op.JUMP instead of raw bytes
opcodes_pattern = Op.JUMPDEST + Op.JUMP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, this isn't correct since the JUMPs are not built correctly so this will fail quite fast. See here to know how JUMP works.

You can also see the gist I shared yesterday. In that commented line, you can see how I make the jump destination of JUMP be the PC+x to target the JUMPDEST.

I think the first link I shared with you explains this correctly, but ping me via DM if you want to chat more about it or have any question!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I actually expected this to fail, but it didn't fail in the testing framework--I was able to generate the json fixture and execute in the Reth fork. Is that expected?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's expected for "everything to work", mainly because it would be a block execution that has tx that fail. That's something that could be intentional from the tester side.

Usually, tests have some side effects that can be detected with the postState assertions, so the test can fail because of that. Unfortunately, in all these test cases we don't do that. We could do some kind of "final SSTORE" and then check it as a postState assertion, but that feels it will complicate stuff a bit in the attack design and use some extra gas for that.

Might still be worth it though -- I don't like having to manually debug tests to be sure they're doing what we want.

Another strategy is having a way for the testing framework to check that the transaction doing the attack failed because of "out of gas reason" and not any other reason. I think with that I would be happy enough. In your case, this would be detected since the tx failure isn't for that reason.

cc @marioevz is something like this possible?

Copy link
Contributor Author

@codygunton codygunton May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using https://www.evm.codes/playground help me write a sensible test. Validated high gas usage in JSON artifact and validated passing test in Reth.

@kevaundray
Copy link
Contributor

At first, I think we want to have basic workloads, so a block filled with one particular opcode.

Once we have that baseline down, then it would be good to explore interesting usecases that take advantage of the particular configuration of zkEVMs and or respectively EVMs like the PR is doing.

@codygunton codygunton force-pushed the cg/jump-costs branch 6 times, most recently from 1fba8e3 to 28cccfc Compare May 15, 2025 20:21
@codygunton codygunton marked this pull request as ready for review May 15, 2025 20:52
@codygunton codygunton changed the base branch from main to fix-clean-coverage-ci-script May 15, 2025 20:53
@codygunton codygunton requested a review from jsign May 15, 2025 20:53
@marioevz marioevz deleted the branch ethereum:fix-clean-coverage-ci-script May 16, 2025 14:34
@marioevz marioevz closed this May 16, 2025
@marioevz
Copy link
Member

The PR was automatically closed because the branch it was pointing to was deleted, and I tried recreating the branch it was pointing to but Github simply does not let me reopen this PR.

Sorry about this issue, I think the only alternative is to create a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants