Safe head fix 2

op-node/rollup/derive/engine_queue.go

@@ -10,7 +10,6 @@ import (
 	"github.com/ethereum-optimism/optimism/op-node/eth"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/rollup/sync"
-	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -380,23 +379,11 @@ func (eq *EngineQueue) forceNextSafeAttributes(ctx context.Context) error {
 // ResetStep Walks the L2 chain backwards until it finds an L2 block whose L1 origin is canonical.
 // The unsafe head is set to the head of the L2 chain, unless the existing safe head is not canonical.
 func (eq *EngineQueue) ResetStep(ctx context.Context, l1Fetcher L1Fetcher) error {
-	finalized, err := eq.engine.L2BlockRefByLabel(ctx, eth.Finalized)
-	if errors.Is(err, ethereum.NotFound) {
-		// default to genesis if we have not finalized anything before.
-		finalized, err = eq.engine.L2BlockRefByHash(ctx, eq.cfg.Genesis.L2.Hash)
-	}
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to find the finalized L2 block: %w", err))
-	}
-	// TODO: this should be resetting using the safe head instead. Out of scope for L2 client bindings PR.
-	prevUnsafe, err := eq.engine.L2BlockRefByLabel(ctx, eth.Unsafe)
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to find the L2 Head block: %w", err))
-	}
-	unsafe, safe, err := sync.FindL2Heads(ctx, prevUnsafe, eq.cfg.SeqWindowSize, l1Fetcher, eq.engine, &eq.cfg.Genesis)
+	result, err := sync.FindL2Heads(ctx, eq.cfg, l1Fetcher, eq.engine)
 	if err != nil {
 		return NewTemporaryError(fmt.Errorf("failed to find the L2 Heads to start from: %w", err))
 	}
+	finalized, safe, unsafe := result.Finalized, result.Safe, result.Unsafe
 	l1Origin, err := l1Fetcher.L1BlockRefByHash(ctx, safe.L1Origin.Hash)
 	if err != nil {
 		return NewTemporaryError(fmt.Errorf("failed to fetch the new L1 progress: origin: %v; err: %w", safe.L1Origin, err))

op-node/rollup/derive/engine_queue_test.go

@@ -17,20 +17,6 @@ func TestEngineQueue_Finalize(t *testing.T) {
 	logger := testlog.Logger(t, log.LvlInfo)
 
 	rng := rand.New(rand.NewSource(1234))
-	// create a short test L2 chain:
-	//
-	// L2:
-	//	A0: genesis
-	//	A1: finalized, incl in B
-	//  B0: safe, incl in C
-	//  B1: not yet included in L1
-	//  C0: head, not included in L1 yet
-	//
-	// L1:
-	//  A: genesis
-	//  B: finalized, incl A1
-	//  C: safe, incl B0
-	//  D: unsafe, not yet referenced by L2
 
 	l1Time := uint64(2)
 	refA := testutils.RandomBlockRef(rng)
@@ -53,6 +39,18 @@ func TestEngineQueue_Finalize(t *testing.T) {
 		ParentHash: refC.Hash,
 		Time:       refC.Time + l1Time,
 	}
+	refE := eth.L1BlockRef{
+		Hash:       testutils.RandomHash(rng),
+		Number:     refD.Number + 1,
+		ParentHash: refD.Hash,
+		Time:       refD.Time + l1Time,
+	}
+	refF := eth.L1BlockRef{
+		Hash:       testutils.RandomHash(rng),
+		Number:     refE.Number + 1,
+		ParentHash: refE.Hash,
+		Time:       refE.Time + l1Time,
+	}
 
 	refA0 := eth.L2BlockRef{
 		Hash:           testutils.RandomHash(rng),
@@ -103,42 +101,138 @@ func TestEngineQueue_Finalize(t *testing.T) {
 		L1Origin:       refC.ID(),
 		SequenceNumber: 0,
 	}
+	refC1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refC0.Number + 1,
+		ParentHash:     refC0.Hash,
+		Time:           refC0.Time + cfg.BlockTime,
+		L1Origin:       refC.ID(),
+		SequenceNumber: 1,
+	}
+	refD0 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refC1.Number + 1,
+		ParentHash:     refC1.Hash,
+		Time:           refC1.Time + cfg.BlockTime,
+		L1Origin:       refD.ID(),
+		SequenceNumber: 0,
+	}
+	refD1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refD0.Number + 1,
+		ParentHash:     refD0.Hash,
+		Time:           refD0.Time + cfg.BlockTime,
+		L1Origin:       refD.ID(),
+		SequenceNumber: 1,
+	}
+	refE0 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refD1.Number + 1,
+		ParentHash:     refD1.Hash,
+		Time:           refD1.Time + cfg.BlockTime,
+		L1Origin:       refE.ID(),
+		SequenceNumber: 0,
+	}
+	refE1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refE0.Number + 1,
+		ParentHash:     refE0.Hash,
+		Time:           refE0.Time + cfg.BlockTime,
+		L1Origin:       refE.ID(),
+		SequenceNumber: 1,
+	}
+	refF0 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refE1.Number + 1,
+		ParentHash:     refE1.Hash,
+		Time:           refE1.Time + cfg.BlockTime,
+		L1Origin:       refF.ID(),
+		SequenceNumber: 0,
+	}
+	refF1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refF0.Number + 1,
+		ParentHash:     refF0.Hash,
+		Time:           refF0.Time + cfg.BlockTime,
+		L1Origin:       refF.ID(),
+		SequenceNumber: 1,
+	}
+	t.Log("refA", refA.Hash)
+	t.Log("refB", refB.Hash)
+	t.Log("refC", refC.Hash)
+	t.Log("refD", refD.Hash)
+	t.Log("refE", refE.Hash)
+	t.Log("refF", refF.Hash)
+	t.Log("refA0", refA0.Hash)
+	t.Log("refA1", refA1.Hash)
+	t.Log("refB0", refB0.Hash)
+	t.Log("refB1", refB1.Hash)
+	t.Log("refC0", refC0.Hash)
+	t.Log("refC1", refC1.Hash)
+	t.Log("refD0", refD0.Hash)
+	t.Log("refD1", refD1.Hash)
+	t.Log("refE0", refE0.Hash)
+	t.Log("refE1", refE1.Hash)
+	t.Log("refF0", refF0.Hash)
+	t.Log("refF1", refF1.Hash)
 
 	metrics := &TestMetrics{}
 	eng := &testutils.MockEngine{}
-	eng.ExpectL2BlockRefByLabel(eth.Finalized, refA1, nil)
-	// TODO(Proto): update expectation once we're using safe block label properly for sync starting point
-	eng.ExpectL2BlockRefByLabel(eth.Unsafe, refC0, nil)
-
 	// we find the common point to initialize to by comparing the L1 origins in the L2 chain with the L1 chain
 	l1F := &testutils.MockL1Source{}
-	l1F.ExpectL1BlockRefByLabel(eth.Unsafe, refD, nil)
-	l1F.ExpectL1BlockRefByNumber(refC0.L1Origin.Number, refC, nil)
-	eng.ExpectL2BlockRefByHash(refC0.ParentHash, refB1, nil)   // good L1 origin
-	eng.ExpectL2BlockRefByHash(refB1.ParentHash, refB0, nil)   // need a block with seqnr == 0, don't stop at above
-	l1F.ExpectL1BlockRefByHash(refB0.L1Origin.Hash, refB, nil) // the origin of the safe L2 head will be the L1 starting point for derivation.
 
-	eq := NewEngineQueue(logger, cfg, eng, metrics)
-	require.NoError(t, RepeatResetStep(t, eq.ResetStep, l1F, 3))
+	eng.ExpectL2BlockRefByLabel(eth.Finalized, refA1, nil)
+	eng.ExpectL2BlockRefByLabel(eth.Safe, refE0, nil)
+	eng.ExpectL2BlockRefByLabel(eth.Unsafe, refF1, nil)
 
-	// TODO(proto): this is changing, needs to be a sequence window ago, but starting traversal back from safe block,
-	// safe blocks with canon origin are good, but we go back a full window to ensure they are all included in L1,
-	// by forcing them to be consolidated with L1 again.
-	require.Equal(t, eq.SafeL2Head(), refB0, "L2 reset should go back to sequence window ago")
+	// unsafe
+	l1F.ExpectL1BlockRefByNumber(refF.Number, refF, nil)
+	eng.ExpectL2BlockRefByHash(refF1.ParentHash, refF0, nil)
+	eng.ExpectL2BlockRefByHash(refF0.ParentHash, refE1, nil)
 
+	// meet previous safe, counts 1/2
+	l1F.ExpectL1BlockRefByNumber(refE.Number, refE, nil)
+	eng.ExpectL2BlockRefByHash(refE1.ParentHash, refE0, nil)
+	eng.ExpectL2BlockRefByHash(refE0.ParentHash, refD1, nil)
+
+	// now full seq window, inclusive
+	l1F.ExpectL1BlockRefByNumber(refD.Number, refD, nil)
+	eng.ExpectL2BlockRefByHash(refD1.ParentHash, refD0, nil)
+	eng.ExpectL2BlockRefByHash(refD0.ParentHash, refC1, nil)
+
+	// now one more L1 origin
+	l1F.ExpectL1BlockRefByNumber(refC.Number, refC, nil)
+	eng.ExpectL2BlockRefByHash(refC1.ParentHash, refC0, nil)
+	// parent of that origin will be considered safe
+	eng.ExpectL2BlockRefByHash(refC0.ParentHash, refB1, nil)
+
+	// and we fetch the L1 origin of that as starting point for engine queue
+	l1F.ExpectL1BlockRefByHash(refB.Hash, refB, nil)
+
+	eq := NewEngineQueue(logger, cfg, eng, metrics)
+	require.NoError(t, RepeatResetStep(t, eq.ResetStep, l1F, 20))
+
+	require.Equal(t, refB1, eq.SafeL2Head(), "L2 reset should go back to sequence window ago: blocks with origin E and D are not safe until we reconcile, C is extra, and B1 is the end we look for")
+	require.Equal(t, refB, eq.Progress().Origin, "Expecting to be set back derivation L1 progress to B")
 	require.Equal(t, refA1, eq.Finalized(), "A1 is recognized as finalized before we run any steps")
 
-	// we are not adding blocks in this test,
-	// but we can still trigger post-processing for the already existing safe head,
-	// so the engine can prepare to finalize that.
+	// now say C1 was included in D and became the new safe head
+	eq.progress.Origin = refD
+	eq.safeHead = refC1
+	eq.postProcessSafeL2()
+
+	// now say D0 was included in E and became the new safe head
+	eq.progress.Origin = refE
+	eq.safeHead = refD0
 	eq.postProcessSafeL2()
-	// let's finalize C, which included B0, but not B1
-	eq.Finalize(refC.ID())
+
+	// let's finalize D (current L1), from which we fully derived C1 (it was safe head), but not D0 (included in E)
+	eq.Finalize(refD.ID())
 
 	// Now a few steps later, without consuming any additional L1 inputs,
-	// we should be able to resolve that B0 is now finalized
+	// we should be able to resolve that B1 is now finalized, since it was included in finalized L1 block C
 	require.NoError(t, RepeatStep(t, eq.Step, eq.progress, 10))
-	require.Equal(t, refB0, eq.Finalized(), "B0 was included in finalized C, and should now be finalized")
+	require.Equal(t, refC1, eq.Finalized(), "C1 was included in finalized D, and should now be finalized")
 
 	l1F.AssertExpectations(t)
 	eng.AssertExpectations(t)