[MegaRedHand]

🗒️ Description / Motivation

The sync gate reported Synced while a node was far behind (#559), so it attested and proposed on a stale head. It took the freshest block the network knows from the node's own imported chain, which trails the head during a catch-up, so the network-stall override fired. The beacon follower had the same bug: docs/beacon_wire.md records it reporting synced through a 10-minute catch-up up to 98 slots behind.

This follows Lighthouse and Prysm: a node is syncing only when its head trails the wall clock and the head its peers report in Status is ahead of its own. Peers decide "behind vs. stalled"; the clock keeps any peer claim from idling a node that is current.

A kurtosis run found a second cause on the beacon follower: the gate was judged only on the actor's slot tick, which waits behind a whole catch-up in the actor's mailbox. It is now judged after every block arrival too (second commit).

What Changed

• crates/blockchain/src/sync_status.rs: new rule with per-chain SyncThresholds. Lean keeps leanSpec ci: add a Commit-Boost check to the kurtosis battery #708's 4/2. Beacon uses Lighthouse's numbers: 2 epochs of clock lag (DEFAULT_SYNC_TOLERANCE_EPOCHS, also Prysm's shouldReSync) and 1 epoch of peer lead (SLOT_IMPORT_TOLERANCE). There is also a bounded startup hold before the first Status (lean 8 slots, beacon 1 epoch).
• crates/net/p2p/src/peer_heads.rs (new): each connected peer's latest Status head, dropped on disconnect.
  • A head more than 1 slot past the wall clock is ignored (Lighthouse/Prysm FUTURE_SLOT_TOLERANCE).
  • The published value is the second-highest report once 3+ peers have reported, the highest below that.
  • It is published on Store::network_head.
• p2p: each peer is re-asked for its Status every 4 slots (lean) or every half epoch (beacon, Prysm's cadence), counted from that peer's own last exchange in either direction.
  • The timer ticks every slot and asks at most ceil(peers / interval) of the due peers, longest-waiting first, so the requests are spread rather than sent to every peer at once. With the default 200 beacon peers, that is at most 13 per slot.
  • A refresh answer starts range sync only beyond the threshold lead, so it never races gossip.
• crates/blockchain/src/lib.rs:
  • The tracker reads Store::network_head, and the beacon fork guard is kept.
  • The gate is judged on every tick and also after every block arrival: on beacon in recompute_beacon_head (which every arrival, tick and envelope release ends in, ahead of the forkchoiceUpdated call), on lean at the end of on_block.
  • Why: range sync forwards a whole catch-up into the actor's mailbox at once, and each import holds the actor for an engine round trip, so the tick waited behind all of it and the gate kept answering Synced until the catch-up was over.
• Docs: a new spec_deviations.md entry (lean departs from leanSpec ci: add a Commit-Boost check to the kurtosis battery #708), plus updates to architecture.md, rpc.md and beacon_wire.md.

Correctness / Behavior Guarantees

• Stall: when no peer is ahead, the node stays synced at any clock lag (leanSpec's stall tests still hold). A run of missed proposals (leanSpec #1194) no longer closes the gate.
• Unauthenticated input is bounded: pending or unverified blocks are not counted, and a peer's claim alone cannot stop a node whose head is within the clock threshold.
• Losing every peer: reads as a stall, and duties continue. That is what Lighthouse's duty endpoints do (Stalled passes not_while_syncing_filter).
• One signal, matching Lighthouse's duty filter: ethlambda's is_syncing is the decision that gates duties, since its validator client reads is_syncing.
  • Lighthouse's own /eth/v1/node/syncing derives is_syncing from its sync state (!is_synced(), except with zero target peers), so it reads true while range-syncing even within the 2-epoch tolerance, and while Stalled.
  • Lighthouse's duty endpoints (not_while_syncing_filter) refuse only while range-syncing beyond the tolerance, and let Stalled through. That is what ethlambda's single flag does.
• Staleness bound: the verdict is judged on ticks and block arrivals only, so between those it can lag until the next one is handled.
• Lean deviation: a node behind its peers with nothing newer imported is now syncing, where leanSpec lets it do duties.

Tests Added / Run

• Unit tests for the tracker rule, peer aggregation, Store::network_head sharing, and the beacon follower cases.
• a_block_arrival_judges_the_gate_without_waiting_for_the_tick drives the real arrival path (on_block) with no tick. It fails on db61388 (status stays Idle) and passes now. The lean half of the second commit is a one-line mirror with no lean-specific test.
• make fmt, make lint and make test pass on 5e0a9d1 (2,935 tests, 0 failed).
• Kurtosis: lighthouse×2 plus ethlambda beacon (cl-3) with an ethlambda VC (vc-3), mainnet preset.

  • Two tools: iptables drops on the CL p2p ports 9000–9001 in cl-3's network namespace ("cut"), and a netem delay on cl-3's outbound engine-API traffic (TCP 8551) to model a slow EL.

  • Results below are from 5e0a9d1, on a fresh enclave. Of the later commits, d907b60 is docs-only. 6f08c98 spreads the Status refresh over the interval and is covered by unit tests; the devnet scenarios were not rerun on it. They exercise the handshake Status, not the refresh.

  • The slow catch-up is simulated with the netem delay. Lean was not run on a live network: kurtosis runs the beacon chain.

  • Synced: synced from genesis, 2 peers, duty endpoints 200, and the VC attests, aggregates and does sync-committee duties every slot.

  • Cut off with the VC stopped, then reconnected under a slow EL: the VC was stopped so the node's own head stalls, as a lone cut-off node's does. The cut ran from slot 12 to slot 112, and reconnect happened with a 1 s engine delay, so each import takes about 2 s.

    • Stall: peers fell to 0 within about 20 s. The node read is_syncing=false with duty endpoints at 200 throughout, up to 100 slots behind.
    • Reconnect: 2 s after the link came back the node logged Sync status changed current_slot=112 head_slot=14 network_head=111 clock_lag=98 peer_lead=97 starting_up=false syncing=true. The network head came from peers' Status, not from the startup hold.
    • Duty endpoints answered 503 in all 42 one-second samples until clock_lag=64 peer_lead=59 … syncing=false, 48 s later.
    • The VC, started as soon as the gate read Syncing, logged Failed to refresh duties … beacon node is syncing every slot and published nothing (no block, attestation or sync-committee message) until the gate reopened.

  • Restart after downtime: cl-3 was stopped at slot 121 and started at slot 201 (Resuming from existing DB head_slot=120 current_slot=201 gap=81).

    • It came up Syncing through the startup hold (starting_up=true).
    • The gate reopened 174 ms later, once range sync had moved the head from 120 to 137 (lag 64). That is too short for the 1 s monitor to catch; the two transitions are in the node's log.
    • The VC had no duty in that window and was attesting on a current head by the next slot.

  • Short cut with the VC running: 12 slots, from slot 207 to 219.

    • The node kept reading synced with 0 peers, and the VC kept signing on its own view: 3 blocks, 9 slots of attestations, 10 of aggregates, 11 of sync-committee messages.
    • After the link was restored, cl-3 and Lighthouse had the same head root at slot 232.

  • Before the second commit (db61388), same cut-and-slow-EL scenario:

    • The stall phase behaved the same.
    • On reconnect the node stayed is_syncing=false for about 53 s while 101 to 64 slots behind with a peer back, and no "Sync status changed" line was logged.
    • The VC, started 22 s after reconnect, proposed a block at slot 270 on a head about 80 slots stale and published sync-committee messages. Its attestations were refused (target epoch is too far from this node's head).

  • Expected, within tolerance: once the gate reopens at 64 slots behind, the VC works again while the node finishes catching up. The node refused its attestations there (target epoch is too far …), and sync-committee messages went out. This is the 2-epoch tolerance Lighthouse's duty filter applies too.

Follow-ups (not in this PR)

• Range sync forwards a whole catch-up into the chain actor's mailbox ahead of import, and the tick has no priority. That starves other tick-driven work during a catch-up, not just the gate.
• Judging the gate at read time in the RPC would remove the remaining one-handler staleness.

Related Issues / PRs

• Closes Duty sync gate reports Synced while the node is far behind, so it attests and proposes on a stale head #559
• Alternative to fix(sync): prevent validator duties on stale heads #618

✅ 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) — all passing

[github-actions]

🤖 Codex Code Review

1. [P1] Incoming Status requests can prevent catch-up indefinitely — peer_heads.rs:83. Every incoming request resets last_exchange, postponing our refresh. However, incoming Status handlers only record the head and respond; only response handlers initiate range sync. A peer sending requests every refresh interval can therefore keep a node behind without triggering catch-up, while the advertised lead disables Lean validator duties. Process catch-up decisions for incoming Status messages too, or track outgoing refresh timing separately.

2. [P2] Peer head claims never expire after refresh failures — peer_heads.rs:102. Reports remain eligible until disconnection or another Status message replaces them. Status timeouts do neither. A peer can advertise an accepted head, then stop answering Status and block requests while keeping its connection alive; if the local head remains behind, that stale claim can suppress Lean duties indefinitely. Track successful report timestamps separately from request attempts and expire reports after a bounded period.

---

Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

[github-actions]

🤖 Grok Code Review

I'll inspect the sync-status and peer-head changes in context, especially ordering, overflow, and how network_head is published.

---

Automated review by Grok (xAI) · grok-4.7 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: PR 791, peer-reported network head for the sync gate

The direction is right. Comparing against peers' Status heads instead of imported blocks fixes the catch-up blind spot (issue 559). Gating on both the clock and the peers matches how Lighthouse and Prysm do it. The tests are thorough and the deviation is well documented. Findings follow, most important first.

Correctness and security

1. Stall-time liveness attack: peer claims are never checked against delivery.

   • Where: peer_heads.rs, network_head; sync_status.rs, update.
   • What protects the node today: the clock condition stops a liar from idling a node whose head is current.
   • When that protection disappears: during a network-wide stall, or a run of missed proposals (leanSpec 1194), every honest node has clock_lag over the threshold. Then the peer claim alone decides.
   • How cheap the attack is:
     • Below PEER_HEAD_QUORUM, one peer reporting wall_slot + 1 closes the gate.
     • At quorum, two colluding peers are enough.
     • This turns validators off exactly when they are needed for recovery. That matters most on small lean devnets, where 2–3 peers is normal.
   • What Lighthouse adds: its condition is "range sync is running", and a peer that cannot serve the blocks it advertised fails the batch and gets downscored.
   • What is missing here: there is no such feedback loop. A claimed head stays in heads until the next refresh, and the liar simply repeats it.
   • Suggestion: when a range request to a peer returns nothing past our head, drop that peer's entry in PeerHeads (or ignore it for a while), and/or penalize the peer. At minimum, document this as a known limitation in spec_deviations.md.

2. A Status that lands after disconnect leaves a stale head behind.

   • Where: peer_heads::observe, called from handle_lean_status_request/response and handle_status_request/response.
   • What it does: observe unconditionally inserts into heads and last_exchange.
   • The race: if a response or request event is processed after ConnectionClosed (event ordering through the actor mailbox, or multi-connection edge cases), the entry is never forgotten. That inflates network_head indefinitely.
   • Suggestion: a cheap guard in observe:

     if !server.connected_peers.contains_key(&peer) { return; }

3. The refresh mark can be consumed by a slow handshake answer.

   • What happens: due_for_refresh treats a first-seen peer as having just exchanged. If its handshake answer takes longer than interval, a refresh gets sent and begin_refresh marks the peer.
   • Consequence: the handshake answer is then treated as a refresh, so a lead of 4 or less (lean) or 32 or less (beacon) does not start range sync.
   • Leak: marks for refresh requests that fail or time out are never cleared until disconnect.
   • Suggestion: low impact, but clear the mark in the outbound-failure path, or skip refreshing peers whose handshake hasn't completed.

4. Inconsistent digest check in beacon handle_status_response.

   • In handle_status_request you gate observe on is_syncable_digest.
   • In handle_status_response, observe sits after the "handshake complete" log. Please confirm that every path reaching it has already rejected or returned for unsyncable digests. Otherwise a peer on a superseded digest feeds network_head through the response path.

5. Beacon has no hysteresis, but the log level went to info!.

   • Where: sync_status.rs, SyncThresholds::BEACON.
   • Why it flaps: peer_lead now moves with every refresh answer, and the gate is judged on every block arrival as well as every tick. Near the 64/32-slot boundaries the status can flap.
   • Suggestion: either a small band (e.g. 2–4 slots) or keep the transition log at debug! for beacon.

Performance

6. Per-block cost on beacon catch-up.

   • Where: recompute_beacon_head now calls update_sync_status, which calls head_slot() (a beacon_head table decode).
   • When it bites: this runs on every forwarded block during range sync.
   • Assessment: probably negligible next to block import. If update_head_from_fork_choice already holds the head slot, pass it through instead of re-reading.

7. PeerHeads::network_head allocates and sorts on every observe/forget. That is fine at 200 peers. If you want, select_nth_unstable avoids the full sort.

Rust and maintainability

8. Possible dead code. Store::max_live_chain_slot may now be unused. Check for a dead-code warning, or remove it if this was the last caller.

9. Duplicated wall-clock computation. peer_heads::wall_clock_slot duplicates the chain actor's wall-clock-slot logic. Consider a shared helper on Config/TimeGrid so the two cannot drift.

10. Test depends on GENESIS_TIME. In lib.rs, a_block_arrival_judges_the_gate_without_waiting_for_the_tick uses the real wall clock. It only passes if wall_clock_slot() is more than 2 * SLOTS_PER_EPOCH from the head. That holds if GENESIS_TIME is old, but an assertion or comment making the dependency explicit would avoid a confusing failure if the fixture changes.

11. Nit: shadowing wire with expect. The let wire = server.wire.beacon().expect("checked above"); re-borrow in handle_status_request is correct under NLL, but the shadowing is easy to misread. A short comment that the re-borrow exists because observe needs &mut server would help.

Spec

12. Interop on mixed-client devnets. The lean deviation from leanSpec PR 708 is consensus-adjacent: it changes when validators sign. It is clearly documented, but on mixed-client lean devnets this client will idle in cases where others keep working. That is worth raising upstream in leanSpec so the rules converge.

Overall the PR is solid. Item 1 (no delivery feedback on claimed heads) and Item 2 (post-disconnect insert) are the ones I'd want addressed or explicitly accepted before merge.

---

Automated review by Claude · claude-opus-5-5 · custom prompt

[github-actions]

🤖 DeepSeek Code Review

The PR substantially improves sync gating by comparing against peer-reported heads rather than imported blocks. The logic is well-structured and documented. A few issues remain:

High priority

1. Lean block-arrival path may not update the sync gate. The PR adds an immediate update_sync_status call inside recompute_beacon_head (crates/blockchain/src/lib.rs), which is used only for the beacon path. The lean path in on_tick receives an update only when the tick runs, but block arrivals on lean (e.g. during a range-sync catch-up) do not appear to trigger update_sync_status. The new test a_block_arrival_judges_the_gate_without_waiting_for_the_tick only exercises the beacon server (syncing_server returns a beacon server). This leaves lean vulnerable to the exact issue described in the PR (gate stays Synced during a catch-up). Please verify the lean block-arrival path and add the equivalent sync-status update if missing.

Medium priority

2. A failed refresh request leaves refreshing set indefinitely. In handle_refresh_peer_status (crates/net/p2p/src/lib.rs), begin_refresh is called before request_status. If the request fails or times out while the peer remains connected, the peer ID stays in PeerHeads::refreshing. The next time a Status arrives from that peer (e.g. an unsolicited response or a later handshake), take_refresh will incorrectly return true, potentially suppressing range sync even when the peer is far ahead. Consider clearing the mark on request failure or tracking request state separately.

Low priority / suggestions

3. Use stronger memory ordering for Store::network_head. The atomic is written via set_network_head with Ordering::Relaxed and read with Ordering::Relaxed. While relaxed is sufficient for correctness (no associated data), using Release for the store and Acquire for the load would provide clearer publication semantics across the P2P and chain actor threads.

4. Sequential await in the refresh loop may add latency. In handle_refresh_peer_status, each request_status is awaited inside a for loop. If send_request or send_status ever blocks on I/O (e.g. waiting for the request to be queued), this could stall the P2P actor. The number of peers per slot is bounded (≤13 for beacon default), so this is unlikely to be a problem, but spawning tasks or using join_all would be safer.

5. heard_from_peers is set by synthetic peer heads. In update_sync_status (crates/blockchain/src/lib.rs), the beacon branch overriding network_head to Some(current_slot) when the clock is past an unfollowed fork causes SyncStatusTracker to mark heard_from_peers = true without an actual peer. This could end the startup hold earlier than intended, though the situation likely only arises after startup.

Overall the core design is sound and the tests cover the important cases. The lean path deserves immediate attention.

---

Automated review by DeepSeek · deepseek-v4-pro · custom prompt

[github-actions]

🤖 Gemini Code Review

The changes look solid. Moving the sync tracker from max_live_chain_slot to quorum-filtered, periodically refreshed peer Status heads (combined with local wall-clock lag and a bounded startup grace period) cleanly solves the catch-up false-synced issue while preserving stall recovery and bounding untrusted peer claims. Judging the gate on block arrivals in addition to ticks also addresses actor mailbox queueing during range sync.

One minor doc nit:

1. crates/blockchain/src/sync_status.rs (lines 70–72):

   /// Recovery band: once syncing, both lags must drop this many slots below
   /// their thresholds before the node reports synced again, so the status
   /// does not flap at the boundary.
   

   In SyncStatusTracker::update (lines 173–174), self.syncing is clock_lag > clock_threshold - band && peer_lead > lead_threshold - band, so the node stays syncing while both remain above their reduced thresholds and returns to Synced as soon as either lag drops hysteresis slots below its threshold.

---

Automated review by Gemini · gemini-4-argon · custom prompt

[github-actions]

🤖 Kimi Code Review

• [P2] Expire peer-head claims when refreshes fail — crates/net/p2p/src/peer_heads.rs:73-130, together with handle_refresh_peer_status in crates/net/p2p/src/lib.rs:1754-1781. Entries in PeerHeads::heads never age out: last_exchange is advanced when a request is sent, while network_head() continues including the last response indefinitely. A peer can therefore report a high head, stop answering Status, and remain connected while its stale claim continues driving Store::network_head and the quorum. On a node whose head trails the clock, this can leave it Syncing with validator duties gated without current peer evidence. Track response/report timestamps separately, expire claims after a bounded refresh timeout or several missed responses, and republish the aggregate on timer ticks since expiry can occur without a new Status or disconnection. A test covering a connected but unresponsive peer would help pin this down.

Otherwise, the threshold arithmetic, startup handling, cross-actor atomic state, and refresh batching look sound and are well covered by tests.

---

Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt