fix(l1): serve GetBlockHeaders by non-canonical hash so syncing peers can fetch their head

[ilitteri]

Motivation

On glamsterdam-devnet-6 several ethrex nodes that fell behind got stuck or lagged (buildoor-prysm-ethrex-1 wedged ~15h; lighthouse-ethrex-1/nimbus-ethrex-1 transient lag + a missed/orphaned proposal). The common trigger: a syncing node requests headers from its sync-head hash, and peers answer with peer(s) queried but did not serve headers, so full sync aborts after MAX_HEADER_FETCH_ATTEMPTS.

Root cause is in how we serve GetBlockHeaders by hash. fetch_headers translated any by-hash start to a block number via get_block_number — which is populated for every stored block regardless of canonicity — then served by number, returning the responder's canonical block at that height. When the requested hash is non-canonical on the serving peer (routine on a reorg-prone ePBS devnet where a sync head isn't yet canonical everywhere), headers[0].hash() != requested, so the requester (request_block_headers_from_hash, which pins headers[0].hash() == start) rejects the whole batch.

Description

In GetBlockHeaders::fetch_headers (crates/networking/p2p/rlpx/eth/blocks.rs):

1. Only translate a by-hash start to a number when that hash is the canonical block at its height (get_canonical_block_hash(n) == requested); otherwise keep it as a hash and serve the exact block.
2. For a by-hash start with reverse (NewToOld) and skip == 0, walk parent hashes so a non-canonical chain (a peer's fork/sync head) can be served beyond a single header. Ascending/skipping by hash isn't representable, so we stop after the requested header (unchanged from before).

The canonical by-number path is unchanged. Output is internally hash-chained (headers[i].parent_hash == headers[i+1].hash()), matching are_block_headers_chained, so both ethrex and other clients accept it. This mirrors geth's serviceNonContiguousBlockHeaderQuery (reverse ancestor walk).

How to test

cargo test -p ethrex-p2p serves_non_canonical_block_by_hash — stores a canonical block and a non-canonical sibling at the same height; a by-hash NewToOld request for the sibling must return the sibling (then its parent), not the canonical block. Fails against the pre-fix serve path.

Notes / follow-ups

• The non-canonical parent walk is bounded by limit (<= BLOCK_HEADER_LIMIT) and by stored data (an unknown parent hash ends it). geth additionally caps non-canonical traversal at maxNonCanonical = 100; a similar small cap here would avoid long side-chain scans during reorg storms — happy to add if preferred.
• This is the trigger; a separate follow-up addresses the recovery side (a wedged sync cycle holding the syncer lock, and the resume-state path that currently requires ethrex removedb).

[github-actions]

🤖 Kimi Code Review

The PR correctly fixes a critical sync bug where non-canonical blocks requested by hash would incorrectly resolve to their canonical counterparts at the same height. The implementation is sound and the test coverage is good.

Highlights:

• Logic correctness: The canonical verification check (lines 108-115) correctly implements the "hash -> number -> canonical hash" round-trip validation before switching to number-based traversal.
• Fork handling: The parent-hash walking logic (lines 167-175) properly serves non-canonical chains during reverse sync, which is essential for fork resolution.
• Safety: Graceful degradation when get_canonical_block_hash fails (treating as non-canonical) prevents serving wrong blocks under error conditions.
• Test quality: The regression test creates a realistic fork scenario and verifies both the specific fix (orphan retrieval) and parent traversal.

Minor suggestions:

1. Test coverage completeness (lines 366+): Consider adding assertions to verify:

   • headers.len() == 2 (ensures no extra headers are returned)
   • Ascending requests by hash return only the single requested header (current behavior, but worth testing to prevent regression)

2. Comment clarity (line 100): The phrase "not-yet-canonical" could be slightly misleading since the code treats these as non-canonical (hash-based path). Consider "non-canonical or uncanonicalized" for precision.

3. Debuggability (line 167): When walking by parent hash fails (e.g., pruned parent), the loop breaks silently. Consider adding a trace! log similar to line 154 for the hash-walking case:

   trace!(parent_hash=%block_header.parent_hash, "Following parent hash for non-canonical chain");

The code is otherwise well-structured, memory-safe, and follows Rust idioms. The fix addresses the root cause of the sync stall described in the comments.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/networking/p2p/rlpx/eth/blocks.rs:112-117 — get_canonical_block_hash(...).await.ok().flatten() swallows StoreError and changes behavior on DB faults. If that lookup fails for a canonical hash request, the code silently falls back to the hash path, which only serves a parent walk for reverse && skip == 0 and otherwise stops after one header. That turns a storage error into protocol-visible misbehavior and removes the error signal from logs. This should match on the Result explicitly and either log + return an empty response, or propagate the error through fetch_headers.

The non-canonical-by-hash fix itself looks correct, and the added regression test covers the main sync-stall case well. I couldn’t run the targeted test here because cargo/rustup is blocked by a read-only filesystem in this environment.

---

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

[github-actions]

Lines of code report

Total lines added: 81
Total lines removed: 0
Total lines changed: 81

Detailed view

+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/eth/blocks.rs | 354   | +81  |
+-------------------------------------------------+-------+------+

[greptile-apps]

Greptile Summary

This PR changes how GetBlockHeaders serves by-hash requests. The main changes are:

• Canonical hashes still use the by-number path.
• Stored non-canonical hashes are served directly by hash.
• Reverse no-skip hash requests can walk parent hashes.
• A regression test covers non-canonical sibling header serving.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.
• The new hash path matches the current sync request shape.
• Parent traversal is bounded and stops cleanly when storage has no next header.

Important Files Changed

Filename	Overview
crates/networking/p2p/rlpx/eth/blocks.rs	Updates by-hash header serving for non-canonical blocks and adds regression coverage for reverse parent traversal.

_{Reviews (1): Last reviewed commit: "test(l1): GetBlockHeaders serves a non-c..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Now I have enough information to write the complete review.

---

PR 6921 Review: fix(l1): serve GetBlockHeaders by non-canonical hash so syncing peers can fetch their head

Overview

This PR fixes a real sync-stall bug: when a syncing peer requests headers starting from a non-canonical hash (common during ePBS reorgs), the old code translated the hash to a block number and served the canonical block at that height instead, causing the requester to reject the whole batch. The fix correctly distinguishes canonical vs. non-canonical hashes, keeps non-canonical starts as a hash, and for reverse=true, skip=0 walks parent hashes to serve an honest ancestor chain. The test exercises the exact failure scenario and confirms the fix. The approach mirrors geth's serviceNonContiguousBlockHeaderQuery.

---

Findings

---

blocks.rs:164–169 — No cap on non-canonical parent-hash traversal; acknowledged but not implemented

The new hash-walk path iterates up to BLOCK_HEADER_LIMIT = 1024 times, each iteration doing a separate get_block_header_by_hash storage call (open read txn → lookup → close). Canonical traversal is cheaper because it walks by sequential number within a single path, while hash-chased reads are individually dispatched (each spawns a blocking task via read_async). A peer can deliberately request limit=1024, skip=0, reverse=true starting at any stored non-canonical hash, and the responder will do 1024 sequential single-key lookups.

The PR description explicitly notes that geth caps non-canonical traversal at maxNonCanonical = 100. Even if this is deferred to a follow-up, adding a const MAX_NON_CANONICAL: u64 = 100 constant here now — so the knob exists in the code and is findable — would prevent the scan from widening silently as BLOCK_HEADER_LIMIT grows or the call pattern changes.

Concrete failure: during a reorg storm on glamsterdam-devnet, multiple peers each issuing large non-canonical traversal requests can create sustained sequential-read pressure on the responder's storage backend, disproportionate to the serving cost for canonical requests of the same limit.

---

blocks.rs:436–454 (test) — Test does not cover skip > 0 or reverse=false for non-canonical hash

The only test verifies reverse=true, skip=0. The None arm at line 167 — which silently truncates the response to a single header for all other (reverse, skip) combinations on a non-canonical hash — is not tested.

If a peer sends reverse=true, skip=1 from a non-canonical head expecting ancestry-with-skip, it gets 1 header back with no error signal. The requester has no way to distinguish "this peer doesn't have the block" from "this peer only served 1 instead of N". Adding assertions like:

// reverse=true, skip>0 → single header only
let h = GetBlockHeaders::new(3, HashOrNumber::Hash(orphan_hash), 10, 1, true)
    .fetch_headers(&store).await;
assert_eq!(h.len(), 1);
assert_eq!(h[0].hash(), orphan_hash);

// reverse=false → single header only
let h = GetBlockHeaders::new(4, HashOrNumber::Hash(orphan_hash), 10, 0, false)
    .fetch_headers(&store).await;
assert_eq!(h.len(), 1);

...documents the intentional behavior and prevents regressions if the branch is later changed.

---

blocks.rs:451–454 (test) — Canonical path assertion is incomplete

let headers_canon = GetBlockHeaders::new(2, HashOrNumber::Hash(canon_hash), 10, 0, true)
    .fetch_headers(&store)
    .await;
assert_eq!(headers_canon.first().map(|h| h.hash()), Some(canon_hash));

This only checks the first element. The canonical path converts to HashOrNumber::Number(1) and continues walking; with limit=10, reverse=true, skip=0 from height 1, the response should be [canon, genesis]. If the by-number walk were broken for subsequent elements, this test would still pass. Adding assert_eq!(headers_canon.len(), 2) and checking headers_canon[1].hash() == genesis_hash would close this gap.

---

blocks.rs:110–122 — Two-call canonicity check duplicates Store::is_canonical_sync

Store::is_canonical_sync (store.rs:3320) performs exactly the same two-step check (get_block_number_sync + get_canonical_block_hash_sync) under a single read transaction. The new code does it with two separate .await calls, each dispatching an independent spawn_blocking task and opening its own read transaction. The extra task-boundary overhead is small, but more importantly the logic is duplicated — any future caching or transactional improvement to is_canonical_sync won't apply here.

An async wrapper Store::is_canonical (wrapping the sync version in spawn_blocking) would make both the intent and the transaction boundary explicit and unify the two implementations. This crate already calls is_canonical_sync directly in adjacent sync paths.

---

What looks correct

• The canonicity guard logic (get_canonical_block_hash(n) == requested) is correct: it prevents the wrong-block substitution that caused the stall.
• Parent-hash chaining produces responses that satisfy are_block_headers_chained(NewToOld) by construction, so request_block_headers_from_hash's validation gate passes.
• The hash[0] == start pin check in request_block_headers_from_hash (the original rejection point) is satisfied because current_block starts at the requested hash and the first header fetched is that exact block.
• All callers (request_block_headers_from_hash, ask_peer_head_number, full-sync loop) are compatible with the new behavior.
• Genesis termination (following parent_hash = H256::zero() to a lookup that returns None) is handled cleanly by the existing break.
• The test fixture's assert_ne!(canon_hash, orphan_hash) guards against silent hash collisions in the helper.

---

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

[ElFantasma]

It looks good to me. Just left a nitpick

[ElFantasma]

nit: Store::is_canonical_sync (store.rs:3320) already implements exactly this get_block_number + get_canonical_block_hash pair. An async wrapper (or get_block_number_if_canonical(hash) -> Result<Option<BlockNumber>>) would let this callsite express the intent directly, and would give the storage-error case a single place to debug!/propagate — right now .ok().flatten() == Some(block_hash) silently absorbs both Err(_) and Ok(None). Both silent paths default to keeping the hash + serving by-hash, which is safe, but a real DB read fault or the (rare) inconsistency where get_block_number returns Some(n) and get_canonical_block_hash(n) returns None currently disappears without a log.