Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

op-node: Stall block creation when block finalization lags behind #216

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions op-node/node/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,21 +221,32 @@ func (n *OpNode) initRuntimeConfig(ctx context.Context, cfg *Config) error {
return eth.L1BlockRef{}, err
}

// Apply confirmation-distance
blNum := l1Head.Number
if blNum >= confDepth {
blNum -= confDepth
}
fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10)
confirmed, 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
if cfg.Driver.SequencerUseFinalized {
fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10)
safeBlock, err = n.l1Source.L1BlockRefByLabel(fetchCtx, eth.Finalized)
fetchCancel()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct? why are you directly calling the kill function? i think the pattern is something like: defer killCtx() or similar

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i see it's a copy-paste from above.. but still weird :P

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
}
Comment on lines +224 to +245

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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

}

fetchCtx, fetchCancel = context.WithTimeout(ctx, time.Second*10)
err = n.runCfg.Load(fetchCtx, confirmed)
err = n.runCfg.Load(fetchCtx, safeBlock)
fetchCancel()
if err != nil {
n.log.Error("failed to fetch runtime config data", "err", err)
Expand Down
6 changes: 6 additions & 0 deletions op-node/rollup/driver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,10 @@ type Config struct {
// SequencerMaxSafeLag is the maximum number of L2 blocks for restricting the distance between L2 safe and unsafe.
// Disabled if 0.
SequencerMaxSafeLag uint64 `json:"sequencer_max_safe_lag"`

// SequencerUseFinalized is true when the sequencer should use only finalized L1 blocks as origin.
// 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"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just a little comment on this. That is probably measured in blocks from the L2

}
15 changes: 11 additions & 4 deletions op-node/rollup/driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,16 @@ func NewDriver(

l1 = NewMeteredL1Fetcher(l1, metrics)
l1State := NewL1State(log, metrics)
sequencerConfDepth := NewConfDepth(driverCfg.SequencerConfDepth, l1State.L1Head, l1)
findL1Origin := NewL1OriginSelector(log, cfg, sequencerConfDepth)
verifConfDepth := NewConfDepth(driverCfg.VerifierConfDepth, l1State.L1Head, l1)
var l1Blocks L1Blocks
var l1Fetcher derive.L1Fetcher
if driverCfg.SequencerUseFinalized {
l1Blocks = NewFinalized(l1State.L1Finalized, l1)
l1Fetcher = NewFinalized(l1State.L1Finalized, l1)
palango marked this conversation as resolved.
Show resolved Hide resolved
} else {
l1Blocks = NewConfDepth(driverCfg.SequencerConfDepth, l1State.L1Head, l1)
l1Fetcher = NewConfDepth(driverCfg.VerifierConfDepth, l1State.L1Head, l1)
}
findL1Origin := NewL1OriginSelector(log, cfg, l1Blocks)
ec := engine.NewEngineController(l2, log, metrics, cfg, syncCfg.SyncMode, synchronousEvents)
engineResetDeriver := engine.NewEngineResetDeriver(driverCtx, log, cfg, l1, l2, syncCfg, synchronousEvents)
clSync := clsync.NewCLSync(log, cfg, metrics, synchronousEvents)
Expand All @@ -191,7 +198,7 @@ func NewDriver(
}

attributesHandler := attributes.NewAttributesHandler(log, cfg, driverCtx, l2, synchronousEvents)
derivationPipeline := derive.NewDerivationPipeline(log, cfg, verifConfDepth, l1Blobs, plasma, l2, metrics)
derivationPipeline := derive.NewDerivationPipeline(log, cfg, l1Fetcher, l1Blobs, plasma, l2, metrics)
pipelineDeriver := derive.NewPipelineDeriver(driverCtx, derivationPipeline, synchronousEvents)
attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, l2)
meteredEngine := NewMeteredEngine(cfg, ec, metrics, log) // Only use the metered engine in the sequencer b/c it records sequencing metrics.
Expand Down
33 changes: 33 additions & 0 deletions op-node/rollup/driver/finalized.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package driver

import (
"context"

"github.com/ethereum/go-ethereum"

"github.com/ethereum-optimism/optimism/op-node/rollup/derive"
"github.com/ethereum-optimism/optimism/op-service/eth"
)

type finalized struct {
derive.L1Fetcher
l1Finalized func() eth.L1BlockRef
}

func NewFinalized(l1Finalized func() eth.L1BlockRef, fetcher derive.L1Fetcher) *finalized {
return &finalized{L1Fetcher: fetcher, l1Finalized: l1Finalized}
}

func (f *finalized) L1BlockRefByNumber(ctx context.Context, num uint64) (eth.L1BlockRef, error) {
// Don't apply the finalized wrapper if l1Finalized is empty (as it is during the startup case before the l1State is initialized).
l1Finalized := f.l1Finalized()
if l1Finalized == (eth.L1BlockRef{}) {
return f.L1Fetcher.L1BlockRefByNumber(ctx, num)
}
if num == 0 || num <= l1Finalized.Number {
return f.L1Fetcher.L1BlockRefByNumber(ctx, num)
}
palango marked this conversation as resolved.
Show resolved Hide resolved
return eth.L1BlockRef{}, ethereum.NotFound
}

var _ derive.L1Fetcher = (*finalized)(nil)
56 changes: 56 additions & 0 deletions op-node/rollup/driver/finalized_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package driver

import (
"context"
"testing"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common"
"github.com/stretchr/testify/require"

"github.com/ethereum-optimism/optimism/op-service/eth"
"github.com/ethereum-optimism/optimism/op-service/testutils"
)

type finalizedTest struct {
name string
final uint64
hash common.Hash // hash of finalized block
req uint64
pass bool
}

func (ft *finalizedTest) Run(t *testing.T) {
l1Fetcher := &testutils.MockL1Source{}
l1Finalized := eth.L1BlockRef{Number: ft.final, Hash: ft.hash}
l1FinalizedGetter := func() eth.L1BlockRef { return l1Finalized }

f := NewFinalized(l1FinalizedGetter, l1Fetcher)

if ft.pass {
// no calls to the l1Fetcher are made if the block number is not finalized yet
l1Fetcher.ExpectL1BlockRefByNumber(ft.req, eth.L1BlockRef{Number: ft.req}, nil)
}

out, err := f.L1BlockRefByNumber(context.Background(), ft.req)
l1Fetcher.AssertExpectations(t)

if ft.pass {
require.NoError(t, err)
require.Equal(t, out, eth.L1BlockRef{Number: ft.req})
} else {
require.Equal(t, ethereum.NotFound, err)
}
}

func TestFinalized(t *testing.T) {
testCases := []finalizedTest{
{name: "finalized", final: 10, hash: exHash, req: 10, pass: true},
{name: "finalized past", final: 10, hash: exHash, req: 8, pass: true},
{name: "not finalized", final: 10, hash: exHash, req: 11, pass: false},
{name: "no L1 state", req: 10, pass: true},
}
for _, tc := range testCases {
t.Run(tc.name, tc.Run)
}
}
30 changes: 30 additions & 0 deletions op-node/rollup/driver/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,36 @@ func (s *Driver) eventLoop() {
s.log.Error("unexpected error from event-draining", "err", err)
}

if s.driverConfig.SequencerEnabled && !s.driverConfig.SequencerStopped &&
s.l1State.L1Head() != (eth.L1BlockRef{}) && s.l1State.L1Finalized() != (eth.L1BlockRef{}) &&
s.Derivation.DerivationReady() {

// If the L1 does fall behind the limit with finalizing blocks, delay creating new blocks
// until the finalized lag is below SequencerMaxFinalizedLag.
numBlocksSinceFinalization := s.l1State.L1Head().Number - s.l1State.L1Finalized().Number
if numBlocksSinceFinalization > s.driverConfig.SequencerMaxFinalizedLag {
if sequencerCh != nil {
s.log.Warn(
"Delay creating new block since finalized lag exceeds limit",
"finalized_l1", s.l1State.L1Finalized(),
"l1_head", s.l1State.L1Head(),
"unsafe_l2", s.Engine.UnsafeL2Head(),
"safe_l2", s.Engine.SafeL2Head(),
)
sequencerCh = nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the same exact scenario that they are handling below this one, to just avoid block creations

}
// respCh := make(chan hashAndError)
// s.stopSequencer <- respCh
// resp := <-respCh
// if resp.err != nil {
// log.Error("Failed to stop sequencer", "err", resp.err)
// } else {
// log.Info("Sequencer has been stopped", "hash", resp.hash)
// }
// } else {
// s.startSequencer
}
}
// If we are sequencing, and the L1 state is ready, update the trigger for the next sequencer action.
// This may adjust at any time based on fork-choice changes or previous errors.
// And avoid sequencing if the derivation pipeline indicates the engine is not ready.
Expand Down
Loading