fix: repoint canonical index on BSC reorg

[iovoid]

Problem

The BSC block-import paths (p2p/rlpx/connection/server.rs NewBlock import and p2p/sync/full.rs forward sync) canonicalize each block via Blockchain::advance_canonical_head, which:

• early-returns when number <= latest, and
• passed only vec![(number, hash)] (the single new head) to forkchoice_update.

forkchoice_update_inner writes only the pairs it's handed and deletes entries above the head — it never rewrites heights between the common ancestor and the head. So when a heavier chain reorgs blocks at heights <= the current head and then extends past it, the reorged heights are stored (by hash) but their block_number -> canonical_hash index entries are never repointed: they keep pointing at the reorged-out fork.

BLOCKHASH / get_block_hash read that by-number index, so they return stale hashes for the reorged heights. Any contract reading BLOCKHASH of such a height (e.g. blockhash-based randomness) then diverges from canonical execution.

This wedged a BSC mainnet node at block 105536921: blocks 105536905–105536909 had a stale canonical index after a 5-block reorg; tx 205 (settle(betId)) read BLOCKHASH of a reorged height, computed the wrong outcome, skipped its payout, and produced a block gas total 49,559 short of the header — rejected on every retry.

The L1 engine-API and L2 paths are unaffected: they canonicalize through apply_fork_choice -> find_link_with_canonical_chain, which already rewrites the whole branch.

Fix

advance_canonical_head now mirrors apply_fork_choice: when the new head extends the chain (number > latest), it walks back to the common canonical ancestor via find_link_with_canonical_chain and hands the whole branch to forkchoice_update, so every reorged height is repointed — not just the head. The number <= latest guard (which avoids rewinding a concurrently-advanced head) is preserved, and it falls back to head-only if the header/link can't be found. find_link_with_canonical_chain is now pub(crate).

Linear-extension overhead: one extra header lookup + one is_canonical check.

Test

advance_canonical_head_repoints_reorged_heights (test/tests/blockchain/smoke_tests.rs): canonicalizes chain A via advance_canonical_head, then imports a heavier chain B that reorgs heights 1–3 and extends to 4, and asserts every chain-B height (including the reorged ones) is canonical. Fails before the fix ("height 1 should be canonical after reorg"), passes after. All 7 smoke tests pass; clippy + fmt clean.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Why and resolution path

PR #6527 broadened
test-stateless to extract the entire for_amsterdam/ tree from the
zkevm bundle and run all of it under --features stateless; combined with
this branch's bal-devnet-7 semantics that scope produces ~549
GasUsedMismatch / ReceiptsRootMismatch /
BlockAccessListHashMismatch failures.

test-stateless-zkevm filters cargo to the eip8025_optional_proofs
suite, which still validates the stateless harness without the bal-version
mismatch.

Re-broaden by switching test: back to test-stateless in
tooling/ef_tests/blockchain/Makefile once the zkevm bundle is regenerated
against the current bal spec.

[github-actions]

🤖 Kimi Code Review

This PR fixes a critical consensus bug where the canonical number→hash index was not updated for reorged heights during per-block canonicalization (BSC-style import). This caused BLOCKHASH to return stale hashes after reorgs, leading to EVM state divergence.

Overall Assessment: Correct and necessary fix. The logic properly walks back to the common ancestor to repoint all affected heights, mirroring the existing apply_fork_choice behavior.

Specific Feedback

crates/blockchain/blockchain.rs

Lines 1955-1961: Consider returning an explicit error when the header is not found rather than proceeding with an empty vector.

// Current:
None => Vec::new(),

// Suggested:
None => return Err(BlockchainError::BlockNotFound(hash)), // or appropriate error type

While the old code didn't validate existence either, silently proceeding with an empty new_canonical_blocks vector means forkchoice_update receives a head hash that doesn't exist in storage. This could mask logic errors upstream.

Lines 1944-1954: Excellent comment explaining the BSC regression and the rationale for walking back to the common ancestor.

crates/blockchain/fork_choice.rs

Line 219: The pub(crate) visibility change is appropriate. This keeps the function internal to the crate while allowing reuse from blockchain.rs.

test/tests/blockchain/smoke_tests.rs

Lines 345-412: The regression test effectively captures the failure mode:

• Creates a 3-block chain A and canonicalizes per-block
• Creates a 4-block chain B reorging from genesis
• Verifies all heights 1-3 are repointed to chain B
• Verifies stale chain A hashes are evicted from the index

The use of new_block (which randomizes extra data to ensure unique hashes) is correct for ensuring distinct block hashes between chains.

Minor suggestion: Add a comment noting that new_block uses randomized fields to guarantee hash(A_n) != hash(B_n) at the same height, as this is crucial for the test's validity.

Security Consideration

This fix is consensus-critical. Without it, post-reorg BLOCKHASH operations return hashes from the orphaned fork, causing deterministic state divergence between nodes. The fix ensures atomic repointing of the entire reorged segment, maintaining EVM execution consistency.

Performance Note

find_link_with_canonical_chain walks the chain backwards until finding a canonical ancestor. In deep reorgs (e.g., >1000 blocks), this incurs O(reorg_depth) storage lookups. While acceptable for correctness, consider documenting this behavior if deep reorgs are expected in the target network (e.g., BSC's fast finality vs Ethereum's probabilistic finality).

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. The reorg fix is correct in isolation, but the concurrency guarantee in advance_canonical_head is still not sound. The number <= latest guard is evaluated from the cache before entering the serialized fork-choice path at crates/blockchain/blockchain.rs:1940, and this PR adds extra async ancestry work before the write at crates/blockchain/blockchain.rs:1953. Two BSC tip imports can still both pass that stale check, then the older one can acquire the store lock second at crates/storage/store.rs:2334 and rewind the canonical head by deleting newer mappings above its head in crates/storage/store.rs:1046. The comment claiming the guard prevents concurrent rewinds is therefore misleading unless the guard is re-checked under the same lock/transaction as forkchoice_update.

Aside from that, the branch-repointing change and the regression test look good: they address the stale BLOCKHASH/canonical-index problem the old single-height update could leave behind.

I could not run the targeted test locally because cargo tried to update the Rust toolchain under /home/runner/.rustup, which is read-only in this environment.

---

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

[github-actions]

🤖 Claude Code Review

I now have enough context for a thorough review. Here it is:

---

PR Review: fix: repoint canonical index on BSC reorg

Overview

This PR fixes a real, production-confirmed bug in the BSC block-import path. When a heavier chain reorgs heights at or below the current head and then extends past it, advance_canonical_head previously passed only the single new head to forkchoice_update. That left the number → hash index entries for all reorged heights pointing at the old fork, causing BLOCKHASH/get_block_hash to return stale data — a divergence that wedged a BSC mainnet node at block 105536921. The fix mirrors the existing apply_fork_choice path by walking back to the common canonical ancestor via find_link_with_canonical_chain and handing the full branch to forkchoice_update.

The approach is sound, the root-cause analysis is correct, and the test is solid.

---

Correctness

The happy path is correct. find_link_with_canonical_chain returns the intermediate non-canonical blocks (ancestor+1 … head-1) as (number, hash) pairs, and forkchoice_update_inner writes those plus the head separately. All reorged heights are repointed atomically in a single RocksDB transaction. This matches what apply_fork_choice does.

The Ok(Some([])) case is correct. When the new head is already canonical (linear extension, no reorg), the function returns an empty vec, and forkchoice_update_inner still writes the head entry and updates LatestBlockNumber — no regression.

Silent fallback to broken behavior in two places.

// blockchain.rs:1953-1960
let new_canonical_blocks = match self.storage.get_block_header_by_hash(hash)? {
    Some(header) => {
        crate::fork_choice::find_link_with_canonical_chain(&self.storage, &header)
            .await?
            .unwrap_or_default()   // ← covers Ok(None): not linked to canonical chain
    }
    None => Vec::new(),            // ← header not found in DB
};

• None => Vec::new(): if the header isn't in the store, we revert to head-only — the exact behavior the PR is fixing. Every call site calls add_block_pipeline / add_block before advance_canonical_head, so this branch should be unreachable. Silently degrading instead of erroring (or at least warning) makes the bug harder to spot if the invariant is ever violated.
• .unwrap_or_default() on Ok(None): find_link_with_canonical_chain returns Ok(None) when it exhausts the ancestry without hitting a canonical block (i.e. the block is on a completely disconnected sidechain). In that case passing Vec::new() again reverts to head-only — also silently. apply_fork_choice returns InvalidForkChoice::UnlinkedHead in this situation (line 90 of fork_choice.rs); the BSC path ignoring it is probably intentional (we don't reject blocks we just verified), but a warn! here would make debugging much easier.

Suggestion: Replace the two silent fallbacks with at least a warn! log so deviations are observable in production, e.g.:

None => {
    warn!(hash=%hash, number=%number, "advance_canonical_head: header not in store, falling back to head-only");
    Vec::new()
}

and

.unwrap_or_else(|| {
    warn!(hash=%hash, number=%number, "advance_canonical_head: block not linked to canonical chain, falling back to head-only");
    Vec::new()
})

---

Error handling

server.rs:1616 discards the error (let _ = blockchain.advance_canonical_head(...).await). This is pre-existing and not introduced by this PR, but the fix makes partial repointing a more consequential failure — if an error causes a fallback to head-only, the stale index entries live on silently. Not blocking, but worth a follow-up.

---

Visibility change

Making find_link_with_canonical_chain pub(crate) is the minimal correct change. The function is now called from two places within the same crate. No concern here.

---

Test

advance_canonical_head_repoints_reorged_heights directly exercises the scenario that triggered the production incident:

• Chain A (3 blocks) canonicalized per-block via advance_canonical_head
• Chain B (4 blocks) imports with heights 1–3 arriving with number <= latest (no-ops per the guard), then height 4 triggers the full ancestor walk
• Asserts all of chain B's heights are canonical
• Asserts none of chain A's heights remain indexed

The test correctly models the BSC import sequence. It fails before the fix and passes after. The two assertions (forward: chain B is canonical; reverse: chain A is gone) are exactly right.

One minor note: the test comment says "Reorgs heights 1-3 (<= head)" — worth making explicit in a comment that heights 1–3 are no-ops when processed individually (due to the number <= latest guard) and the whole repointing happens during the height-4 call. This is implicit from the guard semantics but a single sentence would help future readers.

---

Summary

The fix is correct and well-tested. Two issues worth addressing:

1. Silent fallback to head-only in both the None header case and the unlinked-block case. Add warn! logs at minimum; returning an Err in the header-not-found case would be more robust since that branch should be unreachable.
2. No reorg-depth check — intentional for BSC, but a brief comment noting this divergence from apply_fork_choice would help readers.

Neither issue affects correctness under the normal production path the PR targets.

---

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

[github-actions]

Lines of code report

Total lines added: 8
Total lines removed: 0
Total lines changed: 8

Detailed view

+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2747  | +8   |
+----------------------------------------+-------+------+

[greptile-apps]

Greptile Summary

This PR fixes a BSC-specific bug where the canonical number→hash index was not updated for reorged heights ≤ the current head. When a heavier chain arrived, advance_canonical_head only handed the single new head to forkchoice_update; the fix mirrors apply_fork_choice by using find_link_with_canonical_chain to collect the full non-canonical branch and rewrite every reorged height atomically.

• blockchain.rs: advance_canonical_head now calls find_link_with_canonical_chain to build the full branch before calling forkchoice_update, with a graceful fallback to head-only behaviour when the header or link cannot be found.
• fork_choice.rs: find_link_with_canonical_chain is promoted from private to pub(crate) so it can be called from blockchain.rs.
• smoke_tests.rs: A dedicated regression test that canonicalizes chain A via advance_canonical_head, then imports a heavier 4-block chain B that reorgs heights 1–3 and extends to height 4, asserting all chain-B heights (including the reorged ones) are canonical afterwards.

Confidence Score: 5/5

Safe to merge. The change is a targeted, well-tested fix that adds one extra DB header lookup per BSC block import on the forward-sync path, with no impact on the L1 engine-API or L2 paths.

The fix is mechanically equivalent to the already-audited apply_fork_choice path: it reuses find_link_with_canonical_chain without altering its logic, passes the branch to the same forkchoice_update_inner call, and preserves the number <= latest guard that prevents concurrent-advance races. The regression test recreates the exact real-world scenario (BSC mainnet block 105536921) and exercises both positive and negative invariants. Fallback behaviour when the header or link is absent is identical to the pre-fix code. No unrelated code is touched.

No files require special attention. The only structural change is in crates/blockchain/blockchain.rs; the other two files are a one-line visibility tweak and a new test.

Important Files Changed

Filename	Overview
crates/blockchain/blockchain.rs	Core fix: advance_canonical_head now walks back to the common canonical ancestor before calling forkchoice_update, correctly repointing every reorged height instead of only the new head. Fallback to Vec::new() (head-only) when the header/link is unavailable preserves the original behaviour for blocks not yet in storage.
crates/blockchain/fork_choice.rs	One-line visibility change: find_link_with_canonical_chain is widened from private to pub(crate) to allow reuse from blockchain.rs. No logic changes.
test/tests/blockchain/smoke_tests.rs	Adds advance_canonical_head_repoints_reorged_heights: a well-scoped regression test that recreates the exact BSC scenario (per-block canonicalization with a heavier fork), asserting both positive (chain B canonical) and negative (chain A stale hashes gone) invariants.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant BSC as BSC Sync / NewBlock
    participant ACH as advance_canonical_head
    participant FLCC as find_link_with_canonical_chain
    participant FCU as forkchoice_update_inner

    BSC->>ACH: "(number=4, hash=B4)"
    ACH->>ACH: "latest=3, number>latest → proceed"
    ACH->>ACH: get_block_header_by_hash(B4) → header
    ACH->>FLCC: find_link_with_canonical_chain(B4_header)
    Note over FLCC: Walk B4→B3→B2→B1→genesis<br/>Collecting non-canonical pairs
    FLCC-->>ACH: Ok(Some([(3,B3),(2,B2),(1,B1)]))
    ACH->>FCU: "forkchoice_update([(3,B3),(2,B2),(1,B1)], head=4,B4)"
    Note over FCU: Write B3,B2,B1 to canonical index<br/>Delete entries above head (none)<br/>Write B4 as head
    FCU-->>ACH: Ok(())
    Note over ACH: Heights 1-4 all point to chain B ✓

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant BSC as BSC Sync / NewBlock
    participant ACH as advance_canonical_head
    participant FLCC as find_link_with_canonical_chain
    participant FCU as forkchoice_update_inner

    BSC->>ACH: "(number=4, hash=B4)"
    ACH->>ACH: "latest=3, number>latest → proceed"
    ACH->>ACH: get_block_header_by_hash(B4) → header
    ACH->>FLCC: find_link_with_canonical_chain(B4_header)
    Note over FLCC: Walk B4→B3→B2→B1→genesis<br/>Collecting non-canonical pairs
    FLCC-->>ACH: Ok(Some([(3,B3),(2,B2),(1,B1)]))
    ACH->>FCU: "forkchoice_update([(3,B3),(2,B2),(1,B1)], head=4,B4)"
    Note over FCU: Write B3,B2,B1 to canonical index<br/>Delete entries above head (none)<br/>Write B4 as head
    FCU-->>ACH: Ok(())
    Note over ACH: Heights 1-4 all point to chain B ✓

Loading

_{Reviews (1): Last reviewed commit: "fix: repoint canonical index on BSC reor..." | Re-trigger Greptile}