[sequencer] Consider delaying L1 message inclusion slightly

[jonastheis]

In #343 we discovered a few issues.

the Docker compose test is configured with Anvil as L1 with a block time of 1s and 4 slots per epoch. this means that blocks are finalized after ~8s. however, during testing I ran into a problem that l2geth didn't know some L1 messages (and banned RN peers). this is due to a hardcoded fixed request interval of L1 messages of 10s in l2geth. so this is only an issue for testing but still needs to be fixed. I'll open a PR to add a flag so that we can configure it in the test to 1 or 2 seconds.

This can not only happen with l2geth but also with other rollup node followers, especially in the case where we relay L1 messages after a few block confirmations instead of L1 finalization. Every time we hit the inclusion threshold there is a chance that some nodes have not yet seen L1 messages for at least the

rollup-node/crates/watcher/src/lib.rs

Line 42 in 6b1a715

pub const SLOW_SYNC_INTERVAL: Duration = Duration::from_secs(2);

With block times of lower hundreds ms this could be dozens of L2 blocks where a "unlucky" follower node can't process the blocks because it simply hasn't queried the L1 yet.

To address this we should add a safety margin of at least SLOW_SYNC_INTERVAL+X where X is some constant, to allow in sync nodes to receive the necessary information from L1.

[Thegaram]

especially in the case where we relay L1 messages after a few block confirmations instead of L1 finalization

What is the difference between these two cases?

I think it's the same, there is a window [t, t + SYNC_INTERVAL] from when L1 messages become available and processable, and multiple L2 blocks can be created within this window. There is always a race between sequencer and follower nodes, though the chance of this being an issue in practice seems inversely proportional to the interval length.

The most robust solution is this:

• We need to differentiate between "this L1 message is unknown" vs "this L1 message does not match my local db", i.e. I have a different message with the same index.
• In the unknown case, we should not reject blocks but defer processing.

[frisitano]

I agree that the SLOW_SYNC_INTERVAL can have implications but the more important factor is the number of L1 blocks we wait before inclusion combine with the L1 block time. Provided we have inclusion_block_depth * l1_block_time >> SLOW_SYNC_INTERVAL and inclusion_block_depth * l1_block_time >> block_propogation_latency then I think we can be fairly confident this case should not be an issue. To take a concrete example if we set the inclusion block depth to 3 combined with Ethereum block time of 12 seconds, this would give nodes 36 seconds to consume and process the L1 messages. This is more than sufficient. Let me know if I'm misunderstanding this.

The most robust solution is this:

• We need to differentiate between "this L1 message is unknown" vs "this L1 message does not match my local db", i.e. I have a different message with the same index.
• In the unknown case, we should not reject blocks but defer processing.

Agreed

[Thegaram]

So there are two concepts:

1. sync_block_depth: The L1 depth at which we index L1 messages and store them in our local DB.
2. inclusion_block_depth: The L1 depth at which the sequencer starts including L1 messages in L2 blocks.

For l2geth these are the same ("finalized"), i.e. we wait for a long time, then when L1 messages become available we index and include them immediately. Based on your description @frisitano, I assume these two are decoupled in RN.

We either need to ensure that follower nodes are slightly ahead of the sequencer, i.e. sequencer-included L1 messages are guaranteed to be known by follower nodes. Or, we need to ensure that "L1 message not synced yet" is a recoverable/retriable block validation error (my suggestion above).

[frisitano]

For l2geth these are the same ("finalized"), i.e. we wait for a long time, then when L1 messages become available we index and include them immediately. Based on your description @frisitano, I assume these two are decoupled in RN.

We don't have the concept of a sync_block_depth in the RN. As soon as messages are received from the L1 watcher we index them. If the L1 was to reorg then we purge the previously indexed messages. We do have the concept of a inclusion_block_depth - the default is finalized to align with l2geth but this can be configured to a specified number of blocks.

We either need to ensure that follower nodes are slightly ahead of the sequencer, i.e. sequencer-included L1 messages are guaranteed to be known by follower nodes. Or, we need to ensure that "L1 message not synced yet" is a recoverable/retriable block validation error (my suggestion above).

I think the former is preferable - we set the inclusion_block_depth to a value for which we have strong confidence that all nodes in the network will have indexed the messages. My concern about the latter approach is that we open up a potential attack vector for griefing - a malicious actor could send blocks with invalid L1 messages, and we would have to wait until the next L1 message is received from the watcher before we can validate the block, ultimately putting block validation latency at the mercy of L1 message activity / frequency.

When specifying the inclusion_block_depth, we will need to balance inclusion latency, network consistency (as we have been discussing), and reorg risk.

[Thegaram]

As soon as messages are received from the L1 watcher we index them

Oh I see. If RN syncs pending L1 messages immediately, then this is not a concern -- at least not once we deprecate l2geth. For now, we still need to consider race conditions between RN sequencer and l2geth follower nodes. The simplest fix would be to delay inclusion, e.g. set inclusion_block_depth to finalized + 1.

My concern about the latter approach is that we open up a potential attack vector for griefing - a malicious actor could send blocks with invalid L1 messages

This malicious actor an only be the sequencer, right? And they could not make follower nodes accept forged messages, they could only stall them for some time. Since there are no clear incentives to do this, and it's just a temporary attack (after which follower nodes reject the chain anyway), I don't think we need to worry about this too much.

[frisitano]

Oh I see. If RN syncs pending L1 messages immediately, then this is not a concern -- at least not once we deprecate l2geth. For now, we still need to consider race conditions between RN sequencer and l2geth follower nodes. The simplest fix would be to delay inclusion, e.g. set inclusion_block_depth to finalized + 1.

Good catch, I hadn't considered the race condition with l2geth. I will open an issue for this and implement this. Are we confident that finalized + 1 is sufficient, as I will set this as the default value?

Another related question is whether follower nodes should validate L1 message inclusion rules, i.e. should they validate the inclusion_block_depth or should they just validate the correctness of messages (currently they only validate correctness). My initial thought is that we shouldn't validate inclusion rules on follower nodes, as if we want to change it, then we would need a means of syncing this config change with follower nodes. We could potentially leverage the scroll-wire protocol to communicate this config, but it seems somewhat impure.

[jonastheis]

whether follower nodes should validate L1 message inclusion rules, i.e. should they validate the inclusion_block_depth or should they just validate the correctness of messages (currently they only validate correctness)

I think correctness is sufficient. Once a L1 message gets reorged it will also reorg the L2 chain so there's no safety concern. And as you mentioned this way we have great flexibility to lower bridging times at any point we feel confident that reorgs are handled correctly.

finalized + 1

As in finalized + 1 block depth? This should give l2geth nodes enough time to sync the messages as the sync interval is 10s. But maybe we can do 2 just to be safe. Few seconds more or less doesn't matter so much when it's already ~15min. Once we deprecate l2geth we can revert it

[Thegaram]

But maybe we can do 2 just to be safe
I will set this as the default value

finalized + 2 sounds good to me, but it's best to make it configurable on the sequencer. Plus, when we migrate from l2geth to reth, it's possible that l2geth already relayed messages that RN would not have, let's make sure this would not lead to any issues (I think it won't).

Another related question is whether follower nodes should validate L1 message inclusion rules

This was considered before. I think this would be useful, especially in a multi-sequencer setting. But it might be impossible without some kind of L2 concensus, because L2 nodes do not have a 100% consistent view of the L1 ledger.

[frisitano]

This was considered before. I think this would be useful, especially in a multi-sequencer setting. But it might be impossible without some kind of L2 concensus, because L2 nodes do not have a 100% consistent view of the L1 ledger.

I think it would be possible to leverage the L1 system contract. We could add configuration parameters that are activated based on some activation rule (similar to the L1 message inclusion rule).