perf(l1): serve account and storage reads from flat maps

[diegokingston]

Stacked on #7275 (nibble-stack decode) — review only the last commit.

GuestProgramState now builds, during the same anchored DFS decode, flat read maps —
accounts_flat: FxHashMap<H256, Vec<u8>> and storage_flat: FxHashMap<(H256, H256), Vec<u8>> —
and serves get_account_state / get_storage_slot from them instead of descending the
trie level by level. apply_account_updates writes through to both (trie for roots,
maps for reads).

The "proven absent" vs "witness incomplete" distinction is preserved: on a map miss we
fall back to the trie, which errors on accounts/slots the witness does not cover
(EIP-8025 witness_validation_state tests).

Measurement

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

	cycles
main (89e1602)	28,939,689
#7273 (DFS stream)	26,673,351
#7274 (JUMPDEST bitmaps)	25,131,179
#7275 (nibble stack)	23,776,494
this PR	23,381,352 (−1.7% additional; −19.2% cumulative vs main)

BranchNode::get drops from 966k to 416k in the profile. Keccak/ECSM call counts
unchanged; the real-block test (native + VM) passes.

[github-actions]

⚠️ 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.rs → unsync_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]

🤖 Kimi Code Review

I'll review this PR which adds flat read maps (accounts_flat and storage_flat) to optimize witness-based state access in the guest program. Let me analyze the changes carefully.

Overall Assessment

The PR introduces a performance optimization: flat hash maps for O(1) account and storage reads instead of walking Merkle tries. The core challenge is maintaining consistency between the flat maps and the tries during state updates, while preserving the security property that missing witness data errors rather than returning default values.

---

Issues Found

1. Critical: Double trie lookup in get_storage_slot (Performance + Correctness Bug)

File: crates/common/types/block_execution_witness.rs
Lines: 792-828

if self.get_valid_storage_trie(address, crypto)?.is_none() {
    return Ok(None);
}
// ...
let Some(storage_trie) = self.get_valid_storage_trie(address, crypto)? else {  // line 817

Problem: get_valid_storage_trie is called twice — once for verification (line 798), then again for the fallback read (line 817). This function mutates verified_storage_roots (marks storage as verified) and performs trie validation. The second call may behave differently since the first call already marked it verified.

Fix: Cache the result. Also, the second call's unreachable error branch is dead code that complicates reasoning:

// Suggested fix:
let storage_trie_opt = self.get_valid_storage_trie(address, crypto)?;
if storage_trie_opt.is_none() {
    return Ok(None);
}
// ... flat map miss ...
let Some(storage_trie) = storage_trie_opt else {
    return Err(GuestProgramStateError::Unreachable(...));
};

Actually, better — since we already know it's Some:

let storage_trie = storage_trie_opt.unwrap(); // safe: checked is_none() above

Or restructure to avoid the double call entirely.

---

2. High: get_account_state fallback decodes twice on trie hit

File: crates/common/types/block_execution_witness.rs
Lines: 738-765

Ok(Some(encoded_state)) => {
    let state = AccountState::decode(&encoded_state).map_err(|_| {...})?;
    return Ok(Some(state));
}
// ...
let state = AccountState::decode(encoded_state).map_err(|_| {...})?;

Problem: In the fallback path (trie hit when flat map misses), the code decodes AccountState, returns it directly. But in the normal path (flat map hit), it also decodes. The error messages are inconsistent: "Failed to get decode account from trie" vs "failed to read storage from trie" (different casing). More importantly, the fallback path does not populate accounts_flat — so subsequent reads for the same account will again miss the cache and hit the trie.

Fix: Populate accounts_flat on fallback trie hit to amortize the cost:

Ok(Some(encoded_state)) => {
    let state = AccountState::decode(&encoded_state).map_err(...)?;
    self.accounts_flat.insert(hashed_address, encoded_state.to_vec()); // add this
    return Ok(Some(state));
}

Same issue exists for get_storage_slot — flat map not populated on trie fallback.

---

3. Medium: storage_flat key type uses (H256, H256) without custom hasher optimization

File: crates/common/types/block_execution_witness.rs
Lines: 59-60, 411

pub storage_flat: FxHashMap<(H256, H256), Vec<u8>>,

Problem: FxHashMap uses FxHasher which is fast for small keys. (H256, H256) is 64 bytes — larger than ideal for FxHasher which is optimized for usize-sized keys. The tuple's Hash implementation hashes both H256s sequentially, which is fine, but consider if a custom struct with a precomputed hash or H512 (concatenated) would be better. More importantly, H256::from_slice is called repeatedly to construct keys for lookup — this allocates/constructs a new H256 on every storage access.

Note: This is likely acceptable for the guest program's constraints, but worth profiling. The hashed_key in get_storage_slot is already Vec<u8> or similar — converting to H256 for each lookup is overhead.

---

4. Medium: apply_account_updates removes from accounts_flat but not from account_hashes_by_address

File: crates/common/types/block_execution_witness.rs
Lines: 602-606

if update.removed {
    self.state_trie.remove(hashed_address.as_bytes())?;
    self.accounts_flat.remove(&hashed_address);
    // account_hashes_by_address NOT cleaned up
}

Problem: account_hashes_by_address accumulates cached address hashes but never evicts. For long-running contexts or many account deletions, this grows unbounded. Similarly, verified_storage_roots is not cleaned up when account is removed.

Fix: Consider cleanup, or document intentional retention:

if update.removed {
    self.state_trie.remove(hashed_address.as_bytes())?;
    self.accounts_flat.remove(&hashed_address);
    self.account_hashes_by_address.remove(&address); // add if desired
    self.verified_storage_roots.remove(&address);    // add if desired
    self.storage_flat.retain(|(addr, _), _| *addr != hashed_address); // already done elsewhere
}

Wait — storage_flat cleanup for removed accounts is missing here! Only done on removed_storage (line 614), not on full account removal.

This is a bug: Removing an account should remove all its storage entries.

if update.removed {
    self.state_trie.remove(hashed_address.as_bytes())?;
    self.accounts_flat.remove(&hashed_address);
    self.storage_flat.retain(|(addr, _), _| *addr != hashed_address); // MISSING
    // ... also consider cleaning account_hashes_by_address, verified_storage_roots
}

---

5. Medium: Inconsistent H256::from_slice usage — potential panic on wrong-length slices

File: crates/common/types/block_execution_witness.rs
Lines: 421, 439, 650, 657, 799

let hashed_address = H256::from_slice(path_bytes);  // line 421
H256::from_slice(&hashed_key)                       // line 650, 657, 799

Problem: H256::from_slice panics if slice length ≠ 32. The code checks path_bytes.len() != 32 before some calls (lines 420, 438) but not before H256::from_slice(&hashed_key) in apply_account_updates and get_storage_slot.

In apply_account_updates (line 650):

self.storage_flat.insert(
    (hashed_address, H256::from_slice(&hashed_key)),  // hashed_key from partition, length unchecked
    encoded,
);

hashed_key comes from update.storage_updates — is this guaranteed 32 bytes? The type appears to be Vec<u8> or H256 — need to verify. If it's H256, use *hashed_key or hashed_key.0 instead of from_slice.

In get_storage_slot (line 799):

.get(&(hashed_address, H256::from_slice(&hashed_key)))

Same concern — hashed_key is computed by hash_key which returns Vec<u8> or similar? If hash_key returns H256, this is fine. If it returns variable-length, this panics.

Action needed: Verify hash_key return type and ensure it's H256 or add length check.

---

6. Low: accounts_flat not updated when account exists but info is None

File: crates/common/types/block_execution_witness.rs
Lines: 602-644

In apply_account_updates, when update.removed is false, the code always re-encodes and inserts to accounts_flat (line 644). But if info is None and no storage changes occur, the account state might be unchanged — still re-encoded and inserted. This is correct (idempotent) but the encoded_state clone at line 641 is unnecessary in no-op cases.

Minor optimization: Only update accounts_flat if account state actually changed. Not a bug.

---

7. Low: Error message inconsistency and typo

File: crates/common/types/block_execution_witness.rs
Lines: 755, 758, 808, 820

"Failed to get decode account from trie"  // "get decode" — typo
"failed to read storage from trie"        // lowercase, different structure

Fix: Standardize error messages. The "get decode" typo exists in two places (lines 755, 758).

---

8. Low: build_tries_from_records returns 4-tuple, hard to maintain

File: crates/common/types/block_execution_witness.rs
Lines: 381-389

The #[allow(clippy::type_complexity)] suppresses a real readability issue. Consider a named struct:

struct BuiltTries {
    state_trie: Trie,
    storage_tries: BTreeMap<H256, Trie>,
    accounts_flat: FxHashMap<H256, Vec<u8>>,
    storage_flat: FxHashMap<(H256, H256), Vec<u8>>,
}

This would improve call sites (line 545) and make the API self-documenting.

---

9. Security Consideration: Flat map and trie divergence on apply_account_updates failure

File: crates/common/types/block_execution_witness.rs
Lines: 602-644

If self.state_trie.insert(...) succeeds but self.accounts_flat.insert(...) fails (e.g., memory exhaustion in guest), or vice versa, the structures diverge. However, since both are in-memory and this is single-threaded with ? propagation, partial updates are rolled back by error propagation... except the early returns.

Actually, check: storage_trie.insert at line 648, then storage_flat.insert at line 650 — if storage_flat.insert somehow failed (it won't, FxHashMap::insert is infallible), but if there were a fallible operation, order matters. Current order: trie first, flat second. On read, flat is checked first, trie fallback. This is safe as long as both succeed or both fail atomically.

Since FxHashMap::insert is infallible and Trie::insert returns Result, the pattern is: do fallible trie op, then infallible flat op. If trie op fails, flat op is skipped. This is correct.

---

10. Question: storage_flat not cleared on removed_storage for non-existent account?

File: crates/common/types/block_execution_witness.rs
Lines: 612-615

if update.removed_storage {
    account_state.storage_root = EMPTY_TRIE_HASH;
    self.storage_flat.retain(|(addr, _), _| *addr != hashed_address);
}

If update.removed_storage is true but account doesn't exist in storage_tries (new account, or already empty), the retain still runs — harmless but could be conditional.

---

Summary

Priority	Issue	Location
Critical	Double get_valid_storage_trie call	get_storage_slot, lines 798, 817
High	Flat maps not populated on trie fallback; inconsistent decode paths	get_account_state, get_storage_slot
High	Account removal doesn't clear storage_flat	apply_account_updates, line 604
Medium	H256::from_slice panic risk on hashed_key	lines 650, 657, 799
Medium	(H256, H256) key efficiency	storage_flat type
Low	Typo in error message; message inconsistency	lines 755, 758, 808, 820
Low	4-tuple return type complexity	build_tries_from_records

The core logic is sound but the double-verification and missing cleanup on account removal are genuine bugs that should be fixed before merge.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/common/types/block_execution_witness.rs:602-605
   Removing an account now clears state_trie and accounts_flat, but leaves the new storage_flat cache untouched. That breaks the “kept in sync by apply_account_updates” invariant and retains stale slot data for the rest of the batch. At minimum this is a memory leak in the guest; in destroy-then-recreate flows it also compounds the existing stale per-account storage state. This path should purge the account’s storage_flat entries, and ideally clear the related storage_tries / verified_storage_roots state as well.

2. crates/common/types/block_execution_witness.rs:435-445
   Every storage subtree is now decoded with Some(&mut storage_leaves), so you materialize and clone all leaves before checking whether the subtree root is even referenced by any account. The comment below explicitly says unbound subtries are tolerated and skipped, so this turns previously ignored witness data into full allocation/copy work. In a stateless/zkVM setting that is a real DoS/perf regression.

3. crates/common/types/block_execution_witness.rs:616-617
self.storage_flat.retain(|(addr, _), _| *addr != hashed_address) makes removed_storage O(total cached slots), not O(slots for this account). A block that wipes many large-storage contracts will repeatedly rescan the whole cache. A per-account structure or secondary index would avoid this quadratic behavior.

I did not see a new EVM-consensus or gas-accounting bug in the read-path logic itself; the main concerns are cache invalidation and the new witness-memory/performance cost.

cargo test was not runnable in this sandbox because rustup attempted to write under /home/runner/.rustup, which is read-only here.

---

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

[github-actions]

Lines of code report

Total lines added: 71
Total lines removed: 0
Total lines changed: 71

Detailed view

+-------------------------------------------------------+-------+------+
| File                                                  | Lines | Diff |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_execution_witness.rs | 889   | +71  |
+-------------------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

Review: PR 7276 — Serve account/storage reads from flat maps

Overall this is a solid, well-reasoned optimization. The critical "proven-absent vs witness-incomplete" invariant (EIP-8025 witness_validation_state) is correctly preserved: every flat-map miss falls back to the trie rather than silently treating the read as absent, and every trie mutation in apply_account_updates has a matching flat-map mutation using the same encoded bytes (no separate re-encoding that could drift). I did not find a case where the flat map and trie can disagree on a read result.

Findings

1. storage_flat.retain is an O(n) full-table scan on every removed_storage update (block_execution_witness.rs:616-617)

self.storage_flat
    .retain(|(addr, _), _| *addr != hashed_address);

storage_flat accumulates entries for every account touched in the batch, so clearing one account's storage (e.g. on SELFDESTRUCT) walks the whole map regardless of how many slots that account actually had. Since the entire point of this PR is shaving guest cycles, this is the one place that can silently regress on blocks with many storage-clearing accounts. A nested map (FxHashMap<H256, FxHashMap<H256, Vec<u8>>>) would make this O(k) (just drop the inner map) instead of O(n).

2. get_storage_slot calls get_valid_storage_trie twice (block_execution_witness.rs:801 and 817)
The second call is dead-weight in the common case: by the time it's reached, verified_storage_roots is already true, so it just re-does the account_hashes_by_address/verified_storage_roots lookups to re-fetch a reference you already had. Correctness is fine (the borrow-checker workaround is explained in the comment), but it's avoidable rework in exactly the hot path this PR is trying to shrink. Consider inlining the field access instead of routing through the method twice.

3. Pre-existing storage_by_root collision risk is now duplicated into storage_flat (block_execution_witness.rs:418-429, 439-446)
storage_by_root: FxHashMap<H256, H256> binds a storage subtree to exactly one hashed address, keyed by root hash. If two distinct accounts happen to have byte-identical storage (same root hash), only the last-inserted address wins the binding — the other account's storage_tries/storage_flat entries are silently absent, and a later read for that account hits get_valid_storage_trie's _ => Err("invalid storage trie for account") instead of succeeding. This predates this PR (the storage_tries binding had the same limitation), but the new storage_flat population reuses the same lookup, so the failure mode now exists in two places instead of one. Worth a tracking issue if not already known — it's a correctness risk for a niche but not impossible input, and this PR is a natural place to guard against it (e.g. by also keying off the leaf's expected parent when available) or to at least add a test case.

4. Doc-comment glitch (block_execution_witness.rs:745)

"the trie knows — a read of an account the witness does not cover must error"

Reads as a leftover edit fragment ("the trie knows [that it's proven-absent or not]"?). Minor, but worth tightening since this comment guards a consensus-critical invariant and should be unambiguous.

5. Memory footprint doubling for leaf data (design-level, not a bug)
accounts_flat/storage_flat duplicate the RLP bytes already held inside state_trie/storage_tries (each leaf value is .clone()d into the flat map at block_execution_witness.rs:424 and 444, and again on every apply_account_updates write). That's the expected time/memory tradeoff for this optimization, but worth calling out explicitly since this runs inside a zkVM guest where memory budgets can be tight — presumably already covered by the real-block benchmark passing, just flagging for visibility.

Minor nits

• encoded_key is used as a variable name for the value, not the key, in both the new get_storage_slot flat-map branch and the pre-existing trie-fallback branch (block_execution_witness.rs:804, 826) — confusing, though it's a pre-existing naming choice being replicated rather than introduced fresh.
• H256::from_slice(&hashed_key) is repeated three times across apply_account_updates/get_storage_slot; a small local binding would reduce duplication, purely cosmetic.

No issues found with RLP encoding/decoding correctness, gas accounting (not touched), or the account-removal/removed_storage bookkeeping logic itself — the flat-map updates are correctly ordered relative to the trie updates they mirror.

---

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

[github-actions]

Benchmark Block Execution Results Comparison Against Main

Command	Mean [s]	Min [s]	Max [s]	Relative
base	84.181 ± 0.365	83.639	84.709	1.00 ± 0.01
head	84.092 ± 0.249	83.767	84.602	1.00