Skip to content

op-node: fix sequencer time drift edge case, strictly enforce sequencer conf depth #4758

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
protolambda wants to merge 1 commit into from
Closed

op-node: fix sequencer time drift edge case, strictly enforce sequencer conf depth #4758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions op-e2e/actions/l2_sequencer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
28 changes: 24 additions & 4 deletions op-node/rollup/derive/batches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
102 changes: 91 additions & 11 deletions op-node/rollup/derive/batches_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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,
}},
},
Expand Down Expand Up @@ -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{
Expand All @@ -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},
Expand Down
7 changes: 4 additions & 3 deletions op-node/rollup/driver/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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)
Expand Down
55 changes: 22 additions & 33 deletions op-node/rollup/driver/origin_selector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}

Expand Down
Loading