Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

feat(node/engine): refactor engine tasks. fix sequencer startup logic#2388

Merged
theochap merged 1 commit intomainfrom
theo/sequencer-startup
Jul 10, 2025
Merged

feat(node/engine): refactor engine tasks. fix sequencer startup logic#2388
theochap merged 1 commit intomainfrom
theo/sequencer-startup

Conversation

@theochap
Copy link
Member

@theochap theochap commented Jul 9, 2025

Description

This PR refactors the core logic of the engine task handler. In particular, it moves duplicate code from the build and the insert unsafe tasks to the forkchoice task.

Changes:

  • Add an associated Output type for the EngineTaskExt trait which allows the implementors of this trait to be composed. In particular, the ForkchoiceTask may returns the payload id that was inserted in the EL, which can be used by the BuildTask to consolidate the payload into the engine state.
  • Allows the ForkchoiceTask to directly insert newly built payload attributes into the EL (by adding an Option<OpAttributeWithParent> argument to the ForkchoiceTask). This allows to refactors both calls to the FCU inside the BuildTask using the ForkchoiceTask and avoids manually handling the forkchoice updates
  • Moves the PayloadStatus verification logic to the ForkchoiceTask to avoid having to perform the same logic in both the BuildTask and the InsertUnsafeTask, which allows to fix the sequencer kickstart issue described in feat(node/service): Sequencer startup routine #2372

Close #2372

Potential follow-ups

  • Remove the clones inside the engine tasks
  • Refactor the new_payload logic into an engine task to remove duplicate code in the InsertUnsafe and BuildTasks

@theochap theochap self-assigned this Jul 9, 2025
@theochap theochap added K-feature Kind: feature A-node Area: cl node (eq. Go op-node) handles single-chain consensus A-engine Area: engine labels Jul 9, 2025
@theochap theochap moved this to In Review in Project Tracking Jul 9, 2025
@codecov
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Project coverage is 82.2%. Comparing base (f7e2933) to head (fb9a5c8).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/node/rpc/src/ws.rs 0.0% 9 Missing ⚠️
crates/node/rpc/src/rollup.rs 0.0% 5 Missing ⚠️
crates/protocol/protocol/src/block.rs 0.0% 3 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@clabby clabby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're packing a bit too much into the ForkchoiceTask here; It now has two responsibilities:

  1. Begin a block building job
  2. Synchronize the rollup node's view of the forkchoice with the EL

each of which have slight semantic differences in the error handling, response validation checks, etc.

Though I am a fan of the attempt to modularize here. What do you think about separating the concerns here, and renaming ForkchoiceTask (as-is) to SynchronizeTask, and then creating a new task for kicking off block building?

@theochap theochap force-pushed the theo/sequencer-startup branch 5 times, most recently from 295c3d6 to 4640cf6 Compare July 10, 2025 16:47
@theochap
Copy link
Member Author

I have reworked this PR a little bit to simplify the data flow/updates for the engine state. Now:

  • The engine state only gets updated inside the forkchoice update method. This ensures that we don't have inconsistent state views between the EL and the CL. This simplifies caller methods and improves code reusability.
  • The FCU methods take as an input a EngineSyncStateUpdate struct that specifies how to update the engine sync state. This is basically a EngineSyncState with every field being an option. If one wants to update a field of the EngineState, they'll have to set the relevant EngineSyncStateUpdate field to Some(updated_value), which will ensure that the update will get both sent to a FCU and (if successful) updated inside the EngineState

@theochap theochap force-pushed the theo/sequencer-startup branch 4 times, most recently from bec8f5c to 7a87905 Compare July 10, 2025 17:04
@theochap theochap force-pushed the theo/sequencer-startup branch 2 times, most recently from c4e426c to e7d00b8 Compare July 10, 2025 20:16
@theochap theochap force-pushed the theo/sequencer-startup branch from e7d00b8 to fb9a5c8 Compare July 10, 2025 20:33
@theochap theochap enabled auto-merge July 10, 2025 20:46
@theochap theochap added this pull request to the merge queue Jul 10, 2025
Merged via the queue into main with commit de0c12d Jul 10, 2025
24 of 25 checks passed
@theochap theochap deleted the theo/sequencer-startup branch July 10, 2025 21:31
@github-project-automation github-project-automation bot moved this from In Review to Done in Project Tracking Jul 10, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jul 25, 2025
…nchronize task. move block building logic to build task (#2524)

## Description

Attempt at cleaning-up the engine and restoring the behavior from
pre-refactor https://github.com/op-rs/kona/pull/2388/files.
In particular:
- Split up the forkchoice task into separated build and synchronize
logic. The synchronize logic is now closer to the pre #2388 state
- Restore the build FCU method in the build task. Ensure that the
side-effects match the pre-#2388 state
- Audit the other tasks and ensure the composition (if any), follow the
side-effects from #2388
- Remove the `Forkchoice` task from the `EngineTask`s.
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
…op-rs/kona#2388)

## Description

This PR refactors the core logic of the engine task handler. In
particular, it moves duplicate code from the build and the insert unsafe
tasks to the forkchoice task.

Changes:
- Add an associated `Output` type for the `EngineTaskExt` trait which
allows the implementors of this trait to be composed. In particular, the
`ForkchoiceTask` may returns the payload id that was inserted in the EL,
which can be used by the `BuildTask` to consolidate the payload into the
engine state.
- Allows the `ForkchoiceTask` to directly insert newly built payload
attributes into the EL (by adding an `Option<OpAttributeWithParent>`
argument to the `ForkchoiceTask`). This allows to refactors both calls
to the `FCU` inside the `BuildTask` using the `ForkchoiceTask` and
avoids manually handling the forkchoice updates
- Moves the `PayloadStatus` verification logic to the `ForkchoiceTask`
to avoid having to perform the same logic in both the `BuildTask` and
the `InsertUnsafeTask`, which allows to fix the sequencer kickstart
issue described in op-rs/kona#2372

Close op-rs/kona#2372 

## Potential follow-ups

- Remove the clones inside the engine tasks
- Refactor the `new_payload` logic into an engine task to remove
duplicate code in the `InsertUnsafe` and `Build`Tasks
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
…nchronize task. move block building logic to build task (op-rs/kona#2524)

## Description

Attempt at cleaning-up the engine and restoring the behavior from
pre-refactor https://github.com/op-rs/kona/pull/2388/files.
In particular:
- Split up the forkchoice task into separated build and synchronize
logic. The synchronize logic is now closer to the pre op-rs/kona#2388 state
- Restore the build FCU method in the build task. Ensure that the
side-effects match the pre-#2388 state
- Audit the other tasks and ensure the composition (if any), follow the
side-effects from op-rs/kona#2388
- Remove the `Forkchoice` task from the `EngineTask`s.
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Jan 14, 2026
…op-rs/kona#2388)

## Description

This PR refactors the core logic of the engine task handler. In
particular, it moves duplicate code from the build and the insert unsafe
tasks to the forkchoice task.

Changes:
- Add an associated `Output` type for the `EngineTaskExt` trait which
allows the implementors of this trait to be composed. In particular, the
`ForkchoiceTask` may returns the payload id that was inserted in the EL,
which can be used by the `BuildTask` to consolidate the payload into the
engine state.
- Allows the `ForkchoiceTask` to directly insert newly built payload
attributes into the EL (by adding an `Option<OpAttributeWithParent>`
argument to the `ForkchoiceTask`). This allows to refactors both calls
to the `FCU` inside the `BuildTask` using the `ForkchoiceTask` and
avoids manually handling the forkchoice updates
- Moves the `PayloadStatus` verification logic to the `ForkchoiceTask`
to avoid having to perform the same logic in both the `BuildTask` and
the `InsertUnsafeTask`, which allows to fix the sequencer kickstart
issue described in #2372

Close #2372 

## Potential follow-ups

- Remove the clones inside the engine tasks
- Refactor the `new_payload` logic into an engine task to remove
duplicate code in the `InsertUnsafe` and `Build`Tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-engine Area: engine A-node Area: cl node (eq. Go op-node) handles single-chain consensus K-feature Kind: feature

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

feat(node/service): Sequencer startup routine

2 participants