op-node: ensure sequencer selects L1 origin within sequencer time drift

and use conf depth util

op-e2e/actions/l2_sequencer.go

@@ -18,11 +18,11 @@ type MockL1OriginSelector struct {
 	originOverride eth.L1BlockRef // override which origin gets picked
 }
 
-func (m *MockL1OriginSelector) FindL1Origin(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
+func (m *MockL1OriginSelector) FindL1Origin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
 	if m.originOverride != (eth.L1BlockRef{}) {
 		return m.originOverride, nil
 	}
-	return m.actual.FindL1Origin(ctx, l1Head, l2Head)
+	return m.actual.FindL1Origin(ctx, l2Head)
 }
 
 // L2Sequencer is an actor that functions like a rollup node,
@@ -40,8 +40,9 @@ type L2Sequencer struct {
 func NewL2Sequencer(t Testing, log log.Logger, l1 derive.L1Fetcher, eng L2API, cfg *rollup.Config, seqConfDepth uint64) *L2Sequencer {
 	ver := NewL2Verifier(t, log, l1, eng, cfg)
 	attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, eng)
+	seqConfDepthL1 := driver.NewConfDepth(seqConfDepth, ver.l1State.L1Head, l1)
 	l1OriginSelector := &MockL1OriginSelector{
-		actual: driver.NewL1OriginSelector(log, cfg, l1, seqConfDepth),
+		actual: driver.NewL1OriginSelector(log, cfg, seqConfDepthL1),
 	}
 	return &L2Sequencer{
 		L2Verifier:              *ver,
@@ -62,7 +63,7 @@ func (s *L2Sequencer) ActL2StartBlock(t Testing) {
 		return
 	}
 
-	err := s.sequencer.StartBuildingBlock(t.Ctx(), s.l1State.L1Head())
+	err := s.sequencer.StartBuildingBlock(t.Ctx())
 	require.NoError(t, err, "failed to start block building")
 
 	s.l2Building = true
@@ -106,7 +107,7 @@ func (s *L2Sequencer) ActBuildToL1Head(t Testing) {
 func (s *L2Sequencer) ActBuildToL1HeadExcl(t Testing) {
 	for {
 		s.ActL2PipelineFull(t)
-		nextOrigin, err := s.mockL1OriginSelector.FindL1Origin(t.Ctx(), s.l1State.L1Head(), s.derivation.UnsafeL2Head())
+		nextOrigin, err := s.mockL1OriginSelector.FindL1Origin(t.Ctx(), s.derivation.UnsafeL2Head())
 		require.NoError(t, err)
 		if nextOrigin.Number >= s.l1State.L1Head().Number {
 			break

op-node/rollup/driver/driver.go

@@ -66,10 +66,10 @@ type L1StateIface interface {
 }
 
 type SequencerIface interface {
-	StartBuildingBlock(ctx context.Context, l1Head eth.L1BlockRef) error
+	StartBuildingBlock(ctx context.Context) error
 	CompleteBuildingBlock(ctx context.Context) (*eth.ExecutionPayload, error)
 	PlanNextSequencerAction() time.Duration
-	RunNextSequencerAction(ctx context.Context, l1Head eth.L1BlockRef) *eth.ExecutionPayload
+	RunNextSequencerAction(ctx context.Context) *eth.ExecutionPayload
 	BuildingOnto() eth.L2BlockRef
 }
 
@@ -81,7 +81,8 @@ type Network interface {
 // NewDriver composes an events handler that tracks L1 state, triggers L2 derivation, and optionally sequences new L2 blocks.
 func NewDriver(driverCfg *Config, cfg *rollup.Config, l2 L2Chain, l1 L1Chain, network Network, log log.Logger, snapshotLog log.Logger, metrics Metrics) *Driver {
 	l1State := NewL1State(log, metrics)
-	findL1Origin := NewL1OriginSelector(log, cfg, l1, driverCfg.SequencerConfDepth)
+	sequencerConfDepth := NewConfDepth(driverCfg.SequencerConfDepth, l1State.L1Head, l1)
+	findL1Origin := NewL1OriginSelector(log, cfg, sequencerConfDepth)
 	verifConfDepth := NewConfDepth(driverCfg.VerifierConfDepth, l1State.L1Head, l1)
 	derivationPipeline := derive.NewDerivationPipeline(log, cfg, verifConfDepth, l2, metrics)
 	attrBuilder := derive.NewFetchingAttributesBuilder(cfg, l1, l2)

op-node/rollup/driver/origin_selector.go

@@ -2,7 +2,10 @@ package driver
 
 import (
 	"context"
+	"errors"
+	"fmt"
 
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/log"
 
 	"github.com/ethereum-optimism/optimism/op-node/eth"
@@ -20,62 +23,48 @@ type L1OriginSelector struct {
 	cfg *rollup.Config
 
 	l1 L1Blocks
-	sequencingConfDepth uint64
 }
 
-func NewL1OriginSelector(log log.Logger, cfg *rollup.Config, l1 L1Blocks, sequencingConfDepth uint64) *L1OriginSelector {
+func NewL1OriginSelector(log log.Logger, cfg *rollup.Config, l1 L1Blocks) *L1OriginSelector {
 	return &L1OriginSelector{
 		log: log,
 		cfg: cfg,
 		l1:  l1,
-		sequencingConfDepth: sequencingConfDepth,
 	}
 }
 
 // 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.
-func (los *L1OriginSelector) FindL1Origin(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
-	// If we are at the head block, don't do a lookup.
-	if l2Head.L1Origin.Hash == l1Head.Hash {
-		return l1Head, nil
-	}
-
-	// Grab a reference to the current L1 origin block.
+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)
 	if err != nil {
 		return eth.L1BlockRef{}, err
 	}
+	log := los.log.New("current", currentOrigin, "current_time", currentOrigin.Time,
+		"l2_head", l2Head, "l2_head_time", l2Head.Time)
 
 	// 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 := l2Head.Time+los.cfg.BlockTime > currentOrigin.Time+los.cfg.MaxSequencerDrift
 	if pastSeqDrift {
-		log.Info("Next L2 block time is past the sequencer drift + current origin time",
-			"current", currentOrigin, "current_time", currentOrigin.Time,
-			"l1_head", l1Head, "l1_head_time", l1Head.Time,
-			"l2_head", l2Head, "l2_head_time", l2Head.Time,
-			"depth", los.sequencingConfDepth)
-	}
-
-	if !pastSeqDrift && currentOrigin.Number+1+los.sequencingConfDepth > l1Head.Number {
-		// TODO: we can decide to ignore confirmation depth if we would be forced
-		//  to make an empty block (only deposits) by staying on the current origin.
-		log.Info("sequencing with old origin to preserve conf depth",
-			"current", currentOrigin, "current_time", currentOrigin.Time,
-			"l1_head", l1Head, "l1_head_time", l1Head.Time,
-			"l2_head", l2Head, "l2_head_time", l2Head.Time,
-			"depth", los.sequencingConfDepth)
-		return currentOrigin, nil
+		log.Warn("Next L2 block time is past the sequencer drift + current origin time")
 	}
 
 	// 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(ctx, currentOrigin.Number+1)
 	if err != nil {
-		// TODO: this could result in a bad origin being selected if we are past the seq
-		// drift & should instead advance to the next origin.
+		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")
+		} else {
 			log.Error("Failed to get next origin. Falling back to current origin", "err", err)
+		}
 		return currentOrigin, nil
 	}
 

op-node/rollup/driver/origin_selector_test.go

@@ -27,6 +27,7 @@ func TestOriginSelectorAdvances(t *testing.T) {
 		BlockTime:         2,
 	}
 	l1 := &testutils.MockL1Source{}
+	defer l1.AssertExpectations(t)
 	a := eth.L1BlockRef{
 		Hash:   common.Hash{'a'},
 		Number: 10,
@@ -46,9 +47,8 @@ func TestOriginSelectorAdvances(t *testing.T) {
 	l1.ExpectL1BlockRefByHash(a.Hash, a, nil)
 	l1.ExpectL1BlockRefByNumber(b.Number, b, nil)
 
-	s := NewL1OriginSelector(log, cfg, l1, 0)
-
-	next, err := s.FindL1Origin(context.Background(), b, l2Head)
+	s := NewL1OriginSelector(log, cfg, l1)
+	next, err := s.FindL1Origin(context.Background(), l2Head)
 	require.Nil(t, err)
 	require.Equal(t, b, next)
 }
@@ -68,6 +68,7 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) {
 		BlockTime:         2,
 	}
 	l1 := &testutils.MockL1Source{}
+	defer l1.AssertExpectations(t)
 	a := eth.L1BlockRef{
 		Hash:   common.Hash{'a'},
 		Number: 10,
@@ -87,15 +88,14 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) {
 	l1.ExpectL1BlockRefByHash(a.Hash, a, nil)
 	l1.ExpectL1BlockRefByNumber(b.Number, b, nil)
 
-	s := NewL1OriginSelector(log, cfg, l1, 0)
-
-	next, err := s.FindL1Origin(context.Background(), b, l2Head)
+	s := NewL1OriginSelector(log, cfg, l1)
+	next, err := s.FindL1Origin(context.Background(), l2Head)
 	require.Nil(t, err)
 	require.Equal(t, a, next)
 }
 
 // TestOriginSelectorRespectsConfDepth ensures that the origin selector
-// will respects the confirmation depth requirement
+// will respect the confirmation depth requirement
 //
 // There are 2 L1 blocks at time 20 & 25. The L2 Head is at time 27.
 // The next L2 time is 29 which enough to normally select block `b`
@@ -108,6 +108,7 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) {
 		BlockTime:         2,
 	}
 	l1 := &testutils.MockL1Source{}
+	defer l1.AssertExpectations(t)
 	a := eth.L1BlockRef{
 		Hash:   common.Hash{'a'},
 		Number: 10,
@@ -125,33 +126,32 @@ func TestOriginSelectorRespectsConfDepth(t *testing.T) {
 	}
 
 	l1.ExpectL1BlockRefByHash(a.Hash, a, nil)
-	l1.ExpectL1BlockRefByNumber(b.Number, b, nil)
-
-	s := NewL1OriginSelector(log, cfg, l1, 10)
+	confDepthL1 := NewConfDepth(10, func() eth.L1BlockRef { return b }, l1)
+	s := NewL1OriginSelector(log, cfg, confDepthL1)
 
-	next, err := s.FindL1Origin(context.Background(), b, l2Head)
+	next, err := s.FindL1Origin(context.Background(), l2Head)
 	require.Nil(t, err)
 	require.Equal(t, a, next)
 }
 
-// TestOriginSelectorRespectsMaxSeqDrift ensures that the origin selector
-// will advance if the time delta between the current L1 origin and the next
-// L2 block is greater than the sequencer drift. This needs to occur even
-// if conf depth needs to be ignored
+// TestOriginSelectorStrictConfDepth ensures that the origin selector will maintain the sequencer conf depth,
+// even while the time delta between the current L1 origin and the next
+// L2 block is greater than the sequencer drift.
+// It's more important to maintain safety with an empty block than to maintain liveness with poor conf depth.
 //
 // 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.
-// Even though the LOS would normally refuse to advance because block `b` does not
-// have enough confirmations, it should in this instance.
-func TestOriginSelectorRespectsMaxSeqDrift(t *testing.T) {
+// We maintain confirmation distance, even though we would shift to the next origin if we could.
+func TestOriginSelectorStrictConfDepth(t *testing.T) {
 	log := testlog.Logger(t, log.LvlCrit)
 	cfg := &rollup.Config{
 		MaxSequencerDrift: 8,
 		BlockTime:         2,
 	}
 	l1 := &testutils.MockL1Source{}
+	defer l1.AssertExpectations(t)
 	a := eth.L1BlockRef{
 		Hash:   common.Hash{'a'},
 		Number: 10,
@@ -169,13 +169,11 @@ func TestOriginSelectorRespectsMaxSeqDrift(t *testing.T) {
 	}
 
 	l1.ExpectL1BlockRefByHash(a.Hash, a, nil)
-	l1.ExpectL1BlockRefByNumber(b.Number, b, nil)
-
-	s := NewL1OriginSelector(log, cfg, l1, 10)
+	confDepthL1 := NewConfDepth(10, func() eth.L1BlockRef { return b }, l1)
+	s := NewL1OriginSelector(log, cfg, confDepthL1)
 
-	next, err := s.FindL1Origin(context.Background(), b, l2Head)
-	require.Nil(t, err)
-	require.Equal(t, b, next)
+	_, err := s.FindL1Origin(context.Background(), l2Head)
+	require.ErrorContains(t, err, "sequencer time drift")
 }
 
 // TestOriginSelectorSeqDriftRespectsNextOriginTime
@@ -191,6 +189,7 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) {
 		BlockTime:         2,
 	}
 	l1 := &testutils.MockL1Source{}
+	defer l1.AssertExpectations(t)
 	a := eth.L1BlockRef{
 		Hash:   common.Hash{'a'},
 		Number: 10,
@@ -210,9 +209,76 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) {
 	l1.ExpectL1BlockRefByHash(a.Hash, a, nil)
 	l1.ExpectL1BlockRefByNumber(b.Number, b, nil)
 
-	s := NewL1OriginSelector(log, cfg, l1, 10)
-
-	next, err := s.FindL1Origin(context.Background(), b, l2Head)
+	s := NewL1OriginSelector(log, cfg, l1)
+	next, err := s.FindL1Origin(context.Background(), l2Head)
 	require.Nil(t, err)
 	require.Equal(t, a, next)
 }
+
+// TestOriginSelectorHandlesLateL1Blocks tests the forced repeat of the previous origin,
+// but with a conf depth that first prevents it from learning about the need to repeat.
+//
+// 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.
+// 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) {
+	log := testlog.Logger(t, log.LvlCrit)
+	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,
+	}
+	c := eth.L1BlockRef{
+		Hash:       common.Hash{'c'},
+		Number:     12,
+		Time:       150,
+		ParentHash: b.Hash,
+	}
+	d := eth.L1BlockRef{
+		Hash:       common.Hash{'d'},
+		Number:     13,
+		Time:       200,
+		ParentHash: c.Hash,
+	}
+	l2Head := eth.L2BlockRef{
+		L1Origin: a.ID(),
+		Time:     27,
+	}
+
+	// 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 := NewConfDepth(2, func() eth.L1BlockRef { return l1Head }, l1)
+	s := NewL1OriginSelector(log, cfg, confDepthL1)
+
+	_, err := s.FindL1Origin(context.Background(), l2Head)
+	require.ErrorContains(t, err, "sequencer time drift")
+
+	l1Head = c
+	_, err = s.FindL1Origin(context.Background(), l2Head)
+	require.ErrorContains(t, err, "sequencer time drift")
+
+	l1Head = d
+	next, err := s.FindL1Origin(context.Background(), 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")
+}

op-node/rollup/driver/sequencer.go

@@ -20,7 +20,7 @@ type Downloader interface {
 }
 
 type L1OriginSelectorIface interface {
-	FindL1Origin(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error)
+	FindL1Origin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error)
 }
 
 // Sequencer implements the sequencing interface of the driver: it starts and completes block building jobs.
@@ -51,11 +51,11 @@ func NewSequencer(log log.Logger, cfg *rollup.Config, engine derive.EngineContro
 }
 
 // StartBuildingBlock initiates a block building job on top of the given L2 head, safe and finalized blocks, and using the provided l1Origin.
-func (d *Sequencer) StartBuildingBlock(ctx context.Context, l1Head eth.L1BlockRef) error {
+func (d *Sequencer) StartBuildingBlock(ctx context.Context) error {
 	l2Head := d.engine.UnsafeL2Head()
 
 	// Figure out which L1 origin block we're going to be building on top of.
-	l1Origin, err := d.l1OriginSelector.FindL1Origin(ctx, l1Head, l2Head)
+	l1Origin, err := d.l1OriginSelector.FindL1Origin(ctx, l2Head)
 	if err != nil {
 		d.log.Error("Error finding next L1 Origin", "err", err)
 		return err
@@ -159,7 +159,7 @@ func (d *Sequencer) BuildingOnto() eth.L2BlockRef {
 // RunNextSequencerAction starts new block building work, or seals existing work,
 // and is best timed by first awaiting the delay returned by PlanNextSequencerAction.
 // If a new block is successfully sealed, it will be returned for publishing, nil otherwise.
-func (d *Sequencer) RunNextSequencerAction(ctx context.Context, l1Head eth.L1BlockRef) *eth.ExecutionPayload {
+func (d *Sequencer) RunNextSequencerAction(ctx context.Context) *eth.ExecutionPayload {
 	if _, buildingID, _ := d.engine.BuildingPayload(); buildingID != (eth.PayloadID{}) {
 		payload, err := d.CompleteBuildingBlock(ctx)
 		if err != nil {
@@ -174,7 +174,7 @@ func (d *Sequencer) RunNextSequencerAction(ctx context.Context, l1Head eth.L1Blo
 			return payload
 		}
 	} else {
-		err := d.StartBuildingBlock(ctx, l1Head)
+		err := d.StartBuildingBlock(ctx)
 		if err != nil {
 			d.log.Error("sequencer failed to start building new block", "err", err)
 			d.nextAction = d.timeNow().Add(time.Second)

op-node/rollup/driver/sequencer_test.go

@@ -132,10 +132,10 @@ func (fn testAttrBuilderFn) PreparePayloadAttributes(ctx context.Context, l2Pare
 
 var _ derive.AttributesBuilder = (testAttrBuilderFn)(nil)
 
-type testOriginSelectorFn func(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error)
+type testOriginSelectorFn func(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error)
 
-func (fn testOriginSelectorFn) FindL1Origin(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
-	return fn(ctx, l1Head, l2Head)
+func (fn testOriginSelectorFn) FindL1Origin(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
+	return fn(ctx, l2Head)
 }
 
 var _ L1OriginSelectorIface = (testOriginSelectorFn)(nil)
@@ -262,7 +262,7 @@ func TestSequencerChaosMonkey(t *testing.T) {
 	maxL1BlockTimeGap := uint64(100)
 	// The origin selector just generates random L1 blocks based on RNG
 	var originErr error
-	originSelector := testOriginSelectorFn(func(ctx context.Context, l1Head eth.L1BlockRef, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
+	originSelector := testOriginSelectorFn(func(ctx context.Context, l2Head eth.L2BlockRef) (eth.L1BlockRef, error) {
 		if originErr != nil {
 			return eth.L1BlockRef{}, originErr
 		}
@@ -298,8 +298,6 @@ func TestSequencerChaosMonkey(t *testing.T) {
 	seq := NewSequencer(log, cfg, engControl, attrBuilder, originSelector)
 	seq.timeNow = clockFn
 
-	l1Head := eth.L1BlockRef{} // TODO this is getting removed
-
 	// try to build 1000 blocks, with 5x as many planning attempts, to handle errors and clock problems
 	desiredBlocks := 1000
 	for i := 0; i < 5*desiredBlocks && engControl.totalBuiltBlocks < desiredBlocks; i++ {
@@ -339,7 +337,7 @@ func TestSequencerChaosMonkey(t *testing.T) {
 		default:
 			// no error
 		}
-		payload := seq.RunNextSequencerAction(context.Background(), l1Head)
+		payload := seq.RunNextSequencerAction(context.Background())
 		if payload != nil {
 			require.Equal(t, engControl.UnsafeL2Head().ID(), payload.ID(), "head must stay in sync with emitted payloads")
 			var tx types.Transaction

op-node/rollup/driver/state.go

@@ -212,7 +212,7 @@ func (s *Driver) eventLoop() {
 
 		select {
 		case <-sequencerCh:
-			payload := s.sequencer.RunNextSequencerAction(ctx, s.l1State.L1Head())
+			payload := s.sequencer.RunNextSequencerAction(ctx)
 			if s.network != nil && payload != nil {
 				// Publishing of unsafe data via p2p is optional.
 				// Errors are not severe enough to change/halt sequencing but should be logged and metered.