Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
10f056f to
59214ce
Compare
54fbe8d to
12835bb
Compare
karlfloersch
left a comment
There was a problem hiding this comment.
Didn't have too much but some thoughts
- I didn't think about the fact that we can test kona-node light CL with the same sync tester tests!
- Tests seem good, testing reorgs, p2p enabled and disabled. Tried to but couldn't think of extra cases
- Great that we can reuse the sync tester ext configs
- The reorg detection logic is MUCH cleaner now. Do you feel good about it?
Generally looks pretty dang good! I'll throw an approval even tho I think a second pair of eyes is probably good. But the logic looked pretty solid to me
fe7a184 to
e91771e
Compare
There was a problem hiding this comment.
Testing is very comprehensive and for the most part I like the way the solution looks!
The main thing to solve is our ongoing reliance on an L1 source. Removing the L1 source from the node altogether is a huge payoff for a lite-node, and even though they're not deriving with it, they are still fully reliant on a highly available L1 source.
Rather, they should be able to utilize their L2 source to supply all derivation aspects of the Safe chain, which would fully eliminate the L1 connection.
How easy do you think it would be to eliminate the L1 connection through this feature?
EDIT: I see that this PR introduces a CL based approach which eliminates the L1: #18500
But @pcw109550 what is the reason for staring with EL based following only? Seems like a feature we would not want to use since it still requires L1.
Actually -- is there a reason we don't just say that sync_status is the required API to do safe following? This would enable safe following on basically all CLs by default.
| presets.WithReqRespSyncDisabled(), | ||
| presets.WithNoDiscovery(), | ||
| presets.WithCompatibleTypes(compat.SysGo), | ||
| presets.WithUnsafeOnly(), |
There was a problem hiding this comment.
Does WithUnsafeOnly only modify one of the two Verifiers, then?
There was a problem hiding this comment.
So WithUnsafeOnly modifies every op-node CLs to use unsafe only.
func WithUnsafeOnly() stack.CommonOption {
return stack.MakeCommon(
sysgo.WithGlobalL2CLOption(sysgo.L2CLOptionFn(
func(_ devtest.P, id stack.L2CLNodeID, cfg *sysgo.L2CLConfig) {
cfg.SequencerUnsafeOnly = true
cfg.VerifierUnsafeOnly = true
})))
}This is a global CL option and applies to every CL. However if every CL does not do derivation, there is no safe source. Therefore at least we need a single CL that does actual derivation.
To make this preset, I implemented DefaultSingleChainTwoVerifiersFollowL2System at
optimism/op-devstack/sysgo/system_singlechain_twoverifiers.go
Lines 71 to 73 in 2202d0e
disabling unsafe only to perform derivation. This works because global option (
WithUnsafeOnly) applies first and the node specific option(L2CLVerifierDisableUnsafeOnly()) applies after.
| // Make sure L1 reorged | ||
| sys.L1EL.WaitForBlockNumber(l1BlockBeforeReorg.Number) | ||
| l1BlockAfterReorg := sys.L1EL.BlockRefByNumber(l1BlockBeforeReorg.Number) | ||
| logger.Info("Triggered L1 reorg", "l1", l1BlockAfterReorg) | ||
| require.NotEqual(l1BlockAfterReorg.Hash, l1BlockBeforeReorg.Hash) | ||
|
|
||
| // Need to poll until the L2CL detects L1 Reorg and trigger L2 Reorg | ||
| // What happens: | ||
| // L2CL detects L1 Reorg and reset the pipeline. op-node example logs: "reset: detected L1 reorg" | ||
| // L2ELB detects L2 Reorg and reorgs. op-geth example logs: "Chain reorg detected" | ||
| sys.L2ELB.ReorgTriggered(l2BlockBeforeReorg, 30) |
There was a problem hiding this comment.
Can we verify that L2ELB does not process the reorg on its own somehow?
There was a problem hiding this comment.
The L2ELB never progresses or reorgs independently; it is always driven by the L2CLB via the Engine API. So the L2ELB cannot reorg on its own.
ReorgTriggered only checks that the canonical block at the divergence height has changed (same parent, different hash). This can only happen if the CL has processed the L1 reorg and sent a forkchoice update with the new head to the EL.
| // In this mode, the node does not derive from L1; instead, it uses L1 as a mandatory | ||
| // upstream anchor for its unsafe head, and may optionally import safe/finalized state |
There was a problem hiding this comment.
There is a lot of infrastructure value to not having these lite nodes use L1 connections at all. I need to read more closely to understand, but this comment leads me to think the L1 connection remains required?
I suggest we don't need a mandatory anchor from the L1, and can instead use the Safe Source for the L1 as well.
op-node/node/node.go
Outdated
There was a problem hiding this comment.
Same comment as earlier - we should be able to follow the L1 source through the L2 Follow Source.
There was a problem hiding this comment.
nit: Consider that the naming flipped between the individual and composite interface. I suggest UpstreamFollowSource for the composite.
|
Let me share my thought process of the relation between the L1 source and the light CL feature. So there are two features: As I mentioned at the PR description, L1 Reorg detection is embedded at the derivation pipeline: optimism/op-node/rollup/derive/l1_traversal.go Lines 60 to 71 in 532d1e4 So this means if we turn off derivation ( Case 1: When derivation disabled && follow source disabled.
So my take is that L1 connection remains required for the light CL for Case 1. Case 2: When derivation disabled && follow source enabled.
Food for thought: If we only allow the combination: [derivation disabled && follow source enabled && follow source is the CL endpoint: |
eab29d5 to
f17bedd
Compare
Description
This PR introduces a new upstream sync loop (Driver → Engine Controller) that (1) detects L1 reorgs in unsafe-only mode and resets the node appropriately, and (2) optionally mirrors external L2 safe/finalized state when
--l2.follow.sourceis enabled. It replaces the derivation-pipeline-based reorg logic for unsafe-only nodes and ensures L2 always tracks L1 and external safe/finalized sources correctly.This PR builds on #18290 andimplements two features:Task A – Follow an external L2 source for safe and finalized blocks
Enabled with
--l2.follow.source(requires--l2.unsafe-only).Task B – Trigger an L2 reset when an L1 reorg occurs
Enabled with
--l2.unsafe-only.Note:
--l2.follow.sourcecan only be used when--l2.unsafe-onlyis enabled.A new control-flow path is added:
DriverchangesWe need a periodical ticker that is responsible to perform upper two tasks. Similar to the
altSyncTickerwhich periodically tries to close the unsafe gap, I added theupstreamSyncTickerto do perform tasks. op-node/rollup/driver/driver.go.upstreamSyncTickeris only enabled when--l2.unsafe-onlyis enabled.The ticker calls
followUpstream()method periodically. The method implements Task B and calls Engine Controller to do Task A, by below stepsoptimism/op-node/rollup/derive/pipeline.go
Line 221 in ef97055
CurrentL1view inside the CL. This means we cannot perform the same L1 reorg check.--l2.follow.sourceis enabled thefollowUpstream()fetches external L2 info (esafe,efinalized) and tries to apply to the Engine Controller by callings.SyncDeriver.Engine.FollowSource(eSafe, eFinalized)Engine ControllerChangesFollowSource(eSafe, eFinalized)is implemented at Engine Controller, implementing Task A. Engine Controller has the responsibility to update its internal state based on the injected external state. The Engine Controller performs mirroring by below steps:Tests
Four tests are added for testing the
--l2.follow.source:TestFollowL2_ReorgRecovery: checks follow source seq / ver reorgs when L1 reorgsTestFollowL2_SafeAndFinalized: happy case, external safe and finalized is mirrored when local unsafe head is ahead of external safe and finalizedTestFollowL2_WithoutCLP2P: CLP2P down, so the data source of safe / finalized is only the follow source. Unsafe and safe will advance together.TestSyncTesterFollowL2ReachTips: Using the sync tester, test that the op-node can mirror safe / finalized of op-sepolia.One test was added to test the
--l2.unsafe-only:TestUnsafeOnly_ReorgRecovery: checks unsafe only seq / ver reorgs when L1 reorgsAdditional context
Before this PR, when
--l2.unsafe-onlyis enabled, the node did not L2 reorg when L1 reorg. We now always track the L1 reorg by the newly addedupstreamSyncTickerand do a proper L2 reorg via reset.Metadata