op-node: Stall block creation when block finalization lags behind #216
Conversation
palango
changed the title
| var safeBlock eth.L1BlockRef | ||
| if cfg.Driver.SequencerUseFinalized { | ||
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | ||
| safeBlock, err = n.l1Source.L1BlockRefByLabel(fetchCtx, eth.Finalized) | ||
| fetchCancel() | ||
| if err != nil { | ||
| n.log.Error("failed to fetch finalized L1 block for runtime config loading", "err", err) | ||
| return eth.L1BlockRef{}, err | ||
| } | ||
| } else { | ||
| // Apply confirmation-distance | ||
| blNum := l1Head.Number | ||
| if blNum >= confDepth { | ||
| blNum -= confDepth | ||
| } | ||
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | ||
| safeBlock, err = n.l1Source.L1BlockRefByNumber(fetchCtx, blNum) | ||
| fetchCancel() | ||
| if err != nil { | ||
| n.log.Error("failed to fetch confirmed L1 block for runtime config loading", "err", err, "number", blNum) | ||
| return eth.L1BlockRef{}, err | ||
| } |
There was a problem hiding this comment.
Don't really know if it's going to make things clearer or even if it going to make the merges harder, but the cancel logic could be common for both
| var safeBlock eth.L1BlockRef | |
| if cfg.Driver.SequencerUseFinalized { | |
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | |
| safeBlock, err = n.l1Source.L1BlockRefByLabel(fetchCtx, eth.Finalized) | |
| fetchCancel() | |
| if err != nil { | |
| n.log.Error("failed to fetch finalized L1 block for runtime config loading", "err", err) | |
| return eth.L1BlockRef{}, err | |
| } | |
| } else { | |
| // Apply confirmation-distance | |
| blNum := l1Head.Number | |
| if blNum >= confDepth { | |
| blNum -= confDepth | |
| } | |
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | |
| safeBlock, err = n.l1Source.L1BlockRefByNumber(fetchCtx, blNum) | |
| fetchCancel() | |
| if err != nil { | |
| n.log.Error("failed to fetch confirmed L1 block for runtime config loading", "err", err, "number", blNum) | |
| return eth.L1BlockRef{}, err | |
| } | |
| var safeBlock eth.L1BlockRef | |
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | |
| if cfg.Driver.SequencerUseFinalized { | |
| safeBlock, err = n.l1Source.L1BlockRefByLabel(fetchCtx, eth.Finalized) | |
| } else { | |
| // Apply confirmation-distance | |
| blNum := l1Head.Number | |
| if blNum >= confDepth { | |
| blNum -= confDepth | |
| } | |
| safeBlock, err = n.l1Source.L1BlockRefByNumber(fetchCtx, blNum) | |
| } | |
| fetchCancel() | |
| if err != nil { | |
| if cfg.Driver.SequencerUseFinalized { | |
| n.log.Error("failed to fetch finalized L1 block for runtime config loading", "err", err) | |
| } else { | |
| n.log.Error("failed to fetch confirmed L1 block for runtime config loading", "err", err, "number", blNum) | |
| } | |
| return eth.L1BlockRef{}, err | |
| } |
Also if we change the log to something understandable by both like would be cleaner. But it's only a nit, and we are not gaining too much either, so it's up to you
| // If this is set to true, the value of `SequencerConfDepth` is ignored. | ||
| SequencerUseFinalized bool `json:"sequencer_use_finalized"` | ||
|
|
||
| SequencerMaxFinalizedLag uint64 `json:"sequencer_max_finalized_lag"` |
There was a problem hiding this comment.
Maybe just a little comment on this. That is probably measured in blocks from the L2
| if cfg.Driver.SequencerUseFinalized { | ||
| fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10) | ||
| safeBlock, err = n.l1Source.L1BlockRefByLabel(fetchCtx, eth.Finalized) | ||
| fetchCancel() |
There was a problem hiding this comment.
is this correct? why are you directly calling the kill function? i think the pattern is something like: defer killCtx() or similar
There was a problem hiding this comment.
ok, i see it's a copy-paste from above.. but still weird :P
| "unsafe_l2", s.Engine.UnsafeL2Head(), | ||
| "safe_l2", s.Engine.SafeL2Head(), | ||
| ) | ||
| sequencerCh = nil |
There was a problem hiding this comment.
not familiar enough to know that this transales to "pause the sequencer"...
things that pop into my head... is this for loop going to be called inmediately after or is it going to be called a after x seconds pass? I mean, was "do nothing/ skip" already an option in this for loop, and so we're just adding another trigger for that action, or is this the first trigger that wants to 'do nothing'?
There was a problem hiding this comment.
I think it's the same exact scenario that they are handling below this one, to just avoid block creations
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
Does this stall the L2 if l2_unsafe - l2_safe > sequencerWindowSize? |
No, it would stall if the number of blocks since the last finalized blocks reaches some threshold. numBlocksSinceFinalization := s.l1State.L1Head().Number - s.l1State.L1Finalized().Number
if numBlocksSinceFinalization > s.driverConfig.SequencerMaxFinalizedLag {
// stall chain |
Cool, probably we want to set this config to something slightly lower to |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
Closing as this is outdated |
Builds on #209