feat: Allow follower nodes to backfill blocks#679
Conversation
b49a15a to
6b5d502
Compare
fcb563f to
4847c48
Compare
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #679 adds P2P block backfill for lagging followers and recovering leaders, but the new request/response protocol and leader-side import path have several actionable safety and liveness issues. I left inline comments on the diff for the findings.
Reviewer Callouts
- ⚡ Backfill protocol metadata: Several findings stem from peer-presence-only request/response state. Please review the full protocol for request IDs, range/count binding, deadlines, and byte budgets.
- ⚡ Leader recovery trust model: If manifest followers are Byzantine-tolerated, recovering leaders need block certificates/finality evidence rather than raw follower-provided block bytes.
- ⚡ Persisted vs canonical wording:
serve_backfillreadsbest_block_number()andblock_by_number(), which can include canonical in-memory blocks, while the README says responses come from the “persisted canonical chain.” - ⚡ Missing-block completion behavior:
serve_backfillreturns before sendingCompleteBackfillif a block is missing mid-page, leaving the requester waiting for timeout-based retry.
| warn!(target: "zone::p2p", %peer, size = bytes.len(), "Ignoring malformed backfill request"); | ||
| continue; | ||
| }; | ||
| P2pEvent::BackfillRequested { peer, start: u64::from_be_bytes(bytes) } |
There was a problem hiding this comment.
🚨 [SECURITY] Ungated backfill requests are served inline, enabling amplification DoS of block import
run_receivers emits BackfillRequested for any authenticated manifest peer that sends an 8-byte request; unlike backfill blocks/completions, this branch has no role/eligibility gate. The node then awaits serve_backfill inline in the block-sync loop, so a compromised manifest peer can turn tiny request messages into repeated block reads/RLP encodes and stall live block import/backpressure the P2P event loop.
Recommended Fix: Gate request messages by role/eligible peer, add per-peer serving-side rate limits or one in-flight page per peer, and move/drop/coalesce expensive serving work so it cannot starve live block import.
There was a problem hiding this comment.
Fixed. The backfill now runs in its own channel with its own limits and timeouts
| @@ -235,7 +511,7 @@ where | |||
| let payload = ZonePayloadTypes::block_to_payload(block, None); | |||
There was a problem hiding this comment.
🚨 [SECURITY] Leader backfill imports follower-supplied blocks without leader-authorship or zone-specific validation
Leaders now import backfilled blocks through ZonePayloadTypes::block_to_payload, new_payload, and FCU. This validates execution/parent/forkchoice, but it does not verify a leader signature/quorum certificate or enforce ZonePayloadBuilder invariants such as L1 continuity and system-transaction layout. A Byzantine follower can race a leader recovery/probe window with an execution-valid follower-authored block and have the leader canonicalize it.
Recommended Fix: Do not let active leaders canonicalize raw follower-returned blocks by default. Require independent authorship/finality evidence before import, and move zone-specific L1/system-transaction validation into the external block import path.
There was a problem hiding this comment.
This is TBD, leader backfill needs slightly different code, will be in a follow-up PR.
| } | ||
| task_executor.spawn_critical_task( | ||
| "zone-p2p-block-sync", | ||
| run_block_sync(provider, engine, events, commands), |
There was a problem hiding this comment.
🚨 [SECURITY] Leader backfill advances the execution head without reconciling ZoneEngine state
Spawning run_block_sync for leaders lets P2P imports advance the provider/EL head through the Reth engine handle, but that path cannot update ZoneEngine::last_header, DepositQueue, or the TIP-403 policy-cache baseline. A valid imported block can leave the sequencer believing it is still at the old parent, causing duplicate L1 reprocessing attempts, competing blocks, or persistent leader liveness failure.
Recommended Fix: Disable leader P2P import while ZoneEngine is active, run recovery before starting/seeding the sequencer state, or add an atomic reconciliation API that advances last_header, confirms queued L1 blocks, and updates the policy cache through imported history.
There was a problem hiding this comment.
This is TBD, leader backfill needs slightly different code, will be in a follow-up PR.
| warn!(target: "zone::p2p", %peer, "Ignoring backfill block from ineligible peer"); | ||
| continue; | ||
| } | ||
| if !outstanding_backfill_responses |
There was a problem hiding this comment.
🚨 [SECURITY] Backfill response windows never expire and enforce no receive-side page cap
The receive-side authorization is only outstanding_backfill_responses.contains_key(&peer). It does not enforce the requested start/max end, 64-block page size, received count, byte budget, request ID, or deadline; the 30s timeout only controls future sends. A responder that withholds completion after one legitimate request can stream unlimited accepted backfill block messages and force repeated decode work or fill large byte queues/pending buffers.
Recommended Fix: Track explicit request state {request_id,start,max_end,received_count,deadline,byte_budget}, reject out-of-range/over-count blocks, expire windows independently of completion, and add byte-budgeted buffering.
There was a problem hiding this comment.
Fixed. The backfill now runs in its own channel with its own limits and timeouts
| continue; | ||
| }; | ||
| { | ||
| let mut outstanding = outstanding_backfill_responses.lock().await; |
There was a problem hiding this comment.
Backfill blocks and completions are received on separate Commonware channels under an unbiased tokio::select!. If the completion branch runs before all queued block messages from the same page have been drained, it removes the peer from outstanding_backfill_responses; later page blocks are then dropped as unsolicited before they reach the buffering/import layer.
Recommended Fix: Use a single ordered response channel, or include page metadata and remove the outstanding entry only after all expected blocks for that request/page have been observed.
There was a problem hiding this comment.
Fixed. The backfill now runs in its own channel with its own limits and timeouts
| /// Leader-to-follower sealed block replication channel. | ||
| pub(crate) const BLOCK_CHANNEL: u64 = 0; | ||
| /// Catch-up range requests sent by a lagging node to eligible peers. | ||
| pub(crate) const BACKFILL_REQUEST_CHANNEL: u64 = 1; |
There was a problem hiding this comment.
This adds channels 1-3 but leaves the protocol namespace version unchanged (WIRE_PROTOCOL_VERSION = 0). PR-head and merge-base nodes can authenticate into the same Commonware namespace while registering different channel sets; pinned Commonware treats an unknown channel as InvalidChannel and tears down the peer, so an upgraded follower's mandatory channel-1 backfill probe can repeatedly disconnect an older leader.
Recommended Fix: Bump WIRE_PROTOCOL_VERSION for this incompatible channel set, or add explicit capability/version negotiation and avoid sending backfill channels to v0/channel-0-only peers.
There was a problem hiding this comment.
This is not live yet, so there's nothing to break for backward compatibility.
Note: