Skip to content

Commit f1b9f17

Browse files
vmarkushinclaude
andcommitted
fix(node): push the startup head rewind to the execution node (PR review pass 67)
Codex review pass 67 raised one P1 on the issue #38 CI-stabilization branch. The pass-66 startup reconciliation dragged the in-memory ForkchoiceState mirror down to the persisted head and updated the database, but never sent engine_forkchoiceUpdated to the execution node. Engine::new only stores the mirror, ChainOrchestrator::new issues no forkchoice update, and the run loop reissues a head FCU only in response to derivation/finalization/peer events — so on a quiescent or non-producing node the execution client kept its canonical head (and its RPC) on the discarded block indefinitely, letting remote followers still import the block the repair intended to drop. The same latent gap existed in the pre-existing repair loop whenever the execution node ran ahead of the database above finalized. Fix: capture the execution node's live head before any startup head-repair, and after the engine is built, if a repair rewound the mirror BELOW that live head, issue a checked forkchoice update to the execution node to actually rewind its canonical head. A VALID answer confirms the rewind; INVALID (the execution node rejecting a head recovered from the node's own persisted state) fails startup; a SYNCING/ACCEPTED answer is left to be reasserted by later FCUs. Skipped when the provider forkchoice could not be read (engine unreachable / genesis fallback), since there is no live head to rewind. This covers both the pass-66 addition and the pre-existing repair loop with a single push. The lack of automated coverage for this startup-rewind path (it needs an e2e restart with the execution node ahead of the database) is recorded in .claude/vmark-pr-review-follow-ups.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 2246ea1 commit f1b9f17

2 files changed

Lines changed: 81 additions & 1 deletion

File tree

.claude/vmark-pr-review-follow-ups.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,27 @@ from every pass is either fixed in the PR or recorded here.
55

66
## Unresolved
77

8+
- **No e2e coverage for the startup rewind of an execution node that ran ahead
9+
of the persisted head** (`crates/node/src/args.rs` startup head-repair + the
10+
new post-`Engine::new` forkchoice push)
11+
- Impact/evidence: Codex pass 67 P1 (2026-09-03) found that the pass-66
12+
startup reconciliation only moved the in-memory mirror and the database, and
13+
never pushed `engine_forkchoiceUpdated` to Reth, so a quiescent node would
14+
keep serving — and followers keep importing — the discarded head. FIXED this
15+
pass by issuing a checked FCU to the execution node after the engine is
16+
built whenever startup repair lowered the mirror below the execution node's
17+
live head (also closes the same latent gap in the pre-existing repair loop).
18+
The new path has no automated test: it needs an e2e restart where the
19+
execution node holds a block above the persisted L2 head (an unsigned
20+
sequenced block left by a crash between commit and sign), asserting the
21+
execution node's RPC head is rewound on restart.
22+
- First/most-recent pass: Codex pass 67 (2026-09-03).
23+
- Why unaddressed: exercising it requires a full node restart against a real
24+
execution client with a head deliberately ahead of the database — an e2e
25+
harness addition beyond this review loop's local-fix scope; the production
26+
fix itself is landed and verified by build/clippy.
27+
- Suggested Linear title: "rollup-node: e2e test for startup rewind when the execution node runs ahead of the persisted head"
28+
829
- **Remote block source has no metrics** (`crates/node/src/add_ons/remote_block_source.rs`)
930
- Impact/evidence: Claude pass 1 m6 and pass 5 m2 — the add-on exports no
1031
metrics and `ChainOrchestratorStatus` does not model it, so a node can

crates/node/src/args.rs

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,13 @@ impl ScrollRollupNodeConfig {
582582
}
583583
};
584584

585+
// The execution node's live head, before any startup head-repair below
586+
// moves the in-memory mirror. If a repair rewinds the mirror BELOW this,
587+
// the rewind must be pushed to the execution node (see the FCU after the
588+
// engine is built); otherwise only the mirror and database move and the
589+
// execution node keeps serving the discarded head.
590+
let provider_head_number = fcs.head_block_info().number;
591+
585592
let (l1_block_startup_info, mut l2_head_block_number) = db
586593
.tx_mut(move |tx| async move {
587594
// On startup we replay the latest batch of blocks from the database as such we set
@@ -723,7 +730,59 @@ impl ScrollRollupNodeConfig {
723730
ctx.task_executor.spawn_task(scroll_network_manager.run());
724731

725732
tracing::info!(target: "scroll::node::args", fcs = ?fcs, payload_building_duration = ?self.sequencer_args.payload_building_duration, "Starting engine driver");
726-
let engine = Engine::new(Arc::new(engine_api), fcs);
733+
let mut engine = Engine::new(Arc::new(engine_api), fcs);
734+
735+
// If the startup head-repair above rewound the mirror BELOW the
736+
// execution node's live head, the repair so far has only moved the
737+
// in-memory mirror and the database — the execution node still holds the
738+
// discarded head (e.g. an unsigned block committed to the engine by a
739+
// crash between commit and sign, or an EL that simply ran ahead). Push
740+
// and validate the rewind against the execution node now, before
741+
// launching: otherwise a quiescent node keeps serving — and followers
742+
// keep importing — the block this repair discarded, because nothing in
743+
// the run loop reissues a head FCU on an idle chain. Skipped when the
744+
// provider forkchoice could not be read (engine unreachable / genesis
745+
// fallback), since there is no live head to rewind.
746+
if !provider_fcs_missing && engine.fcs().head_block_info().number < provider_head_number {
747+
let head = *engine.fcs().head_block_info();
748+
match engine.update_fcs_checked(Some(head), None, None).await {
749+
Ok(result) if result.is_valid() => {
750+
tracing::info!(
751+
target: "scroll::node::args",
752+
?head,
753+
provider_head_number,
754+
"Rewound the execution node head to the recovered startup head"
755+
);
756+
}
757+
Ok(result) if result.is_invalid() => {
758+
// The execution node actively rejected a head the node
759+
// recovered from its own persisted state — a genuine
760+
// divergence that must not launch.
761+
eyre::bail!(
762+
"execution node rejected the recovered startup head {head:?} as INVALID: \
763+
{:?}",
764+
result.payload_status.status
765+
);
766+
}
767+
Ok(_result) => {
768+
// SYNCING/ACCEPTED: the execution node took the head but has
769+
// not finished adopting it. The mirror already holds it and
770+
// every later FCU reasserts it, so do not fail startup.
771+
tracing::warn!(
772+
target: "scroll::node::args",
773+
?head,
774+
"Execution node has not yet adopted the recovered startup head; it will \
775+
be reasserted"
776+
);
777+
}
778+
Err(err) => {
779+
eyre::bail!(
780+
"failed to push the recovered startup head {head:?} to the execution \
781+
node: {err}"
782+
);
783+
}
784+
}
785+
}
727786

728787
// Create the consensus.
729788
let authorized_signer = if let Some(provider) = l1_provider.as_ref() {

0 commit comments

Comments
 (0)