ethereum-optimism / ethereum-optimism/optimism
op-acceptance-tests: add real regression guard for EL restart SYNCING recovery
- Dominant language
- Go
- Stars
- 6.5k
- Forks
- 4k
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 145
Description
## Context
PR #19384 fixes a stall in `EngineController.tryUpdateEngineInternal`: when an EL returns `SYNCING` to a forkchoice update (e.g. after an op-reth restart that loses in-memory canonical state), op-node previously recorded the FCU as accepted and never retried. The fix triggers a pipeline reset in CL-sync mode so `FindL2Heads` can re-discover the EL's actual chain state.
The PR originally included an acceptance test at `op-acceptance-tests/tests/sync/el_restart/TestELRestartRecovery` that exercised this flow end-to-end: stop the verifier op-reth, let the sequencer advance ~10 blocks, restart op-reth, and verify the verifier catches up.
## Problem
Empirical validation (with and without the op-node fix applied locally against current `develop`):
| Check | With fix | Without fix |
|---|---|---|
| Unit test `TestTryUpdateEngine_SyncingInCLModeTriggersReset` | PASS | **FAIL** — `An error is expected but got nil` |
| Unit test `TestTryUpdateEngine_SyncingInELSyncModeIsAccepted` | PASS | PASS (preserved behavior) |
| Acceptance test `TestELRestartRecovery` | PASS (~40s) | **PASS** (~41s) |
The unit tests are real regression guards. The acceptance test, as written, is not — it passes identically whether or not the fix is applied. The observed recovery path does not appear to exercise the SYNCING-response-from-cached-FCU scenario that the fix addresses. Likely explanations:
- op-reth's persistence on clean shutdown is sufficient that its canonical head survives restart, so FCU returns VALID rather than SYNCING.
- CL P2P gossip delivers newer blocks that op-reth imports via `NewPayload` before op-node's FCU with the cached heads hits the EL, so op-reth already "knows" the chain when FCU arrives.
- The 10-block gap is too small or the restart too clean to reproduce the in-memory state loss the fix assumes.
Because the acceptance test did not reliably demonstrate the bug, it has been dropped from the replayed PR (see below). The op-node fix ships with unit-test coverage only.
## What's needed
An acceptance test that **deterministically** reproduces the SYNCING-on-FCU condition after an EL event, so it FAILs on the pre-fix `engine_controller.go` and PASSes with the fix. Possible directions:
1. **Force op-reth to drop canonical state on restart** — e.g. wipe or bypass the on-disk head marker via a sysgo helper that simulates the async-persistence-lost-on-crash case.
2. **Directly drive the engine** — bypass the full restart and use a testing hook or mock EL that returns `SYNCING` to the next FCU; assert op-node emits a ResetEvent and recovers.
3. **Ungraceful restart** — if op-reth has a code path where killing the process mid-write leaves in-memory state unpersisted, reproduce that via `Kill()` rather than `Stop()`.
Option (1) or (3) is closest to the production failure mode. Option (2) is cheaper but further from reality.
## References
- PR #19384 — original fix + acceptance test (acceptance test later dropped)
- Replayed PR on current `develop`: _to be linked_
- op-node change: `op-node/rollup/engine/engine_controller.go` — added `checkForkchoiceUpdatedStatus` gate before recording `lastForkchoice`
🤖 *Generated by Claude Code*
Contributor guide
Research direction
Start with op-acceptance-tests/tests/sync/el_restart/TestELRestartRecovery and compare it with op-node/rollup/engine/engine_controller.go and the unit test TestTryUpdateEngine_SyncingInCLModeTriggersReset. Determine how to produce SYNCING on a forkchoice update after an EL restart, using an ungraceful restart, altered persistence, or a direct engine hook. Done means the acceptance test fails without the fix and passes with it.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- distributed-systems, testing-qa
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100