Skip to content

perf(l1): ship JUMPDEST bitmaps in the execution witness - #7274

Open
diegokingston wants to merge 1 commit into
perf/witness-dfs-streamfrom
perf/witness-codes-jumpdests
Open

diegokingston wants to merge 1 commit into
perf/witness-dfs-streamfrom
perf/witness-codes-jumpdests

Conversation

@diegokingston

Copy link
Copy Markdown
Contributor

Stacked on #7273 (witness DFS stream) — review only the last commit.

The witness now carries, in parallel with codes, each bytecode's JUMPDEST bitmap (one bit per byte, exactly Code::jumpdests). Guests build each Code with Code::from_parts_unchecked(hash, code, bitmap) instead of re-scanning the whole bytecode corpus in-circuit to compute jump destinations.

Measurement

Guest cycles for mainnet block 25368371 (LambdaVM interpreter, same host):

cycles
main (89e1602) 28,939,689
#7273 (DFS stream) 26,673,351
this PR 25,131,179 (−5.8% additional; −13.2% cumulative vs main)

compute_jumpdests goes from 1.64M cycles (6.3% self) to 0 in the guest. Keccak/ECSM call counts unchanged; the real-block test (native + VM) passes.

Notes

  • The bitmap is computed by the host once; on the wire it costs ~12.5% of code size, but guests read it without any processing.
  • Trust model: like the shipped node hashes — a forged bitmap makes a JUMP wrongly succeed or fail, execution diverges, and the final state root / receipts anchors reject the block.
  • The rkyv layout of ExecutionWitness changes (new field) → fixtures must be regenerated (producers updated: into_execution_witness, from_ssz, blockchain witness generation).
  • Contract codes created during execution (initcode) still compute jumpdests in-circuit — unavoidable and untouched.

@github-actions

github-actions Bot commented Sep 14, 2026

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 github-actions Bot added L1 Ethereum client performance Block execution throughput and performance in general labels Sep 14, 2026
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR which adds precomputed JUMPDEST bitmaps to execution witnesses to avoid in-circuit bytecode scanning. Let me analyze the changes carefully.

Overall Assessment

The PR introduces a parallel codes_jumpdests array to ExecutionWitness to enable efficient Code::from_parts_unchecked construction in the guest. The approach is sound but there are several issues to address.


Critical Issues

1. Missing codes_jumpdests in encode_witness_for_engine_rpc / decode_witness_for_engine_rpc

File: crates/networking/rpc/engine/payload.rs (not shown in diff, but implied by test)

The RPC encoding/decoding functions for engine API witness exchange likely need to handle the new field. The test at line 1710 only sets codes_jumpdests: vec![] but doesn't verify round-trip serialization. If encode_witness_for_engine_rpc uses SSZ or similar, the new field must be included or the guest will receive mismatched arrays.

Action: Verify and update encode_witness_for_engine_rpc and decode_witness_for_engine_rpc to include codes_jumpdests.


2. from_parts_unchecked Safety — Bitmap Trust Model Documentation Mismatch

File: crates/common/types/block_execution_witness.rs, lines 82-90

The comment states: "a forged bitmap makes execution diverge (a JUMP wrongly succeeds or fails), so the final state root / receipts anchors reject the block."

This is incorrect for some cases. A forged bitmap that marks a non-JUMPDEST as valid can cause:

  • A JUMP to succeed where it should fail → execution continues at wrong PC
  • This can lead to arbitrary code execution within the contract, reading wrong storage, emitting wrong events

The state root check only catches this if the divergence affects state. A malicious prover could craft a bitmap that:

  • Makes JUMP land on STOP → execution halts early with same state
  • Makes JUMP land on benign opcode sequence → different gas used, potentially different refund, different receipt root

Actually, the receipt root would differ, so this is caught. But the comment should be more precise: the receipts root check catches this, not just state root. More critically, if the block has no transactions to the affected contract, the witness bitmap is unused— but a malicious prover could still cause issues if they forge a block they propose.

Suggested fix at line 82-90:

/// Trust model: like the shipped node hashes, a forged bitmap causes EVM
/// execution divergence (a JUMP succeeds where it should fail, or vice versa).
/// This changes the state root, receipt root, or gas used, all of which are
/// checked by the final block hash / state root commitment. The bitmap is thus
/// self-anchoring: any forgery is detectable.

3. Arc::from(jumpdests.as_slice()) Creates New Allocation Per Call

File: crates/common/types/block_execution_witness.rs, line 532

std::sync::Arc::from(jumpdests.as_slice()),

Arc::from(slice) allocates a new Arc<[T]> every time. For large code corpuses in witnesses, this causes unnecessary allocations. The jumpdests is already owned (Vec<u8> from into_iter()), but zip gives us ownership of both.

Actually, looking more carefully: value.codes_jumpdests is Vec<Vec<u8>>, and into_iter() yields Vec<u8>. The zip with value.codes.into_iter() gives owned Vec<u8> for both. But Arc::from(jumpdests.as_slice()) copies the slice into a new Arc allocation.

Better approach:

std::sync::Arc::from(jumpdests), // Vec<u8> implements Into<Arc<[u8]>>

This reuses the Vec's allocation instead of copying. Verify Code::from_parts_unchecked signature accepts Arc<[u8]> or Arc<Vec<u8>>.


4. Missing codes_jumpdests in ExecutionWitness Default/Partial Constructions

File: Multiple test files

Lines 614-617, 795-798 in l1_advancer.rs, line 1710 in payload.rs — tests use ..Default::default() or manual construction with codes_jumpdests: vec![].

The real risk: production code paths using ExecutionWitness { ..., ..Default::default() } will now get empty codes_jumpdests while potentially having non-empty codes, leading to the length mismatch error at line 520-524.

Audit all ExecutionWitness constructions to ensure codes and codes_jumpdests are always consistent.


Medium Issues

5. compute_jumpdests Called Multiple Times on Same Data

File: crates/blockchain/blockchain.rs, lines 2214-2217 and 2421-2424

let codes_jumpdests = codes
    .iter()
    .map(|c| ethrex_common::types::Code::compute_jumpdests(c).to_vec())
    .collect();

This is duplicated in two functions (build_witness_for_block_range and build_witness_for_block). Consider a helper method on ExecutionWitness or Code to keep in sync.

Also, compute_jumpdests returns some type with .to_vec() — what's the return type? If it returns a borrowed slice or a small vec, the .to_vec() is fine. If it already returns an owned Vec, the .to_vec() clones unnecessarily.


6. RpcExecutionWitness::into_execution_witness Recomputes Jumpdests Instead of Using Provided

File: crates/common/types/block_execution_witness.rs, lines 229-234

let codes_jumpdests = self
    .codes
    .iter()
    .map(|b| Code::compute_jumpdests(b).to_vec())
    .collect();

The RpcExecutionWitness doesn't carry codes_jumpdests from the wire — it recomputes them. This is correct for validation but means the RPC format doesn't support the optimization. If this is intentional (RPC peers validate rather than trust), document it. If the RPC should carry precomputed bitmaps for efficiency, add the field.


7. ExecutionWitness::from_rpc_input Also Recomputes

File: crates/common/types/block_execution_witness.rs, lines 347-352

Same as Point 6 — from_rpc_input recomputes rather than deserializing. Consistent with RPC not having the field, but verify this is intentional.


Minor Issues

8. Inconsistent Variable Naming: codes vs code in Closure

File: crates/common/types/block_execution_witness.rs, line 530

.map(|(code, jumpdests)| {
    let code = Code::from_parts_unchecked(
        H256(crypto.keccak256(&code)),
        &code,
        std::sync::Arc::from(jumpdests.as_slice()),
    );

Shadowing code (was Vec<u8>, becomes Code) is legal but confusing. Rename:

.map(|(bytecode, jumpdests)| {
    let code = Code::from_parts_unchecked(
        H256(crypto.keccak256(&bytecode)),
        &bytecode,
        std::sync::Arc::from(jumpdests),
    );

9. Error Message Formatting

File: crates/common/types/block_execution_witness.rs, lines 520-525

return Err(GuestProgramStateError::Custom(format!(
    "witness carries {} codes but {} jumpdest bitmaps",
    value.codes.len(),
    value.codes_jumpdests.len()
)));

Use structured error or at least consistent formatting. Not critical.


Questions for Author

  1. Does Code::from_parts_unchecked validate hash-code consistency? The name suggests "unchecked" — does it verify keccak256(code) == hash? If not, the witness also trusts the hash, which is fine (same trust model), but should be documented.

  2. What's the compute_jumpdests return type? The .to_vec() suggests it's not already Vec<u8> — clarify.

  3. Is the RPC engine API witness format versioned? Adding a field may break older clients that deserialize strictly.


Positive Notes

  • The parallel array design with explicit length check (line 520-525) is correct and safe
  • The trust model documentation, while imprecise, shows awareness of the security considerations
  • Using from_parts_unchecked in the guest avoids expensive in-circuit analysis, which is the intended optimization

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

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. High: crates/common/types/block_execution_witness.rs:519 only checks that codes.len() == codes_jumpdests.len(), then 531-535 passes each bitmap straight into Code::from_parts_unchecked. That is not enough to preserve the VM’s safety invariant: jump() trusts is_valid_jumpdest() (crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs:487) and next_opcode() does an unchecked read under the assumption that pc <= bytecode_len + 32 (crates/vm/levm/src/call_frame.rs:485). A forged witness bitmap can mark offsets beyond code.len() as valid jump destinations; for sufficiently large offsets that can drive pc out of the padded buffer and make the unsafe read UB. At minimum, validate bitmap shape before constructing Code: require either the shared empty case or exactly ceil(code.len()/8) bytes, and reject any set bits above the last real bytecode offset. Given this is consensus-critical guest input, a malformed-bitmap regression test is warranted too.

Notes

The rest of the change looks mechanically consistent: host-side witness generation populates the new field, and the RPC/SSZ conversion paths that rebuild witnesses still recompute jumpdest bitmaps rather than trusting wire data.

I could not run cargo check in this environment because rustup tried to write under a read-only location.


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

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

Lines of code report

Total lines added: 39
Total lines removed: 0
Total lines changed: 39

Detailed view
+---------------------------------------------------------+-------+------+
| File                                                    | Lines | Diff |
+---------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs                  | 3532  | +10  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_execution_witness.rs   | 821   | +26  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/l1_advancer.rs | 644   | +2   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs          | 1438  | +1   |
+---------------------------------------------------------+-------+------+

… Code via from_parts_unchecked, no in-circuit bytecode scan (−1.88M cycles)
@github-actions

Copy link
Copy Markdown

Benchmark Block Execution Results Comparison Against Main

Command Mean [s] Min [s] Max [s] Relative
base 82.813 ± 0.226 82.375 83.084 1.00
head 82.831 ± 0.234 82.476 83.213 1.00 ± 0.00

This branch has not been deployed

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

Labels

L1 Ethereum client performance Block execution throughput and performance in general

Projects

Status: No status
Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant