Skip to content

chore(levm): remove the revm comparison - #7090

Open
edg-l wants to merge 2 commits into
mainfrom
chore/remove-revm-comparison-bench
Open

chore(levm): remove the revm comparison#7090
edg-l wants to merge 2 commits into
mainfrom
chore/remove-revm-comparison-bench

Conversation

@edg-l

@edg-l edg-l commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Motivation

Removes both halves of the levm-vs-revm comparison: the microbenchmark crate and the EF state-test re-run. Neither is used, and EELS fixtures are the authoritative oracle for correctness, as benchmarkoor and newPayload latency are for performance.

The microbenchmark is unowned and its output is noise.

  • Last intentional change was 2026-03-02 (refactor(l2): use decode_hex utility in calldata module #5807, a repo-wide decode_hex refactor). Everything since is a version bump, an actions bump, or a compile-fix from someone changing an API it happened to call.
  • Perf work moved to benchmarkoor and newPayload latency, which measure what users feel. A synthetic loop timed on a shared 2-core runner does not.
  • tooling/ef_tests/state/Makefile's flamegraph-benchmarks and samply-benchmarks referenced REPETITIONS and BENCH_*, which that file never defines, so they were passing empty iteration counts. Dead, not just unused.
  • The bench job in pr-main_levm.yaml built revm and levm in release on every levm PR and every main push, then uploaded nothing, posted nothing and gated nothing.

The EF state-test re-run never runs, and would be misleading if it did.

  • Every CI and standard target passes --summary (run-evm-ef-tests-ci, test-levm, test-levm-nostd-crypto), and run_ef_tests returns on the summary path before reaching re_run_with_revm. The re-run only fired locally, after a test had already failed, to annotate a report.
  • revm_runner.rs mapped Fork::Amsterdam to SpecId::OSAKA "until revm adds AMSTERDAM SpecId", so on the fork under active development every mismatch it reported was comparing two different rulesets.
  • It taxes feature work: feat(levm): implement EIP-8141 frame transactions #6326 (EIP-8141 frame transactions) and perf(levm): pad bytecode with 33 STOPs to remove hot-path bounds checks #6866 (bytecode padding) both had to touch revm_runner.rs purely to keep it compiling.
  • Nobody reads the mismatch report. The fixtures carry the expected post-state, so revm was a second opinion, not the verdict.

Description

Benchmark:

  • crates/vm/levm/bench/revm_comparison/, including its separate Cargo.lock
  • the root Cargo.toml exclude entry, which existed only for this crate
  • its cargo tree / cargo metadata --locked lines in the root Makefile, and its lockfile entry in docs/developers/release-process.md
  • .github/workflows/pr_perf_levm.yaml and .github/scripts/compare_levm_and_revm_benchmarks.sh
  • the bench job in pr-main_levm.yaml
  • the revm-comparison, build-revm-comparison, render-benches, flamegraph-levm-* and samply-run-bench targets in crates/vm/levm/Makefile, and the bench flamegraph/samply targets in tooling/ef_tests/state/Makefile

EF state tests:

  • runner/revm_runner.rs and runner/revm_db.rs, the --revm flag, run_with_revm and re_run_with_revm
  • ComparisonReport, TestReRunReport, and the mismatch sections of the report Display
  • the report cache (report::cache, report::load): only the revm re-run ever wrote it, and with no writer left a stale levm_ef_tests_cache.json would silently skip a whole run
  • EFTestRunnerError::VMExecutionMismatch and InternalError::ReRunInternal, both unreachable without the revm path
  • the revm dependency, which takes 454 lines out of tooling/Cargo.lock
  • the test-revm target and the revm halves of flamegraph-run-ef-tests / samply-run-ef-tests

load_initial_state_revm became load_initial_state_store, since its only remaining caller (post_state_root) discarded the RevmState and used just the store and block hash.

levm EF test profiling (flamegraph-run-ef-tests, samply-run-ef-tests) is untouched.

Verified: cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --check clean in the tooling workspace, make check-cargo-lock passes, and both Makefiles still parse.

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync. N/A: deletion only.

Unowned since 2026-03-02 and superseded by benchmarkoor and the newPayload
latency metrics. Removes the crate, its lockfile plumbing, the two CI jobs and
the profiling targets that drove it.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Stateless (zkEVM) Amsterdam+ EF tests skipped

Where: tooling/ef_tests/blockchain/test_runner.rsparse_and_execute skips
fixtures with network >= Fork::Amsterdam when running with a stateless backend.
Affects make test-stateless (the vectors_zkevm/ run); make test-levm is
unaffected.

Why: The stateless run uses the tests-zkevm@v0.5.0 bundle, filled against
glamsterdam-devnet v6.1.0, which predeploys the EIP-8282 builder deposit/exit
contracts at the OLD addresses (0x0000884d…d9008282 / 0x000014574a…0f008282).
This client uses the devnet-7 addresses (0x0000bff4…300d8282 /
0x000064d6…800e8282, matching the live tests-glamsterdam-devnet@v7.2.0 bundle
used by make test-levm). Every Amsterdam+ block runs the end-of-block EIP-8282
builder system call; with the new addresses absent from the v0.5.0 bundle, each
stateless Amsterdam+ block fails with
SystemContractCallFailed("System contract: 0x0000…8282 has no code after deployment").
The skip is by fork rather than by test name, since cross-fork directories such as
for_amsterdam/prague/... still execute at the Amsterdam fork.

Removal: Delete the skip_stateless_amsterdam branch in parse_and_execute
once a tests-zkevm bundle filled with the devnet-7 builder predeploy addresses is
released and .fixtures_url_zkevm is bumped to it.

@github-actions github-actions Bot added the levm Lambda EVM implementation label Aug 3, 2026
@edg-l
edg-l marked this pull request as ready for review August 3, 2026 10:18
@edg-l
edg-l requested a review from a team as a code owner August 3, 2026 10:18
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

This confirms the PR description's claim: revm remains as a direct dependency of tooling/ef_tests/state for the differential oracle, untouched by this deletion.

Review: PR #7090 — chore(levm): remove the revm comparison benchmark

This is a clean, well-scoped deletion PR. I verified the removal is complete and non-destructive:

Verified correctness of the deletion:

  • No dangling references to revm_comparison, revm-comparison, or the removed Make targets (render-benches, flamegraph-levm-*, samply-run-bench, flamegraph-benchmarks, samply-benchmarks, BENCH_REPS, etc.) remain anywhere in the repo (Makefiles, workflows, docs).
  • The root Cargo.toml exclude entry was removed along with the crate it referenced, and the workspace members list is untouched — no orphaned member paths.
  • Root Cargo.lock has zero references to revm_comparison.
  • tooling/ef_tests/state/Cargo.toml still depends on revm = "27.0.3" directly, confirming the PR's claim that the differential EF-test oracle is independent of the deleted crate and remains intact.
  • .github/workflows/pr-main_levm.yaml and tooling/ef_tests/state/Makefile end cleanly after the removed sections — no truncated targets or broken .PHONY declarations (the .PHONY line in crates/vm/levm/Makefile was correctly updated to drop the removed target names).
  • pr_perf_blocks_exec.yaml, a separate unrelated benchmark workflow (block execution vs. main), is untouched, as expected.

Assessment:

  • The stated justification (unowned since 2026-03-02, superseded by benchmarkoor/newPayload latency metrics, and the EF-tests Makefile already had dead references to undefined REPETITIONS/BENCH_* vars) is a reasonable, low-risk case for deletion.
  • This carries no consensus, gas-accounting, or state-trie implications — it's tooling/CI removal only, and revm remains available for correctness differential testing.
  • No correctness, security, or performance concerns: this is a pure subtraction with no surviving references.

I have no findings to raise — the PR does what it says and the deletion is complete and consistent.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

Removes the unused LEVM-versus-REVM benchmark infrastructure and the dormant REVM rerun path from EF state testing.

  • Deletes the standalone comparison crate, benchmark workflow, scripts, and Makefile targets.
  • Simplifies EF test execution and reporting to use LEVM and fixture post-state expectations directly.
  • Removes REVM-specific dependencies, database adapters, report structures, cache handling, and lockfile entries.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
tooling/ef_tests/state/runner/mod.rs Simplifies EF test orchestration by removing REVM-only execution, rerun, cache, and CLI paths.
tooling/ef_tests/state/report.rs Removes REVM comparison data and output while preserving LEVM test summaries and failure reporting.
tooling/ef_tests/state/utils.rs Replaces the REVM-oriented initial-state helper with a narrower genesis-backed store helper.
tooling/ef_tests/state/Makefile Removes REVM test and profiling targets while retaining LEVM EF-test profiling.
crates/vm/levm/Makefile Removes targets and configuration belonging to the deleted synthetic benchmark crate.
.github/workflows/pr-main_levm.yaml Removes the non-gating LEVM-versus-REVM benchmark job.
tooling/ef_tests/state/Cargo.toml Removes the REVM dependency after all REVM-specific code paths were deleted.

Reviews (2): Last reviewed commit: "chore(ef_tests): remove the revm compari..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

This PR is a straight removal of LEVM-vs-REVM benchmark tooling and its CI wiring, not a change to execution, consensus, trie, RLP, or transaction-validation paths. I checked the touched integration points in .github/workflows/pr-main_levm.yaml, Cargo.toml, Makefile, crates/vm/levm/Makefile, and tooling/ef_tests/state/Makefile, and I did not find stale references to the deleted revm_comparison crate or removed make targets.

Residual risk is limited to process/observability: this drops benchmark automation and local benchmark helpers, so future LEVM performance regressions will be less visible unless replaced elsewhere. I don’t see a correctness or security problem in the code that remains.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

⚠️ Warning: Diff was truncated from 13157 to 10000 lines. Some changes were not reviewed.

This PR removes the LEVM vs REVM comparison benchmark suite. The changes are primarily deletions of CI configuration, Makefiles, and the benchmark crate itself.

Summary of changes:

  • Deletes .github/workflows/pr_perf_levm.yaml and the benchmark comparison script
  • Removes the bench job from .github/workflows/pr-main_levm.yaml (lines 228-250)
  • Removes the revm_comparison crate exclusion from root Cargo.toml
  • Removes benchmark-related targets from crates/vm/levm/Makefile (all run_benchmark, revm-comparison, flamegraph-* targets)
  • Deletes the entire crates/vm/levm/bench/revm_comparison/ directory including Solidity contracts and Rust sources

Review feedback:

The deletion appears complete and consistent across all configuration files. No dangling references remain.

Minor note: In crates/vm/levm/Makefile, the .PHONY declaration on line 1 was updated to remove render-benches and samply-run-bench targets, which is correct since those targets are being removed.

No security vulnerabilities, memory safety issues, or consensus-critical code changes are present in this PR (it's purely a cleanup/removal of testing infrastructure).

The PR is ready for merge assuming the removal of these benchmarks is intentional from a product perspective.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@edg-l edg-l moved this to In Review in ethrex_l1 Aug 3, 2026
The levm-vs-revm re-run is skipped by every CI and standard target (all pass
--summary, which returns before it), and it maps Amsterdam to SpecId::OSAKA, so
its mismatches are uninterpretable on the fork under development. EELS fixtures
are the authority. Drops the revm dependency, the runner, the comparison report
and the report cache that only that path wrote.
@edg-l edg-l changed the title chore(levm): remove the revm comparison benchmark chore(levm,ef_tests): remove the revm comparison Aug 3, 2026
@edg-l
edg-l marked this pull request as draft August 3, 2026 10:28
@edg-l
edg-l marked this pull request as ready for review August 3, 2026 10:39
@edg-l edg-l changed the title chore(levm,ef_tests): remove the revm comparison chore(levm): remove the revm comparison Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

⚠️ Warning: Diff was truncated from 16054 to 10000 lines. Some changes were not reviewed.

This PR removes the LEVM vs REVM comparison benchmarking suite. The changes are consistent and complete:

Review Summary:

  1. Completeness of Removal: All components of the benchmark suite are properly removed:

    • GitHub Actions workflows (.github/workflows/pr_perf_levm.yaml and the bench job in pr-main_levm.yaml)
    • Benchmark scripts (.github/scripts/compare_levm_and_revm_benchmarks.sh)
    • The entire crates/vm/levm/bench/revm_comparison/ directory (Cargo.toml, Cargo.lock, source files, and Solidity contracts)
    • References in root Cargo.toml (removing the exclude entry)
    • Makefile targets (both root and crates/vm/levm/Makefile)
  2. Correctness: The removal is syntactically correct:

    • The .PHONY declarations in crates/vm/levm/Makefile are properly updated to remove deleted targets
    • Workspace exclude is removed from Cargo.toml since the directory no longer exists
    • No remaining references to revm_comparison in the codebase
  3. Safety: This is a safe deletion because:

    • The revm_comparison crate was excluded from the workspace, meaning no other crate could depend on it via workspace dependencies
    • No Rust source files in the main crates are modified, confirming the benchmark was standalone
    • The Solidity contracts were strictly for benchmarking purposes, not protocol logic
  4. CI Impact: Removing the bench job and pr_perf_levm.yaml workflow will reduce CI time and resource usage.

No issues found. The PR is ready for merge.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. tooling/ef_tests/state/Makefile:76-80 removes the test-revm entrypoint, but tooling/ef_tests/state/README.md:58-66 still tells contributors to run make test-revm. After this PR, that command hard-fails, so the state-test docs become incorrect on merge. Update the README in the same PR, or keep a compatibility alias that prints a deprecation message.

No code-correctness or security bugs stood out in the Rust changes themselves. The only executable Rust delta is the load_initial_state_revm -> load_initial_state_store refactor used by tooling/ef_tests/state/runner/levm_runner.rs:594-600, and that preserves the same post-state-root computation path while removing unused REVM scaffolding.

Residual risk: this PR intentionally removes REVM differential reruns and the LEVM-vs-REVM benchmark workflows, so it reduces two useful safety nets for opcode/gas/account-update regressions, even though it does not change EVM execution logic directly.

Verification note: I could not complete a full cargo check in this environment because Cargo needs git dependencies that are unavailable offline, and the default rustup wrapper also tries to write into a read-only temp dir.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR 7090 — remove the revm comparison (levm bench + EF-tests re-run)

Summary: This is a pure deletion PR (71 additions, ~15k deletions) removing the levm-vs-revm microbenchmark crate and the EF state-test revm re-run/comparison path. I traced every non-mechanical file change (report.rs, runner/mod.rs, runner/levm_runner.rs, utils.rs, Cargo.toml, both Makefiles, the workflow YAMLs) against the current HEAD (which already contains this PR merged), and verified no dangling references remain.

Correctness

  • The run_ef_tests control flow in runner/mod.rs is simplified correctly: report::load()/cache logic is gone (nothing wrote the cache anymore since only the revm re-run did), and the function now unconditionally runs run_with_levm. This matches the stated rationale that the cache was a landmine (stale cache silently skipping a run).
  • post_state_root in levm_runner.rs correctly switches from load_initial_state_revm (which returned an unused RevmState) to the new load_initial_state_store in utils.rs, dropping the discarded first tuple element. Verified the new function signature and callers line up.
  • The EFTestRunnerOptions::summary doc comment was misplaced before this PR (it read "For running tests only with LEVM without the REVM re-run" but sat on top of the wrong field once revm field is removed) — the PR corrects it to "For printing only the failure summary instead of writing a report file," which now accurately describes that field. Good catch, not just a mechanical deletion.
  • Verified format_duration_as_mm_ss is still defined and used inside report.rs (in progress()), and correctly dropped from mod.rs's import list since its only caller there (run_with_revm/re_run_with_revm) was deleted — no unused-import breakage.
  • EFTestRunnerError::VMExecutionMismatch and InternalError::ReRunInternal are removed along with the only code paths that constructed or matched them (confirmed via grep against the current tree — no stray references anywhere).

Consistency / build hygiene

  • Cargo.toml workspace exclude entry, the two cargo tree/cargo metadata --locked lines in the root Makefile, and the revm-comparison lockfile note in docs/developers/release-process.md are all removed together — consistent with deleting the crate.
  • tooling/Cargo.lock shrinks by 400 lines (454 removed / 54 added) from dropping the revm dependency — matches removing the revm = {...} block from tooling/ef_tests/state/Cargo.toml.
  • Grepped the working tree for revm_runner, revm_db, load_initial_state_revm, EF_TESTS_CACHE_FILE_PATH, ComparisonReport, TestReRunReport, VMExecutionMismatch, ReRunInternal, revm_comparison, opts.revm — zero hits, confirming no leftover call sites, imports, or CI references.
  • Both Makefile targets that referenced REPETITIONS/BENCH_* variables not defined in tooling/ef_tests/state/Makefile (flamegraph-benchmarks, samply-benchmarks) are removed — the PR description's claim that these were dead code checks out against the diff.
  • The bench job in pr-main_levm.yaml and all of pr_perf_levm.yaml are removed cleanly with no other workflow referencing them.

Notes

  • No production/consensus code (VM opcodes, gas accounting, trie/RLP/state logic) is touched at all — this PR is scoped entirely to dev tooling and CI, which matches its stated intent and keeps risk low.
  • The reasoning in the PR body (Amsterdam→OSAKA SpecId mapping making revm comparisons on the active fork meaningless, --summary always short-circuiting before re_run_with_revm) is corroborated by the code that's being deleted — this wasn't exercised in practice, so removing it doesn't reduce real test coverage.

No issues found. The removal is thorough, self-consistent, and the one non-mechanical change (the doc-comment fix) is a genuine improvement.


Automated review by Claude (Anthropic) · sonnet · custom prompt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

levm Lambda EVM implementation

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

1 participant