Can we make this any clearer to prevent misconfiguration?

Maybe something like this?

Agreed, would even love to add the URL to the design doc if possible

The reason I chose the flag name l2.follow.source is to leave room for a small namespace around how the CL follows an external L2 source, e.g.:

• l2.follow.source
• l2.follow.verify
• l2.follow.poll-interval

This mirrors the existing naming in op-node, which already uses the l2.follow.* prefix, so I was trying to stay consistent with established conventions.

Conceptually, the CL is "following" an external L2 derivation source, which is why follow felt like the right abstraction to me. That said, I understand the concern about misconfiguration and ambiguity, especially since this is an RPC URL and not just an abstract source.

Open to tightening the name if we think the explicitness is worth the verbosity, and I agree that adding the concrete URL to the design doc would help reduce confusion regardless of the final flag name.

Nit: we could remove the option inside DerivationDelegateArgs and instead just wrap the whole thing inside an Option so that it is really optional to use the derivation delegation client:

Can we rely on the optionality of DerivationDelegateConfig instead of DerivationDelegateArgs?

/// L2 derivation delegate connection arguments.
#[derive(Clone, Debug, Default, clap::Args)]
pub struct DerivationDelegateArgs {
    /// The source must be an OP Stack L2 CL RPC exposing optimism_syncStatus.
    #[arg(long, visible_alias = "l2.follow.source", env = "KONA_NODE_L2_FOLLOW_SOURCE")]
    pub l2_follow_source: Option<Url>,
}

impl DerivationDelegateArgs {
    /// Builds the derivation delegate configuration if an L2 CL URL was provided.
    pub fn config(self) -> Option<DerivationDelegateConfig> {
        self.l2_follow_source.map(|url| DerivationDelegateConfig { l2_cl_url: url })
    }
}

I followed the existing pattern for the L2ClientArgs.

We probably said it somewhere, but would it make sense to use websockets instead? Is that on the roadmap as a future improvement? Hardcoded polls are quite brittle in my experience, specially if we want to guarantee a similar level of responsiveness we had before the light CL introduction

Good question. Today, the light CL design consumes delegate derivation info via the HTTP RPC optimism_syncStatus. As far as I know, op-node does not currently expose this over WebSockets, so polling is effectively the only option right now.

That said, I agree that hardcoded polling can be brittle if we want to preserve the same level of responsiveness. There is definitely room to generalize both the connection mechanism and the data source.

I would consider WS / push based updates a reasonable future improvement rather than something we can rely on today.

That said, the external source itself could potentially be generalized to an EL source ( via eth_getBlockByNumber(safe|finalized)).

If we had a separate DelegateDerivationActor struct we could:

• Define an associated type
• Add this associated type as an argument of send_derived_attributes
• For the normal derivation client implementation the associated type can be OpAttributesWithParent, for the delegate client this can be L2BlockInfo.

Associated types: https://doc.rust-lang.org/rust-by-example/generics/assoc_items/types.html

My understanding is that we want to use a unified EngineActorRequest::ProcessSafeL2SignalRequest. This request must handle two different types, l2blockinfo and attributes. So we need a enum or a trait(suggested at #3245 (comment)).

if we use the associated type, we still need to convert or wrap with the trait / enum. afaik we cannot use the traits because of #3245 (comment).

I think it is better to use the union (trait / enum) and combine two safe related method into one like:

 async fn send_safe_l2_signal(&self, signal: ConsolidateInput) -> EngineClientResult<()>;

Attempted to unify by using enums at 442d9f2

Instead of making it an enum, I'd make two structures and have them implement the same trait. The parent struct (that holds the ConsolidateInput) can be generic over the ConsolidateInput and when we do the actor wiring (after CLI parsing), we can decide which type to plug in there.

I attempted this approach. So making the ConsolidateInput as a trait. Currently ConsolidateInput is a field of ConsolidateTask:

pub struct ConsolidateTask<EngineClient_: EngineClient> {
    /// The engine client.
    pub client: Arc<EngineClient_>,
    /// The [`RollupConfig`].
    pub cfg: Arc<RollupConfig>,
    /// The input for consolidation (either attributes or block info).
    pub input: ConsolidateInput,
}

So this struct will receive another trait like

pub struct ConsolidateTask<EngineClient_: EngineClient, ConsolidateInput_: ConsolidateInput> {
   /// The engine client.
   pub client: Arc<EngineClient_>,
   /// The [`RollupConfig`].
   pub cfg: Arc<RollupConfig>,
   /// The input for consolidation (either attributes or block info).
   pub input: ConsolidateInput_,
}

The tasks are consumed at

    pub fn enqueue(&mut self, task: EngineTask<EngineClient_>) {
        self.tasks.push(task);
        self.task_queue_length.send_replace(self.tasks.len());
    }

so if we add an additional trait, my understanding is that all other tasks must be aware of this type. Are there any clear methods to use the additional trait types in this case?

Yeah I suppose that this would entail that EngineTask become generic, ie instead of being an enum, they implement a trait.

Let's leave it that way for now. I will revisit in the future and try to simplify this structure

Acknowledging that @op-will suggested that DerivationDelegateClient and AlloyChainProvider may be swapped to a trait for unit testing. The previous comment is gone because i attempted to create a separate delegation actor.

I think in the future we should try to make this enum go away. This is certainly out of the scope of this PR but this showcases that our current implementation of the RollupNode is not flexible enough. Ideally swapping out one actor for another one should be easier at the rollup node level.

Good to merge the PR as is. Dealing with the rollup node interface is a workstream in itself