fix(follower): handle syncing semantics in the executor - #7353
Conversation
b65ca11 to
7c49ea4
Compare
|
cyclops audit fast |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change adds follower-executor syncing retries and startup backfill, but two verified liveness failures remain.
Reviewer Callouts
- ⚡ Execution-provider test stub: It returns
VALIDfor fabricated heads never submitted throughnew_payload, masking Reth's unknown-headSYNCINGbehavior. - ⚡
last_fcusemantics: It represents both a speculative pipeline target and an accepted forkchoice head, although those states diverge afterSYNCING. - ⚡ Single pending acknowledgement: With
max_pending_acks = 1, any indefinitely withheld acknowledgement halts consensus-to-execution delivery.
| match submit_forkchoice_update(&context, &execution_engine, &forkchoice).await { | ||
| Ok(()) => ExecutionTaskResult::Completed(forkchoice), | ||
| Ok(ForkchoiceOutcome::Valid) => ExecutionTaskResult::Completed(forkchoice), | ||
| Ok(ForkchoiceOutcome::Syncing) => ExecutionTaskResult::Completed(forkchoice), |
There was a problem hiding this comment.
🚨 [SECURITY] Speculative certified head permanently deadlocks finalized-block delivery
A head-only FCU that returns SYNCING is recorded as completed, so its block-unknown certified head becomes last_fcu. The next finalized block FCU is built from that speculative head; because the certificate has a higher round, advancing finality does not repoint the head to the delivered block. Reth returns SYNCING, the block is requeued at the front without acknowledgement, and max_pending_acks = 1 prevents marshal from delivering the blocks needed to make the head reachable. Normal certificate-before-block ordering can therefore make the follower retry forever.
Recommended Fix:
Track speculative pipeline targets separately from FCUs confirmed VALID. Build block FCUs from a confirmed head (or the just-accepted block), acknowledge a block whose new_payload succeeded even if only its speculative-head FCU is syncing, and retry the speculative head asynchronously. Update the stub to return SYNCING for unknown heads and add this regression case.
There was a problem hiding this comment.
Non issue. The head will be reachable over el p2p. Marshal will deliver Update::Block as they come in
| .marshal | ||
| .get_block(Height::new(height)) | ||
| .await | ||
| .ok_or_else(|| eyre!("marshal missing backfill block at height `{height}`"))?; |
There was a problem hiding this comment.
Any missing marshal block between the EL finalized height and the stored floor becomes an error, and run immediately returns. The follow engine is torn down and, while storage remains unchanged, every restart fails at the same height. The unbounded backfill retry loop also prevents the actor from servicing normal block and finalization messages while Reth remains syncing.
Recommended Fix:
Make startup backfill recoverable: do not terminate the actor when marshal temporarily lacks a height, and schedule bounded/yielding retries while continuing to service the mailbox (or reconcile the floor and resume normal marshal delivery). Add tests for a missing intermediate block and persistent SYNCING during backfill.
There was a problem hiding this comment.
Non issue. Anything marshal has processed should be durable in order. There cannot be gaps or this would be corrupted state
| ); | ||
| actor.start(); | ||
|
|
||
| context.sleep(Duration::from_secs(2)).await; |
There was a problem hiding this comment.
The first part of this test has new_payload syncing for the first time so we need to sleep to account for the retry
There was a problem hiding this comment.
I think it makes sense to split this into a unique test though
04bdc3f
into
hamdi/follow.executor.updates
3 participants
Uh oh!
There was an error while loading. Please reload this page.