ethereum-optimism / ethereum-optimism/optimism

op-acceptance-tests: add real regression guard for EL restart SYNCING recovery

Open
#20,307 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.