fix(p2p): start beacon range sync after the store's head

[MegaRedHand]

🗒️ Description / Motivation

A beacon follower that has caught up keeps downloading blocks it already has.

P2PServer::beacon_fetched_through is the watermark range sync starts from, and only handle_beacon_blocks_by_range_response advances it. Once the follower is at the tip it imports from gossip, so the watermark stays at its last range batch while the head moves on. Every peer that connects later advertises a head above the watermark, and its Status answer starts a range session from there:

watermark (last range batch)           our head           peer head
          |                                |                  |
          [ already imported from gossip ][ owed            ]
          ^ range session starts here      ^ should start here

Each re-fetched block also brings its custody columns (request_beacon_data_columns_by_range runs for the same span), and each one costs the chain actor an import attempt that ends at "already in the store", followed by a head recompute.

Observed on the Platåberget follower (build c184e736) between 14:08Z and 17:02Z on 2026-10-01:

Bursts of arrivals that imported nothing	66
Blocks in them	748
Burst size	the slots since the burst before it
Cost per arrival	~65 ms (cascade_ms ~30-47 ms + get_head_ms ~20 ms)
Largest burst	67 blocks at 16:52:36Z; the gossip block for slot 354263 waited 4.2 s behind it in the queue

What Changed

• crates/net/p2p/src/req_resp/handlers.rs: beacon_sync_target takes the store's head slot and measures the peer against max(fetched_through, head_slot). handle_status_response reads the head with Store::beacon_head, the same whole-block decode build_status already does for every handshake we send, and logs it beside fetched_through.
• crates/net/p2p/src/lib.rs: the beacon_fetched_through doc says only range answers advance it.
• docs/beacon_wire.md, CLAUDE.md: describe the two floors.

Correctness / Behavior Guarantees

• The watermark still wins while a delivered batch is draining through the actor mailbox: there the head is below it. That is the case it was introduced for (11,213 blocks fetched to import 100 when range sync was driven off the head alone), and it is unchanged.
• The head is a safe floor because a head block's whole ancestry is already imported, so nothing at or below it can be missing from the canonical chain.
• A peer on another fork whose head is above ours gets a session from our head onward. Blocks of its branch at or below our head slot come through the by-root parent lookup, as they already did whenever the watermark was current.
• A peer exactly one block ahead (its block still in gossip flight) still costs a one-block request. I left that in rather than adding a tolerance, since a tolerance would delay real catch-up.
• The startup seed is unchanged: it was already the anchor head.

Tests Added / Run

• beacon_sync_target_starts_after_a_head_gossip_moved_past_fetched_through (new): a head above the watermark moves the start to head + 1; a peer at or behind the head gets None.
• beacon_sync_target_keys_off_fetched_through_while_the_head_lags (was ..._not_store_head): same assertions, now with a head below the watermark passed in explicitly.
• beacon_sync_target_is_bounded_by_max_sync_range: updated for the new argument.

cargo test -p ethlambda-p2p --lib --profile release-fast
cargo clippy -p ethlambda-p2p --all-targets --profile release-fast -- -D warnings

Not deployed to a follower yet.

Related Issues / PRs

• Related to feat(beacon): follow gloas live #639 (the build deployed on the follower).

✅ Verification Checklist

• Ran make fmt: clean
• Ran clippy on ethlambda-p2p with -D warnings: clean
• Ran ethlambda-p2p unit tests: passing

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the beacon sync target logic changes. This is a networking/syncing fix, not consensus-critical state transition code, but correctness still matters for liveness.

Overall Assessment

Good fix for a real operational issue. The change is well-documented with clear problem/solution explanation. Two subtle issues to address.

---

Issues Found

1. Potential inconsistency: head_slot decode failure silently falls back to 0

File: crates/net/p2p/src/req_resp/handlers.rs, lines 1703-1706

let head_slot = server.store.beacon_head().map_or(0, |(slot, _)| slot);

Problem: If beacon_head() returns None, head_slot becomes 0, which could cause synced_through to be fetched_through.max(0) == fetched_through (if fetched_through > 0). That's fine. But if fetched_through is also 0 and the peer is at slot 0, you get None (no sync needed). The real concern: why would beacon_head() return None in a running node?

If this indicates a store that's not yet initialized, using 0 as fallback may cause unnecessary range sync from genesis. More critically, this diverges from build_status which presumably handles the same decode — the comment says "A whole-block decode per handshake, the same one build_status already pays to send ours."

Suggestion: Check how build_status handles the None case. If it can't happen in practice (store always initialized before P2P), consider expect with a message. If it can happen, document why 0 is safe.

---

2. Test gap: No test for fetched_through > head_slot with peer exactly at fetched_through + 1

File: crates/net/p2p/src/req_resp/handlers.rs, tests around line 2885

The existing tests cover:

• fetched_through dominates (100 vs 40, peer at 150)
• head_slot dominates (100 vs 149, peer at 150)
• Peer at/below floor
• MAX_SYNC_RANGE bounding

Missing: fetched_through == head_slot case, and the boundary where peer_head_slot == synced_through + 1.

Add:

#[test]
fn beacon_sync_target_when_floors_are_equal() {
    assert_eq!(beacon_sync_target(100, 100, 150), Some(101..151));
    assert_eq!(beacon_sync_target(100, 100, 101), Some(101..102));
}

This guards against future refactoring errors in the max() logic.

---

3. Nit: Comment typo in docstring

File: crates/net/p2p/src/req_resp/handlers.rs, line 1630

/// given how far this node has the chain and a peer's advertised head.

Should be "has in the chain" or "has of the chain".

---

Non-Issues (Acknowledged)

• MAX_SYNC_RANGE bounding logic: Correct. gap.min(MAX_SYNC_RANGE) prevents overflow and bounds request size.
• Saturating arithmetic: saturating_add(1) and saturating_add(gap.min(...)) correctly prevent overflow on u64::MAX.
• Watermark update on response: Line 2432-2434 correctly updates beacon_fetched_through with max(), preserving monotonicity even if responses arrive out of order.
• Doc updates: CLAUDE.md and docs/beacon_wire.md accurately reflect the dual-floor logic.

---

Summary

Priority	Item	Action
Low	beacon_head() None handling	Verify against build_status; consider expect or document
Low	Missing test for equal floors	Add test
Trivial	Comment grammar	Fix "has the chain" → "has in the chain"

The core logic is correct and the operational data (11,213 blocks → 1.7:1 ratio, 748 blocks in 66 bursts) validates the fix.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 645: start beacon range sync after the store's head

I read the change and ran no tests. It looks correct and well-scoped.

What changed: beacon_sync_target (crates/net/p2p/src/req_resp/handlers.rs:1652) now measures a peer against fetched_through.max(head_slot) instead of fetched_through alone.

Why it holds up

• Gossip case: a caught-up follower imports from gossip, so fetched_through goes stale. Using the store's head as a second floor stops the re-fetch of blocks already imported, which fixes the 748-block, 66-burst problem the PR describes.
• Safe floor: a head block's whole ancestry is already imported, so starting at head_slot + 1 can't skip a block this node is missing.
• Delivered-but-unimported batches: keeping fetched_through in the max preserves the earlier fix for the 11,213-fetch regression. The head lags a delivered batch by the actor mailbox, and this still covers that.
• Arithmetic: the saturating_add and min(MAX_SYNC_RANGE) bounds are unchanged. The peer_head_slot <= synced_through early return rules out underflow in gap.
• Tests: the two behaviors are covered separately. The head-lagging case asserts (100, 40, 150) → 101..151. The gossip-advanced case asserts (100, 149, 150) → 150..151, plus the "peer at or behind head" cases. The u64::MAX bound test is kept.
• Docs and logging: the doc comments explain both floors and the numbers behind them. The debug log now includes head_slot, which will help diagnose any recurrence.

Nits (non-blocking)

1. server.store.beacon_head().map_or(0, |(slot, _)| slot) (handlers.rs:1706) treats a read failure or missing head as slot 0. That quietly falls back to the old behavior, which is safe. A debug! or warn! on the None case would make a regression visible.
2. beacon_head() does a whole-block decode on every Status response, and the code comment says so. A peer-churn flood would pay that each time. It's the same cost build_status already pays, so it's fine now. A cached head slot would be the fix if this ever shows up in profiles.
3. The lib.rs:1174 doc and the handlers.rs:2432 comment now point at beacon_sync_target for the two-floor rationale. They are consistent.

Approve.

---

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

[github-actions]

🤖 Codex Code Review

Findings

• Looks good overall. The core change in beacon_sync_target now correctly avoids the “re-request everything since last range batch” behavior after gossip advances head, while still protecting against mailbox lag on range-imports. I don’t see a consensus-safety regression in fork choice, attestation, state transition, XMSS, or SSZ paths because this PR only changes P2P range-sync targeting.

• Minor readability nit: the new logic depends on an important invariant — using head_slot is safe only because it is the canonical imported head. That invariant is stated in comments, but a short code-level note near let synced_through = fetched_through.max(head_slot); would help future maintainers avoid “optimizing” this back to one source. See crates/net/p2p/src/req_resp/handlers.rs:1660.

Why it looks correct

• beacon_sync_target now starts from max(fetched_through, head_slot) + 1, which matches the intended “highest definitely-known imported-or-delivered slot” floor. See crates/net/p2p/src/req_resp/handlers.rs:1660.
• The range remains capped by MAX_SYNC_RANGE, and uses saturating arithmetic, so no obvious overflow bug was introduced. See crates/net/p2p/src/req_resp/handlers.rs:1665.
• handle_status_response only adds one store.beacon_head() read per handshake; that’s a negligible cost relative to the avoided duplicate sync traffic. See crates/net/p2p/src/req_resp/handlers.rs:1706.
• The added tests cover both asymmetric lag cases: actor-mailbox lag and gossip-ahead-of-watermark. That’s the right boundary for this fix. See crates/net/p2p/src/req_resp/handlers.rs:2885 and crates/net/p2p/src/req_resp/handlers.rs:2900.

If you want, I can also do a second pass specifically on denial-of-service angles in the beacon req/resp handlers.

---

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