Conversation
theochap
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the engine task system by renaming and restructuring the forkchoice update logic. The main purpose is to split forkchoice responsibilities into separate synchronization and build tasks, restoring behavior from pre-refactor state while cleaning up the engine architecture.
- Renames
ForkchoiceTasktoSynchronizeTaskand removes it from the mainEngineTaskenum - Moves block building logic from forkchoice task into the
BuildTaskwith a newstart_buildmethod - Updates all task references and error handling to use the new task structure
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/docs/pages/node/design/engine.mdx | Updates documentation to reflect ForkchoiceTask rename to SynchronizeTask |
| crates/node/engine/src/task_queue/tasks/task.rs | Removes ForkchoiceTask from EngineTask enum and associated error handling |
| crates/node/engine/src/task_queue/tasks/synchronize/task.rs | Creates new SynchronizeTask implementation for forkchoice updates without attributes |
| crates/node/engine/src/task_queue/tasks/build/task.rs | Adds start_build method to handle forkchoice updates with payload attributes |
| Multiple error files | Updates error type references from ForkchoiceTaskError to SynchronizeTaskError |
| Multiple task files | Updates task instantiation calls to use SynchronizeTask instead of ForkchoiceTask |
| Err(InsertTaskError::UnexpectedPayloadStatus(e)) | ||
| if self.attributes.is_deposits_only() => | ||
| { | ||
| error!(target: "engine_builder", error = ?e, "Critical: Deposit-only payload import failed"); | ||
| return Err(BuildTaskError::DepositOnlyPayloadFailed) | ||
| } | ||
| // HOLOCENE: Re-attempt payload import with deposits only | ||
| Err(InsertTaskError::ForkchoiceUpdateFailed( | ||
| ForkchoiceTaskError::InvalidPayloadStatus(e), | ||
| )) if self | ||
| .cfg | ||
| .is_holocene_active(self.attributes.inner().payload_attributes.timestamp) => | ||
| Err(InsertTaskError::UnexpectedPayloadStatus(e)) |
There was a problem hiding this comment.
The error pattern matching is inconsistent with the error type being matched. The code is matching InsertTaskError::UnexpectedPayloadStatus(e) but based on the context and other files, this should likely be matching a different error variant that actually exists in the InsertTaskError enum.
| )) if self | ||
| .cfg | ||
| .is_holocene_active(self.attributes.inner().payload_attributes.timestamp) => | ||
| Err(InsertTaskError::UnexpectedPayloadStatus(e)) |
There was a problem hiding this comment.
Same issue as line 276 - the error pattern matching appears to be using a non-existent error variant UnexpectedPayloadStatus on the InsertTaskError enum.
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| if let Err(err) = ForkchoiceTask::new( | ||
| // Retry to synchronize the engine until we succeeds or a critical error occurs. | ||
| while let Err(err) = SynchronizeTask::new( |
There was a problem hiding this comment.
Motivation, the synchronize task can only fail if:
- RPC error (temporary error) -> we should retry to reset
- finalized ahead of unsafe (critical error) -> should never happen if
find_starting_forkchoiceis correct - unexpected payload status (temporary error) -> this should never happen when we don't provide payload attributes. The synchronize task should only return
ValidorEngineSyncingstatus - invalid forkchoice state -> try to reset again
If we don't have a while loop here:
- we will keep adding tasks with the former engine state
- this will cause the engine to reset because the tasks will return
invalid forkchoice state
| /// The engine is syncing. | ||
| #[error("Attempting to update forkchoice state while EL syncing")] | ||
| #[error("The engine is syncing")] | ||
| EngineSyncing, |
There was a problem hiding this comment.
Those variants are the same as before #2388 https://github.com/op-rs/kona/pull/2388/files
| } | ||
| Self::EngineBuildError(EngineBuildError::EngineSyncing) => { | ||
| EngineTaskErrorSeverity::Temporary | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
Note here that:
ForkchoiceUpdateFailed(pre feat(node/engine): refactor engine tasks. fix sequencer startup logic #2388) ->AttributesInsertionFailed. Pre-feat(node/engine): refactor engine tasks. fix sequencer startup logic #2388, the SECOND failing FCU caused aForkchoiceErrorwhich did not appear inside theBuildErrorvariants. See https://github.com/op-rs/kona/pull/2388/files#diff-47bc5e44b8483f9c829c8829f56e6f5b6d9211888f53f7afe36b44c185515ccbL359.UnexpectedPayloadStatussplit betweenInvalidPayloadandUnexpectedPayloadStatus.
| } | ||
| Self::EngineBuildError(EngineBuildError::UnexpectedPayloadStatus(_)) => { | ||
| EngineTaskErrorSeverity::Temporary | ||
| } |
There was a problem hiding this comment.
To check: I think that this should instead be
| } | |
| Self::EngineBuildError(EngineBuildError::UnexpectedPayloadStatus(_)) => { | |
| EngineTaskErrorSeverity::Critical | |
| } |
The forkchoice_updated should never return the Accepted variant
| EngineTaskErrorSeverity::Temporary | ||
| } | ||
| Self::EngineBuildError(EngineBuildError::InvalidPayload(_)) => { | ||
| EngineTaskErrorSeverity::Temporary |
There was a problem hiding this comment.
This also should probably be
| EngineTaskErrorSeverity::Temporary | |
| EngineTaskErrorSeverity::Critical |
- trying to insert an invalid payload should never happen
| Self::NewPayloadFailed(_) => EngineTaskErrorSeverity::Temporary, | ||
| Self::HoloceneInvalidFlush => EngineTaskErrorSeverity::Flush, | ||
| Self::MissingPayloadId => EngineTaskErrorSeverity::Critical, | ||
| Self::UnexpectedPayloadStatus(_) => EngineTaskErrorSeverity::Critical, |
There was a problem hiding this comment.
Some of these variants were either unused or outdated
| update | ||
| .payload_id | ||
| .ok_or(BuildTaskError::EngineBuildError(EngineBuildError::MissingPayloadId)) | ||
| } |
There was a problem hiding this comment.
| )) if self.attributes.is_deposits_only() => { | ||
| Err(InsertTaskError::UnexpectedPayloadStatus(e)) | ||
| if self.attributes.is_deposits_only() => | ||
| { |
There was a problem hiding this comment.
This was a bug in the refactor. The error we should match on should be raised when we first try to call new_payload inside the InsertTask. See https://github.com/op-rs/kona/pull/2388/files#diff-47bc5e44b8483f9c829c8829f56e6f5b6d9211888f53f7afe36b44c185515ccbR169-R220 (for the former build code), and
kona/crates/node/engine/src/task_queue/tasks/insert/task.rs
Lines 112 to 120 in 572de05
InsertTask)
| @@ -134,7 +134,6 @@ impl EngineTaskExt for InsertTask { | |||
| safe_head: self.is_payload_safe.then_some(new_unsafe_ref), | |||
| ..Default::default() | |||
| }, | |||
There was a problem hiding this comment.
| s => { | ||
| // Other codes are not expected. | ||
| Err(SynchronizeTaskError::UnexpectedPayloadStatus(s.clone())) | ||
| } |
There was a problem hiding this comment.
I think in this case we should return a Critical error. The forkchoice_updated method shouldn't ever return the other status enum if we're not providing payloads
There was a problem hiding this comment.
It should only critical if the error is actually invalid. Accepted is fine. We should be very explicit about what is a critical error
There was a problem hiding this comment.
See https://github.com/alloy-rs/alloy/blob/ff758f86622c343457c4fe856a88033819f11d6e/crates/rpc-types-engine/src/payload.rs#L1807-L1830.
I don't think is dramatic if we're Temporary instead of Critical here, although the error is actually invalid in that case because the client should never return Accepted inside the forkchoice_update method.
There was a problem hiding this comment.
Although I am also a bit unsure about returning a critical error here because we might also get affected if there is any change to the downstream api (if alloy ever decide to return Accepted inside forkchoice_update)
| Self::FinalizedAheadOfUnsafe(_, _) => EngineTaskErrorSeverity::Critical, | ||
| Self::UnexpectedPayloadStatus(_) => EngineTaskErrorSeverity::Critical, | ||
| Self::ForkchoiceUpdateFailed(_) => EngineTaskErrorSeverity::Temporary, | ||
| Self::UnexpectedPayloadStatus(_) => EngineTaskErrorSeverity::Temporary, |
There was a problem hiding this comment.
See below for the UnexpectedPayloadStatus that should be Critical
theochap
force-pushed
the
theo/ensure-engine-paths-match
branch
2 times, most recently
from
6877a48 to
d8f2d59
Compare
| #[async_trait] | ||
| impl EngineTaskExt for SynchronizeTask { |
There was a problem hiding this comment.
I'm not sure I like how we're still defining this operation as a task.
To me a task is an isolated unit or work that's enqueued, but this is something that is composed within other tasks.
Maybe these operations should be refactored into methods on the EngineState and EngineClient and just inlined. This is definitely worth thinking more about because it is incredibly confusing for someone to look at the task abstraction and then this synchronize task not being used properly like the other tasks, but rather being composed.
There was a problem hiding this comment.
I agree, I think we can work on that in a follow-up PR. I would do as you said and just add methods on a Synchronize struct and not implement the EngineTaskExt anymore
theochap
force-pushed
the
theo/ensure-engine-paths-match
branch
2 times, most recently
from
78b2a92 to
8cabd01
Compare
theochap
requested review from
dhyaniarun1993,
itschaindev and
sadiq1971
as code owners
…ynchronize task. move block building logic to build task
theochap
force-pushed
the
theo/ensure-engine-paths-match
branch
from
8cabd01 to
eac943c
Compare
…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.
…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 #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.
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:
Forkchoicetask from theEngineTasks.