feat(node-service): Refactor EngineActor
#1867
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the EngineActor by reorganizing its control loop and splitting responsibilities into dedicated modules, including updating communication channels from mpsc to oneshot types.
- Replace mpsc unbounded channels with oneshot channels for synchronization signals.
- Introduce new InboundEngineMessage types to structure engine event handling.
- Reorganize and re-export related types (e.g., L2Finalizer) across modules.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/node/service/src/service/validator.rs | Updated import to include oneshot alongside mpsc, reflecting a shift in channel usage. |
| crates/node/service/src/lib.rs | Adjusted re-exports to include new message types and finalizer functionality. |
| crates/node/service/src/actors/mod.rs | Re-exported additional items from the engine module including InboundEngineMessage. |
| crates/node/service/src/actors/engine/* | Major reorganization of EngineActor logic: using oneshot for sync signaling, integrating a dedicated finalizer module, and adding enhanced task drainage and error handling. |
| crates/node/service/src/actors/derivation.rs | Transitioned channel types from mpsc to oneshot for engine sync notifications, with corresponding select pattern changes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -13,6 +13,7 @@ use tokio::{ | |||
| select, | |||
| sync::{ | |||
| mpsc::{UnboundedReceiver, UnboundedSender}, | |||
| oneshot::Receiver as OneshotReceiver, | |||
There was a problem hiding this comment.
nit: I would tend to prefer, for tokio channels, to actually specify the entire import path inside for every type of channel we're using. Here it would be tokio::sync::oneshot::Receiver. We can also just import tokio::sync to the file namespace. The advantage of doing that are:
- less code
- we can easily audit whether all our channels are tokio channels (and not std channels). Pretty easy to import the wrong channel type, which may create pretty subtle bugs.
That is more a matter of personal taste, not blocking for this PR. However, it would be amazing if we could:
- find a convention for the codebase for how we do imports
- add it to the contribution guidelines (for external contributors)
There was a problem hiding this comment.
I agree. For now I added named aliases to the channels used in the engine actor. Let's follow up with a standard convention; I prefer shorter qualified paths such as mpsc::{Sender/Receiver}, but glad to discuss w/e is best.
| sync_complete_tx: UnboundedSender<()>, | ||
| /// A way for the engine actor to signal back to the derivation actor | ||
| /// if a block building task produced an `INVALID` response. | ||
| sync_complete_tx: Option<OneshotSender<()>>, |
There was a problem hiding this comment.
Question: I probably asked that already but is there a situation where the EL engine falls out of sync so much (because the CL is down or we haven't received updates from the L1/other L2 nodes for a while) that it has to kickoff EL sync again? In that case, how is it going to be handled here? We just restart a new node process?
There was a problem hiding this comment.
Yes, this can happen. This notification only unblocks derivation for the first time. Let's say that we don't receive any new batches nor unsafe blocks for a while (like 2h) - we'd end up kicking off EL sync again implicitly when the next payload gets inserted upon reconnecting, but we don't necessarily need to block derivation while that's happening because it's already been inititated.
The big reason we have this EL sync state machine within EngineState is mainly to synchronize the initialization routine. We should definitely look for common issues there, though, once things are further along (i.e. just testing turning off the EL for a while, simulating p2p dropping off, etc. etc.)
db198d4 to
f4a3a77
Compare
f4a3a77 to
716beed
Compare
716beed to
0a847d5
Compare
Overview
Refactors the
EngineActora bit, thinning out the control loop and spreading out the module.