Skip to content

fix(l1,levm): report why a frame transaction was rejected - #7197

Open
ilitteri wants to merge 1 commit into
mainfrom
fix/frame-tx-validation-granularity
Open

fix(l1,levm): report why a frame transaction was rejected#7197
ilitteri wants to merge 1 commit into
mainfrom
fix/frame-tx-validation-granularity

Conversation

@ilitteri

@ilitteri ilitteri commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Motivation

A frame transaction that failed static validation, carried an invalid signature, or broke a gas or blob bound was rejected with one error whose message describes only one of those cases: "VERIFY frame did not call APPROVE or payer not approved". The block was always correctly rejected, but the reason was wrong, which is why consume-engine fails 62 tests in the hive frames suite while consume-rlp passes 24,916/24,916 on the same client and fixtures.

Four rules also had no frame-transaction path at all, so those transactions executed and were caught only by block-access-list validation.

Description

Splits the error into InvalidFrameTransactionFormat, carrying the reason validate_static_constraints already produced and the call site discarded, and InvalidFrameSignature. The approval case keeps the original variant and message.

Adds the missing checks, reusing existing error variants so no new strings enter the exception-mapper contract: EIP-7825's transaction gas cap over max_gas(), which covers both the frame-gas and calldata-floor anchors; the EIP-4844 versioned-hash and blob-count rules; and a nonce at the u64 ceiling.

On the pinned frames bundle this takes exception mismatches from 83 to 78, and to 2 together with the harness alignment already on frames-devnet-0. Adds 6 regression tests, and repoints batched_verify_revert_invalidates_tx, which was passing vacuously: EIP-8141 forbids the atomic batch flag on a VERIFY frame, so its transaction was rejected by static validation and the reverting frame never ran.

Checklist

  • Updated STORE_SCHEMA_VERSION if the PR includes breaking changes to the Store requiring a re-sync. — not applicable, no Store changes.

…ry reason

into the approval error

A frame transaction that fails static validation, carries an invalid signature,
or exceeds a gas or blob bound was rejected with
TxValidationError::InvalidFrameTransaction, whose message describes only one of
those cases: "VERIFY frame did not call APPROVE or payer not approved". The
block was always correctly rejected, but the reason reported was wrong, which is
indistinguishable to a conformance harness from a client that rejected it by
accident.

Split the variant into InvalidFrameTransactionFormat, carrying the reason
validate_static_constraints already produced and the call site discarded, and
InvalidFrameSignature. The approval case keeps the original variant and message.

Enforce four rules that had no frame-transaction path at all, reusing the
existing error variants so no new strings are needed: EIP-7825's transaction gas
cap over max_gas (which covers both the frame-gas and calldata-floor anchors),
the EIP-4844 blob versioned-hash and blob-count rules, and a nonce at the u64
ceiling. Those transactions previously executed and were only caught downstream,
by block-access-list validation.

batched_verify_revert_invalidates_tx was passing vacuously: EIP-8141 forbids the
atomic batch flag on a VERIFY frame, so its transaction was rejected by static
validation and the reverting frame never ran. Carrying the reason exposed it.
Repointed to the rule it actually exercises.
@ilitteri
ilitteri requested a review from a team as a code owner August 21, 2026 20:48
@github-actions github-actions Bot added L1 Ethereum client levm Lambda EVM implementation labels Aug 21, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.


The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.


ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rsunsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.


Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

  1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
    reviewers, and add environment: to the two jobs that sign.
  2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

The PR improves error-reporting granularity for EIP-8141 frame transaction validation and adds missing checks for EIP-7825 gas limits, nonce overflow, and EIP-4844 blob rules. Overall, the changes are correct and well-tested.

Issues Found:

  1. Doc comment misplacement in vm.rs (lines 2398–2400)
    The doc comment ending with "OQ1); when set, the access-restriction skip fires for that frame." belongs to run_frame_validation_prefix, but the new validate_frame_tx_blobs method is inserted between this comment and the function definition. This breaks the documentation association for run_frame_validation_prefix.

    Suggestion: Move the new method after run_frame_validation_prefix or ensure the doc comment remains attached to the correct function.

  2. Potential overflow in gas calculation (line ~1583)
    The code calculates max_gas() and compares it to TX_MAX_GAS_LIMIT_AMSTERDAM. While the check itself is correct (>), ensure that FrameTransaction::max_gas() cannot overflow when summing frame gas limits with the intrinsic cost. If the sum could overflow u64, the comparison would be incorrect.

    Suggestion: Verify that max_gas() uses checked arithmetic (e.g., checked_add) or ensure the individual frame gas limits are bounded such that the sum cannot overflow u64.

  3. Error type consistency in blob validation (lines ~2430–2435)
    In validate_frame_tx_blobs, if blob_schedule.max exceeds usize::MAX, the function returns InternalError::TypeConversion. This is appropriate for a configuration error, but ensure that upstream callers handle VMError::Internal appropriately and do not confuse it with a validation failure.

    Suggestion: Consider adding a comment clarifying that this is a node configuration error, not a transaction validation error.

Positive aspects:

  • The validation order (blobs → static constraints → gas → nonce → signature) ensures specific error messages are reported as required by the EIPs.
  • The new tests comprehensively cover boundary conditions (exact gas cap, u64::MAX nonce) and specific rejection reasons.
  • Changing generic InvalidFrameTransaction to specific errors (InvalidFrameTransactionFormat, InvalidFrameSignature, NonceIsMax) significantly improves debugging and conformance testing.

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

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. execute_frame_tx() and the mempool/prefix path have drifted again. The new checks added in crates/vm/levm/src/vm.rs:1577 through crates/vm/levm/src/vm.rs:1644 are not mirrored in crates/vm/levm/src/vm.rs:2481 through crates/vm/levm/src/vm.rs:2513, even though run_frame_validation_prefix() is documented as running the same preamble. That is a real correctness issue for admission: the generic mempool gas-cap check only applies on Osaka..pre-Amsterdam (crates/blockchain/blockchain.rs:3584), so an Amsterdam+/Hegota frame tx with max_gas() > TX_MAX_GAS_LIMIT_AMSTERDAM can still enter the pool and then be rejected only at execution time. The same mismatch will also resurface for blob-specific error mapping if frame-tx blobs are ever re-enabled. I would extract a shared frame-tx preamble validator and call it from both execute_frame_tx() and run_frame_validation_prefix(), then add prefix-path regressions alongside the new execution-only tests in test/tests/levm/eip8141_tests.rs:4067.

No other material issues stood out in the diff. I did not run the test suite.


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

@github-actions

Copy link
Copy Markdown

Lines of code report

Total lines added: 59
Total lines removed: 0
Total lines changed: 59

Detailed view
+-------------------------------------+-------+------+
| File                                | Lines | Diff |
+-------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/errors.rs | 260   | +4   |
+-------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/vm.rs     | 2238  | +55  |
+-------------------------------------+-------+------+

@github-actions

Copy link
Copy Markdown

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Detailed Results

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
main_revm_BubbleSort 3.265 ± 0.023 3.228 3.311 1.16 ± 0.01
main_levm_BubbleSort 2.820 ± 0.020 2.802 2.867 1.00
pr_revm_BubbleSort 3.262 ± 0.011 3.245 3.282 1.16 ± 0.01
pr_levm_BubbleSort 2.828 ± 0.027 2.799 2.881 1.00 ± 0.01

Benchmark Results: ERC20Approval

Command Mean [s] Min [s] Max [s] Relative
main_revm_ERC20Approval 1.046 ± 0.011 1.037 1.069 1.01 ± 0.01
main_levm_ERC20Approval 1.034 ± 0.007 1.026 1.049 1.00
pr_revm_ERC20Approval 1.060 ± 0.006 1.052 1.071 1.03 ± 0.01
pr_levm_ERC20Approval 1.034 ± 0.007 1.025 1.051 1.00 ± 0.01

Benchmark Results: ERC20Mint

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ERC20Mint 141.7 ± 1.0 140.1 143.1 1.00
main_levm_ERC20Mint 157.3 ± 0.7 156.3 158.1 1.11 ± 0.01
pr_revm_ERC20Mint 143.1 ± 1.5 141.2 144.9 1.01 ± 0.01
pr_levm_ERC20Mint 157.7 ± 1.5 156.0 161.2 1.11 ± 0.01

Benchmark Results: ERC20Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ERC20Transfer 250.7 ± 2.3 248.1 255.0 1.00
main_levm_ERC20Transfer 265.1 ± 1.6 263.1 267.7 1.06 ± 0.01
pr_revm_ERC20Transfer 254.1 ± 2.0 251.1 257.2 1.01 ± 0.01
pr_levm_ERC20Transfer 267.3 ± 6.5 262.8 283.6 1.07 ± 0.03

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Factorial 237.7 ± 5.6 231.1 245.0 1.01 ± 0.04
main_levm_Factorial 259.1 ± 1.7 256.8 261.6 1.10 ± 0.03
pr_revm_Factorial 235.7 ± 6.1 230.1 246.0 1.00
pr_levm_Factorial 260.5 ± 1.6 257.0 262.5 1.11 ± 0.03

Benchmark Results: FactorialRecursive

Command Mean [s] Min [s] Max [s] Relative
main_revm_FactorialRecursive 1.616 ± 0.032 1.569 1.663 1.00
main_levm_FactorialRecursive 9.525 ± 0.049 9.450 9.606 5.89 ± 0.12
pr_revm_FactorialRecursive 1.647 ± 0.028 1.591 1.698 1.02 ± 0.03
pr_levm_FactorialRecursive 9.520 ± 0.032 9.471 9.561 5.89 ± 0.12

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Fibonacci 233.8 ± 8.7 224.3 246.7 1.01 ± 0.05
main_levm_Fibonacci 239.6 ± 17.6 228.4 287.2 1.04 ± 0.08
pr_revm_Fibonacci 231.3 ± 8.4 221.2 245.2 1.00
pr_levm_Fibonacci 238.3 ± 8.1 228.6 254.3 1.03 ± 0.05

Benchmark Results: FibonacciRecursive

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_FibonacciRecursive 861.1 ± 6.5 849.9 870.1 1.19 ± 0.02
main_levm_FibonacciRecursive 721.1 ± 7.8 713.8 739.0 1.00
pr_revm_FibonacciRecursive 857.7 ± 7.1 850.8 873.9 1.19 ± 0.02
pr_levm_FibonacciRecursive 733.7 ± 9.9 723.3 754.6 1.02 ± 0.02

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ManyHashes 9.1 ± 0.1 9.0 9.3 1.00
main_levm_ManyHashes 10.1 ± 0.1 9.9 10.3 1.10 ± 0.02
pr_revm_ManyHashes 9.1 ± 0.1 9.0 9.4 1.00 ± 0.02
pr_levm_ManyHashes 10.2 ± 0.3 10.0 11.1 1.12 ± 0.04

Benchmark Results: MstoreBench

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_MstoreBench 294.9 ± 2.9 291.8 301.1 1.46 ± 0.03
main_levm_MstoreBench 202.0 ± 3.2 198.8 208.5 1.00
pr_revm_MstoreBench 297.9 ± 4.8 291.7 306.4 1.48 ± 0.03
pr_levm_MstoreBench 211.4 ± 22.3 198.1 273.4 1.05 ± 0.11

Benchmark Results: Push

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Push 332.7 ± 6.7 325.5 343.4 1.30 ± 0.04
main_levm_Push 255.6 ± 4.8 250.3 262.9 1.00
pr_revm_Push 331.4 ± 6.4 324.7 343.9 1.30 ± 0.04
pr_levm_Push 257.2 ± 4.3 250.3 261.5 1.01 ± 0.03

Benchmark Results: SstoreBench_no_opt

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_SstoreBench_no_opt 178.5 ± 3.1 174.6 184.9 1.56 ± 0.03
main_levm_SstoreBench_no_opt 114.6 ± 1.0 113.7 116.6 1.00
pr_revm_SstoreBench_no_opt 187.9 ± 23.6 174.8 232.6 1.64 ± 0.21
pr_levm_SstoreBench_no_opt 115.6 ± 1.5 113.7 118.8 1.01 ± 0.02

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

Labels

L1 Ethereum client levm Lambda EVM implementation

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant