Skip to content

fix(sync): prevent validator duties on stale heads - #618

Open
dicethedev wants to merge 1 commit into
lambdaclass:mainfrom
dicethedev:fix/duty-sync-gate
Open

dicethedev wants to merge 1 commit into
lambdaclass:mainfrom
dicethedev:fix/duty-sync-gate

Conversation

@dicethedev

Copy link
Copy Markdown
Contributor

🗒️ Description / Motivation

This PR fixes the duty sync gate reporting Synced while the local node is far behind the network.
Previously, the gate used the highest slot in the local LiveChain table. Pending blocks and peer-advertised heads were excluded, so the freshest-known slot advanced alongside the stale local head during backfill. The heuristic interpreted this as a network-wide stall and incorrectly allowed proposals and attestations.
The node now tracks the highest block slot observed from valid peer status messages, pending blocks, imported blocks, and the persisted head after restart.

What Changed

  • Added a shared, runtime-only latest-known-block-slot marker to Store.
  • Initialized the marker from the persisted head when resuming an existing database.
  • Advanced the marker when pending or fully processed blocks are stored.
  • Recorded peer heads from inbound and outbound Status exchanges.
  • Rejected Status messages whose heads are implausibly ahead of the wall-clock slot.
  • Updated the blockchain sync gate to use the latest-known slot instead of scanning LiveChain.
  • Added regression tests for stale-head duty suppression, pending blocks, peer heads, future heads, and database restoration.

Correctness / Behavior Guarantees

  • A node with evidence that the network is ahead remains Syncing and does not propose or attest on a stale head.
  • Pending blocks can update the sync signal without becoming visible to fork choice.
  • The latest-known slot is monotonic and shared across Store clones.
  • Implausibly future peer heads cannot permanently disable validator duties.
  • The existing network-stall behavior is preserved when no peer or block provides evidence of a fresher chain.
  • No new state is persisted and the database schema remains unchanged.

Tests Added / Run

Added tests covering:

  • Duty suppression when the network head is 550 slots ahead.
  • Shared and monotonic latest-known-slot tracking.
  • Pending blocks advancing the latest-known slot.
  • Valid peer Status messages advancing the marker.
  • Implausibly future peer heads being rejected.
  • Restored stores initializing the marker from the persisted head.

Related Issues / PRs

✅ Verification Checklist

  • Formatting check is clean
  • Ran make fmt — clean
  • Ran make lint (clippy with -D warnings) — clean
  • Ran make test (cargo test --workspace --profile release-fast) — all passing

@pablodeymo pablodeymo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, @dicethedev! The PR is well scoped, the description is clear, and the shared Arc<AtomicU64> marker is a nice fit, since both actors already share the Store. I ran fmt, clippy and the full workspace suite locally and everything passes.

A few comments below.

1. Future-head rejection changes range-sync behavior

handle_status_response now returns early for a peer whose head is more than one slot past our wall clock, so we no longer range-sync from that peer. The tolerance is also slot-granular and wall-clock based, while the block path uses store time plus GOSSIP_DISPARITY_INTERVALS. Could the two use the same bound?

Smaller items

  • Store::max_live_chain_slot has no callers left, so it can be removed.
  • The wall-clock slot calculation in observe_peer_head is a third copy (see rpc/src/node.rs and bin/ethlambda/src/main.rs). A small helper would keep them from drifting apart.
  • Tests:
    • Nothing exercises the actual wiring, where update_sync_status reads the store's marker. A test driving a pending block, or a peer head, through to duties_allowed() would pin the fix end to end.
    • fresh_peer_head_blocks_duties_during_long_range_sync is effectively the same as the existing sync_status_detects_local_lag_when_fresh_blocks_are_known.
    • The new early return in handle_status_response has no test.

Thanks again, this closes a real gap!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duty sync gate reports Synced while the node is far behind, so it attests and proposes on a stale head

2 participants