diff --git a/op-e2e/actions/l2_sequencer.go b/op-e2e/actions/l2_sequencer.go index a0243473ff5..cbc8c639a18 100644 --- a/op-e2e/actions/l2_sequencer.go +++ b/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 diff --git a/op-node/rollup/derive/batches.go b/op-node/rollup/derive/batches.go index cbed3270bee..1d3a65faed3 100644 --- a/op-node/rollup/derive/batches.go +++ b/op-node/rollup/derive/batches.go @@ -100,11 +100,31 @@ func CheckBatch(cfg *rollup.Config, log log.Logger, l1Blocks []eth.L1BlockRef, l return BatchDrop } - // If we ran out of sequencer time drift, then we drop the batch and produce an empty batch instead, - // as the sequencer is not allowed to include anything past this point without moving to the next epoch. + // Check if we ran out of sequencer time drift if max := batchOrigin.Time + cfg.MaxSequencerDrift; batch.Batch.Timestamp > max { - log.Warn("batch exceeded sequencer time drift, sequencer must adopt new L1 origin to include transactions again", "max_time", max) - return BatchDrop + if len(batch.Batch.Transactions) == 0 { + // If the sequencer is co-operating by producing an empty batch, + // then allow the batch if it was the right thing to do to maintain the L2 time >= L1 time invariant. + // We only check batches that do not advance the epoch, to ensure epoch advancement regardless of time drift is allowed. + if epoch.Number >= batchOrigin.Number { + if len(l1Blocks) < 2 { + log.Info("without the next L1 origin we cannot determine yet if this empty batch that exceeds the time drift is still valid") + return BatchUndecided + } + nextOrigin := l1Blocks[1] + if batch.Batch.Timestamp >= nextOrigin.Time { // check if the next L1 origin could have been adopted + log.Info("batch exceeded sequencer time drift without adopting next origin, and next L1 origin would have been valid") + return BatchDrop + } else { + log.Info("continuing with empty batch before late L1 block to preserve L2 time invariant") + } + } + } else { + // If the sequencer is ignoring the time drift rule, then drop the batch and force an empty batch instead, + // as the sequencer is not allowed to include anything past this point without moving to the next epoch. + log.Warn("batch exceeded sequencer time drift, sequencer must adopt new L1 origin to include transactions again", "max_time", max) + return BatchDrop + } } // We can do this check earlier, but it's a more intensive one, so we do this last. diff --git a/op-node/rollup/derive/batches_test.go b/op-node/rollup/derive/batches_test.go index 9e9d6bcd54d..118d3f8fe1e 100644 --- a/op-node/rollup/derive/batches_test.go +++ b/op-node/rollup/derive/batches_test.go @@ -151,6 +151,22 @@ func TestValidBatch(t *testing.T) { SequenceNumber: 0, } + l2A4 := eth.L2BlockRef{ + Hash: testutils.RandomHash(rng), + Number: l2A3.Number + 1, + ParentHash: l2A3.Hash, + Time: l2A3.Time + conf.BlockTime, // 4*2 = 8, higher than seq time drift + L1Origin: l1A.ID(), + SequenceNumber: 4, + } + + l1BLate := eth.L1BlockRef{ + Hash: testutils.RandomHash(rng), + Number: l1A.Number + 1, + ParentHash: l1A.Hash, + Time: l2A4.Time + 1, // too late for l2A4 to adopt yet + } + testCases := []ValidBatchTestCase{ { Name: "missing L1 info", @@ -249,16 +265,16 @@ func TestValidBatch(t *testing.T) { Expected: BatchDrop, }, { - Name: "epoch too old", // repeat of now outdated l2A3 data + Name: "epoch too old, but good parent hash and timestamp", // repeat of now outdated l2A3 data L1Blocks: []eth.L1BlockRef{l1B, l1C, l1D}, L2SafeHead: l2B0, // we already moved on to B Batch: BatchWithL1InclusionBlock{ L1InclusionBlock: l1C, Batch: &BatchData{BatchV1{ - ParentHash: l2A3.ParentHash, + ParentHash: l2B0.Hash, // build on top of safe head to continue EpochNum: rollup.Epoch(l2A3.L1Origin.Number), // epoch A is no longer valid EpochHash: l2A3.L1Origin.Hash, - Timestamp: l2A3.Time, + Timestamp: l2B0.Time + conf.BlockTime, // pass the timestamp check to get too epoch check Transactions: nil, }}, }, @@ -313,23 +329,23 @@ func TestValidBatch(t *testing.T) { Expected: BatchDrop, }, { - Name: "sequencer time drift on same epoch", + Name: "sequencer time drift on same epoch with non-empty txs", L1Blocks: []eth.L1BlockRef{l1A, l1B}, L2SafeHead: l2A3, Batch: BatchWithL1InclusionBlock{ L1InclusionBlock: l1B, Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0 - ParentHash: l2A3.Hash, - EpochNum: rollup.Epoch(l2A3.L1Origin.Number), - EpochHash: l2A3.L1Origin.Hash, - Timestamp: l2A3.Time + conf.BlockTime, - Transactions: nil, + ParentHash: l2A4.ParentHash, + EpochNum: rollup.Epoch(l2A4.L1Origin.Number), + EpochHash: l2A4.L1Origin.Hash, + Timestamp: l2A4.Time, + Transactions: []hexutil.Bytes{[]byte("sequencer should not include this tx")}, }}, }, Expected: BatchDrop, }, { - Name: "sequencer time drift on changing epoch", + Name: "sequencer time drift on changing epoch with non-empty txs", L1Blocks: []eth.L1BlockRef{l1X, l1Y, l1Z}, L2SafeHead: l2X0, Batch: BatchWithL1InclusionBlock{ @@ -339,11 +355,75 @@ func TestValidBatch(t *testing.T) { EpochNum: rollup.Epoch(l2Y0.L1Origin.Number), EpochHash: l2Y0.L1Origin.Hash, Timestamp: l2Y0.Time, // valid, but more than 6 ahead of l1Y.Time - Transactions: nil, + Transactions: []hexutil.Bytes{[]byte("sequencer should not include this tx")}, }}, }, Expected: BatchDrop, }, + { + Name: "sequencer time drift on same epoch with empty txs and late next epoch", + L1Blocks: []eth.L1BlockRef{l1A, l1BLate}, + L2SafeHead: l2A3, + Batch: BatchWithL1InclusionBlock{ + L1InclusionBlock: l1BLate, + Batch: &BatchData{BatchV1{ // l2A4 time < l1BLate time, so we cannot adopt origin B yet + ParentHash: l2A4.ParentHash, + EpochNum: rollup.Epoch(l2A4.L1Origin.Number), + EpochHash: l2A4.L1Origin.Hash, + Timestamp: l2A4.Time, + Transactions: nil, + }}, + }, + Expected: BatchAccept, // accepted because empty & preserving L2 time invariant + }, + { + Name: "sequencer time drift on changing epoch with empty txs", + L1Blocks: []eth.L1BlockRef{l1X, l1Y, l1Z}, + L2SafeHead: l2X0, + Batch: BatchWithL1InclusionBlock{ + L1InclusionBlock: l1Z, + Batch: &BatchData{BatchV1{ + ParentHash: l2Y0.ParentHash, + EpochNum: rollup.Epoch(l2Y0.L1Origin.Number), + EpochHash: l2Y0.L1Origin.Hash, + Timestamp: l2Y0.Time, // valid, but more than 6 ahead of l1Y.Time + Transactions: nil, + }}, + }, + Expected: BatchAccept, // accepted because empty & still advancing epoch + }, + { + Name: "sequencer time drift on same epoch with empty txs and no next epoch in sight yet", + L1Blocks: []eth.L1BlockRef{l1A}, + L2SafeHead: l2A3, + Batch: BatchWithL1InclusionBlock{ + L1InclusionBlock: l1B, + Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0 + ParentHash: l2A4.ParentHash, + EpochNum: rollup.Epoch(l2A4.L1Origin.Number), + EpochHash: l2A4.L1Origin.Hash, + Timestamp: l2A4.Time, + Transactions: nil, + }}, + }, + Expected: BatchUndecided, // we have to wait till the next epoch is in sight to check the time + }, + { + Name: "sequencer time drift on same epoch with empty txs and but in-sight epoch that invalidates it", + L1Blocks: []eth.L1BlockRef{l1A, l1B, l1C}, + L2SafeHead: l2A3, + Batch: BatchWithL1InclusionBlock{ + L1InclusionBlock: l1C, + Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0 + ParentHash: l2A4.ParentHash, + EpochNum: rollup.Epoch(l2A4.L1Origin.Number), + EpochHash: l2A4.L1Origin.Hash, + Timestamp: l2A4.Time, + Transactions: nil, + }}, + }, + Expected: BatchDrop, // dropped because it could have advanced the epoch to B + }, { Name: "empty tx included", L1Blocks: []eth.L1BlockRef{l1A, l1B}, diff --git a/op-node/rollup/driver/driver.go b/op-node/rollup/driver/driver.go index e6fdf7cd939..16e963a0ad6 100644 --- a/op-node/rollup/driver/driver.go +++ b/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) diff --git a/op-node/rollup/driver/origin_selector.go b/op-node/rollup/driver/origin_selector.go index dceaf03bead..dbc7a04db0e 100644 --- a/op-node/rollup/driver/origin_selector.go +++ b/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" @@ -19,63 +22,49 @@ type L1OriginSelector struct { log log.Logger cfg *rollup.Config - l1 L1Blocks - sequencingConfDepth uint64 + l1 L1Blocks } -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, + log: log, + cfg: cfg, + l1: l1, } } // 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. - log.Error("Failed to get next origin. Falling back to current origin", "err", err) + 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 } diff --git a/op-node/rollup/driver/origin_selector_test.go b/op-node/rollup/driver/origin_selector_test.go index a33fe53b5f5..fde037995d5 100644 --- a/op-node/rollup/driver/origin_selector_test.go +++ b/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,9 @@ func TestOriginSelectorAdvances(t *testing.T) { l1.ExpectL1BlockRefByHash(a.Hash, a, nil) l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(log, cfg, l1, 0) + s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), b, l2Head) + next, err := s.FindL1Origin(context.Background(), l2Head) require.Nil(t, err) require.Equal(t, b, next) } @@ -68,6 +69,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 +89,15 @@ func TestOriginSelectorRespectsOriginTiming(t *testing.T) { l1.ExpectL1BlockRefByHash(a.Hash, a, nil) l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(log, cfg, l1, 0) + s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), b, l2Head) + 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 +110,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 +128,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 +171,12 @@ 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 +192,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 +212,77 @@ func TestOriginSelectorSeqDriftRespectsNextOriginTime(t *testing.T) { l1.ExpectL1BlockRefByHash(a.Hash, a, nil) l1.ExpectL1BlockRefByNumber(b.Number, b, nil) - s := NewL1OriginSelector(log, cfg, l1, 10) + s := NewL1OriginSelector(log, cfg, l1) - next, err := s.FindL1Origin(context.Background(), b, l2Head) + 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) + + head := b + confDepthL1 := NewConfDepth(2, func() eth.L1BlockRef { return head }, l1) + s := NewL1OriginSelector(log, cfg, confDepthL1) + + _, err := s.FindL1Origin(context.Background(), l2Head) + require.ErrorContains(t, err, "sequencer time drift") + + head = c + _, err = s.FindL1Origin(context.Background(), l2Head) + require.ErrorContains(t, err, "sequencer time drift") + + head = 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") +} diff --git a/op-node/rollup/driver/sequencer.go b/op-node/rollup/driver/sequencer.go index f2b2ab52737..4d78fc28cb5 100644 --- a/op-node/rollup/driver/sequencer.go +++ b/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) diff --git a/op-node/rollup/driver/sequencer_test.go b/op-node/rollup/driver/sequencer_test.go index 588cf5b88f2..a1ae06e95f7 100644 --- a/op-node/rollup/driver/sequencer_test.go +++ b/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 diff --git a/op-node/rollup/driver/state.go b/op-node/rollup/driver/state.go index 18bbca0484b..0beaeb8b95a 100644 --- a/op-node/rollup/driver/state.go +++ b/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. diff --git a/specs/derivation.md b/specs/derivation.md index c9de145ed82..8f270de1127 100644 --- a/specs/derivation.md +++ b/specs/derivation.md @@ -601,7 +601,17 @@ Rules, in validation order: - `batch.timestamp > batch_origin.time + max_sequencer_drift` -> `drop`: i.e. a batch that does not adopt the next L1 within time will be dropped, in favor of an empty batch that can advance the L1 origin. This enforces the max L2 timestamp rule. -- `batch.timestamp < batch_origin.time` -> `drop`: enforce the min L2 timestamp rule. +- `batch.timestamp < batch_origin.time`: enforce the min L2 timestamp rule, + but with exceptions to handle preserve invariants: + - `len(batch.transactions) == 0`: + - `epoch.number >= batch.epoch_num`: + this implies the batch does not already advance the L1 origin, and must thus be checked against `next_epoch`. + - If `next_epoch` is not known -> `undecided`: + without the next L1 origin we cannot yet determine if time invariant could have been kept. + - If `batch.timestamp >= next_epoch.time` -> `drop`: + the batch could have adopted the next L1 origin without breaking the `L2 time >= L1 time` invariant. + - `len(batch.transactions) > 0`: -> `drop`: + when exceeding the sequencer time drift, never allow the sequencer to include transactions. - `batch.transactions`: `drop` if the `batch.transactions` list contains a transaction that is invalid or derived by other means exclusively: - any transaction that is empty (zero length byte string)