Conversation
228d549 to
ee9bb1e
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds end-to-end support for finalizing L2 blocks once their originating L1 blocks are finalized.
- Extends the derivation pipeline and
OpAttributesWithParentto carry the L1 origin for each L2 block. - Enhances the L1 watcher to poll and emit finalized L1 blocks alongside head updates.
- Updates the engine actor to track the highest derived L2 per L1, enqueue finalization tasks, and integrate
FinalizeTaskinto the task queue.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/protocol/derive/src/pipeline/core.rs | Renamed attributes to inner and added default l1_origin in test helper |
| crates/node/sources/src/sync/mod.rs | Treats finalized blocks as safe in the sync start logic |
| crates/node/service/src/service/validator.rs | Expanded new_da_watcher signature to include new_finalized_tx |
| crates/node/service/src/service/standard/node.rs | Supplied new_finalized_tx when constructing L1WatcherRpc |
| crates/node/service/src/actors/l1_watcher_rpc.rs | Replaced alloy’s head stream with a custom BlockStream and added finalized_sender |
| crates/node/service/src/actors/engine.rs | Introduced awaiting_finalization map, finalization branch, and FinalizeTask enqueue logic |
| crates/node/service/src/actors/derivation.rs | Minor formatting alignment after refactor |
| crates/node/service/Cargo.toml | Added async-stream workspace dependency |
| crates/node/engine/src/task_queue/tasks/task.rs | Added FinalizeTask to EngineTask enum and priority ordering |
| crates/node/engine/src/task_queue/tasks/mod.rs | Exposed FinalizeTask module |
| crates/node/engine/src/task_queue/tasks/finalize/* | New FinalizeTask implementation and error types |
| crates/node/engine/src/task_queue/tasks/build/task.rs | Updated builder to use inner() accessor |
| crates/node/engine/src/task_queue/core.rs | Added state_mut() to expose mutable EngineState |
| crates/node/engine/src/metrics/mod.rs | Added FINALIZE_TASK_LABEL for metrics |
| crates/node/engine/src/lib.rs | Re-exported FinalizeTask and its error |
| crates/node/engine/src/client.rs | Added cfg() accessor for EngineClient |
| crates/node/engine/src/attributes.rs | Refactored attribute matchers to use inner() |
| Cargo.toml | Added top-level async-stream dependency |
Comments suppressed due to low confidence (1)
crates/node/service/src/actors/engine.rs:321
- There are no tests covering the new finalization logic (mapping L1 → L2 and enqueuing
FinalizeTask). Consider adding unit or integration tests to validate that L2 blocks are correctly finalized when their originating L1 block stream signals finalization.
new_finalized_l1 = self.finalized_block_rx.recv() => {
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f7cd8d4 to
1eff747
Compare
📚 PR StackThis comment was auto-generated |
theochap
left a comment
There was a problem hiding this comment.
Looks great! Most of the comments are nits or can be treated as follow ups. Going to approve to not block it. It would be nice to at least have tickets for the points that would deserve some more attention in your opinion!
|
|
||
| // Clear the queue of attributes awaiting finalization. It will be re-saturated following | ||
| // derivation. | ||
| self.awaiting_finalization.clear(); |
There was a problem hiding this comment.
Question: do we really need to reset the engine task queue when the engine resets? Can't we use an optimistic approach and keep the tasks around and they will be naturally rejected if they don't build on the canonical chain anymore?
There was a problem hiding this comment.
This isn't the engine task queue, but just the map of L1 blocks -> the highest L2 block in each present L1 block's epoch. But yeah, we also clear the engine task queue.
It's important to clear this, at least, since an L1 reorg could have caused the L1 block which a payload was derived within to disappear. For the tasks, we might be able to hold onto them, but it's not too important. For example - we'd receive a new unsafe block anyways, and the EL would backfill, and for derived blocks (Consolidate tasks), we'd just re-derive them.
| fn new(l1_provider: &'a RootProvider, tag: BlockNumberOrTag, poll_interval_secs: u64) -> Self { | ||
| if matches!(tag, BlockNumberOrTag::Number(_)) { | ||
| panic!("Invalid BlockNumberOrTag variant - Must be a tag"); | ||
| } |
There was a problem hiding this comment.
Not blocking for this PR, but we could avoid that if we actually had a struct for the allowed block tags and only two variants in the block number or tag enum. We can take care of that in a follow up but panicking here may be a bit risky
There was a problem hiding this comment.
Yeah that'd be nice. We're using alloy-rpc-types' BlockNumberOrTag - maybe let's make an issue there to add a BlockTag enum?
1eff747 to
64f5a65
Compare
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes #1693 closes #1694 closes #1695 closes #1696 closes #1698
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes #1693 closes #1694 closes #1695 closes #1696 closes #1698
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes op-rs/kona#1693 closes op-rs/kona#1694 closes op-rs/kona#1695 closes op-rs/kona#1696 closes op-rs/kona#1698
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes #1693 closes #1694 closes #1695 closes #1696 closes #1698
Overview
Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this:
OpAttributesWithParent.l1_block_number => highest_l2_block_in_epoch, saturated when it receives attributes from the derivation actor.This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors.
Periphery changes
OpAttributesWithParenttype.L1WatcherRpcnow uses a customBlockStream, since alloy'swatch_blocksstream doesn't dedup nor allow for observing anything other than head block updates.Result
Finalized blocks are correctly streaming in 😄
Meta
closes #1693
closes #1694
closes #1695
closes #1696
closes #1698