fix(l1): fix BLOCKHASH cross-batch resolution during block import

[edg-l]

Fixes #6385.

When importing blocks in batches, BLOCKHASH could fail to resolve hashes for blocks that landed in a prior batch -- those blocks aren't canonical yet during import, so they were absent from the cache and any lookup returned zero.

The fix walks the parent chain from the first block of the current batch, caching up to 256 prior block hashes into block_hash_cache before execution begins. The walk breaks early if storage doesn't have a header (e.g. genesis boundary), so there's no impact on single-batch or shallow imports.

Regression test in test/tests/blockchain/batch_tests.rs: deploys a contract that calls BLOCKHASH(number - 1) and invokes it from a second batch, confirming the hash is resolved correctly.

[github-actions]

Lines of code report

Total lines added: 21
Total lines removed: 0
Total lines changed: 21

Detailed view

+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2507  | +21  |
+----------------------------------------+-------+------+

[github-actions]

🤖 Codex Code Review

1. Error handling regression in the new cache warmup path: blockchain.rs:2164. The loop matches get_block_header_by_hash(hash) with _ => break, so a real StoreError is silently downgraded into a truncated block_hash_cache. In batch import that can turn a storage corruption / I/O failure into a later, less actionable BLOCKHASH miss, or hide the underlying DB problem entirely if that hash is never queried. This should propagate Err(e) as ChainError::StoreError(e) and only stop early on Ok(None).

No other blocking correctness, gas-accounting, or consensus issues stood out in the diff. The cache preload itself follows the exact parent-hash chain, so it looks branch-correct for non-canonical batch imports.

Non-blocking: the regression test is directionally good, but it only asserts that batch 2 succeeds. It would be stronger to also assert the contract stored block2.hash() after execution, so a wrong-hash return path cannot pass unnoticed. Relevant area: batch_tests.rs:395.

I could not run the test locally in this sandbox because cargo/rustup needs writable dependency state under /home/runner/.cargo and /home/runner/.rustup, and network access is blocked.

---

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

[github-actions]

🤖 Claude Code Review

Here is my review of PR 6386:

---

PR 6386 — Fix BLOCKHASH cross-batch resolution during block import

Overall: The fix is conceptually sound and correctly addresses the reported bug. The approach of pre-populating the block_hash_cache with the prior 256 headers before execution is the right place to solve this. A few correctness and quality concerns are worth addressing before merging.

---

crates/blockchain/blockchain.rs

Redundant storage read on the first loop iteration

parent_header was already fetched just above the walk block, but on the first loop iteration get_block_header_by_hash(hash) re-fetches it (since hash = parent_header.hash()). That is one wasted I/O round-trip per batch. The fix is to seed the walk one step deeper:

// before the loop, we already have parent_header in hand
let mut hash = parent_header.parent_hash;
let mut number = parent_header.number.saturating_sub(1);
block_hash_cache.entry(parent_header.number).or_insert(parent_header.hash());
block_hash_cache.entry(number).or_insert(hash);
let lookback = first_block_header.number.saturating_sub(256);
while number > lookback {
    match self.storage.get_block_header_by_hash(hash) { ... }
}

This saves one header lookup per add_blocks_in_batch call.

Silent error swallow on storage failure (_ => break at line 2170)

Any Err(_) from the store breaks the loop early without propagating the error, leaving the cache potentially incomplete. A block later in the same batch that references a prior-batch hash via BLOCKHASH would then silently get H256::zero(). Whether this is acceptable (degrade gracefully) or should be a hard error depends on design intent, but at minimum it deserves a log/trace statement:

Err(e) => {
    tracing::warn!("block_hash_cache walk interrupted: {e}");
    break;
}
Ok(None) => break,

number counter maintained via saturating_sub(1) rather than header.number (line 2167)

The code assumes the fetched header's block number equals the current counter value, then decrements the counter arithmetically. If there were ever an unexpected header shape in storage this could silently drift. Using header.number.saturating_sub(1) (or asserting the invariant) would be more defensive:

Ok(Some(header)) => {
    // header.number should equal `number`; use it directly for safety
    number = header.number.saturating_sub(1);
    hash = header.parent_hash;
    block_hash_cache.entry(number).or_insert(hash);
}

Lookback boundary is based on first_block_header.number — this is correct

The BLOCKHASH opcode can look back at most 256 blocks. The earliest reachable block from any block in the batch is first_block_number - 256 (the first block has the smallest number, hence the widest window). Using first_block_header.number.saturating_sub(256) is the correct anchor. No issue here.

The extra block scope { ... } around the walk (lines 2158–2173)

The block scope does not prevent any variable leaking (both hash and number are let-bound inside it), so the extra braces add visual noise without benefit. Consider removing them.

---

test/tests/blockchain/batch_tests.rs

Dead code left in the deploy code builder (lines ~295–307)

The first few init.push(...) lines before init.clear() are reachable dead code that was clearly exploratory:

init.push(0x60 + 0); // PUSH1
init.push(runtime.len() as u8);
init.push(0x60); // PUSH1
init.push(0x00); // offset 0
init.push(0x39); // CODECOPY — but we need the runtime in memory first

// Simpler: use PUSH bytes directly
// ...
init.clear();

All of this from init.push(0x60 + 0) through init.clear() should be removed before merging.

Stale comment "// we'll verify this"

The init_len line reads:

let init_len = 12; // we'll verify this

The assertion below does verify it, so the comment is misleading. Just remove it.

Test only checks import success, not the BLOCKHASH result value

The regression is about BLOCKHASH returning the correct non-zero value, but the test only asserts result2.is_ok(). A spurious zero returned by BLOCKHASH would still make the import succeed (no panic). Adding a storage-slot read from store_b to confirm the stored hash equals the expected hash of block 2 would make the test catch actual regression, not just crashes:

// After batch 2 succeeds, read slot 0 of contract_address from store_b
// and assert it equals block2.hash()

---

Minor

• The get_chain_config() call at line 2142 is unrelated to this diff, but it returns ChainConfig by value without any error path — if this is ever fallible in the future it will require a larger refactor. No action needed here.
• 0x60 + 0 in the dead code is just 0x60; in the live code the comment // PUSH1 is sufficient.

---

Summary

Severity	Item
Minor correctness	Redundant get_block_header_by_hash on first loop iteration (wasted I/O)
Observability	_ => break silently drops store errors; add a tracing::warn!
Robustness	Use header.number instead of arithmetic saturating_sub(1) for the counter
Test quality	Dead code before init.clear() should be removed
Test quality	Assert the actual stored BLOCKHASH value, not just import success

The core logic (range, direction, or_insert semantics) is correct. Clean up the items above and this is ready to merge.

---

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

[github-actions]

🤖 Codex Code Review

1. The new regression test is too weak to prove BLOCKHASH correctness. In batch_tests.rs:404 it only asserts that batch 2 does not error. That catches the old “hash not found” failure mode, but it would still pass if the opcode returned the wrong in-range hash and execution stayed successful. Since the contract writes the result to storage and never validates it, I’d add an assertion after batch 2 that storage slot 0x0 at contract_address equals U256::from_big_endian(block2.hash().as_bytes()).

No production-side correctness or security issues stood out in blockchain.rs:2144. The ancestor preload matches the EVM’s 256-block BLOCKHASH window and fits the non-canonical lookup path in vm.rs:151.

---

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

[github-actions]

🤖 Claude Code Review

Here is the review:

---

Review: fix(l1): BLOCKHASH cross-batch resolution during block import

Overall: The fix is logically correct and addresses a real consensus bug. Most issues below are minor, but one deserves attention before merge.

---

crates/blockchain/blockchain.rs

Walk logic correctness — OK

The hash walk (lines 2158-2172) is correct:

• Before the loop, parent_header.hash() is cached at parent_header.number (first_block_number - 1). ✓
• lookback = first_block_header.number.saturating_sub(256) and the loop exits at number <= lookback, so the range [first_block_number - 256, first_block_number - 1] is fully populated (255 iterations + the pre-loop entry = 256 entries). ✓
• or_insert correctly avoids overwriting current-batch hashes when blocks from the previous batch overlap the 256-block window. ✓
• Genesis boundary is handled: saturating_sub(256) = 0 when the batch starts near genesis, and the loop exits naturally when number = 0. ✓

For multi-block batches: any block at position j inside the batch can BLOCKHASH back to first_block_number + j - 256. Since j >= 0, the furthest back is still first_block_number - 256, so the single walk from first_block_number - 1 is sufficient for all blocks in the batch. ✓

Silent error suppression (minor bug)

// blockchain.rs ~line 2164
_ => break,

The wildcard arm silently absorbs both Ok(None) (expected genesis boundary) and Err(e) (a real storage failure). A storage error during the cache walk would cause an incomplete cache and lead to BLOCKHASH returning 0 for blocks that exist, with no diagnostic output. Distinguish the two cases:

Ok(None) => break, // genesis boundary
Err(e) => {
    warn!("Failed to fetch block header during hash cache warmup: {e}");
    break;
}

Unnecessary scope block (style)

The { ... } block enclosing the walk (lines 2158-2173) has no functional purpose — there are no resources that need early dropping, and hash/number are local scalars. Remove the scope to reduce indentation noise.

Performance note (acceptable)

Up to 256 sequential synchronous storage reads per batch invocation. This is a one-time cost per batch, not per block, and is the minimum needed to populate the cache. Acceptable given the alternative.

---

test/tests/blockchain/batch_tests.rs

Test validity — sound, but worth a comment

The test correctly relies on state root verification: block3 was built with the correct BLOCKHASH result, so its header.state_root encodes the correct contract storage. On re-import via blockchain_b, if BLOCKHASH returns 0 (the pre-fix behavior), the stored value differs, the state root mismatches, and add_blocks_in_batch returns an error. So assert!(result2.is_ok()) is a meaningful assertion, not just a liveness check.

This is non-obvious. A comment like the following would help readers:

// The block was built with the correct BLOCKHASH result, so its state_root
// reflects that. If BLOCKHASH resolves incorrectly (returns 0) during
// re-import, the computed state root will diverge and the block is rejected.

Dead code in deploy_code construction (cleanup required)

Lines 281-285 push 5 bytes into init, then line 292 calls init.clear(), discarding all of it. Those lines and the abandoned comments are dead code left from an abandoned approach:

// Dead — cleared immediately after:
init.push(0x60 + 0); // PUSH1
init.push(runtime.len() as u8);
init.push(0x60);      // PUSH1
init.push(0x00);      // offset 0
init.push(0x39);      // CODECOPY — but we need the runtime in memory first

// Simpler: use PUSH bytes directly
// PUSH10 <runtime_bytes> PUSH1 0 MSTORE ...
// Actually let's just use a minimal init code:
let rt_len = runtime.len();
init.clear();   // <-- clears everything above

This should be cleaned up: drop the dead pushes, the abandoned comments, and start with let mut init = Vec::new() immediately before the CODECOPY-based init code. The let rt_len line can also move inline.

Bytecode trace is correct

NUMBER PUSH1 1 SWAP1 SUB BLOCKHASH PUSH1 0 SSTORE STOP correctly computes block_number - 1:

• After PUSH1 1: stack = [1, block_number]
• After SWAP1: stack = [block_number, 1]
• After SUB (top − second): stack = [block_number - 1]

The init_len = 12 assertion is a reasonable guard. ✓

---

Summary

Severity	Location	Finding
Minor bug	blockchain.rs:2170	_ => break swallows storage errors silently
Cleanup	batch_tests.rs:281-292	5 dead pushes + stale comments before init.clear()
Style	blockchain.rs:2158	Unnecessary { } scope around the walk block
Nit	batch_tests.rs:395-411	Add a comment explaining why is_ok() is sufficient

The core fix is correct. The dead code in the test is the most visible thing to clean up before merge.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a cross-batch BLOCKHASH resolution bug in add_blocks_in_batch: blocks imported in a prior batch are not yet canonical during import of the next batch, so their hashes were absent from the per-batch cache and BLOCKHASH returned zero, producing a state-root mismatch.

Key changes:

• Before beginning batch execution, a parent-chain walk traverses up to 256 ancestors starting from the batch's parent header, populating block_hash_cache with (block_number → block_hash) entries for any blocks that predate the current batch.
• or_insert is used throughout so entries already present from the current batch are never overwritten, and the walk breaks early on a missing header (e.g. genesis boundary).
• A regression test builds blocks 1-2 in one batch and block 3 (which calls BLOCKHASH(block_number - 1)) in a second batch, verifying the state root matches — which only holds if the hash was resolved correctly.

Minor observations:

• The loop's first iteration redundantly fetches parent_header from storage via get_block_header_by_hash, even though parent_header is already in scope.
• The test contains dead code from an abandoned first approach to constructing init bytecode (5 bytes pushed to init before init.clear()).

Confidence Score: 5/5

• Safe to merge — the fix is logically correct, well-scoped, and covered by a targeted regression test.
• The parent-chain walk correctly caches exactly the 256 hashes that EVM's BLOCKHASH can reference before the batch start, uses or_insert to avoid clobbering in-batch entries, and breaks safely at genesis. The regression test is valid because it relies on state-root verification to confirm the hash was resolved correctly, not just that no exception was thrown. Remaining comments are style-level only.
• No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/blockchain.rs	Adds a pre-execution parent-chain walk caching up to 256 block hashes before batch execution begins, correctly fixing cross-batch BLOCKHASH resolution. Logic is sound; one minor redundant storage lookup in the first loop iteration.
test/tests/blockchain/batch_tests.rs	Valid regression test that deploys a BLOCKHASH-dependent contract in batch 1 and invokes it in batch 2; relies on state-root mismatch to detect incorrect results. Contains dead code from an abandoned first attempt at building init bytecode (5 bytes pushed before init.clear()).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[add_blocks_in_batch called] --> B[Build batch cache\nblocks.iter → block_hash_cache]
    B --> C[Fetch parent_header\nfrom storage by hash]
    C --> D[Seed walk:\ncache parent_header.number → parent_header.hash]
    D --> E{number > lookback?\nlookback = first_block_number − 256}
    E -- Yes --> F[get_block_header_by_hash hash]
    F -- Ok Some header --> G[hash = header.parent_hash\nnumber -= 1\ncache entry with or_insert]
    G --> E
    F -- Err / None --> H[break — genesis boundary reached]
    E -- No --> I[256 entries cached]
    H --> I
    I --> J[StoreVmDatabase::new_with_block_hash_cache\nparent_header + block_hash_cache]
    J --> K[Execute each block in batch\nBLOCKHASH resolved from cache]
    K --> L[Validate state root\nstore_block_updates]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/tests/blockchain/batch_tests.rs
Line: 278-289

Comment:
**Dead code from abandoned first attempt**

The initial `init` construction (lines 278–289) is never used — `init.clear()` is called immediately after, discarding those 5 bytes. This is leftover scaffolding from an earlier draft of the test and adds noise. Consider removing it:

```suggestion
    let deploy_code = {
        let runtime = vec![0x43, 0x60, 0x01, 0x90, 0x03, 0x40, 0x60, 0x00, 0x55, 0x00];
        let rt_len = runtime.len();
        let mut init = Vec::new();
        // PUSH1 rt_len  PUSH1 (init_len) PUSH1 0 CODECOPY  PUSH1 rt_len PUSH1 0 RETURN
        let init_len = 12; // we'll verify this
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2158-2173

Comment:
**Redundant storage round-trip on first iteration**

The loop opens by calling `get_block_header_by_hash(hash)` where `hash = parent_header.hash()`. This fetches `parent_header` from storage again, even though it is already available in the local variable `parent_header`. The redundant lookup can be avoided by seeding the walk directly from `parent_header.parent_hash`:

```suggestion
        {
            // Seed the walk with the grandparent hash directly from the
            // already-fetched parent_header, avoiding a redundant storage lookup.
            let mut hash = parent_header.parent_hash;
            let mut number = parent_header.number.saturating_sub(1);
            block_hash_cache.entry(number).or_insert(hash);
            let lookback = first_block_header.number.saturating_sub(256);
            while number > lookback {
                match self.storage.get_block_header_by_hash(hash) {
                    Ok(Some(header)) => {
                        hash = header.parent_hash;
                        number = number.saturating_sub(1);
                        block_hash_cache.entry(number).or_insert(hash);
                    }
                    _ => break,
                }
            }
        }
```

Note: this suggestion also drops the separate `block_hash_cache.entry(parent_header.number).or_insert(parent_header.hash())` line before the loop since that entry was already inserted via the initial `blocks.iter()` map if the parent happens to be in the batch, but `parent_header` is always one block _before_ the batch, so you may want to keep that first `or_insert` call as well and just remove the first loop iteration.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix(l1): populate bl..."}

[greptile-apps]

Dead code from abandoned first attempt

The initial init construction (lines 278–289) is never used — init.clear() is called immediately after, discarding those 5 bytes. This is leftover scaffolding from an earlier draft of the test and adds noise. Consider removing it:

Suggested change

	let runtime = vec![0x43, 0x60, 0x01, 0x90, 0x03, 0x40, 0x60, 0x00, 0x55, 0x00];
	let mut init = Vec::new();
	// PUSH<len> <runtime> PUSH1 0 RETURN
	init.push(0x60 + 0); // PUSH1
	init.push(runtime.len() as u8);
	init.push(0x60); // PUSH1
	init.push(0x00); // offset 0
	init.push(0x39); // CODECOPY — but we need the runtime in memory first
	// Simpler: use PUSH bytes directly
	// PUSH10 <runtime_bytes> PUSH1 0 MSTORE PUSH1 10 PUSH1 22 RETURN
	// Actually let's just use a minimal init code:
	let deploy_code = {
	let runtime = vec![0x43, 0x60, 0x01, 0x90, 0x03, 0x40, 0x60, 0x00, 0x55, 0x00];
	let rt_len = runtime.len();
	let mut init = Vec::new();
	// PUSH1 rt_len PUSH1 (init_len) PUSH1 0 CODECOPY PUSH1 rt_len PUSH1 0 RETURN
	let init_len = 12; // we'll verify this

Prompt To Fix With AI

This is a comment left during a code review.
Path: test/tests/blockchain/batch_tests.rs
Line: 278-289

Comment:
**Dead code from abandoned first attempt**

The initial `init` construction (lines 278–289) is never used — `init.clear()` is called immediately after, discarding those 5 bytes. This is leftover scaffolding from an earlier draft of the test and adds noise. Consider removing it:

```suggestion
    let deploy_code = {
        let runtime = vec![0x43, 0x60, 0x01, 0x90, 0x03, 0x40, 0x60, 0x00, 0x55, 0x00];
        let rt_len = runtime.len();
        let mut init = Vec::new();
        // PUSH1 rt_len  PUSH1 (init_len) PUSH1 0 CODECOPY  PUSH1 rt_len PUSH1 0 RETURN
        let init_len = 12; // we'll verify this
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Redundant storage round-trip on first iteration

The loop opens by calling get_block_header_by_hash(hash) where hash = parent_header.hash(). This fetches parent_header from storage again, even though it is already available in the local variable parent_header. The redundant lookup can be avoided by seeding the walk directly from parent_header.parent_hash:

Suggested change

	{
	let mut hash = parent_header.hash();
	let mut number = parent_header.number;
	block_hash_cache.entry(number).or_insert(hash);
	let lookback = first_block_header.number.saturating_sub(256);
	while number > lookback {
	match self.storage.get_block_header_by_hash(hash) {
	Ok(Some(header)) => {
	hash = header.parent_hash;
	number = number.saturating_sub(1);
	block_hash_cache.entry(number).or_insert(hash);
	}
	_ => break,
	}
	}
	}
	{
	// Seed the walk with the grandparent hash directly from the
	// already-fetched parent_header, avoiding a redundant storage lookup.
	let mut hash = parent_header.parent_hash;
	let mut number = parent_header.number.saturating_sub(1);
	block_hash_cache.entry(number).or_insert(hash);
	let lookback = first_block_header.number.saturating_sub(256);
	while number > lookback {
	match self.storage.get_block_header_by_hash(hash) {
	Ok(Some(header)) => {
	hash = header.parent_hash;
	number = number.saturating_sub(1);
	block_hash_cache.entry(number).or_insert(hash);
	}
	_ => break,
	}
	}
	}

Note: this suggestion also drops the separate block_hash_cache.entry(parent_header.number).or_insert(parent_header.hash()) line before the loop since that entry was already inserted via the initial blocks.iter() map if the parent happens to be in the batch, but parent_header is always one block before the batch, so you may want to keep that first or_insert call as well and just remove the first loop iteration.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2158-2173

Comment:
**Redundant storage round-trip on first iteration**

The loop opens by calling `get_block_header_by_hash(hash)` where `hash = parent_header.hash()`. This fetches `parent_header` from storage again, even though it is already available in the local variable `parent_header`. The redundant lookup can be avoided by seeding the walk directly from `parent_header.parent_hash`:

```suggestion
        {
            // Seed the walk with the grandparent hash directly from the
            // already-fetched parent_header, avoiding a redundant storage lookup.
            let mut hash = parent_header.parent_hash;
            let mut number = parent_header.number.saturating_sub(1);
            block_hash_cache.entry(number).or_insert(hash);
            let lookback = first_block_header.number.saturating_sub(256);
            while number > lookback {
                match self.storage.get_block_header_by_hash(hash) {
                    Ok(Some(header)) => {
                        hash = header.parent_hash;
                        number = number.saturating_sub(1);
                        block_hash_cache.entry(number).or_insert(hash);
                    }
                    _ => break,
                }
            }
        }
```

Note: this suggestion also drops the separate `block_hash_cache.entry(parent_header.number).or_insert(parent_header.hash())` line before the loop since that entry was already inserted via the initial `blocks.iter()` map if the parent happens to be in the batch, but `parent_header` is always one block _before_ the batch, so you may want to keep that first `or_insert` call as well and just remove the first loop iteration.

How can I resolve this? If you propose a fix, please make it concise.

[ElFantasma]

LGTM, just pointed out the same nit that greptile found.