Repository navigation
fix(op-node): keep sequencing responsive during derivation bursts - #554
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Port ethereum-optimism/optimism@cc632b4. Adapt the change to the older Celo engine-controller API and omit SuperAuthority deny-list handling, which is not present in celo-v2.2.3. Include the stale sequencer-parent guard required by the direct-call path.
ethereum-optimism/optimism@80d82a2 runs the sequencer on its own goroutine. Preserve Celo's fixed 50 ms sealing duration, SequencerUseFinalized L1-origin selection, and AltDA unsafe-gap ticker. Carry the max-safe-lag stall provenance needed by the dedicated loop because this Celo base predates that upstream prerequisite. Omit ErrPayloadDenied handling because celo-v2.2.3 has no SuperAuthority integration.
Port ethereum-optimism/optimism@606f890 (ethereum-optimism#22360). An early timer fire after a backward wall-clock step could mark the deadline as handled without running the action or re-arming it, freezing block production. Keep the upstream implementation and regression test intact, adapted only to this branch's existing sequencer field names and older context.
Make the nil control flow explicit after os.Exit so staticcheck does not report a possible dereference of the missing gate.
9df3949 to
ac32a65
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df394961d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
palango
left a comment
palango
left a comment
There was a problem hiding this comment.
No blocking bugs. The port matches upstream ethereum-optimism#22238, ethereum-optimism#22241 and ethereum-optimism#22360 apart from the listed Celo adaptations, and it carries the ethereum-optimism#21913 and ethereum-optimism#21690 guards too. go test -race -count=20 ./op-node/rollup/sequencing/... ./op-node/rollup/engine/... ./op-node/rollup/driver/... passes on ac32a65, the op-e2e action tests pass, and the conductor, p2p and AltDA op-e2e suites behave the same as on the base. Locks are always taken sequencer lock first, then e.mu, then the origin selector, so I can't find a deadlock. go-tests-short is red, but #549 and #551 fail the same job.
Nothing tests on-time sealing while the event loop is busy, so let's soak it on Sepolia or Chaos across a batcher post before mainnet.
Roll this out one conductor voter at a time. Nothing changes in the Raft payload, p2p or flags, so mixed versions are fine.
The dedicated sequencer goroutine introduced by ethereum-optimism/optimism@80d82a24 updates unsafeHead while the driver event loop reads it for AltSync scheduling. Protect the getter with the engine controller RWMutex to avoid racing with ProcessPayload. Add a race-detector regression test covering concurrent head reads and updates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a184277394
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| go func() { | ||
| defer s.wg.Done() | ||
| s.sequencer.RunLoop(s.driverCtx) |
There was a problem hiding this comment.
When this goroutine runs alongside eventLoop, sequencer actions can call d.emitter.Emit directly (and direct engine calls emit through the engine controller) while the executor is delivering an event to the same actor. op-service/event/system.go:156 reads systemActor.currentEvent in Emit, while RunEvent writes and restores it at lines 173-179 without synchronization, so normal sequencing concurrent with derivation introduces a Go data race and can attribute emitted events to an unrelated derivation context. The actor emitter's context tracking must be made concurrency-safe, or out-of-band goroutines must emit through a path that does not access currentEvent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified that the sequencer-attached emitter can race on currentEvent when an out-of-band reset, error, or cancel emission overlaps event ingestion. The engine-controller part does not have this race because its emitter is registered without a deriver, so its currentEvent is never changed. For the sequencer, this field is used only for tracer and rate-limit provenance; event enqueueing, processing, block sealing, and consensus state do not depend on it. Since this PR is a production hotfix, I would prefer not to add new Celo-only emitter wiring here. Upstream develop at cd4fdde has the same behavior, so I will track a race regression test and detached-emitter design as an upstream follow-up.
The sequencer goroutine writes unsafeHead under e.mu while the driver event loop reads it unlocked. Same fix as #554 on celo-rebase-16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sequencer goroutine writes unsafeHead under e.mu while the driver event loop reads it unlocked. Same fix as #554 on celo-rebase-16.
The sequencer goroutine writes unsafeHead under e.mu while the driver event loop reads it unlocked. Same fix as #554 on celo-rebase-16.
The sequencer goroutine writes unsafeHead under e.mu while the driver event loop reads it unlocked. Same fix as #554 on celo-rebase-16.
2 participants
Summary
Move Celo op-node sequencing off the shared driver event loop so derivation bursts cannot delay the seal timer. This addresses the production behavior tracked in celo-org/celo-blockchain-planning#1469, where processing one batcher channel could occupy the event loop for 0.8-1.4 seconds and make a block seal late.
The change ports the upstream direct-call engine API and dedicated sequencer goroutine, then includes the required early-timer follow-up that prevents the new run loop from losing a deadline after a backward wall-clock adjustment.
Upstream ports
Celo adaptations
SequencerUseFinalizedL1-origin selection, and AltDA unsafe-gap ticker.maxSafeLagstall and resume behavior required by the dedicated scheduling loop.SuperAuthorityandErrPayloadDeniedhandling because they are not present in thecelo-v2.2.3base.Deliberate scope
This hotfix does not port ethereum-optimism#19638 or its companion ethereum-optimism#20310. Batching safe-head FCUs may further reduce derivation work and mutex contention, but it is not required to remove the confirmed shared-event-loop sealing delay and would change forkchoice behavior on every node. It also leaves ethereum-optimism#22458 for a separate follow-up because that fixes
Sequencer.Stopand conductor handoff rather than the normal sealing path.Validation
go test ./op-node/...go test -race ./op-node/rollup/sequencing ./op-node/rollup/driver ./op-node/rollup/engine ./op-node/rollup/attributesgo test ./op-acceptance-tests/cmd/flake-shake-promotermake lint-gogo build ./op-node/...