Safe head fix 2 (#3382)

* op-node: sync start update func signature, port over tests from safe-head-fix pr

* op-node: sync start now starts with current heads by label

* op-node: single loop find-heads

* op-node: sync - clean up go doc

* op-node: use sync fn in engine queue

* op-node: fix engine queue finalization test

* op-node: sync off-by-one fix

* op-node: fix highest l2 block with canon origin, need to reset when l1 reorg

* op-node: handle non-standard safe/finalized not found errors

* op-node: sync start review fixes / comment typos

* op-node: seq window size check with l1 origin, update engine queue test with extra l1 origin, now that off by 1 fix applies properly to chains with multiple L2 blocks per L1 block

* op-node: start from parent block before seq nr 0

* Update op-node/rollup/sync/start.go
Co-authored-by: Joshua Gutow <jgutow@optimism.io>

* review fixes

* fix lint
Co-authored-by: Joshua Gutow <jgutow@optimism.io>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>

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"
@@ -384,23 +383,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)

op-node/sources/l1_client.go

@@ -3,11 +3,13 @@ package sources
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/ethereum-optimism/optimism/op-node/client"
 	"github.com/ethereum-optimism/optimism/op-node/eth"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/sources/caching"
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -67,6 +69,11 @@ func NewL1Client(client client.RPC, log log.Logger, metrics caching.Metrics, con
 func (s *L1Client) L1BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L1BlockRef, error) {
 	info, err := s.InfoByLabel(ctx, label)
 	if err != nil {
+		// Both geth and erigon like to serve non-standard errors for the safe and finalized heads, correct that.
+		// This happens when the chain just started and nothing is marked as safe/finalized yet.
+		if strings.Contains(err.Error(), "block not found") || strings.Contains(err.Error(), "Unknown block") {
+			err = ethereum.NotFound
+		}
 		return eth.L1BlockRef{}, fmt.Errorf("failed to fetch head header: %w", err)
 	}
 	ref := eth.InfoToL1BlockRef(info)

op-node/sources/l2_client.go

@@ -3,12 +3,14 @@ package sources
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/ethereum-optimism/optimism/op-node/client"
 	"github.com/ethereum-optimism/optimism/op-node/eth"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/rollup/derive"
 	"github.com/ethereum-optimism/optimism/op-node/sources/caching"
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -77,6 +79,11 @@ func NewL2Client(client client.RPC, log log.Logger, metrics caching.Metrics, con
 func (s *L2Client) L2BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L2BlockRef, error) {
 	payload, err := s.PayloadByLabel(ctx, label)
 	if err != nil {
+		// Both geth and erigon like to serve non-standard errors for the safe and finalized heads, correct that.
+		// This happens when the chain just started and nothing is marked as safe/finalized yet.
+		if strings.Contains(err.Error(), "block not found") || strings.Contains(err.Error(), "Unknown block") {
+			err = ethereum.NotFound
+		}
 		// w%: wrap to preserve ethereum.NotFound case
 		return eth.L2BlockRef{}, fmt.Errorf("failed to determine L2BlockRef of %s, could not get payload: %w", label, err)
 	}

op-node/testutils/fake_chain.go

@@ -85,13 +85,17 @@ func NewFakeChainSource(l1 []string, l2 []string, l1GenesisNumber int, log log.L
 // what the head block is of the L1 and L2 chains. In addition, it enables re-orgs
 // to easily be implemented
 type FakeChainSource struct {
-	l1reorg int                // Index of the L1 chain to be operating on
-	l2reorg int                // Index of the L2 chain to be operating on
-	l1head  int                // Head block of the L1 chain
-	l2head  int                // Head block of the L2 chain
-	l1s     [][]eth.L1BlockRef // l1s[reorg] is the L1 chain in that specific re-org configuration
-	l2s     [][]eth.L2BlockRef // l2s[reorg] is the L2 chain in that specific re-org configuration
-	log     log.Logger
+	l1reorg     int // Index of the L1 chain to be operating on
+	l2reorg     int // Index of the L2 chain to be operating on
+	l1head      int // Head block of the L1 chain
+	l2head      int // Head block of the L2 chain
+	l1safe      int
+	l2safe      int
+	l1finalized int
+	l2finalized int
+	l1s         [][]eth.L1BlockRef // l1s[reorg] is the L1 chain in that specific re-org configuration
+	l2s         [][]eth.L2BlockRef // l2s[reorg] is the L2 chain in that specific re-org configuration
+	log         log.Logger
 }
 
 func (m *FakeChainSource) L1Range(ctx context.Context, base eth.BlockID, max uint64) ([]eth.BlockID, error) {
@@ -134,26 +138,39 @@ func (m *FakeChainSource) L1BlockRefByHash(ctx context.Context, l1Hash common.Ha
 }
 
 func (m *FakeChainSource) L1BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L1BlockRef, error) {
-	if label != eth.Unsafe {
-		return eth.L1BlockRef{}, fmt.Errorf("testutil FakeChainSource does not support L1BlockRefByLabel(%s)", label)
-	}
-	m.log.Trace("L1HeadBlockRef", "l1Head", m.l1head, "reorg", m.l1reorg)
+	m.log.Trace("L1BlockRefByLabel", "l1Head", m.l1head, "l1Safe", m.l1safe, "l1Finalized", m.l1finalized, "reorg", m.l1reorg)
 	l := len(m.l1s[m.l1reorg])
 	if l == 0 {
 		return eth.L1BlockRef{}, ethereum.NotFound
 	}
-	return m.l1s[m.l1reorg][m.l1head], nil
+	switch label {
+	case eth.Unsafe:
+		return m.l1s[m.l1reorg][m.l1head], nil
+	case eth.Safe:
+		return m.l1s[m.l1reorg][m.l1safe], nil
+	case eth.Finalized:
+		return m.l1s[m.l1reorg][m.l1finalized], nil
+	default:
+		return eth.L1BlockRef{}, fmt.Errorf("testutil FakeChainSource does not support L1BlockRefByLabel(%s)", label)
+	}
 }
 
 func (m *FakeChainSource) L2BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L2BlockRef, error) {
-	if label != eth.Unsafe {
-		return eth.L2BlockRef{}, fmt.Errorf("testutil FakeChainSource does not support L2BlockRefByLabel(%s)", label)
+	m.log.Trace("L2BlockRefByLabel", "l2Head", m.l2head, "l2Safe", m.l2safe, "l2Finalized", m.l2finalized, "reorg", m.l2reorg)
+	l := len(m.l2s[m.l2reorg])
+	if l == 0 {
+		return eth.L2BlockRef{}, ethereum.NotFound
 	}
-	m.log.Trace("L2BlockRefHead", "l2Head", m.l2head, "reorg", m.l2reorg)
-	if len(m.l2s[m.l2reorg]) == 0 {
-		panic("bad test, no l2 chain")
+	switch label {
+	case eth.Unsafe:
+		return m.l2s[m.l2reorg][m.l2head], nil
+	case eth.Safe:
+		return m.l2s[m.l2reorg][m.l2safe], nil
+	case eth.Finalized:
+		return m.l2s[m.l2reorg][m.l2finalized], nil
+	default:
+		return eth.L2BlockRef{}, fmt.Errorf("testutil FakeChainSource does not support L2BlockRefByLabel(%s)", label)
 	}
-	return m.l2s[m.l2reorg][m.l2head], nil
 }
 
 func (m *FakeChainSource) L2BlockRefByNumber(ctx context.Context, l2Num *big.Int) (eth.L2BlockRef, error) {
@@ -204,6 +221,28 @@ func (m *FakeChainSource) ReorgL1() {
 	}
 }
 
+func (m *FakeChainSource) SetL2Safe(safe common.Hash) {
+	m.log.Trace("Set L2 safe head", "new_safe", safe, "old_safe", m.l2safe)
+	for i, v := range m.l2s[m.l2reorg] {
+		if v.Hash == safe {
+			m.l2safe = i
+			return
+		}
+	}
+	panic(fmt.Errorf("unknown safe block: %s", safe))
+}
+
+func (m *FakeChainSource) SetL2Finalized(finalized common.Hash) {
+	m.log.Trace("Set L2 finalized head", "new_finalized", finalized, "old_finalized", m.l2finalized)
+	for i, v := range m.l2s[m.l2reorg] {
+		if v.Hash == finalized {
+			m.l2finalized = i
+			return
+		}
+	}
+	panic(fmt.Errorf("unknown finalized block: %s", finalized))
+}
+
 func (m *FakeChainSource) SetL2Head(head int) eth.L2BlockRef {
 	m.log.Trace("Set L2 head", "new_head", head, "old_head", m.l2head)
 	m.l2head = head