refactor(l1): deduplicate the account-update loop in Store

[ilitteri]

Motivation

Store::apply_account_updates_from_trie_batch and Store::apply_account_updates_from_trie_with_witness were two hand-maintained copies of the same per-update algorithm: remove the account if update.removed; decode-or-default the AccountState; reset storage_root on removed_storage; copy nonce/balance/code_hash and collect code_updates; apply added_storage to a storage trie and collect_changes_since_last_hash; insert the account; finally collect state changes into an AccountUpdatesList. Any future change to the update semantics had to be made twice, and the copies had already drifted cosmetically (hashed-address type, blank-line style), which obscured that the only real difference is how the storage trie is acquired.

Description

Extracts the loop into a private apply_account_updates_from_trie_inner(&self, state_trie, account_updates, storage_tries: Option<&mut StorageTries>) and turns both public functions into thin wrappers with their exact original signatures. Net −42 lines in crates/storage/store.rs (+52/−94), no behavior change.

The two variants' only genuine differences are preserved verbatim behind the Option:

• Storage-trie acquisition (the load-bearing difference):
  • None (batch): a fresh trie is opened per account via open_storage_trie at the account's current storage_root, exactly as before.
  • Some(map) (witness): the entry API on the map keyed by unhashed update.address. Occupied reuses the cached (TrieWitness, Trie) — the cached trie's in-memory state intentionally wins over account_state.storage_root, including ignoring an EMPTY_TRIE_HASH reset from removed_storage. Vacant opens the trie after the removed_storage reset, wraps it in TrieLogger::open_trie, and inserts it into the map so witness nodes for storage tries first touched during update application are recorded; the mutated map is returned to the caller as before.
• Hashed address: unified on hash_address_fixed (H256). The witness copy used hash_address (Vec<u8>) and converted back with H256::from_slice; both are the same keccak digest of the address, so trie keys and storage_updates keys are byte-identical.
• state_root passed to open_storage_trie remains the single pre-loop hash_no_commit snapshot, not recomputed per iteration.
• Borrow checking: the Option<&mut StorageTries> is reborrowed per iteration with as_deref_mut(), and the batch arm uses a deferred-init local_trie so both arms unify as &mut Trie.

Public API, RLP encoding, DB schema, and STORE_SCHEMA_VERSION are untouched. No tests were deleted or modified.

If this change were wrong, one of these would have to be true:

• hash_address and hash_address_fixed would have to produce different bytes for some address — they are both keccak(address.to_fixed_bytes()) (store.rs), one as Vec<u8>, one as H256.
• The witness variant would have to depend on re-opening a cached storage trie at the post-removed_storage root — it cannot: the Occupied arm never consulted account_state.storage_root before this change either.
• A caller would have to key StorageTries by hashed address — callers in blockchain.rs populate and drain it keyed by unhashed Address, which this change keeps.
• The batch variant would have to rely on storage-trie state carrying across accounts — it opened a fresh trie per account before and still does.

How to test

cargo clippy --workspace --all-targets -- -D warnings
cargo test -p ethrex-storage
cargo test -p ethrex-blockchain   # covers the batch and witness call sites
make -C tooling/ef_tests/blockchain test-levm
make -C tooling/ef_tests/engine test

Results on this branch:

• cargo fmt — clean (diff confined to the two functions).
• cargo clippy --workspace --all-targets -- -D warnings — clean.
• cargo test -p ethrex-storage — 91 passed, 0 failed (+ doctests).
• cargo test -p ethrex-blockchain — 58 passed, 0 failed (covers the batch and witness call sites in blockchain.rs).
• ef-tests: the blockchain and engine suites run green, with their fixture counts unchanged.

Checklist

• No Store schema change, so STORE_SCHEMA_VERSION is untouched (private helper extraction only; public API, RLP and DB layout identical).

[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]

🤖 Claude Code Review

This confirms the CI checks (fmt/clippy/tests) already passed per the PR description, and the logic is a faithful behavior-preserving merge. The code looks correct.

Review: refactor(l1): deduplicate the account-update loop in Store

Summary: Pure refactor extracting a shared apply_account_updates_from_trie_inner helper from two previously-duplicated loops. I traced through the diff carefully looking for subtle behavioral drift (a classic risk when merging near-identical branches), and did not find any.

Correctness

• hash_address vs hash_address_fixed unification (line ~5245-5251): Verified these are byte-identical. hash_address_fixed calls ethrex_common::utils::keccak, which is H256(keccak_hash(data)) — the exact same underlying keccak_hash from ethrex_crypto that hash_address calls directly and wraps in Vec<u8>. Unifying on the H256-returning variant and calling .as_bytes() where a slice/vec is needed is safe and slightly reduces allocations (no separate Vec conversion, no H256::from_slice round-trip).
• Option<&mut StorageTries> branch (lines 2443-2465): The None branch (batch caller) opens a fresh trie into local_trie per account, matching the original apply_account_updates_from_trie behavior exactly. The Some branch preserves the entry-API caching semantics of the original witness path, including the load-bearing quirk that Entry::Occupied reuses the cached trie without re-consulting account_state.storage_root (i.e., ignoring an EMPTY_TRIE_HASH reset from removed_storage on a hit) — this matches pre-refactor behavior, not a new bug introduced here.
• state_root snapshot: Still taken once before the loop via state_trie.hash_no_commit(&NativeCrypto), not recomputed per iteration, matching both originals.
• Storage trie borrow (local_trie / as_deref_mut()): The deferred-init pattern (let mut local_trie; then conditionally assigned) is a clean way to unify &mut Trie from either branch without an enum wrapper or Box<dyn>. No aliasing issues — storage_tries is reborrowed per-iteration via as_deref_mut(), and local_trie's scope is confined to a single iteration.

Style / minor nits

• The extracted apply_account_updates_from_trie_inner doc comment (lines ~2453-2458) is a good addition — explains the storage_tries parameter's dual meaning where the original two functions relied on separate doc comments.
• The removed blank lines between statements (previously present in apply_account_updates_from_trie_with_witness, absent in apply_account_updates_from_trie) — the merged version adopts the denser style consistently. This is a readability choice, not a defect; no objection.

Risk assessment

This is a low-risk, mechanical deduplication with no public API, RLP, or DB schema changes. The PR description's "what would have to be true for this to be wrong" section is accurate and I independently confirmed each of its claims. Test coverage (existing ethrex-storage and ethrex-blockchain suites) exercises both call sites without modification, which is appropriate for a behavior-preserving refactor — no new tests are needed here.

Verdict: Looks good, no changes requested.

---

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

[github-actions]

🤖 Codex Code Review

No correctness, security, or performance findings in the diff. This reads as a straight refactor: both entry points now share the same trie-update logic, and the hash_address to hash_address_fixed swap is behaviorally equivalent in this code path.

Minor readability note:

• crates/storage/store.rs:2403-2406: the doc comment says the None branch opens a fresh storage trie “per account”, but the implementation actually opens one per update when the same account appears multiple times. Rewording that comment would make the helper contract more precise.

I could not run cargo test in this environment because Cargo/Rustup attempted to write under read-only /home/runner/.cargo and /home/runner/.rustup, so this review is static only.

---

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

[github-actions]

Lines of code report

Total lines added: 0
Total lines removed: 23
Total lines changed: 23

Detailed view

+--------------------------------+-------+------+
| File                           | Lines | Diff |
+--------------------------------+-------+------+
| ethrex/crates/storage/store.rs | 5084  | -23  |
+--------------------------------+-------+------+

[ilitteri]

Superseded by #7184: renaming the head branch closed this PR (GitHub closes PRs whose head branch is renamed); the same commits continue there under the new branch name.