From 96a5aaf8de9b077cc433adf2efae4b275c564456 Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Mon, 23 Sep 2024 17:21:24 -0700 Subject: [PATCH 1/7] Sequencer: Origin Selector optimistically prefetches the next origin in background --- op-e2e/actions/helpers/l2_sequencer.go | 2 +- op-node/rollup/driver/driver.go | 2 +- op-node/rollup/sequencing/origin_selector.go | 156 +++++-- .../rollup/sequencing/origin_selector_test.go | 439 ++++++++++++++++-- 4 files changed, 537 insertions(+), 62 deletions(-) diff --git a/op-e2e/actions/helpers/l2_sequencer.go b/op-e2e/actions/helpers/l2_sequencer.go index 98becdcc87a4a..aed2534dbbd38 100644 --- a/op-e2e/actions/helpers/l2_sequencer.go +++ b/op-e2e/actions/helpers/l2_sequencer.go @@ -57,7 +57,7 @@ func NewL2Sequencer(t Testing, log log.Logger, l1 derive.L1Fetcher, blobSrc deri attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, eng) seqConfDepthL1 := confdepth.NewConfDepth(seqConfDepth, ver.syncStatus.L1Head, l1) l1OriginSelector := &MockL1OriginSelector{ - actual: sequencing.NewL1OriginSelector(log, cfg, seqConfDepthL1), + actual: sequencing.NewL1OriginSelector(t.Ctx(), log, cfg, seqConfDepthL1), } metr := metrics.NoopMetrics seqStateListener := node.DisabledConfigPersistence{} diff --git a/op-node/rollup/driver/driver.go b/op-node/rollup/driver/driver.go index 81607e612d5a2..ea090b5a78ba9 100644 --- a/op-node/rollup/driver/driver.go +++ b/op-node/rollup/driver/driver.go @@ -245,7 +245,7 @@ func NewDriver( asyncGossiper := async.NewAsyncGossiper(driverCtx, network, log, metrics) attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, l2) sequencerConfDepth := confdepth.NewConfDepth(driverCfg.SequencerConfDepth, statusTracker.L1Head, l1) - findL1Origin := sequencing.NewL1OriginSelector(log, cfg, sequencerConfDepth) + findL1Origin := sequencing.NewL1OriginSelector(driverCtx, log, cfg, sequencerConfDepth) sequencer = sequencing.NewSequencer(driverCtx, log, cfg, attrBuilder, findL1Origin, sequencerStateListener, sequencerConductor, asyncGossiper, metrics) sys.Register("sequencer", sequencer, opts) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index 41bd645054157..cc5228a3b582b 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sync" "time" "github.com/ethereum/go-ethereum" @@ -20,15 +21,23 @@ type L1Blocks interface { } type L1OriginSelector struct { + ctx context.Context log log.Logger cfg *rollup.Config spec *rollup.ChainSpec l1 L1Blocks + + // Internal cache of L1 origins for faster access. + currentOrigin eth.L1BlockRef + nextOrigin eth.L1BlockRef + + mu sync.Mutex } -func NewL1OriginSelector(log log.Logger, cfg *rollup.Config, l1 L1Blocks) *L1OriginSelector { +func NewL1OriginSelector(ctx context.Context, log log.Logger, cfg *rollup.Config, l1 L1Blocks) *L1OriginSelector { return &L1OriginSelector{ + ctx: ctx, log: log, cfg: cfg, spec: rollup.NewChainSpec(cfg), @@ -40,58 +49,137 @@ func NewL1OriginSelector(log log.Logger, cfg *rollup.Config, l1 L1Blocks) *L1Ori // The L1 Origin is either the L2 Head's Origin, or the following L1 block // if the next L2 block's time is greater than or equal to the L2 Head's Origin. func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) { - // Grab a reference to the current L1 origin block. This call is by hash and thus easily cached. - currentOrigin, err := los.l1.L1BlockRefByHash(ctx, l2Head.L1Origin.Hash) + c := make(chan eth.L1BlockRef, 1) + return los.findL1Origin(ctx, l2Head, c) +} + +// findL1Origin determines what the next L1 Origin should be. +// This private method receives a channel to send the next L1 origin block to, +// and may be used in tests to provide deterministic concurrency behavior. +func (los *L1OriginSelector) findL1Origin(ctx context.Context, l2Head eth.L2BlockRef, c chan eth.L1BlockRef) (eth.L1BlockRef, error) { + currentOrigin, nextOrigin, err := los.CurrentAndNextOrigin(ctx, l2Head) if err != nil { + close(c) return eth.L1BlockRef{}, err } + los.tryFetchNextOrigin(currentOrigin, nextOrigin, c) + + // If the next L2 block time is greater than the next origin block's time, we can choose to + // start building on top of the next origin. Sequencer implementation has some leeway here and + // could decide to continue to build on top of the previous origin until the Sequencer runs out + // of slack. For simplicity, we implement our Sequencer to always start building on the latest + // L1 block when we can. + if nextOrigin != (eth.L1BlockRef{}) && l2Head.Time+los.cfg.BlockTime >= nextOrigin.Time { + return nextOrigin, nil + } + msd := los.spec.MaxSequencerDrift(currentOrigin.Time) log := los.log.New("current", currentOrigin, "current_time", currentOrigin.Time, "l2_head", l2Head, "l2_head_time", l2Head.Time, "max_seq_drift", msd) - seqDrift := l2Head.Time + los.cfg.BlockTime - currentOrigin.Time + pastSeqDrift := l2Head.Time+los.cfg.BlockTime-currentOrigin.Time > msd - // If we are past the sequencer depth, we may want to advance the origin, but need to still - // check the time of the next origin. - pastSeqDrift := seqDrift > msd - if pastSeqDrift { - log.Warn("Next L2 block time is past the sequencer drift + current origin time") - seqDrift = msd + // If we are not past the max sequencer drift, we can just return the current origin. + // Alternatively, if the next origin is ahead of the L2 head, we must return the current origin. + if !pastSeqDrift || (nextOrigin != (eth.L1BlockRef{}) && l2Head.Time+los.cfg.BlockTime < nextOrigin.Time) { + return currentOrigin, nil } - // Calculate the maximum time we can spend attempting to fetch the next L1 origin block. - // Time spent fetching this information is time not spent building the next L2 block, so - // we generally prioritize keeping this value small, allowing for a nonzero failure rate. - // As the next L2 block time approaches the max sequencer drift, increase our tolerance for - // slower L1 fetches in order to avoid falling too far behind. - fetchTimeout := time.Second + (9*time.Second*time.Duration(seqDrift))/time.Duration(msd) - fetchCtx, cancel := context.WithTimeout(ctx, fetchTimeout) + // Otherwise, we need to find the next L1 origin block in order to continue producing blocks. + log.Warn("Next L2 block time is past the sequencer drift + current origin time") + + nextOrigin, ok := <-c + if !ok { + return eth.L1BlockRef{}, fmt.Errorf("cannot build next L2 block past current L1 origin %s by more than sequencer time drift, and failed to find next L1 origin", currentOrigin) + } + + // Once again check if the next origin is ahead of the L2 head, and return the current origin if it is. + if l2Head.Time+los.cfg.BlockTime < nextOrigin.Time { + return currentOrigin, nil + } + + return nextOrigin, nil +} + +func (los *L1OriginSelector) CurrentAndNextOrigin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, eth.L1BlockRef, error) { + los.mu.Lock() + defer los.mu.Unlock() + + if l2Head.L1Origin == los.currentOrigin.ID() { + // Most likely outcome: the L2 head is still on the current origin. + } else if l2Head.L1Origin == los.nextOrigin.ID() { + // If the L2 head has progressed to the next origin, update the current and next origins. + los.currentOrigin = los.nextOrigin + los.nextOrigin = eth.L1BlockRef{} + } else { + // If for some reason the L2 head is not on the current or next origin, we need to find the + // current origin block and reset the next origin. + // This is most likely to occur on the first block after a restart. + + // Grab a reference to the current L1 origin block. This call is by hash and thus easily cached. + currentOrigin, err := los.l1.L1BlockRefByHash(ctx, l2Head.L1Origin.Hash) + if err != nil { + return eth.L1BlockRef{}, eth.L1BlockRef{}, err + } + + los.currentOrigin = currentOrigin + los.nextOrigin = eth.L1BlockRef{} + } + + return los.currentOrigin, los.nextOrigin, nil +} + +func (los *L1OriginSelector) maybeSetNextOrigin(nextOrigin eth.L1BlockRef) { + los.mu.Lock() + defer los.mu.Unlock() + + // Set the next origin if it is the immediate child of the current origin. + if los.currentOrigin.Number+1 == nextOrigin.Number { + los.nextOrigin = nextOrigin + } +} + +// tryFetchNextOrigin schedules a fetch for the next L1 origin block if it is not already set. +// This method always closes the channel, even if the next origin is already set. +func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1BlockRef, c chan<- eth.L1BlockRef) { + // If the next origin is already set, we don't need to do anything. + if nextOrigin != (eth.L1BlockRef{}) { + close(c) + return + } + + // If the current origin is not set, we can't schedule the next origin check. + if currentOrigin == (eth.L1BlockRef{}) { + close(c) + return + } + + go func() { + los.fetch(currentOrigin.Number+1, c) + }() +} + +func (los *L1OriginSelector) fetch(number uint64, c chan<- eth.L1BlockRef) { + defer close(c) + // Attempt to find the next L1 origin block, where the next origin is the immediate child of + // the current origin block. + // The L1 source can be shimmed to hide new L1 blocks and enforce a sequencer confirmation distance. + fetchCtx, cancel := context.WithTimeout(los.ctx, 10*time.Second) defer cancel() // Attempt to find the next L1 origin block, where the next origin is the immediate child of // the current origin block. // The L1 source can be shimmed to hide new L1 blocks and enforce a sequencer confirmation distance. - nextOrigin, err := los.l1.L1BlockRefByNumber(fetchCtx, currentOrigin.Number+1) + nextOrigin, err := los.l1.L1BlockRefByNumber(fetchCtx, number) if err != nil { - if pastSeqDrift { - return eth.L1BlockRef{}, fmt.Errorf("cannot build next L2 block past current L1 origin %s by more than sequencer time drift, and failed to find next L1 origin: %w", currentOrigin, err) - } if errors.Is(err, ethereum.NotFound) { - log.Debug("No next L1 block found, repeating current origin") + log.Debug("No next potential L1 origin found") } else { - log.Error("Failed to get next origin. Falling back to current origin", "err", err) + log.Error("Failed to get next L1 origin", "err", err) } - return currentOrigin, nil - } - - // If the next L2 block time is greater than the next origin block's time, we can choose to - // start building on top of the next origin. Sequencer implementation has some leeway here and - // could decide to continue to build on top of the previous origin until the Sequencer runs out - // of slack. For simplicity, we implement our Sequencer to always start building on the latest - // L1 block when we can. - if l2Head.Time+los.cfg.BlockTime >= nextOrigin.Time { - return nextOrigin, nil + return } - return currentOrigin, nil + los.maybeSetNextOrigin(nextOrigin) + c <- nextOrigin } diff --git a/op-node/rollup/sequencing/origin_selector_test.go b/op-node/rollup/sequencing/origin_selector_test.go index 44461eac3077c..e008a5f6a17c3 100644 --- a/op-node/rollup/sequencing/origin_selector_test.go +++ b/op-node/rollup/sequencing/origin_selector_test.go @@ -2,6 +2,7 @@ package sequencing import ( "context" + "errors" "testing" "github.com/ethereum-optimism/optimism/op-node/rollup" @@ -9,12 +10,58 @@ import ( "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum-optimism/optimism/op-service/testlog" "github.com/ethereum-optimism/optimism/op-service/testutils" - "github.com/ethereum/go-ethereum" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/log" "github.com/stretchr/testify/require" ) +// TestOriginSelectorFetchCurrentError ensures that the origin selector +// returns an error when it cannot fetch the current origin and has no +// internal cached state. +func TestOriginSelectorFetchCurrentError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 500, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 25, + ParentHash: a.Hash, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 24, + } + + l1.ExpectL1BlockRefByHash(a.Hash, eth.L1BlockRef{}, errors.New("test error")) + + s := NewL1OriginSelector(ctx, log, cfg, l1) + + _, err := s.FindL1Origin(ctx, l2Head) + require.ErrorContains(t, err, "test error") + + // The same outcome occurs when the cached origin is different from that of the L2 head. + l1.ExpectL1BlockRefByHash(a.Hash, eth.L1BlockRef{}, errors.New("test error")) + + s = NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = b + + _, err = s.FindL1Origin(ctx, l2Head) + require.ErrorContains(t, err, "test error") +} + // TestOriginSelectorAdvances ensures that the origin selector // advances the origin // @@ -23,6 +70,9 @@ import ( // is no conf depth to stop the origin selection so block `b` should // be the next L1 origin func TestOriginSelectorAdvances(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 500, @@ -46,13 +96,141 @@ func TestOriginSelectorAdvances(t *testing.T) { Time: 24, } - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + s.nextOrigin = b + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) + require.Nil(t, err) + require.Equal(t, b, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should not be triggered because the next origin is already known. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have not run") + } +} + +// TestOriginSelectorNextOrigin ensures that the origin selector +// handles the case where the L2 Head is based on the internal next origin. +// +// There are 2 L1 blocks at time 20 & 25. The L2 Head is at time 24. +// The next L2 time is 26 which is after the next L1 block time. There +// is no conf depth to stop the origin selection so block `b` should +// be the next L1 origin +func TestOriginSelectorAdvancesFromCache(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 500, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 25, + ParentHash: a.Hash, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 24, + } + + // This is called as part of the background prefetch job l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), l2Head) + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.nextOrigin = a + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) + require.Nil(t, err) + require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should be triggered because the next origin is not already known. + next, ok := <-c + require.True(t, ok) + require.Equal(t, b, next) +} + +// TestOriginSelectorPrefetchesNextOrigin ensures that the origin selector +// prefetches the next origin when it can. +// +// The next L2 time is 26 which is after the next L1 block time. There +// is no conf depth to stop the origin selection so block `b` will +// be the next L1 origin as soon as it is fetched. +func TestOriginSelectorPrefetchesNextOrigin(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 500, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 25, + ParentHash: a.Hash, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 24, + } + + // This is called as part of the background prefetch job + l1.ExpectL1BlockRefByNumber(b.Number, b, nil) + + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) + require.Nil(t, err) + require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should be triggered because the next origin is not already known. + next, ok := <-c + require.True(t, ok) + require.Equal(t, b, next) + + // The next origin should be `b` now. + c = make(chan eth.L1BlockRef, 1) + next, err = s.findL1Origin(ctx, l2Head, c) require.Nil(t, err) require.Equal(t, b, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should not be triggered because the next origin is already known. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have not run") + } } // TestOriginSelectorRespectsOriginTiming ensures that the origin selector @@ -64,6 +242,9 @@ func TestOriginSelectorAdvances(t *testing.T) { // but it should select block `a` because the L2 block time must be ahead // of the the timestamp of it's L1 origin. func TestOriginSelectorRespectsOriginTiming(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 500, @@ -87,13 +268,79 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) { Time: 22, } - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) - l1.ExpectL1BlockRefByNumber(b.Number, b, nil) + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + s.nextOrigin = b - s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), l2Head) + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) require.Nil(t, err) require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should not be triggered because the next origin is already known. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have not run") + } +} + +// TestOriginSelectorRespectsSeqDrift +// +// There are 2 L1 blocks at time 20 & 25. The L2 Head is at time 27. +// The next L2 time is 29. The sequencer drift is 8 so the L2 head is +// valid with origin `a`, but the next L2 block is not valid with origin `b.` +// This is because 29 (next L2 time) > 20 (origin) + 8 (seq drift) => invalid block. +// The origin selector does not yet know about block `b` so it should wait for the +// background fetch to complete synchronously. +func TestOriginSelectorRespectsSeqDrift(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 8, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 25, + ParentHash: a.Hash, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 27, + } + + l1.ExpectL1BlockRefByHash(a.Hash, a, nil) + + l1.ExpectL1BlockRefByNumber(b.Number, b, nil) + + s := NewL1OriginSelector(ctx, log, cfg, l1) + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) + require.NoError(t, err) + require.Equal(t, b, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } } // TestOriginSelectorRespectsConfDepth ensures that the origin selector @@ -104,6 +351,9 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) { // as the origin, however block `b` is the L1 Head & the sequencer // needs to wait until that block is confirmed enough before advancing. func TestOriginSelectorRespectsConfDepth(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 500, @@ -127,13 +377,20 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) { Time: 27, } - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) + // l1.ExpectL1BlockRefByHash(a.Hash, a, nil) confDepthL1 := confdepth.NewConfDepth(10, func() eth.L1BlockRef { return b }, l1) - s := NewL1OriginSelector(log, cfg, confDepthL1) + s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) + s.currentOrigin = a - next, err := s.FindL1Origin(context.Background(), l2Head) + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) require.Nil(t, err) require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should not return a new origin because the conf depth has not been met. + _, ok := <-c + require.False(t, ok) } // TestOriginSelectorStrictConfDepth ensures that the origin selector will maintain the sequencer conf depth, @@ -147,6 +404,9 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) { // This is because 29 (next L2 time) > 20 (origin) + 8 (seq drift) => invalid block. // We maintain confirmation distance, even though we would shift to the next origin if we could. func TestOriginSelectorStrictConfDepth(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 8, @@ -172,10 +432,20 @@ func TestOriginSelectorStrictConfDepth(t *testing.T) { l1.ExpectL1BlockRefByHash(a.Hash, a, nil) confDepthL1 := confdepth.NewConfDepth(10, func() eth.L1BlockRef { return b }, l1) - s := NewL1OriginSelector(log, cfg, confDepthL1) + s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) - _, err := s.FindL1Origin(context.Background(), l2Head) + c := make(chan eth.L1BlockRef, 1) + _, err := s.findL1Origin(ctx, l2Head, c) require.ErrorContains(t, err, "sequencer time drift") + + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } } func u64ptr(n uint64) *uint64 { @@ -187,6 +457,9 @@ func u64ptr(n uint64) *uint64 { // This time the same L1 origin is returned if no new L1 head is seen, instead of an error, // because the Fjord max sequencer drift is higher. func TestOriginSelector_FjordSeqDrift(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 8, @@ -200,18 +473,32 @@ func TestOriginSelector_FjordSeqDrift(t *testing.T) { Number: 10, Time: 20, } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 22, + } l2Head := eth.L2BlockRef{ L1Origin: a.ID(), Time: 27, // next L2 block time would be past pre-Fjord seq drift } - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) - l1.ExpectL1BlockRefByNumber(a.Number+1, eth.L1BlockRef{}, ethereum.NotFound) - s := NewL1OriginSelector(log, cfg, l1) + // This is called as part of the background prefetch job + l1.ExpectL1BlockRefByNumber(a.Number+1, b, nil) + + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a - l1O, err := s.FindL1Origin(context.Background(), l2Head) + c := make(chan eth.L1BlockRef, 1) + l1O, err := s.findL1Origin(ctx, l2Head, c) require.NoError(t, err, "with Fjord activated, have increased max seq drift") require.Equal(t, a, l1O) + + // Wait for the origin selector's background fetch to finish. + // This fetch should be triggered because the next origin is not already known. + next, ok := <-c + require.True(t, ok) + require.Equal(t, b, next) } // TestOriginSelectorSeqDriftRespectsNextOriginTime @@ -221,6 +508,63 @@ func TestOriginSelector_FjordSeqDrift(t *testing.T) { // drift, the origin should remain on block `a` because the next origin's // time is greater than the next L2 time. func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 8, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + Time: 100, + ParentHash: a.Hash, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 27, + } + + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + s.nextOrigin = b + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) + require.Nil(t, err) + require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should not be triggered because the next origin is already known. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have not run") + } +} + +// TestOriginSelectorSeqDriftRespectsNextOriginTimeNoCache +// +// There are 2 L1 blocks at time 20 & 100. The L2 Head is at time 27. +// The next L2 time is 29. Even though the next L2 time is past the seq +// drift, the origin should remain on block `a` because the next origin's +// time is greater than the next L2 time. +// The L1OriginSelector does not have the next origin cached, and must fetch it +// because the max sequencer drift has been exceeded. +func TestOriginSelectorSeqDriftRespectsNextOriginTimeNoCache(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 8, @@ -244,13 +588,24 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) { Time: 27, } - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), l2Head) + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + + c := make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, c) require.Nil(t, err) require.Equal(t, a, next) + + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-c: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } } // TestOriginSelectorHandlesLateL1Blocks tests the forced repeat of the previous origin, @@ -263,6 +618,9 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) { // Due to a conf depth of 2, block `b` is not immediately visible, // and the origin selection should fail until it is visible, by waiting for block `c`. func TestOriginSelectorHandlesLateL1Blocks(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + log := testlog.Logger(t, log.LevelCrit) cfg := &rollup.Config{ MaxSequencerDrift: 8, @@ -300,23 +658,52 @@ func TestOriginSelectorHandlesLateL1Blocks(t *testing.T) { // l2 head does not change, so we start at the same origin again and again until we meet the conf depth l1.ExpectL1BlockRefByHash(a.Hash, a, nil) - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) - l1.ExpectL1BlockRefByHash(a.Hash, a, nil) + l1.ExpectL1BlockRefByNumber(b.Number, b, nil) l1Head := b confDepthL1 := confdepth.NewConfDepth(2, func() eth.L1BlockRef { return l1Head }, l1) - s := NewL1OriginSelector(log, cfg, confDepthL1) + s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) - _, err := s.FindL1Origin(context.Background(), l2Head) + ch := make(chan eth.L1BlockRef, 1) + _, err := s.findL1Origin(ctx, l2Head, ch) require.ErrorContains(t, err, "sequencer time drift") + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-ch: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } + l1Head = c - _, err = s.FindL1Origin(context.Background(), l2Head) + ch = make(chan eth.L1BlockRef, 1) + _, err = s.findL1Origin(ctx, l2Head, ch) require.ErrorContains(t, err, "sequencer time drift") + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-ch: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } + l1Head = d - next, err := s.FindL1Origin(context.Background(), l2Head) + ch = make(chan eth.L1BlockRef, 1) + next, err := s.findL1Origin(ctx, l2Head, ch) require.Nil(t, err) require.Equal(t, a, next, "must stay on a because the L1 time may not be higher than the L2 time") + + // Wait for the origin selector's background fetch to finish. + // This fetch should already be completed because findL1Origin would have waited for it. + select { + case _, ok := <-ch: + require.False(t, ok) + default: + t.Fatal("expected the background fetch to have already completed") + } } From 52e9def6186b5ffe662145494c26be2a80ce8c7e Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Tue, 24 Sep 2024 15:38:40 -0700 Subject: [PATCH 2/7] L1OriginSelector erases cached state on reset --- op-e2e/actions/helpers/l2_sequencer.go | 4 +++- op-node/rollup/driver/driver.go | 1 + op-node/rollup/sequencing/origin_selector.go | 24 ++++++++++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/op-e2e/actions/helpers/l2_sequencer.go b/op-e2e/actions/helpers/l2_sequencer.go index aed2534dbbd38..424e12b23fda0 100644 --- a/op-e2e/actions/helpers/l2_sequencer.go +++ b/op-e2e/actions/helpers/l2_sequencer.go @@ -56,8 +56,9 @@ func NewL2Sequencer(t Testing, log log.Logger, l1 derive.L1Fetcher, blobSrc deri ver := NewL2Verifier(t, log, l1, blobSrc, altDASrc, eng, cfg, &sync.Config{}, safedb.Disabled, interopBackend) attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, eng) seqConfDepthL1 := confdepth.NewConfDepth(seqConfDepth, ver.syncStatus.L1Head, l1) + originSelector := sequencing.NewL1OriginSelector(t.Ctx(), log, cfg, seqConfDepthL1) l1OriginSelector := &MockL1OriginSelector{ - actual: sequencing.NewL1OriginSelector(t.Ctx(), log, cfg, seqConfDepthL1), + actual: originSelector, } metr := metrics.NoopMetrics seqStateListener := node.DisabledConfigPersistence{} @@ -78,6 +79,7 @@ func NewL2Sequencer(t Testing, log log.Logger, l1 derive.L1Fetcher, blobSrc deri }, } ver.eventSys.Register("sequencer", seq, opts) + ver.eventSys.Register("origin-selector", originSelector, opts) require.NoError(t, seq.Init(t.Ctx(), true)) return &L2Sequencer{ L2Verifier: ver, diff --git a/op-node/rollup/driver/driver.go b/op-node/rollup/driver/driver.go index ea090b5a78ba9..1fd751846cf3e 100644 --- a/op-node/rollup/driver/driver.go +++ b/op-node/rollup/driver/driver.go @@ -246,6 +246,7 @@ func NewDriver( attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, l2) sequencerConfDepth := confdepth.NewConfDepth(driverCfg.SequencerConfDepth, statusTracker.L1Head, l1) findL1Origin := sequencing.NewL1OriginSelector(driverCtx, log, cfg, sequencerConfDepth) + sys.Register("origin-selector", findL1Origin, opts) sequencer = sequencing.NewSequencer(driverCtx, log, cfg, attrBuilder, findL1Origin, sequencerStateListener, sequencerConductor, asyncGossiper, metrics) sys.Register("sequencer", sequencer, opts) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index cc5228a3b582b..e1b5f428e38ed 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -12,6 +12,7 @@ import ( "github.com/ethereum-optimism/optimism/op-node/rollup" "github.com/ethereum-optimism/optimism/op-node/rollup/derive" + "github.com/ethereum-optimism/optimism/op-node/rollup/event" "github.com/ethereum-optimism/optimism/op-service/eth" ) @@ -45,6 +46,16 @@ func NewL1OriginSelector(ctx context.Context, log log.Logger, cfg *rollup.Config } } +func (los *L1OriginSelector) OnEvent(ev event.Event) bool { + switch ev.(type) { + case rollup.ResetEvent: + los.reset() + default: + return false + } + return true +} + // FindL1Origin determines what the next L1 Origin should be. // The L1 Origin is either the L2 Head's Origin, or the following L1 block // if the next L2 block's time is greater than or equal to the L2 Head's Origin. @@ -86,7 +97,7 @@ func (los *L1OriginSelector) findL1Origin(ctx context.Context, l2Head eth.L2Bloc } // Otherwise, we need to find the next L1 origin block in order to continue producing blocks. - log.Warn("Next L2 block time is past the sequencer drift + current origin time") + log.Warn("Next L2 block time is past the sequencer drift + current origin time, attempting to wait for fetch of next L1 origin") nextOrigin, ok := <-c if !ok { @@ -134,7 +145,7 @@ func (los *L1OriginSelector) maybeSetNextOrigin(nextOrigin eth.L1BlockRef) { defer los.mu.Unlock() // Set the next origin if it is the immediate child of the current origin. - if los.currentOrigin.Number+1 == nextOrigin.Number { + if nextOrigin.ParentHash == los.currentOrigin.Hash { los.nextOrigin = nextOrigin } } @@ -181,5 +192,14 @@ func (los *L1OriginSelector) fetch(number uint64, c chan<- eth.L1BlockRef) { } los.maybeSetNextOrigin(nextOrigin) + c <- nextOrigin } + +func (los *L1OriginSelector) reset() { + los.mu.Lock() + defer los.mu.Unlock() + + los.currentOrigin = eth.L1BlockRef{} + los.nextOrigin = eth.L1BlockRef{} +} From 6873d9d19f132111d60ec740bf81c983907e5a08 Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Tue, 24 Sep 2024 17:09:02 -0700 Subject: [PATCH 3/7] L1OriginSelector attempts to fetch on ForkchoiceUpdateEvent --- op-node/rollup/sequencing/origin_selector.go | 21 ++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index e1b5f428e38ed..67a489884fea9 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -12,6 +12,7 @@ import ( "github.com/ethereum-optimism/optimism/op-node/rollup" "github.com/ethereum-optimism/optimism/op-node/rollup/derive" + "github.com/ethereum-optimism/optimism/op-node/rollup/engine" "github.com/ethereum-optimism/optimism/op-node/rollup/event" "github.com/ethereum-optimism/optimism/op-service/eth" ) @@ -47,7 +48,9 @@ func NewL1OriginSelector(ctx context.Context, log log.Logger, cfg *rollup.Config } func (los *L1OriginSelector) OnEvent(ev event.Event) bool { - switch ev.(type) { + switch x := ev.(type) { + case engine.ForkchoiceUpdateEvent: + los.onForkchoiceUpdate(x.UnsafeL2Head) case rollup.ResetEvent: los.reset() default: @@ -150,6 +153,18 @@ func (los *L1OriginSelector) maybeSetNextOrigin(nextOrigin eth.L1BlockRef) { } } +func (los *L1OriginSelector) onForkchoiceUpdate(unsafeL2Head eth.L2BlockRef) { + currentOrigin, nextOrigin, err := los.CurrentAndNextOrigin(los.ctx, unsafeL2Head) + if err != nil { + log.Error("Failed to get current and next L1 origin on forkchoice update", "err", err) + return + } + + c := make(chan eth.L1BlockRef, 1) + los.tryFetchNextOrigin(currentOrigin, nextOrigin, c) + <-c +} + // tryFetchNextOrigin schedules a fetch for the next L1 origin block if it is not already set. // This method always closes the channel, even if the next origin is already set. func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1BlockRef, c chan<- eth.L1BlockRef) { @@ -172,9 +187,7 @@ func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1 func (los *L1OriginSelector) fetch(number uint64, c chan<- eth.L1BlockRef) { defer close(c) - // Attempt to find the next L1 origin block, where the next origin is the immediate child of - // the current origin block. - // The L1 source can be shimmed to hide new L1 blocks and enforce a sequencer confirmation distance. + fetchCtx, cancel := context.WithTimeout(los.ctx, 10*time.Second) defer cancel() From a2eab7f954cce39a30dbadbf59af67cdfa8e5e0c Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Wed, 25 Sep 2024 14:00:53 -0700 Subject: [PATCH 4/7] Move to a fully event-driven model, no extra goroutines --- op-node/rollup/sequencing/origin_selector.go | 43 +-- .../rollup/sequencing/origin_selector_test.go | 260 +++++++----------- 2 files changed, 110 insertions(+), 193 deletions(-) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index 67a489884fea9..cf6ae622e8e08 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -63,20 +63,10 @@ func (los *L1OriginSelector) OnEvent(ev event.Event) bool { // The L1 Origin is either the L2 Head's Origin, or the following L1 block // if the next L2 block's time is greater than or equal to the L2 Head's Origin. func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) { - c := make(chan eth.L1BlockRef, 1) - return los.findL1Origin(ctx, l2Head, c) -} - -// findL1Origin determines what the next L1 Origin should be. -// This private method receives a channel to send the next L1 origin block to, -// and may be used in tests to provide deterministic concurrency behavior. -func (los *L1OriginSelector) findL1Origin(ctx context.Context, l2Head eth.L2BlockRef, c chan eth.L1BlockRef) (eth.L1BlockRef, error) { currentOrigin, nextOrigin, err := los.CurrentAndNextOrigin(ctx, l2Head) if err != nil { - close(c) return eth.L1BlockRef{}, err } - los.tryFetchNextOrigin(currentOrigin, nextOrigin, c) // If the next L2 block time is greater than the next origin block's time, we can choose to // start building on top of the next origin. Sequencer implementation has some leeway here and @@ -100,11 +90,14 @@ func (los *L1OriginSelector) findL1Origin(ctx context.Context, l2Head eth.L2Bloc } // Otherwise, we need to find the next L1 origin block in order to continue producing blocks. - log.Warn("Next L2 block time is past the sequencer drift + current origin time, attempting to wait for fetch of next L1 origin") + log.Warn("Next L2 block time is past the sequencer drift + current origin time, attempting to fetch next L1 origin") - nextOrigin, ok := <-c - if !ok { - return eth.L1BlockRef{}, fmt.Errorf("cannot build next L2 block past current L1 origin %s by more than sequencer time drift, and failed to find next L1 origin", currentOrigin) + if nextOrigin == (eth.L1BlockRef{}) { + // If the next origin is not set, we need to fetch it now. + nextOrigin, err = los.fetch(ctx, currentOrigin.Number+1) + if err != nil { + return eth.L1BlockRef{}, fmt.Errorf("cannot build next L2 block past current L1 origin %s by more than sequencer time drift, and failed to find next L1 origin: %w", currentOrigin, err) + } } // Once again check if the next origin is ahead of the L2 head, and return the current origin if it is. @@ -160,35 +153,27 @@ func (los *L1OriginSelector) onForkchoiceUpdate(unsafeL2Head eth.L2BlockRef) { return } - c := make(chan eth.L1BlockRef, 1) - los.tryFetchNextOrigin(currentOrigin, nextOrigin, c) - <-c + los.tryFetchNextOrigin(currentOrigin, nextOrigin) } // tryFetchNextOrigin schedules a fetch for the next L1 origin block if it is not already set. // This method always closes the channel, even if the next origin is already set. -func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1BlockRef, c chan<- eth.L1BlockRef) { +func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1BlockRef) { // If the next origin is already set, we don't need to do anything. if nextOrigin != (eth.L1BlockRef{}) { - close(c) return } // If the current origin is not set, we can't schedule the next origin check. if currentOrigin == (eth.L1BlockRef{}) { - close(c) return } - go func() { - los.fetch(currentOrigin.Number+1, c) - }() + los.fetch(los.ctx, currentOrigin.Number+1) } -func (los *L1OriginSelector) fetch(number uint64, c chan<- eth.L1BlockRef) { - defer close(c) - - fetchCtx, cancel := context.WithTimeout(los.ctx, 10*time.Second) +func (los *L1OriginSelector) fetch(ctx context.Context, number uint64) (eth.L1BlockRef, error) { + fetchCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() // Attempt to find the next L1 origin block, where the next origin is the immediate child of @@ -201,12 +186,12 @@ func (los *L1OriginSelector) fetch(number uint64, c chan<- eth.L1BlockRef) { } else { log.Error("Failed to get next L1 origin", "err", err) } - return + return eth.L1BlockRef{}, err } los.maybeSetNextOrigin(nextOrigin) - c <- nextOrigin + return nextOrigin, nil } func (los *L1OriginSelector) reset() { diff --git a/op-node/rollup/sequencing/origin_selector_test.go b/op-node/rollup/sequencing/origin_selector_test.go index e008a5f6a17c3..004f5e1546bbe 100644 --- a/op-node/rollup/sequencing/origin_selector_test.go +++ b/op-node/rollup/sequencing/origin_selector_test.go @@ -7,6 +7,7 @@ import ( "github.com/ethereum-optimism/optimism/op-node/rollup" "github.com/ethereum-optimism/optimism/op-node/rollup/confdepth" + "github.com/ethereum-optimism/optimism/op-node/rollup/engine" "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum-optimism/optimism/op-service/testlog" "github.com/ethereum-optimism/optimism/op-service/testutils" @@ -63,12 +64,12 @@ func TestOriginSelectorFetchCurrentError(t *testing.T) { } // TestOriginSelectorAdvances ensures that the origin selector -// advances the origin +// advances the origin with the internal cache // -// There are 2 L1 blocks at time 20 & 25. The L2 Head is at time 24. +// There are 3 L1 blocks at times 20, 22, 24. The L2 Head is at time 24. // The next L2 time is 26 which is after the next L1 block time. There // is no conf depth to stop the origin selection so block `b` should -// be the next L1 origin +// be the next L1 origin, and then block `c` is the subsequent L1 origin. func TestOriginSelectorAdvances(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -88,9 +89,15 @@ func TestOriginSelectorAdvances(t *testing.T) { b := eth.L1BlockRef{ Hash: common.Hash{'b'}, Number: 11, - Time: 25, + Time: 22, ParentHash: a.Hash, } + c := eth.L1BlockRef{ + Hash: common.Hash{'c'}, + Number: 12, + Time: 24, + ParentHash: b.Hash, + } l2Head := eth.L2BlockRef{ L1Origin: a.ID(), Time: 24, @@ -100,29 +107,41 @@ func TestOriginSelectorAdvances(t *testing.T) { s.currentOrigin = a s.nextOrigin = b - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + // Trigger the background fetch via a forkchoice update. + // This should be a no-op because the next origin is already cached. + handled := s.OnEvent(engine.ForkchoiceUpdateEvent{UnsafeL2Head: l2Head}) + require.True(t, handled) + + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, b, next) - // Wait for the origin selector's background fetch to finish. - // This fetch should not be triggered because the next origin is already known. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have not run") + l2Head = eth.L2BlockRef{ + L1Origin: b.ID(), + Time: 26, } + + // The origin is still `b` because the next origin has not been fetched yet. + next, err = s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, b, next) + + l1.ExpectL1BlockRefByNumber(c.Number, c, nil) + + // Trigger the background fetch via a forkchoice update. + // This will actually fetch the next origin because the internal cache is empty. + handled = s.OnEvent(engine.ForkchoiceUpdateEvent{UnsafeL2Head: l2Head}) + require.True(t, handled) + + // The next origin should be `c` now. + next, err = s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, c, next) } -// TestOriginSelectorNextOrigin ensures that the origin selector -// handles the case where the L2 Head is based on the internal next origin. -// -// There are 2 L1 blocks at time 20 & 25. The L2 Head is at time 24. -// The next L2 time is 26 which is after the next L1 block time. There -// is no conf depth to stop the origin selection so block `b` should -// be the next L1 origin -func TestOriginSelectorAdvancesFromCache(t *testing.T) { +// TestOriginSelectorHandlesReset ensures that the origin selector +// resets its internal cached state on derivation pipeline resets. +func TestOriginSelectorHandlesReset(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -149,31 +168,34 @@ func TestOriginSelectorAdvancesFromCache(t *testing.T) { Time: 24, } - // This is called as part of the background prefetch job - l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(ctx, log, cfg, l1) - s.nextOrigin = a + s.currentOrigin = a + s.nextOrigin = b - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) - require.Equal(t, a, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should be triggered because the next origin is not already known. - next, ok := <-c - require.True(t, ok) require.Equal(t, b, next) + + // Trigger the pipeline reset + handled := s.OnEvent(rollup.ResetEvent{}) + require.True(t, handled) + + // The next origin should be `a` now, but we need to fetch it + // because the internal cache was reset. + l1.ExpectL1BlockRefByHash(a.Hash, a, nil) + + next, err = s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, a, next) } -// TestOriginSelectorPrefetchesNextOrigin ensures that the origin selector -// prefetches the next origin when it can. +// TestOriginSelectorFetchesNextOrigin ensures that the origin selector +// fetches the next origin when a fcu is received and the internal cache is empty // // The next L2 time is 26 which is after the next L1 block time. There // is no conf depth to stop the origin selection so block `b` will // be the next L1 origin as soon as it is fetched. -func TestOriginSelectorPrefetchesNextOrigin(t *testing.T) { +func TestOriginSelectorFetchesNextOrigin(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -206,31 +228,23 @@ func TestOriginSelectorPrefetchesNextOrigin(t *testing.T) { s := NewL1OriginSelector(ctx, log, cfg, l1) s.currentOrigin = a - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next) - // Wait for the origin selector's background fetch to finish. - // This fetch should be triggered because the next origin is not already known. - next, ok := <-c - require.True(t, ok) - require.Equal(t, b, next) + // Selection is stable until the next origin is fetched + next, err = s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, a, next) + + // Trigger the background fetch via a forkchoice update + handled := s.OnEvent(engine.ForkchoiceUpdateEvent{UnsafeL2Head: l2Head}) + require.True(t, handled) // The next origin should be `b` now. - c = make(chan eth.L1BlockRef, 1) - next, err = s.findL1Origin(ctx, l2Head, c) + next, err = s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, b, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should not be triggered because the next origin is already known. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have not run") - } } // TestOriginSelectorRespectsOriginTiming ensures that the origin selector @@ -272,19 +286,9 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) { s.currentOrigin = a s.nextOrigin = b - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should not be triggered because the next origin is already known. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have not run") - } } // TestOriginSelectorRespectsSeqDrift @@ -328,19 +332,9 @@ func TestOriginSelectorRespectsSeqDrift(t *testing.T) { s := NewL1OriginSelector(ctx, log, cfg, l1) - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.NoError(t, err) require.Equal(t, b, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") - } } // TestOriginSelectorRespectsConfDepth ensures that the origin selector @@ -382,15 +376,9 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) { s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) s.currentOrigin = a - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should not return a new origin because the conf depth has not been met. - _, ok := <-c - require.False(t, ok) } // TestOriginSelectorStrictConfDepth ensures that the origin selector will maintain the sequencer conf depth, @@ -434,18 +422,8 @@ func TestOriginSelectorStrictConfDepth(t *testing.T) { confDepthL1 := confdepth.NewConfDepth(10, func() eth.L1BlockRef { return b }, l1) s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) - c := make(chan eth.L1BlockRef, 1) - _, err := s.findL1Origin(ctx, l2Head, c) + _, err := s.FindL1Origin(ctx, l2Head) require.ErrorContains(t, err, "sequencer time drift") - - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") - } } func u64ptr(n uint64) *uint64 { @@ -473,32 +451,17 @@ func TestOriginSelector_FjordSeqDrift(t *testing.T) { Number: 10, Time: 20, } - b := eth.L1BlockRef{ - Hash: common.Hash{'b'}, - Number: 11, - Time: 22, - } l2Head := eth.L2BlockRef{ L1Origin: a.ID(), Time: 27, // next L2 block time would be past pre-Fjord seq drift } - // This is called as part of the background prefetch job - l1.ExpectL1BlockRefByNumber(a.Number+1, b, nil) - s := NewL1OriginSelector(ctx, log, cfg, l1) s.currentOrigin = a - c := make(chan eth.L1BlockRef, 1) - l1O, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.NoError(t, err, "with Fjord activated, have increased max seq drift") - require.Equal(t, a, l1O) - - // Wait for the origin selector's background fetch to finish. - // This fetch should be triggered because the next origin is not already known. - next, ok := <-c - require.True(t, ok) - require.Equal(t, b, next) + require.Equal(t, a, next) } // TestOriginSelectorSeqDriftRespectsNextOriginTime @@ -538,19 +501,9 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) { s.currentOrigin = a s.nextOrigin = b - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should not be triggered because the next origin is already known. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have not run") - } } // TestOriginSelectorSeqDriftRespectsNextOriginTimeNoCache @@ -593,19 +546,9 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTimeNoCache(t *testing.T) { s := NewL1OriginSelector(ctx, log, cfg, l1) s.currentOrigin = a - c := make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, c) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next) - - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-c: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") - } } // TestOriginSelectorHandlesLateL1Blocks tests the forced repeat of the previous origin, @@ -665,45 +608,34 @@ func TestOriginSelectorHandlesLateL1Blocks(t *testing.T) { confDepthL1 := confdepth.NewConfDepth(2, func() eth.L1BlockRef { return l1Head }, l1) s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) - ch := make(chan eth.L1BlockRef, 1) - _, err := s.findL1Origin(ctx, l2Head, ch) + _, err := s.FindL1Origin(ctx, l2Head) require.ErrorContains(t, err, "sequencer time drift") - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-ch: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") - } - l1Head = c - ch = make(chan eth.L1BlockRef, 1) - _, err = s.findL1Origin(ctx, l2Head, ch) + _, err = s.FindL1Origin(ctx, l2Head) require.ErrorContains(t, err, "sequencer time drift") - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-ch: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") - } - l1Head = d - ch = make(chan eth.L1BlockRef, 1) - next, err := s.findL1Origin(ctx, l2Head, ch) + next, err := s.FindL1Origin(ctx, l2Head) require.Nil(t, err) require.Equal(t, a, next, "must stay on a because the L1 time may not be higher than the L2 time") +} + +func TestOriginSelectorMiscEvent(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - // Wait for the origin selector's background fetch to finish. - // This fetch should already be completed because findL1Origin would have waited for it. - select { - case _, ok := <-ch: - require.False(t, ok) - default: - t.Fatal("expected the background fetch to have already completed") + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 8, + BlockTime: 2, } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + + s := NewL1OriginSelector(ctx, log, cfg, l1) + + // This event is not handled + handled := s.OnEvent(rollup.L1TemporaryErrorEvent{}) + require.False(t, handled) } From 0f63ecc62697f2503c6a1502d36008f34afb8d19 Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Wed, 25 Sep 2024 14:07:15 -0700 Subject: [PATCH 5/7] Add missing test comment --- op-node/rollup/sequencing/origin_selector_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/op-node/rollup/sequencing/origin_selector_test.go b/op-node/rollup/sequencing/origin_selector_test.go index 004f5e1546bbe..f80e6c23f70e0 100644 --- a/op-node/rollup/sequencing/origin_selector_test.go +++ b/op-node/rollup/sequencing/origin_selector_test.go @@ -371,7 +371,6 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) { Time: 27, } - // l1.ExpectL1BlockRefByHash(a.Hash, a, nil) confDepthL1 := confdepth.NewConfDepth(10, func() eth.L1BlockRef { return b }, l1) s := NewL1OriginSelector(ctx, log, cfg, confDepthL1) s.currentOrigin = a @@ -621,6 +620,8 @@ func TestOriginSelectorHandlesLateL1Blocks(t *testing.T) { require.Equal(t, a, next, "must stay on a because the L1 time may not be higher than the L2 time") } +// TestOriginSelectorMiscEvent ensures that the origin selector ignores miscellaneous events, +// but instead returns false to indicate that the event was not handled. func TestOriginSelectorMiscEvent(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() From c3ea2c70e5c2b153be606df0721d71bb88a2b669 Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Wed, 25 Sep 2024 14:28:29 -0700 Subject: [PATCH 6/7] Minor cleanup, more tests --- op-node/rollup/sequencing/origin_selector.go | 20 +++---- .../rollup/sequencing/origin_selector_test.go | 52 +++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index cf6ae622e8e08..3aa35110c082c 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -84,13 +84,12 @@ func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2Bloc pastSeqDrift := l2Head.Time+los.cfg.BlockTime-currentOrigin.Time > msd // If we are not past the max sequencer drift, we can just return the current origin. - // Alternatively, if the next origin is ahead of the L2 head, we must return the current origin. - if !pastSeqDrift || (nextOrigin != (eth.L1BlockRef{}) && l2Head.Time+los.cfg.BlockTime < nextOrigin.Time) { + if !pastSeqDrift { return currentOrigin, nil } // Otherwise, we need to find the next L1 origin block in order to continue producing blocks. - log.Warn("Next L2 block time is past the sequencer drift + current origin time, attempting to fetch next L1 origin") + log.Warn("Next L2 block time is past the sequencer drift + current origin time") if nextOrigin == (eth.L1BlockRef{}) { // If the next origin is not set, we need to fetch it now. @@ -100,7 +99,7 @@ func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2Bloc } } - // Once again check if the next origin is ahead of the L2 head, and return the current origin if it is. + // If the next origin is ahead of the L2 head, we must return the current origin. if l2Head.Time+los.cfg.BlockTime < nextOrigin.Time { return currentOrigin, nil } @@ -169,7 +168,13 @@ func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1 return } - los.fetch(los.ctx, currentOrigin.Number+1) + if _, err := los.fetch(los.ctx, currentOrigin.Number+1); err != nil { + if errors.Is(err, ethereum.NotFound) { + log.Debug("No next potential L1 origin found") + } else { + log.Error("Failed to get next origin", "err", err) + } + } } func (los *L1OriginSelector) fetch(ctx context.Context, number uint64) (eth.L1BlockRef, error) { @@ -181,11 +186,6 @@ func (los *L1OriginSelector) fetch(ctx context.Context, number uint64) (eth.L1Bl // The L1 source can be shimmed to hide new L1 blocks and enforce a sequencer confirmation distance. nextOrigin, err := los.l1.L1BlockRefByNumber(fetchCtx, number) if err != nil { - if errors.Is(err, ethereum.NotFound) { - log.Debug("No next potential L1 origin found") - } else { - log.Error("Failed to get next L1 origin", "err", err) - } return eth.L1BlockRef{}, err } diff --git a/op-node/rollup/sequencing/origin_selector_test.go b/op-node/rollup/sequencing/origin_selector_test.go index f80e6c23f70e0..7894d4de81322 100644 --- a/op-node/rollup/sequencing/origin_selector_test.go +++ b/op-node/rollup/sequencing/origin_selector_test.go @@ -11,6 +11,7 @@ import ( "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum-optimism/optimism/op-service/testlog" "github.com/ethereum-optimism/optimism/op-service/testutils" + "github.com/ethereum/go-ethereum" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/log" "github.com/stretchr/testify/require" @@ -63,6 +64,57 @@ func TestOriginSelectorFetchCurrentError(t *testing.T) { require.ErrorContains(t, err, "test error") } +// TestOriginSelectorFetchNextError ensures that the origin selector +// gracefully handles an error when fetching the next origin from the +// forkchoice update event. +func TestOriginSelectorFetchNextError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + log := testlog.Logger(t, log.LevelCrit) + cfg := &rollup.Config{ + MaxSequencerDrift: 500, + BlockTime: 2, + } + l1 := &testutils.MockL1Source{} + defer l1.AssertExpectations(t) + a := eth.L1BlockRef{ + Hash: common.Hash{'a'}, + Number: 10, + Time: 20, + } + b := eth.L1BlockRef{ + Hash: common.Hash{'b'}, + Number: 11, + } + l2Head := eth.L2BlockRef{ + L1Origin: a.ID(), + Time: 24, + } + + s := NewL1OriginSelector(ctx, log, cfg, l1) + s.currentOrigin = a + + next, err := s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, a, next) + + l1.ExpectL1BlockRefByNumber(b.Number, eth.L1BlockRef{}, ethereum.NotFound) + + handled := s.OnEvent(engine.ForkchoiceUpdateEvent{UnsafeL2Head: l2Head}) + require.True(t, handled) + + l1.ExpectL1BlockRefByNumber(b.Number, eth.L1BlockRef{}, errors.New("test error")) + + handled = s.OnEvent(engine.ForkchoiceUpdateEvent{UnsafeL2Head: l2Head}) + require.True(t, handled) + + // The next origin should still be `a` because the fetch failed. + next, err = s.FindL1Origin(ctx, l2Head) + require.Nil(t, err) + require.Equal(t, a, next) +} + // TestOriginSelectorAdvances ensures that the origin selector // advances the origin with the internal cache // From bec9b29b8317256fb4cc4a3958702b7323c992cf Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Wed, 25 Sep 2024 14:38:48 -0700 Subject: [PATCH 7/7] Tune the context timeouts --- op-node/rollup/sequencing/origin_selector.go | 23 ++++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/op-node/rollup/sequencing/origin_selector.go b/op-node/rollup/sequencing/origin_selector.go index 3aa35110c082c..b64b45dcfd200 100644 --- a/op-node/rollup/sequencing/origin_selector.go +++ b/op-node/rollup/sequencing/origin_selector.go @@ -92,8 +92,11 @@ func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2Bloc log.Warn("Next L2 block time is past the sequencer drift + current origin time") if nextOrigin == (eth.L1BlockRef{}) { + fetchCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + // If the next origin is not set, we need to fetch it now. - nextOrigin, err = los.fetch(ctx, currentOrigin.Number+1) + nextOrigin, err = los.fetch(fetchCtx, currentOrigin.Number+1) if err != nil { return eth.L1BlockRef{}, fmt.Errorf("cannot build next L2 block past current L1 origin %s by more than sequencer time drift, and failed to find next L1 origin: %w", currentOrigin, err) } @@ -146,18 +149,23 @@ func (los *L1OriginSelector) maybeSetNextOrigin(nextOrigin eth.L1BlockRef) { } func (los *L1OriginSelector) onForkchoiceUpdate(unsafeL2Head eth.L2BlockRef) { - currentOrigin, nextOrigin, err := los.CurrentAndNextOrigin(los.ctx, unsafeL2Head) + // Only allow a relatively small window for fetching the next origin, as this is performed + // on a best-effort basis. + ctx, cancel := context.WithTimeout(los.ctx, 500*time.Millisecond) + defer cancel() + + currentOrigin, nextOrigin, err := los.CurrentAndNextOrigin(ctx, unsafeL2Head) if err != nil { log.Error("Failed to get current and next L1 origin on forkchoice update", "err", err) return } - los.tryFetchNextOrigin(currentOrigin, nextOrigin) + los.tryFetchNextOrigin(ctx, currentOrigin, nextOrigin) } // tryFetchNextOrigin schedules a fetch for the next L1 origin block if it is not already set. // This method always closes the channel, even if the next origin is already set. -func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1BlockRef) { +func (los *L1OriginSelector) tryFetchNextOrigin(ctx context.Context, currentOrigin, nextOrigin eth.L1BlockRef) { // If the next origin is already set, we don't need to do anything. if nextOrigin != (eth.L1BlockRef{}) { return @@ -168,7 +176,7 @@ func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1 return } - if _, err := los.fetch(los.ctx, currentOrigin.Number+1); err != nil { + if _, err := los.fetch(ctx, currentOrigin.Number+1); err != nil { if errors.Is(err, ethereum.NotFound) { log.Debug("No next potential L1 origin found") } else { @@ -178,13 +186,10 @@ func (los *L1OriginSelector) tryFetchNextOrigin(currentOrigin, nextOrigin eth.L1 } func (los *L1OriginSelector) fetch(ctx context.Context, number uint64) (eth.L1BlockRef, error) { - fetchCtx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - // Attempt to find the next L1 origin block, where the next origin is the immediate child of // the current origin block. // The L1 source can be shimmed to hide new L1 blocks and enforce a sequencer confirmation distance. - nextOrigin, err := los.l1.L1BlockRefByNumber(fetchCtx, number) + nextOrigin, err := los.l1.L1BlockRefByNumber(ctx, number) if err != nil { return eth.L1BlockRef{}, err }