feat(l1): state-history journal foundation for deep reorgs

[edg-l]

Part of the deep-reorg support effort tracked in #6685. This is PR 1 of 4 and lays the on-disk audit trail; no reorg consumer is wired yet.

Summary

• Adds a STATE_HISTORY column family that records a per-block reverse-diff journal: for every trie node or flat-KV entry overwritten at commit time, the pre-image is stored atomically in the same write batch.
• Finality pruning deletes the journal up to the new finalized block via a single delete_range call.
• Supersedes the earlier attempt in feat(l1): support deep reorgs #6561.

What's in this PR

• crates/storage/journal.rs (new, 395 LOC): JournalEntry struct, hand-rolled varint codec, 8 codec unit tests.
• crates/storage/api/mod.rs: delete_range method added to StorageWriteBatch trait.
• crates/storage/backend/rocksdb.rs: delete_range_cf impl.
• crates/storage/backend/in_memory.rs: iterate-and-retain delete_range impl.
• crates/storage/api/tables.rs: new STATE_HISTORY table constant.
• crates/storage/layering.rs: TrieLayer carries (block_number, block_hash); commit() returns CommitResult with block identity instead of bare Vec.
• crates/storage/store.rs: apply_trie_updates opens a read view before begin_write, collects pre-images per CF into FlatDiffs, stages the JournalEntry; forkchoice_update_inner prunes the journal when finalized advances. 5 new integration tests at bottom.
• crates/storage/trie.rs: extracted classify_trie_key helper (was duplicated across three call sites).

What's NOT in this PR

• Reorg consumer / state rollback (PR 2: overlay + read cascade).
• Lifting the 128-block reorg cap (PR 4).
• Hot-path pre-image reads are O(N) per block; PR 4 can batch via multi_get_cf.
• In-memory backend commit() is a pre-existing no-op (FIXME); the new delete_range inherits that but it is not a regression.
• Journaling is skipped entirely during full sync (batch_mode == true).

Test plan

• 16 unit + integration tests pass on both InMemory and RocksDB backends (cargo test -p ethrex-storage --features rocksdb).
• cargo fmt --check clean.
• cargo clippy -p ethrex-storage clean on all touched files.

Closes part of #6685. Supersedes #6561.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Stateless (zkEVM) Amsterdam+ EF tests skipped

Where: tooling/ef_tests/blockchain/test_runner.rs — parse_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]

Lines of code report

Total lines added: 1024
Total lines removed: 0
Total lines changed: 1024

Detailed view

+--------------------------------------------+-------+------+
| File                                       | Lines | Diff |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/api/mod.rs           | 54    | +6   |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/api/tables.rs        | 26    | +1   |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/backend/in_memory.rs | 187   | +16  |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/backend/rocksdb.rs   | 495   | +13  |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/journal.rs           | 475   | +475 |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/layering.rs          | 426   | +53  |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/lib.rs               | 18    | +1   |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/store.rs             | 3891  | +455 |
+--------------------------------------------+-------+------+
| ethrex/crates/storage/trie.rs              | 179   | +4   |
+--------------------------------------------+-------+------+

[github-actions]

🤖 Codex Code Review

1. High: crates/storage/journal.rs trusts the on-disk diff count and does Vec::with_capacity(count) before proving the payload can actually contain that many entries. A corrupted journal record can force a near-usize::MAX allocation and OOM/panic the node when history is read. Bound count and the per-entry lengths against bytes.len() - cur.offset before allocating.

2. Medium: crates/storage/journal.rs accepts trailing bytes silently. decode() parses the four sections and returns success without checking cur.offset == bytes.len(), so a corrupted or mixed-version record can be treated as valid if it has a valid prefix. For rollback data, that is too permissive; reject leftover bytes.

3. Medium: crates/storage/store.rs treats malformed FinalizedBlockNumber bytes as 0 via .try_into().ok()...unwrap_or(0). If that value is ever corrupted, the next FCU will think finality advanced from genesis and prune STATE_HISTORY far more aggressively than intended. This should return StoreError on invalid stored length/encoding instead of defaulting to zero.

Aside from those points, the block-number/block-hash plumbing through TrieLayerCache and batching the journal write with the trie writes looks consistent. I didn’t see EVM/gas or consensus-rule changes in this diff.

---

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

[github-actions]

🤖 Claude Code Review

Here is the review:

---

Review: feat(l1): state-history journal foundation for deep reorgs

Overall this is well-structured foundational work. The design decisions (write-once pre-image journal, BE keys for range pruning, journaling skipped in batch mode) are sound. The test coverage is good. A few correctness issues deserve attention before merge.

---

Bug: Varint overflow check is off-by-one (journal.rs:332–349)

if shift >= 64 {
    return Err(JournalDecodeError::VarintOverflow { ... });
}
result |= ((b & 0x7f) as u64) << shift;

shift advances in steps of 7 (0, 7, 14, … 56, 63, 70). The guard fires when shift == 70, meaning the 11th byte. But the 10th byte is processed when shift == 63: (b & 0x7f) as u64) << 63 silently discards data bits 1–6 (positions 64–69) because Rust u64 is 64 bits wide. A crafted journal with an encoded value > u64::MAX would be decoded as a truncated, wrong value rather than VarintOverflow.

The fix is to check the 10th byte explicitly before applying it:

// Before the shift:
if shift == 63 {
    if b & 0x7e != 0 {                     // data bits 1–6 must be zero
        return Err(JournalDecodeError::VarintOverflow { offset: self.offset - 1 });
    }
    result |= ((b & 0x01) as u64) << 63;
    if b & 0x80 != 0 {
        return Err(JournalDecodeError::VarintOverflow { offset: self.offset });
    }
    return Ok(result);
}

Since the journal is self-written the attack surface is low, but the corruption case (node writes a buggy encoding and reads it back wrong) is real.

---

Potential usize overflow in decode_flat_diff (journal.rs:353)

let count = cur.read_varint()? as usize;
let mut out = Vec::with_capacity(count);

A corrupt or truncated journal entry with a very large encoded count (e.g. u64::MAX) casts to usize::MAX on a 64-bit target and Vec::with_capacity will panic with an OOM condition rather than return a graceful Truncated error. The same applies to path_len and value_len in the same function.

Worth adding a sanity cap, e.g. remaining_bytes / MIN_ENTRY_SIZE, or at minimum a reasonable constant upper bound, so corrupt on-disk data doesn't crash the node on startup.

---

read_slice subtraction can underflow in release (journal.rs:316)

if self.bytes.len() - self.offset < n {

If (somehow) self.offset > self.bytes.len() this underflows a usize in release mode, wrapping to a large value and producing a false negative check. The current invariants make this unreachable in practice, but the safer form:

if n > self.bytes.len() - self.offset {   // same check, no underflow
// or
if self.offset.saturating_add(n) > self.bytes.len() {

…is worth using to make the invariant explicit.

---

CommitResult derives Default → silent block-0 journal risk (layering.rs:612)

#[derive(Debug, Default)]
pub struct CommitResult {
    pub block_number: BlockNumber,  // defaults to 0
    pub block_hash: H256,           // defaults to H256::zero()
    ...
}

If any future caller writes trie_mut.commit(root).unwrap_or_default() instead of the current ok_or_else(|| ...), a journal entry keyed at block 0 would be silently staged. The Default impl is not currently used anywhere in this PR; removing it eliminates the footgun with no loss.

---

In-memory delete_range is not transactional (in_memory.rs:70–86)

fn delete_range(...) {
    let mut db = self.backend.write()...;
    // Modifies data immediately — not staged for commit()
    table_ref.retain(|k, _| !(k.as_slice() >= start && k.as_slice() < end));
}

Unlike put/delete (which are presumably also immediate given the FIXME on commit), the range deletion fires during the call, not at commit(). This is consistent with the existing non-transactional in-memory behavior, but the commit FIXME note should mention delete_range too so future implementers know what needs to be made atomic.

---

diff_byte_estimate under-estimates for large values (journal.rs:286–291)

fn diff_byte_estimate(diff: &[ReverseDiffEntry]) -> usize {
    diff.iter()
        .map(|(p, v)| 2 + p.len() + 1 + v.as_ref().map_or(0, |v| 2 + v.len()))
        .sum::<usize>()
        + 2
}

The 2 per path/value assumes a 1-byte varint length prefix, but lengths ≥ 128 encode as 2+ bytes in LEB128. For paths of 65 bytes (account leaf), 67 bytes (storage trie node), 131 bytes (storage leaf), or large values, the estimate is consistently short. The Vec::with_capacity hint will be too small and trigger a reallocation on every non-trivial block. Not a correctness issue, but worth fixing for the write hot path.

A simple correction: replace the constant 2 with varint_len(p.len() as u64) using a small helper.

---

Test: assert_no_journal_entry 50 ms sleep is fragile (store.rs:3965)

fn assert_no_journal_entry(backend: &Arc<dyn StorageBackend>, block_number: BlockNumber) {
    std::thread::sleep(Duration::from_millis(50));
    ...
}

Under CI load (slow VMs, high parallelism) the async trie worker may not have committed within 50 ms, causing a false pass. The absence check should mirror the presence check: poll until await_journal_entry times out, not sleep-then-assert.

---

Minor: journal entry written for multi-layer commits uses newest block identity (layering.rs:587–591)

let top_layer = layers_to_commit.first()?;  // newest, at `state_root`
let committed_block_number = top_layer.block_number;
let committed_block_hash = top_layer.block_hash;
let committed_parent_state_root = top_layer.parent;

layers_to_commit is accumulated by walking parent links from state_root, so first() is the newest layer, not the oldest. In the normal single-layer case this is correct. In the multi-layer case the nodes vector contains merged diffs from multiple blocks, but they're all attributed to the newest block. This would produce an incorrect reverse-diff for rollback. The PR notes multi-layer commits are not normal, and PR 2 will own rollback, but it's worth a comment here so the PR 2 author knows to handle this.

---

Positive observations

• The decision to open the read view before begin_write for pre-image reads is correct and the comment explaining the single-writer invariant is clear.
• Using saturating_add(1) for the prune end key at store.rs:1104 is the right edge-case handling.
• classify_trie_key refactor cleanly removes three duplicated inline conditions.
• Version byte + strict VersionMismatch rejection is the right approach for a write-once format that will evolve.
• Atomic staging of both trie writes and the journal entry in a single write batch is the correct design.

---

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

[greptile-apps]

Greptile Summary

This PR lays the on-disk reverse-diff journal for deep reorg support, writing a per-block JournalEntry into STATE_HISTORY atomically with each trie commit, and pruning finalized entries via a single delete_range. No reorg consumer is wired yet; this is PR 1 of 4.

• New journal.rs with a hand-rolled LEB128 varint codec and JournalEntry struct covering all four trie/flat-KV column families, backed by 8 unit tests.
• TrieLayerCache::commit returns a richer CommitResult carrying block identity; apply_trie_updates opens a snapshot read view before the write batch to capture pre-images, then stages the journal entry in the same atomic WriteBatch; forkchoice_update_inner prunes STATE_HISTORY when finality advances.
• StorageWriteBatch gains a delete_range method implemented for both RocksDB (delete_range_cf) and the in-memory RCU backend.

Confidence Score: 4/5

The journaling foundation is architecturally sound — pre-image reads use the RCU snapshot before the write batch, journal writes are atomic with trie commits, and finality pruning is correctly guarded behind an advancing-finality check. Two narrow issues exist but neither touches production correctness in practice.

The LEB128 decoder silently truncates bits 1-6 of the final byte instead of rejecting the input, and the prev_finalized fallback-to-zero on a length mismatch could prune the entire journal under a future encoding divergence. Both are present defects in the changed code, though the first is harmless for self-written data today and the second requires a future schema change to trigger.

crates/storage/journal.rs (read_varint 10th-byte check) and the prev_finalized read in crates/storage/store.rs forkchoice_update_inner deserve a second look before the reorg consumer (PR 2) starts relying on the journal.

Important Files Changed

Filename	Overview
crates/storage/journal.rs	New file: JournalEntry struct, hand-rolled LEB128 varint codec, and 8 unit tests. A subtle overlong-encoding acceptance flaw exists in read_varint at the 10th byte.
crates/storage/store.rs	apply_trie_updates now collects pre-images via a snapshot read_view opened before the write batch and stages the JournalEntry atomically; forkchoice_update_inner prunes STATE_HISTORY when finality advances. Logic is sound; 5 new integration tests verify write and prune paths.
crates/storage/layering.rs	TrieLayer gains block_number and block_hash fields; commit() returns CommitResult instead of bare Vec. Refactor is clean and correct.
crates/storage/backend/in_memory.rs	delete_range follows the same RCU/Arc::make_mut pattern as existing put_batch and delete; consistent with pre-existing non-atomic commit() semantics.
crates/storage/backend/rocksdb.rs	delete_range_cf delegates to RocksDB's WriteBatch::delete_range_cf, correctly buffered until commit(). Implementation is straightforward.
crates/storage/api/mod.rs	delete_range added to StorageWriteBatch trait with clear half-open range semantics documented. Both implementors updated.
crates/storage/api/tables.rs	STATE_HISTORY column family constant added with big-endian key semantics clearly documented; TABLES array updated to 21 entries.
crates/storage/trie.rs	classify_trie_key extracted as a pub helper, de-duplicating an inlined key-length check from three call sites. Clean refactor with no behavioral change.
crates/storage/lib.rs	journal module added to the crate's public API. Trivial one-line change.

Sequence Diagram

sequenceDiagram
    participant BP as Block Producer
    participant TW as TrieUpdate Worker
    participant Cache as TrieLayerCache
    participant DB as Backend (RocksDB)
    participant SH as STATE_HISTORY CF

    BP->>TW: store_block_updates(BlockN)
    TW->>Cache: put_batch(parent_root, child_root, block_N, hash_N, nodes)
    Cache-->>TW: layer cached
    TW-->>BP: Ok (phase 1 done)

    Note over TW: Phase 2 - disk commit (BlockN-1)
    TW->>Cache: commit(root_N-1)
    Cache-->>TW: CommitResult
    TW->>DB: begin_read() - snapshot (pre-write view)
    loop for each (key, value) in nodes
        TW->>DB: snapshot.get(table, key) - prev_value
        TW->>DB: write_batch.put/delete(table, key, value)
    end
    TW->>SH: write_batch.put(block_N-1.to_be_bytes(), JournalEntry.encode())
    TW->>DB: write_batch.commit() [atomic]

    Note over BP,SH: On finality advance (FCU)
    BP->>DB: begin_read() - get(CHAIN_DATA, finalized_key) - prev_finalized
    BP->>DB: write_batch.put(CHAIN_DATA, finalized_key, new_finalized)
    BP->>SH: write_batch.delete_range([0, new_finalized+1))
    BP->>DB: write_batch.commit() [atomic]

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/storage/journal.rs:211-228
The overflow check fires on the 11th byte (after `shift` increments to 70), not on the 10th. At `shift=63` the check `shift >= 64` is still false, so `result |= ((b & 0x7f) as u64) << 63` runs. Because `u64` is 64 bits, only bit 0 of `b & 0x7f` survives the shift—bits 1-6 are silently dropped. A final byte of, say, `0x7e` (bits 1-5 set, no continuation) decodes to the same value as `0x00`, masking data corruption in the journal without returning `VarintOverflow`. The standard fix is to add a check for `shift == 63 && b & 0x7e != 0`.

```suggestion
    fn read_varint(&mut self) -> Result<u64, JournalDecodeError> {
        let mut result: u64 = 0;
        let mut shift: u32 = 0;
        loop {
            let b = self.read_byte()?;
            // Maximum 10 bytes for u64 LEB128 (10 * 7 = 70 > 64).
            if shift >= 64 {
                return Err(JournalDecodeError::VarintOverflow {
                    offset: self.offset - 1,
                });
            }
            // At shift=63 only bit 0 of the final byte is in range for u64;
            // bits 1-6 would shift past position 63 and be silently dropped.
            if shift == 63 && b & 0x7e != 0 {
                return Err(JournalDecodeError::VarintOverflow {
                    offset: self.offset - 1,
                });
            }
            result |= ((b & 0x7f) as u64) << shift;
            if b & 0x80 == 0 {
                return Ok(result);
            }
            shift += 7;
        }
    }
```

### Issue 2 of 2
crates/storage/store.rs:1081-1090
**`prev_finalized` may spuriously read back as 0 on data length mismatch**

`bytes.try_into().ok()` silently returns `None` (falls through to `unwrap_or(0)`) whenever the stored byte slice is not exactly 8 bytes—e.g., after a future schema change that writes a different width for `FinalizedBlockNumber`. In that case `prev_finalized = 0`, so `finalized > 0` is almost always true, and the entire journal from block 0 onwards is pruned even if finality did not advance. Since `CHAIN_DATA` already encodes this field as `to_le_bytes()` (8 bytes) and the same codec is used when writing, the current code cannot hit this branch in practice—but the silent fallback hides the bug if encoding ever diverges.

_{Reviews (1): Last reviewed commit: "feat(l1): persist state-history journal ..." | Re-trigger Greptile}

[edg-l]

Pushed a fixup addressing all bot findings. Short responses:

Greptile / Claude — varint 10th-byte truncation ; fixed with shift==63 && (b & 0x7e != 0 || b & 0x80 != 0) guard. Tests added: truncating 10th byte, 11th byte, continuation-at-byte-10, and u64::MAX round-trip still works.

Codex / Claude — Vec::with_capacity OOM on malformed count/path_len/value_len ; new LengthExceedsRemaining error variant; all three length prefixes now bounded against cur.remaining() before allocating. 3 OOM-attack tests.

Greptile / Codex — prev_finalized silent fallback to 0 ; length mismatch now returns StoreError::Custom instead. Test plants a 4-byte value and asserts the error path + that no pruning occurred.

Codex — decode() accepts trailing bytes ; new TrailingBytes variant + end-of-decode check. Test added.

Claude — assert_no_journal_entry 50ms sleep is CI-fragile ; now polls every 20ms over a 500ms window, fails loudly on any observation.

Claude — CommitResult: Default footgun ; removed the Default derive; doc-comment explains why.

Claude — read_slice subtraction underflow ; saturating_sub form, added remaining() helper.

Claude — diff_byte_estimate under-estimates ; new varint_len() helper; estimate now matches encoded length exactly (asserted by test).

Claude — multi-layer commit attribution ; added code comment explaining the single-layer invariant.

Claude — in-memory commit() FIXME ; expanded to call out delete_range and the multi-op atomicity gap.

26 storage tests pass on InMemory and RocksDB; clippy + fmt clean.

[ElFantasma]

Three inline findings (all minor) + a body note.

Body finding: the body's checklist says "no STORE_SCHEMA_VERSION bump (no schema migration required)". Verified — create_missing_column_families(true) (rocksdb.rs:34) creates STATE_HISTORY on the fly when a v13 binary opens a v12 DB, and existing CFs are untouched. Downgrade (v12 binary on v13 DB) is rejected by the TABLES.contains(&cf_name) check at rocksdb.rs:203, which is correct one-way-migration behavior. Worth noting explicitly in the body or CHANGELOG so an operator considering rollback knows the v12 binary won't open a v13 DB.

[ElFantasma]

Re-review after clippy Box + docs tweak — still LGTM, no new issues.