[MegaRedHand]

🗒️ Description / Motivation

An ethlambda beacon (v1.0.0-rc.2) paired with geth v1.17.10 on Hoodi stopped importing at slot 4248640 and stayed there for three hours. Found by ethrex's multisync (lambdaclass/ethrex#7461): the pair failed its 10-minute head-stall check while the same image passed with nethermind and ethrex.

The trigger was geth: mid snap sync, it answered engine_newPayloadV4 past the 8 s engine ladder, so block 4248641 got no verdict and was dropped, as designed (ImportOutcome::Held). What should retry such a block is the next arrival: process_or_pend_block resolves the arrival's missing ancestor through pending_block_parents, and the DB walk then re-queues the first stored block whose parent has a state.

That resolution is what went wrong. pending_block_parents stores the missing ancestor as it was when each block pended. Every descendant of 4248641 had pended while 4248639 was the gap, so all of them pointed at 4248639. Once 4248639 imported and 4248641 was dropped after it, every arrival resolved to 4248639. The DB walk re-queued that already-imported block (a no-op through the "already in the store" early return) and returned before request_missing_block, so 4248641 was never retried or requested, and ~900 later blocks stayed parked. Only a re-delivery of 4248642, whose parent is 4248641 itself, could have recovered it, and none came.

The same run's Sepolia pair dropped 11 blocks for lack of a verdict and recovered every time, because its pointers happened to land on the dropped block.

What Changed

• process_or_pend_block (crates/blockchain/src/lib.rs): the pending_block_parents walk stops at an ancestor that already has a post-state. The existing DB walk then steps through the gap block by block, finds the dropped block, re-queues it, and re-points each pending block it passes at its direct parent.
• ImportOutcome::Held doc: a no-verdict block is re-driven by the next arrival that leads to it (from the DB, or by asking the network), not by a timer.

Correctness / Behavior Guarantees

• A walk that never meets an imported ancestor behaves exactly as before.
• Cost: one has_state read per pointer step. Normally that is one step, since pointers jump straight to the top of a gap; while a gap opened this way is still open, later arrivals step through it a block at a time.
• Not changed: a no-verdict block is still dropped, and only an arrival that leads to it retries it. Nothing retries it on a timer.
• The code path is chain-agnostic, so lean gets the same guard.

Tests Added / Run

• a_block_dropped_below_an_imported_ancestor_is_retried_by_the_next_arrival: an imported ancestor, its child dropped with no state, a grandchild still pointing at the ancestor, then one arrival. Asserts the dropped block is what gets queued. Without the guard it fails, queueing the imported ancestor instead.
• cargo test -p ethlambda-blockchain --lib --profile release-fast: 292 passed, 43 ignored.
• Not yet verified live.

Related Issues / PRs

• Related to feat(l1): run any EL+CL pair in multisync and check the CL ethrex#7461 (the multisync check that found it)

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — only ethlambda-blockchain --lib was run

[github-actions]

🤖 Kimi Code Review

I'll review this PR diff focusing on the blockchain crate changes related to stale pending block parent resolution.

Overall Assessment

This PR fixes a legitimate consensus-critical bug where dropped blocks below an imported ancestor would never be retried, causing chain sync to stall. The fix is conceptually correct but has implementation concerns.

Issues Found

1. Critical: expect in hot path can crash the node (line ~2787-2790)

if self
    .store
    .has_state(&ancestor)
    .expect("DB read should succeed")  // ← CRASH RISK
{
    break;
}

Problem: Using .expect() on a database read in consensus-critical code violates fault tolerance. A transient DB error (disk full, corruption, lock timeout) will panic the entire beacon node.

Fix: Propagate the error properly. The function already returns Option<ImportOutcome>, so you need to either:

• Change return type to Result<Option<ImportOutcome>, StoreError> and bubble up
• Or at minimum, treat DB errors as "assume no state" with logging rather than crashing

// Suggested - treat DB error conservatively
match self.store.has_state(&ancestor) {
    Ok(true) => break,
    Ok(false) => {}
    Err(e) => {
        tracing::error!(?e, ?ancestor, "DB error checking state, assuming no state");
        // Could also return Some(ImportOutcome::HoldForColumns) to retry later
    }
}

2. Logic correctness: Stale entry detection ordering (line ~2780-2795)

The loop structure:

let mut missing_root = parent_root;
while let Some(&ancestor) = self.pending_block_parents.get(&missing_root) {
    if self.store.has_state(&ancestor).expect(...) {
        break;  // stale: ancestor imported but gap exists below
    }
    missing_root = ancestor;
}

Question: After break, missing_root still holds the previous value (the one that had ancestor in pending_block_parents), not ancestor itself. Is this intentional?

Looking at the subsequent DB walk code (not fully shown in diff), I believe the intent is:

• missing_root = the last non-imported ancestor we found, which is what we'll request/queue

But the comment says "the DB walk finds the gap" — verify that the DB walk starting from missing_root (not ancestor) correctly identifies ancestor as the imported boundary and then finds the gap below it.

Suggestion: Add explicit assertion or clearer variable naming:

let mut missing_root = parent_root;
let mut found_imported_ancestor = None;

while let Some(&ancestor) = self.pending_block_parents.get(&missing_root) {
    if self.store.has_state(&ancestor)? {  // if propagated
        found_imported_ancestor = Some(ancestor);
        break;
    }
    missing_root = ancestor;
}
// missing_root is now the last non-imported block in the chain
// found_imported_ancestor is the imported boundary (if any)

3. Test coverage gap: No test for DB error path

The new test a_block_dropped_below_an_imported_ancestor_is_retried_by_the_next_arrival is good for the happy path, but given the expect() crash risk, there should be a test with a mocked store that returns Err on has_state() to verify graceful handling.

4. Comment accuracy (line ~2780)

// Following it would land on an imported block, which the
// DB walk below re-queues as a no-op

Verify: does the DB walk actually re-queue as a no-op, or does it skip/ignore? The comment says "re-queues as a no-op" but the bug description says "never reached the dropped block" — these seem slightly inconsistent. Clarify whether "no-op" means it processes but produces no state change, or if it literally does nothing.

Minor Issues

5. Test helper naming (line ~5424)

Test name is descriptive but very long. Acceptable for clarity, but consider:

async fn dropped_block_below_imported_ancestor_gets_retried()

6. Test setup duplication

The test manually constructs pending_blocks and pending_block_parents HashMaps. This bypasses normal process_or_pend_block flow. Consider adding a comment explaining why direct mutation is necessary vs. driving through the actual API.

Positive Observations

1. Good: The stale detection is conservative — only breaks when state exists, not when block exists. This is correct because a block without state (dropped/held) is the gap we need to find.

2. Good: Test scenario matches real-world Hoodi incident, validating the fix addresses actual observed behavior.

3. Good: Comment updates in ImportOutcome::HoldForColumns accurately describe the retry mechanism.

Recommended Changes

Priority	Location	Change
Critical	lib.rs:~2787	Replace .expect() with proper error handling
Medium	lib.rs:~2780	Clarify variable semantics after loop break
Low	mod tests	Add DB error path test
Low	lib.rs:~2783	Verify comment accuracy on "no-op" behavior

Summary

The fix is conceptually correct and addresses a real consensus stall bug, but the .expect() introduces a new liveness risk (crash on DB error) that could be exploited for DoS or cause node instability. This must be fixed before merge.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The fix in crates/blockchain/src/lib.rs:2791 is targeted and appears correct: stopping the pending_block_parents walk when it reaches an ancestor that now has post-state prevents the importer from re-queuing an already-imported block and missing the actual gap below it. That matches the surrounding DB walk logic and does not affect fork choice, attestation handling, or state transition correctness directly.

A few review notes:

• crates/blockchain/src/lib.rs:2792 adds an extra has_state DB read per hop in the stale-pointer walk. That seems acceptable because this path only runs for parent-missing blocks, but it is a small regression on a hot recovery path. Not a blocker, just worth keeping in mind if this map can grow long during sync.
• crates/blockchain/src/lib.rs:5447 inserts the “imported” block via insert_pending_block while also inserting its state. That works for this regression test because the production logic only needs block_entry/get_signed_block, but it models an imported block a bit unusually. Consider a short comment clarifying that the test intentionally uses DB presence + state as the imported signal.
• crates/blockchain/src/lib.rs:5474 sets pending_block_parents[pending_root] = imported_root, which is the exact stale-cache condition being fixed. Good regression coverage; the assertion on the queued root is specific and useful.

No security issues jump out in this patch: it does not change signature verification, SSZ handling, fork-choice state, or consensus validation rules, and the new logic fails closed by continuing to pend/request blocks rather than admitting invalid data.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 720: stop the stale pending_block_parents walk at an imported ancestor

I found no blocking issues. I traced the logic by reading the diff and the surrounding code. I did not build the project or run the new test.

Correctness

• The new has_state(&ancestor) check in process_or_pend_block (crates/blockchain/src/lib.rs, around line 2790) fixes the stale-pointer case.
• When a cached pending_block_parents entry names an ancestor that has since been imported, the walk now stops at the child. It no longer jumps onto the imported block.
• The DB walk below then does the right thing. It reads the child's block_entry, sees the parent has no state, registers the child in pending_blocks and pending_block_parents, and keeps climbing. It stops at the first block whose parent has state and re-queues that block, or asks the network for the missing root.
• The existing stale-entry case still works. An ancestor without a state is followed as before.
• The loop still terminates. It only follows hash-linked parents, and the new break can only end it sooner.
• Step through of the new test: the arrival's parent is pending_root, and its entry points at imported_root, which has a state, so the walk breaks. The DB walk then moves from pending_root to dropped_root. It finds that dropped_root's parent has a state and re-queues dropped_root. This matches the asserted vec![dropped_root].

Performance

• This adds one has_state call per hop of the in-memory walk. has_state checks the cache first and uses peek, so it does not disturb the LRU. The cost is negligible, and the walk only runs on the parent-missing path.

Style and tests

• The comments explain why the check exists, and the ImportOutcome doc update matches the new behavior.
• The regression test covers the exact scenario: an imported ancestor, a dropped child, and a pending grandchild that still points at the ancestor. It asserts that the dropped block is queued and the imported ancestor is not.

Nit, optional

• The .expect("DB read should succeed") on has_state repeats the pattern a few lines below. A small local closure or helper could remove the duplication. It isn't worth blocking on.

LGTM.

---

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