From 52416befa387283dc9f21c06a7b75fd8a102764d Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Wed, 10 Jan 2024 16:16:20 +0900 Subject: [PATCH 01/11] op-node: Restore previous unsafe chain using backupUnsafe --- op-node/rollup/derive/engine_controller.go | 96 ++++++++++++++++++++-- op-node/rollup/derive/engine_queue.go | 20 ++++- 2 files changed, 108 insertions(+), 8 deletions(-) diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index 66f9b63552bb..bfa57b0e9cb2 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -33,6 +33,7 @@ const ( ) var errNoFCUNeeded = errors.New("no FCU call was needed") +var errNoBackupUnsafeReorgNeeded = errors.New("no BackupUnsafeReorg was needed") var _ EngineControl = (*EngineController)(nil) var _ LocalEngineControl = (*EngineController)(nil) @@ -55,11 +56,18 @@ type EngineController struct { clock clock.Clock // Block Head State - unsafeHead eth.L2BlockRef - pendingSafeHead eth.L2BlockRef // L2 block processed from the middle of a span batch, but not marked as the safe block yet. - safeHead eth.L2BlockRef - finalizedHead eth.L2BlockRef - needFCUCall bool + unsafeHead eth.L2BlockRef + pendingSafeHead eth.L2BlockRef // L2 block processed from the middle of a span batch, but not marked as the safe block yet. + safeHead eth.L2BlockRef + finalizedHead eth.L2BlockRef + backupUnsafeHead eth.L2BlockRef + needFCUCall bool + // Track when the rollup node changes the forkchoice to restore previous + // known unsafe chain. e.g. Unsafe Reorg caused by Invalid span batch. + // This update does not retry except engine returns non-input error + // because engine may forgot backupUnsafeHead or backupUnsafeHead is not part + // of the chain. + needFCUCallForBackupUnsafeReorg bool // Building State buildingOnto eth.L2BlockRef @@ -103,6 +111,10 @@ func (e *EngineController) Finalized() eth.L2BlockRef { return e.finalizedHead } +func (e *EngineController) BackupUnsafeL2Head() eth.L2BlockRef { + return e.backupUnsafeHead +} + func (e *EngineController) BuildingPayload() (eth.L2BlockRef, eth.PayloadID, bool) { return e.buildingOnto, e.buildingInfo.ID, e.buildingSafe } @@ -140,6 +152,13 @@ func (e *EngineController) SetUnsafeHead(r eth.L2BlockRef) { e.needFCUCall = true } +// SetBackupUnsafeL2Head implements LocalEngineControl. +func (e *EngineController) SetBackupUnsafeL2Head(r eth.L2BlockRef, triggerReorg bool) { + e.metrics.RecordL2Ref("l2_backupUnsafe", r) + e.backupUnsafeHead = r + e.needFCUCallForBackupUnsafeReorg = triggerReorg +} + // Engine Methods func (e *EngineController) StartPayload(ctx context.Context, parent eth.L2BlockRef, attrs *AttributesWithParent, updateSafe bool) (errType BlockInsertionErrType, err error) { @@ -199,7 +218,10 @@ func (e *EngineController) ConfirmPayload(ctx context.Context, agossip async.Asy if err != nil { return nil, BlockInsertPayloadErr, NewResetError(fmt.Errorf("failed to decode L2 block ref from payload: %w", err)) } - + // Backup unsafeHead when new block is not built on original unsafe head. + if e.unsafeHead.Number >= ref.Number { + e.SetBackupUnsafeL2Head(e.unsafeHead, false) + } e.unsafeHead = ref e.metrics.RecordL2Ref("l2_unsafe", ref) @@ -209,6 +231,8 @@ func (e *EngineController) ConfirmPayload(ctx context.Context, agossip async.Asy if updateSafe { e.safeHead = ref e.metrics.RecordL2Ref("l2_safe", ref) + // Remove backupUnsafeHead because this backup will be never used after consolidation. + e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) } } @@ -275,7 +299,7 @@ func (e *EngineController) TryUpdateEngine(ctx context.Context) error { return errNoFCUNeeded } if e.IsEngineSyncing() { - e.log.Warn("Attempting to update forkchoice state while engine is P2P syncing") + e.log.Warn("Attempting to update forkchoice state while EL sync.") } fc := eth.ForkchoiceState{ HeadBlockHash: e.unsafeHead.Hash, @@ -370,6 +394,64 @@ func (e *EngineController) InsertUnsafePayload(ctx context.Context, envelope *et return nil } +// tryBackupUnsafeReorg attempts to reorg(restore) unsafe head to backupUnsafeHead. +// If succeeds, update current forkchoice state to the rollup node. +func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { + if !e.needFCUCallForBackupUnsafeReorg { + return errNoBackupUnsafeReorgNeeded + } + // This method must be never called when EL sync. If EL sync is in progress, early return. + if e.IsEngineSyncing() { + e.log.Warn("Attempting to update forkchoice state while EL sync.") + return errNoBackupUnsafeReorgNeeded + } + if e.BackupUnsafeL2Head() == (eth.L2BlockRef{}) { // sanity check backupUnsafeHead is there + e.log.Warn("Attempting to unsafe reorg using backupUnsafe even though it is empty") + e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) + return errNoBackupUnsafeReorgNeeded + } + // Only try FCU once because execution engine may forgot backupUnsafeHead + // or backupUnsafeHead is not part of the chain. + // Exception: Retry when forkChoiceUpdate returns non-input error. + e.needFCUCallForBackupUnsafeReorg = false + // Reorg unsafe chain. Safe/Finalized chain will not be updated. + e.log.Warn("trying to restore unsafe head", "backupUnsafe", e.backupUnsafeHead.ID(), "unsafe", e.unsafeHead.ID()) + fc := eth.ForkchoiceState{ + HeadBlockHash: e.backupUnsafeHead.Hash, + SafeBlockHash: e.safeHead.Hash, + FinalizedBlockHash: e.finalizedHead.Hash, + } + fcRes, err := e.engine.ForkchoiceUpdate(ctx, &fc, nil) + if err != nil { + var inputErr eth.InputError + if errors.As(err, &inputErr) { + e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) + switch inputErr.Code { + case eth.InvalidForkchoiceState: + return NewResetError(fmt.Errorf("forkchoice update was inconsistent with engine, need reset to resolve: %w", inputErr.Unwrap())) + default: + return NewTemporaryError(fmt.Errorf("unexpected error code in forkchoice-updated response: %w", err)) + } + } else { + // Retry when forkChoiceUpdate returns non-input error. + // Do not reset backupUnsafeHead because it will be used again. + e.needFCUCallForBackupUnsafeReorg = true + return NewTemporaryError(fmt.Errorf("failed to sync forkchoice with engine: %w", err)) + } + } + if fcRes.PayloadStatus.Status == eth.ExecutionValid { + // Execution engine accepted the reorg. + e.log.Info("successfully reorged unsafe head using backupUnsafe", "unsafe", e.backupUnsafeHead.ID()) + e.SetUnsafeHead(e.BackupUnsafeL2Head()) + e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) + return nil + } + e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) + // Execution engine could not reorg back to previous unsafe head. + return NewTemporaryError(fmt.Errorf("cannot restore unsafe chain using backupUnsafe: err: %w", + eth.ForkchoiceUpdateErr(fcRes.PayloadStatus))) +} + // ResetBuildingState implements LocalEngineControl. func (e *EngineController) ResetBuildingState() { e.resetBuildingState() diff --git a/op-node/rollup/derive/engine_queue.go b/op-node/rollup/derive/engine_queue.go index bf1d6a75e2a5..89901754d1ba 100644 --- a/op-node/rollup/derive/engine_queue.go +++ b/op-node/rollup/derive/engine_queue.go @@ -82,14 +82,17 @@ type LocalEngineControl interface { ResetBuildingState() IsEngineSyncing() bool TryUpdateEngine(ctx context.Context) error + TryBackupUnsafeReorg(ctx context.Context) error InsertUnsafePayload(ctx context.Context, payload *eth.ExecutionPayloadEnvelope, ref eth.L2BlockRef) error PendingSafeL2Head() eth.L2BlockRef + BackupUnsafeL2Head() eth.L2BlockRef SetUnsafeHead(eth.L2BlockRef) SetSafeHead(eth.L2BlockRef) SetFinalizedHead(eth.L2BlockRef) SetPendingSafeL2Head(eth.L2BlockRef) + SetBackupUnsafeL2Head(block eth.L2BlockRef, triggerReorg bool) } // SafeHeadListener is called when the safe head is updated. @@ -256,12 +259,22 @@ func (eq *EngineQueue) LowestQueuedUnsafeBlock() eth.L2BlockRef { return ref } +func (eq *EngineQueue) BackupUnsafeL2Head() eth.L2BlockRef { + return eq.ec.BackupUnsafeL2Head() +} + // Determine if the engine is syncing to the target block func (eq *EngineQueue) isEngineSyncing() bool { return eq.ec.IsEngineSyncing() } func (eq *EngineQueue) Step(ctx context.Context) error { + // If we don't need to call FCU to restore unsafeHead using backupUnsafe, keep going b/c + // this was a no-op(except correcting invalid state when backupUnsafe is empty but reorg triggered). + // If we needed to perform a network call, then we should yield even if we did not encounter an error. + if err := eq.ec.TryBackupUnsafeReorg(ctx); !errors.Is(err, errNoBackupUnsafeReorgNeeded) { + return err + } // If we don't need to call FCU, keep going b/c this was a no-op. If we needed to // perform a network call, then we should yield even if we did not encounter an error. if err := eq.ec.TryUpdateEngine(ctx); !errors.Is(err, errNoFCUNeeded) { @@ -451,6 +464,7 @@ func (eq *EngineQueue) logSyncProgress(reason string) { "l2_safe", eq.ec.SafeL2Head(), "l2_pending_safe", eq.ec.PendingSafeL2Head(), "l2_unsafe", eq.ec.UnsafeL2Head(), + "l2_backupUnsafe", eq.ec.BackupUnsafeL2Head(), "l2_time", eq.ec.UnsafeL2Head().Time, "l1_derived", eq.origin, ) @@ -615,8 +629,11 @@ func (eq *EngineQueue) forceNextSafeAttributes(ctx context.Context) error { // suppress the error b/c we want to retry with the next batch from the batch queue // If there is no valid batch the node will eventually force a deposit only block. If // the deposit only block fails, this will return the critical error above. - return nil + // Try to restore to previous known unsafe chain. + eq.ec.SetBackupUnsafeL2Head(eq.ec.BackupUnsafeL2Head(), true) + + return nil default: return NewCriticalError(fmt.Errorf("unknown InsertHeadBlock error type %d: %w", errType, err)) } @@ -694,6 +711,7 @@ func (eq *EngineQueue) Reset(ctx context.Context, _ eth.L1BlockRef, _ eth.System eq.ec.SetSafeHead(safe) eq.ec.SetPendingSafeL2Head(safe) eq.ec.SetFinalizedHead(finalized) + eq.ec.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) eq.safeAttributes = nil eq.ec.ResetBuildingState() eq.finalityData = eq.finalityData[:0] From 79a7e38d89ae0cf16f0aab217ea8e2405b636f8f Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Wed, 10 Jan 2024 16:16:38 +0900 Subject: [PATCH 02/11] op-e2e: Enable custom error while mocking L2 RPC error --- op-e2e/actions/l2_engine.go | 6 +++--- op-e2e/actions/l2_engine_test.go | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/op-e2e/actions/l2_engine.go b/op-e2e/actions/l2_engine.go index 8eaedb3d8f69..910f487fb7c0 100644 --- a/op-e2e/actions/l2_engine.go +++ b/op-e2e/actions/l2_engine.go @@ -174,13 +174,13 @@ func (e *L2Engine) EngineClient(t Testing, cfg *rollup.Config) *sources.EngineCl return l2Cl } -// ActL2RPCFail makes the next L2 RPC request fail -func (e *L2Engine) ActL2RPCFail(t Testing) { +// ActL2RPCFail makes the next L2 RPC request fail with given error +func (e *L2Engine) ActL2RPCFail(t Testing, err error) { if e.failL2RPC != nil { // already set to fail? t.InvalidAction("already set a mock L2 rpc error") return } - e.failL2RPC = errors.New("mock L2 RPC error") + e.failL2RPC = err } // ActL2IncludeTx includes the next transaction from the given address in the block that is being built diff --git a/op-e2e/actions/l2_engine_test.go b/op-e2e/actions/l2_engine_test.go index 267e033e2ee4..3c752cfaa7df 100644 --- a/op-e2e/actions/l2_engine_test.go +++ b/op-e2e/actions/l2_engine_test.go @@ -1,6 +1,7 @@ package actions import ( + "errors" "math/big" "testing" @@ -192,12 +193,13 @@ func TestL2EngineAPIFail(gt *testing.T) { log := testlog.Logger(t, log.LevelDebug) engine := NewL2Engine(t, log, sd.L2Cfg, sd.RollupCfg.Genesis.L1, jwtPath) // mock an RPC failure - engine.ActL2RPCFail(t) + errStr := "mock L2 RPC error" + engine.ActL2RPCFail(t, errors.New(errStr)) // check RPC failure l2Cl, err := sources.NewL2Client(engine.RPCClient(), log, nil, sources.L2ClientDefaultConfig(sd.RollupCfg, false)) require.NoError(t, err) _, err = l2Cl.InfoByLabel(t.Ctx(), eth.Unsafe) - require.ErrorContains(t, err, "mock") + require.ErrorContains(t, err, errStr) head, err := l2Cl.InfoByLabel(t.Ctx(), eth.Unsafe) require.NoError(t, err) require.Equal(gt, sd.L2Cfg.ToBlock().Hash(), head.Hash(), "expecting engine to start at genesis") From 92b9a57ae89053ee45dcf622b227d0a558c36323 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Wed, 10 Jan 2024 16:17:52 +0900 Subject: [PATCH 03/11] op-e2e: Add BackupUnsafe tests --- op-e2e/actions/l2_verifier.go | 4 + op-e2e/actions/sync_test.go | 475 ++++++++++++++++++++++++++++++++++ 2 files changed, 479 insertions(+) diff --git a/op-e2e/actions/l2_verifier.go b/op-e2e/actions/l2_verifier.go index 3c61c628c9ff..5e94d60e1392 100644 --- a/op-e2e/actions/l2_verifier.go +++ b/op-e2e/actions/l2_verifier.go @@ -155,6 +155,10 @@ func (s *L2Verifier) L2Unsafe() eth.L2BlockRef { return s.engine.UnsafeL2Head() } +func (s *L2Verifier) L2BackupUnsafe() eth.L2BlockRef { + return s.derivation.BackupUnsafeL2Head() +} + func (s *L2Verifier) SyncStatus() *eth.SyncStatus { return ð.SyncStatus{ CurrentL1: s.derivation.Origin(), diff --git a/op-e2e/actions/sync_test.go b/op-e2e/actions/sync_test.go index 485306e94207..fb1ee838b65d 100644 --- a/op-e2e/actions/sync_test.go +++ b/op-e2e/actions/sync_test.go @@ -17,6 +17,7 @@ import ( "github.com/ethereum-optimism/optimism/op-service/testutils" "github.com/ethereum/go-ethereum" + "github.com/ethereum/go-ethereum/beacon/engine" "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/types" @@ -163,6 +164,480 @@ func TestUnsafeSync(gt *testing.T) { } } +func TestBackupUnsafe(gt *testing.T) { + t := NewDefaultTesting(gt) + dp := e2eutils.MakeDeployParams(t, defaultRollupTestParams) + minTs := hexutil.Uint64(0) + // Activate Delta hardfork + dp.DeployConfig.L2GenesisDeltaTimeOffset = &minTs + dp.DeployConfig.L2BlockTime = 2 + sd := e2eutils.Setup(t, dp, defaultAlloc) + log := testlog.Logger(t, log.LvlInfo) + _, dp, miner, sequencer, seqEng, verifier, _, batcher := setupReorgTestActors(t, dp, sd, log) + l2Cl := seqEng.EthClient() + seqEngCl, err := sources.NewEngineClient(seqEng.RPCClient(), log, nil, sources.EngineClientDefaultConfig(sd.RollupCfg)) + require.NoError(t, err) + + rng := rand.New(rand.NewSource(1234)) + signer := types.LatestSigner(sd.L2Cfg.Config) + + sequencer.ActL2PipelineFull(t) + verifier.ActL2PipelineFull(t) + + // Create block A1 ~ A5 + for i := 0; i < 5; i++ { + // Build a L2 block + sequencer.ActL2StartBlock(t) + sequencer.ActL2EndBlock(t) + + // Notify new L2 block to verifier by unsafe gossip + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + verifier.ActL2UnsafeGossipReceive(seqHead)(t) + } + + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + // eventually correct hash for A5 + targetUnsafeHeadHash := seqHead.BlockHash + + // only advance unsafe head to A5 + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) + + // Handle unsafe payload + verifier.ActL2PipelineFull(t) + // only advance unsafe head to A5 + require.Equal(t, verifier.L2Unsafe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Number, uint64(0)) + + c, e := compressor.NewRatioCompressor(compressor.Config{ + TargetFrameSize: 128_000, + TargetNumFrames: 1, + ApproxComprRatio: 1, + }) + require.NoError(t, e) + spanBatchBuilder := derive.NewSpanBatchBuilder(sd.RollupCfg.Genesis.L2Time, sd.RollupCfg.L2ChainID) + // Create new span batch channel + channelOut, err := derive.NewChannelOut(derive.SpanBatchType, c, spanBatchBuilder) + require.NoError(t, err) + + for i := uint64(1); i <= sequencer.L2Unsafe().Number; i++ { + block, err := l2Cl.BlockByNumber(t.Ctx(), new(big.Int).SetUint64(i)) + require.NoError(t, err) + if i == 2 { + // Make block B2 as an valid block different with unsafe block + // Alice makes a L2 tx + n, err := l2Cl.PendingNonceAt(t.Ctx(), dp.Addresses.Alice) + require.NoError(t, err) + validTx := types.MustSignNewTx(dp.Secrets.Alice, signer, &types.DynamicFeeTx{ + ChainID: sd.L2Cfg.Config.ChainID, + Nonce: n, + GasTipCap: big.NewInt(2 * params.GWei), + GasFeeCap: new(big.Int).Add(miner.l1Chain.CurrentBlock().BaseFee, big.NewInt(2*params.GWei)), + Gas: params.TxGas, + To: &dp.Addresses.Bob, + Value: e2eutils.Ether(2), + }) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], validTx}, []*types.Header{}) + } + if i == 3 { + // Make block B3 as an invalid block + invalidTx := testutils.RandomTx(rng, big.NewInt(100), signer) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) + } + // Add A1, B2, B3, B4, B5 into the channel + _, err = channelOut.AddBlock(block) + require.NoError(t, err) + } + + // Submit span batch(A1, B2, invalid B3, B4, B5) + batcher.l2ChannelOut = channelOut + batcher.ActL2ChannelClose(t) + batcher.ActL2BatchSubmit(t) + + miner.ActL1StartBlock(12)(t) + miner.ActL1IncludeTx(dp.Addresses.Batcher)(t) + miner.ActL1EndBlock(t) + + // let sequencer process invalid span batch + sequencer.ActL1HeadSignal(t) + // before stepping, make sure backupUnsafe is empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + // pendingSafe must not be advanced as well + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // Preheat engine queue and consume A1 from batch + for i := 0; i < 4; i++ { + sequencer.ActL2PipelineStep(t) + } + // A1 is valid original block so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(1)) + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + // backupUnsafe is still empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // Process B2 + sequencer.ActL2PipelineStep(t) + sequencer.ActL2PipelineStep(t) + // B2 is valid different block, triggering unsafe chain reorg + require.Equal(t, sequencer.L2Unsafe().Number, uint64(2)) + // B2 is valid different block, triggering unsafe block backup + require.Equal(t, targetUnsafeHeadHash, sequencer.L2BackupUnsafe().Hash) + // B2 is valid different block, so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(2)) + // try to process invalid leftovers: B3, B4, B5 + sequencer.ActL2PipelineFull(t) + // backupUnsafe is used because A3 is invalid. Check backupUnsafe is emptied after used + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // check pendingSafe is reset + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // check backupUnsafe is applied + require.Equal(t, sequencer.L2Unsafe().Hash, targetUnsafeHeadHash) + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + // safe head cannot be advanced because batch contained invalid blocks + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) + + // let verifier process invalid span batch + verifier.ActL1HeadSignal(t) + verifier.ActL2PipelineFull(t) + + // safe head cannot be advanced, while unsafe head not changed + require.Equal(t, verifier.L2Unsafe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Number, uint64(0)) + require.Equal(t, verifier.L2Unsafe().Hash, targetUnsafeHeadHash) + + // Build and submit a span batch with A1 ~ A5 + batcher.ActSubmitAll(t) + miner.ActL1StartBlock(12)(t) + miner.ActL1IncludeTx(dp.Addresses.Batcher)(t) + miner.ActL1EndBlock(t) + + // let sequencer process valid span batch + sequencer.ActL1HeadSignal(t) + sequencer.ActL2PipelineFull(t) + + // safe/unsafe head must be advanced + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + require.Equal(t, sequencer.L2Safe().Number, uint64(5)) + require.Equal(t, sequencer.L2Safe().Hash, targetUnsafeHeadHash) + // check backupUnsafe is emptied after consolidation + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // let verifier process valid span batch + verifier.ActL1HeadSignal(t) + verifier.ActL2PipelineFull(t) + + // safe and unsafe head must be advanced + require.Equal(t, verifier.L2Unsafe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Hash, targetUnsafeHeadHash) + // check backupUnsafe is emptied after consolidation + require.Equal(t, eth.L2BlockRef{}, verifier.L2BackupUnsafe()) +} + +func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { + t := NewDefaultTesting(gt) + dp := e2eutils.MakeDeployParams(t, defaultRollupTestParams) + minTs := hexutil.Uint64(0) + // Activate Delta hardfork + dp.DeployConfig.L2GenesisDeltaTimeOffset = &minTs + dp.DeployConfig.L2BlockTime = 2 + sd := e2eutils.Setup(t, dp, defaultAlloc) + log := testlog.Logger(t, log.LvlInfo) + _, dp, miner, sequencer, seqEng, verifier, _, batcher := setupReorgTestActors(t, dp, sd, log) + l2Cl := seqEng.EthClient() + seqEngCl, err := sources.NewEngineClient(seqEng.RPCClient(), log, nil, sources.EngineClientDefaultConfig(sd.RollupCfg)) + require.NoError(t, err) + + rng := rand.New(rand.NewSource(1234)) + signer := types.LatestSigner(sd.L2Cfg.Config) + + sequencer.ActL2PipelineFull(t) + verifier.ActL2PipelineFull(t) + + // Create block A1 ~ A5 + for i := 0; i < 5; i++ { + // Build a L2 block + sequencer.ActL2StartBlock(t) + sequencer.ActL2EndBlock(t) + + // Notify new L2 block to verifier by unsafe gossip + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + verifier.ActL2UnsafeGossipReceive(seqHead)(t) + } + + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + // eventually correct hash for A5 + targetUnsafeHeadHash := seqHead.BlockHash + + // only advance unsafe head to A5 + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) + + // Handle unsafe payload + verifier.ActL2PipelineFull(t) + // only advance unsafe head to A5 + require.Equal(t, verifier.L2Unsafe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Number, uint64(0)) + + c, e := compressor.NewRatioCompressor(compressor.Config{ + TargetFrameSize: 128_000, + TargetNumFrames: 1, + ApproxComprRatio: 1, + }) + require.NoError(t, e) + spanBatchBuilder := derive.NewSpanBatchBuilder(sd.RollupCfg.Genesis.L2Time, sd.RollupCfg.L2ChainID) + // Create new span batch channel + channelOut, err := derive.NewChannelOut(derive.SpanBatchType, c, spanBatchBuilder) + require.NoError(t, err) + + for i := uint64(1); i <= sequencer.L2Unsafe().Number; i++ { + block, err := l2Cl.BlockByNumber(t.Ctx(), new(big.Int).SetUint64(i)) + require.NoError(t, err) + if i == 2 { + // Make block B2 as an valid block different with unsafe block + // Alice makes a L2 tx + n, err := l2Cl.PendingNonceAt(t.Ctx(), dp.Addresses.Alice) + require.NoError(t, err) + validTx := types.MustSignNewTx(dp.Secrets.Alice, signer, &types.DynamicFeeTx{ + ChainID: sd.L2Cfg.Config.ChainID, + Nonce: n, + GasTipCap: big.NewInt(2 * params.GWei), + GasFeeCap: new(big.Int).Add(miner.l1Chain.CurrentBlock().BaseFee, big.NewInt(2*params.GWei)), + Gas: params.TxGas, + To: &dp.Addresses.Bob, + Value: e2eutils.Ether(2), + }) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], validTx}, []*types.Header{}) + } + if i == 3 { + // Make block B3 as an invalid block + invalidTx := testutils.RandomTx(rng, big.NewInt(100), signer) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) + } + // Add A1, B2, B3, B4, B5 into the channel + _, err = channelOut.AddBlock(block) + require.NoError(t, err) + } + + // Submit span batch(A1, B2, invalid B3, B4, B5) + batcher.l2ChannelOut = channelOut + batcher.ActL2ChannelClose(t) + batcher.ActL2BatchSubmit(t) + + miner.ActL1StartBlock(12)(t) + miner.ActL1IncludeTx(dp.Addresses.Batcher)(t) + miner.ActL1EndBlock(t) + + // let sequencer process invalid span batch + sequencer.ActL1HeadSignal(t) + // before stepping, make sure backupUnsafe is empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + // pendingSafe must not be advanced as well + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // Preheat engine queue and consume A1 from batch + for i := 0; i < 4; i++ { + sequencer.ActL2PipelineStep(t) + } + // A1 is valid original block so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(1)) + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + // backupUnsafe is still empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // Process B2 + sequencer.ActL2PipelineStep(t) + sequencer.ActL2PipelineStep(t) + // B2 is valid different block, triggering unsafe chain reorg + require.Equal(t, sequencer.L2Unsafe().Number, uint64(2)) + // B2 is valid different block, triggering unsafe block backup + require.Equal(t, targetUnsafeHeadHash, sequencer.L2BackupUnsafe().Hash) + // B2 is valid different block, so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(2)) + + // B3 is invalid block + // NextAttributes is called + sequencer.ActL2PipelineStep(t) + // forceNextSafeAttributes is called + sequencer.ActL2PipelineStep(t) + // mock forkChoiceUpdate error while restoring previous unsafe chain using backupUnsafe. + seqEng.ActL2RPCFail(t, eth.InputError{Inner: errors.New("mock L2 RPC error"), Code: eth.InvalidForkchoiceState}) + + // tryBackupUnsafeReorg is called + sequencer.ActL2PipelineStep(t) + + // try to process invalid leftovers: B4, B5 + sequencer.ActL2PipelineFull(t) + + // backupUnsafe is not used because forkChoiceUpdate returned an error. + // Check backupUnsafe is emptied. + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // check pendingSafe is reset + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // unsafe head is not restored due to forkchoiceUpdate error in tryBackupUnsafeReorg + require.Equal(t, sequencer.L2Unsafe().Number, uint64(2)) + // safe head cannot be advanced because batch contained invalid blocks + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) +} + +func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) { + t := NewDefaultTesting(gt) + dp := e2eutils.MakeDeployParams(t, defaultRollupTestParams) + minTs := hexutil.Uint64(0) + // Activate Delta hardfork + dp.DeployConfig.L2GenesisDeltaTimeOffset = &minTs + dp.DeployConfig.L2BlockTime = 2 + sd := e2eutils.Setup(t, dp, defaultAlloc) + log := testlog.Logger(t, log.LvlInfo) + _, dp, miner, sequencer, seqEng, verifier, _, batcher := setupReorgTestActors(t, dp, sd, log) + l2Cl := seqEng.EthClient() + seqEngCl, err := sources.NewEngineClient(seqEng.RPCClient(), log, nil, sources.EngineClientDefaultConfig(sd.RollupCfg)) + require.NoError(t, err) + + rng := rand.New(rand.NewSource(1234)) + signer := types.LatestSigner(sd.L2Cfg.Config) + + sequencer.ActL2PipelineFull(t) + verifier.ActL2PipelineFull(t) + + // Create block A1 ~ A5 + for i := 0; i < 5; i++ { + // Build a L2 block + sequencer.ActL2StartBlock(t) + sequencer.ActL2EndBlock(t) + + // Notify new L2 block to verifier by unsafe gossip + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + verifier.ActL2UnsafeGossipReceive(seqHead)(t) + } + + seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) + require.NoError(t, err) + // eventually correct hash for A5 + targetUnsafeHeadHash := seqHead.BlockHash + + // only advance unsafe head to A5 + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) + + // Handle unsafe payload + verifier.ActL2PipelineFull(t) + // only advance unsafe head to A5 + require.Equal(t, verifier.L2Unsafe().Number, uint64(5)) + require.Equal(t, verifier.L2Safe().Number, uint64(0)) + + c, e := compressor.NewRatioCompressor(compressor.Config{ + TargetFrameSize: 128_000, + TargetNumFrames: 1, + ApproxComprRatio: 1, + }) + require.NoError(t, e) + spanBatchBuilder := derive.NewSpanBatchBuilder(sd.RollupCfg.Genesis.L2Time, sd.RollupCfg.L2ChainID) + // Create new span batch channel + channelOut, err := derive.NewChannelOut(derive.SpanBatchType, c, spanBatchBuilder) + require.NoError(t, err) + + for i := uint64(1); i <= sequencer.L2Unsafe().Number; i++ { + block, err := l2Cl.BlockByNumber(t.Ctx(), new(big.Int).SetUint64(i)) + require.NoError(t, err) + if i == 2 { + // Make block B2 as an valid block different with unsafe block + // Alice makes a L2 tx + n, err := l2Cl.PendingNonceAt(t.Ctx(), dp.Addresses.Alice) + require.NoError(t, err) + validTx := types.MustSignNewTx(dp.Secrets.Alice, signer, &types.DynamicFeeTx{ + ChainID: sd.L2Cfg.Config.ChainID, + Nonce: n, + GasTipCap: big.NewInt(2 * params.GWei), + GasFeeCap: new(big.Int).Add(miner.l1Chain.CurrentBlock().BaseFee, big.NewInt(2*params.GWei)), + Gas: params.TxGas, + To: &dp.Addresses.Bob, + Value: e2eutils.Ether(2), + }) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], validTx}, []*types.Header{}) + } + if i == 3 { + // Make block B3 as an invalid block + invalidTx := testutils.RandomTx(rng, big.NewInt(100), signer) + block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) + } + // Add A1, B2, B3, B4, B5 into the channel + _, err = channelOut.AddBlock(block) + require.NoError(t, err) + } + + // Submit span batch(A1, B2, invalid B3, B4, B5) + batcher.l2ChannelOut = channelOut + batcher.ActL2ChannelClose(t) + batcher.ActL2BatchSubmit(t) + + miner.ActL1StartBlock(12)(t) + miner.ActL1IncludeTx(dp.Addresses.Batcher)(t) + miner.ActL1EndBlock(t) + + // let sequencer process invalid span batch + sequencer.ActL1HeadSignal(t) + // before stepping, make sure backupUnsafe is empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + // pendingSafe must not be advanced as well + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // Preheat engine queue and consume A1 from batch + for i := 0; i < 4; i++ { + sequencer.ActL2PipelineStep(t) + } + // A1 is valid original block so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(1)) + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + // backupUnsafe is still empty + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // Process B2 + sequencer.ActL2PipelineStep(t) + sequencer.ActL2PipelineStep(t) + // B2 is valid different block, triggering unsafe chain reorg + require.Equal(t, sequencer.L2Unsafe().Number, uint64(2)) + // B2 is valid different block, triggering unsafe block backup + require.Equal(t, targetUnsafeHeadHash, sequencer.L2BackupUnsafe().Hash) + // B2 is valid different block, so pendingSafe is advanced + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(2)) + + // B3 is invalid block + // NextAttributes is called + sequencer.ActL2PipelineStep(t) + // forceNextSafeAttributes is called + sequencer.ActL2PipelineStep(t) + + serverErrCnt := 2 + for i := 0; i < serverErrCnt; i++ { + // mock forkChoiceUpdate failure while restoring previous unsafe chain using backupUnsafe. + seqEng.ActL2RPCFail(t, engine.GenericServerError) + // tryBackupUnsafeReorg is called - forkChoiceUpdate returns GenericServerError so retry + sequencer.ActL2PipelineStep(t) + // backupUnsafeHead not emptied yet + require.Equal(t, targetUnsafeHeadHash, sequencer.L2BackupUnsafe().Hash) + } + // now forkchoice succeeds + // try to process invalid leftovers: B4, B5 + sequencer.ActL2PipelineFull(t) + + // backupUnsafe is used because forkChoiceUpdate eventually succeeded. + // Check backupUnsafe is emptied. + require.Equal(t, eth.L2BlockRef{}, sequencer.L2BackupUnsafe()) + + // check pendingSafe is reset + require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) + // check backupUnsafe is applied + require.Equal(t, sequencer.L2Unsafe().Hash, targetUnsafeHeadHash) + require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) + // safe head cannot be advanced because batch contained invalid blocks + require.Equal(t, sequencer.L2Safe().Number, uint64(0)) +} + // TestELSync tests that a verifier will have the EL import the full chain from the sequencer // when passed a single unsafe block. op-geth can either snap sync or full sync here. func TestELSync(gt *testing.T) { From a592c2adf75b55ec191970af6621ab49418e45b9 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Mon, 15 Jan 2024 15:45:06 +0900 Subject: [PATCH 04/11] op-node: Fix comment --- op-e2e/actions/sync_test.go | 6 +++--- op-node/rollup/derive/engine_controller.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/op-e2e/actions/sync_test.go b/op-e2e/actions/sync_test.go index fb1ee838b65d..ef4918b78d9f 100644 --- a/op-e2e/actions/sync_test.go +++ b/op-e2e/actions/sync_test.go @@ -466,7 +466,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { // mock forkChoiceUpdate error while restoring previous unsafe chain using backupUnsafe. seqEng.ActL2RPCFail(t, eth.InputError{Inner: errors.New("mock L2 RPC error"), Code: eth.InvalidForkchoiceState}) - // tryBackupUnsafeReorg is called + // TryBackupUnsafeReorg is called sequencer.ActL2PipelineStep(t) // try to process invalid leftovers: B4, B5 @@ -478,7 +478,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { // check pendingSafe is reset require.Equal(t, sequencer.L2PendingSafe().Number, uint64(0)) - // unsafe head is not restored due to forkchoiceUpdate error in tryBackupUnsafeReorg + // unsafe head is not restored due to forkchoiceUpdate error in TryBackupUnsafeReorg require.Equal(t, sequencer.L2Unsafe().Number, uint64(2)) // safe head cannot be advanced because batch contained invalid blocks require.Equal(t, sequencer.L2Safe().Number, uint64(0)) @@ -616,7 +616,7 @@ func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) { for i := 0; i < serverErrCnt; i++ { // mock forkChoiceUpdate failure while restoring previous unsafe chain using backupUnsafe. seqEng.ActL2RPCFail(t, engine.GenericServerError) - // tryBackupUnsafeReorg is called - forkChoiceUpdate returns GenericServerError so retry + // TryBackupUnsafeReorg is called - forkChoiceUpdate returns GenericServerError so retry sequencer.ActL2PipelineStep(t) // backupUnsafeHead not emptied yet require.Equal(t, targetUnsafeHeadHash, sequencer.L2BackupUnsafe().Hash) diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index bfa57b0e9cb2..9bf6417941ef 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -394,7 +394,7 @@ func (e *EngineController) InsertUnsafePayload(ctx context.Context, envelope *et return nil } -// tryBackupUnsafeReorg attempts to reorg(restore) unsafe head to backupUnsafeHead. +// TryBackupUnsafeReorg attempts to reorg(restore) unsafe head to backupUnsafeHead. // If succeeds, update current forkchoice state to the rollup node. func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { if !e.needFCUCallForBackupUnsafeReorg { From 8f8784aa6ab7cf36e4ff3e47e65f8f23c4caaf88 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Mon, 15 Jan 2024 15:45:40 +0900 Subject: [PATCH 05/11] op-node: Follow convention for backup unsafe head metric --- op-node/rollup/derive/engine_controller.go | 2 +- op-node/rollup/derive/engine_queue.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index 9bf6417941ef..e5f3e04afb89 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -154,7 +154,7 @@ func (e *EngineController) SetUnsafeHead(r eth.L2BlockRef) { // SetBackupUnsafeL2Head implements LocalEngineControl. func (e *EngineController) SetBackupUnsafeL2Head(r eth.L2BlockRef, triggerReorg bool) { - e.metrics.RecordL2Ref("l2_backupUnsafe", r) + e.metrics.RecordL2Ref("l2_backup_unsafe", r) e.backupUnsafeHead = r e.needFCUCallForBackupUnsafeReorg = triggerReorg } diff --git a/op-node/rollup/derive/engine_queue.go b/op-node/rollup/derive/engine_queue.go index 89901754d1ba..1023b3133dc2 100644 --- a/op-node/rollup/derive/engine_queue.go +++ b/op-node/rollup/derive/engine_queue.go @@ -464,7 +464,7 @@ func (eq *EngineQueue) logSyncProgress(reason string) { "l2_safe", eq.ec.SafeL2Head(), "l2_pending_safe", eq.ec.PendingSafeL2Head(), "l2_unsafe", eq.ec.UnsafeL2Head(), - "l2_backupUnsafe", eq.ec.BackupUnsafeL2Head(), + "l2_backup_unsafe", eq.ec.BackupUnsafeL2Head(), "l2_time", eq.ec.UnsafeL2Head().Time, "l1_derived", eq.origin, ) From c0f05d61569b02d1ec58b9591cca804b0d0aafcd Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Mon, 22 Jan 2024 00:33:08 +0900 Subject: [PATCH 06/11] op-e2e: Fix BackupUnsafe tests --- op-e2e/actions/l2_verifier.go | 2 +- op-e2e/actions/sync_test.go | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/op-e2e/actions/l2_verifier.go b/op-e2e/actions/l2_verifier.go index 5e94d60e1392..e98c0758cd6a 100644 --- a/op-e2e/actions/l2_verifier.go +++ b/op-e2e/actions/l2_verifier.go @@ -156,7 +156,7 @@ func (s *L2Verifier) L2Unsafe() eth.L2BlockRef { } func (s *L2Verifier) L2BackupUnsafe() eth.L2BlockRef { - return s.derivation.BackupUnsafeL2Head() + return s.engine.BackupUnsafeL2Head() } func (s *L2Verifier) SyncStatus() *eth.SyncStatus { diff --git a/op-e2e/actions/sync_test.go b/op-e2e/actions/sync_test.go index ef4918b78d9f..13c5839ce4c6 100644 --- a/op-e2e/actions/sync_test.go +++ b/op-e2e/actions/sync_test.go @@ -9,6 +9,7 @@ import ( "github.com/ethereum-optimism/optimism/op-batcher/compressor" "github.com/ethereum-optimism/optimism/op-e2e/e2eutils" + "github.com/ethereum-optimism/optimism/op-node/rollup" "github.com/ethereum-optimism/optimism/op-node/rollup/derive" "github.com/ethereum-optimism/optimism/op-node/rollup/sync" "github.com/ethereum-optimism/optimism/op-service/eth" @@ -26,6 +27,10 @@ import ( "github.com/stretchr/testify/require" ) +var ( + rollupCfg rollup.Config +) + // TestSyncBatchType run each sync test case in singular batch mode and span batch mode. func TestSyncBatchType(t *testing.T) { tests := []struct { @@ -199,7 +204,7 @@ func TestBackupUnsafe(gt *testing.T) { seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) require.NoError(t, err) // eventually correct hash for A5 - targetUnsafeHeadHash := seqHead.BlockHash + targetUnsafeHeadHash := seqHead.ExecutionPayload.BlockHash // only advance unsafe head to A5 require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) @@ -247,7 +252,7 @@ func TestBackupUnsafe(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(block) + _, err = channelOut.AddBlock(&rollupCfg, block) require.NoError(t, err) } @@ -371,7 +376,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) require.NoError(t, err) // eventually correct hash for A5 - targetUnsafeHeadHash := seqHead.BlockHash + targetUnsafeHeadHash := seqHead.ExecutionPayload.BlockHash // only advance unsafe head to A5 require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) @@ -419,7 +424,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(block) + _, err = channelOut.AddBlock(&rollupCfg, block) require.NoError(t, err) } @@ -519,7 +524,7 @@ func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) { seqHead, err := seqEngCl.PayloadByLabel(t.Ctx(), eth.Unsafe) require.NoError(t, err) // eventually correct hash for A5 - targetUnsafeHeadHash := seqHead.BlockHash + targetUnsafeHeadHash := seqHead.ExecutionPayload.BlockHash // only advance unsafe head to A5 require.Equal(t, sequencer.L2Unsafe().Number, uint64(5)) @@ -567,7 +572,7 @@ func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(block) + _, err = channelOut.AddBlock(&rollupCfg, block) require.NoError(t, err) } From b387b4b385452737f9471d46726fe981595f97a4 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Tue, 30 Jan 2024 14:55:36 +0900 Subject: [PATCH 07/11] op-node: Tailered/Consistent log message --- op-node/rollup/derive/engine_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index e5f3e04afb89..8143974919a2 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -299,7 +299,7 @@ func (e *EngineController) TryUpdateEngine(ctx context.Context) error { return errNoFCUNeeded } if e.IsEngineSyncing() { - e.log.Warn("Attempting to update forkchoice state while EL sync.") + e.log.Warn("Attempting to update forkchoice state while EL syncing") } fc := eth.ForkchoiceState{ HeadBlockHash: e.unsafeHead.Hash, @@ -402,7 +402,7 @@ func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { } // This method must be never called when EL sync. If EL sync is in progress, early return. if e.IsEngineSyncing() { - e.log.Warn("Attempting to update forkchoice state while EL sync.") + e.log.Warn("Attempting to unsafe reorg using backupUnsafe while EL syncing") return errNoBackupUnsafeReorgNeeded } if e.BackupUnsafeL2Head() == (eth.L2BlockRef{}) { // sanity check backupUnsafeHead is there From bffdeec5da4eafa8ef0991dca8ee38704fc056e9 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Tue, 30 Jan 2024 15:01:09 +0900 Subject: [PATCH 08/11] op-e2e: Better coding style --- op-e2e/actions/l2_engine_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/op-e2e/actions/l2_engine_test.go b/op-e2e/actions/l2_engine_test.go index 3c752cfaa7df..36e146c282a6 100644 --- a/op-e2e/actions/l2_engine_test.go +++ b/op-e2e/actions/l2_engine_test.go @@ -193,13 +193,13 @@ func TestL2EngineAPIFail(gt *testing.T) { log := testlog.Logger(t, log.LevelDebug) engine := NewL2Engine(t, log, sd.L2Cfg, sd.RollupCfg.Genesis.L1, jwtPath) // mock an RPC failure - errStr := "mock L2 RPC error" - engine.ActL2RPCFail(t, errors.New(errStr)) + mockErr := errors.New("mock L2 RPC error") + engine.ActL2RPCFail(t, mockErr) // check RPC failure l2Cl, err := sources.NewL2Client(engine.RPCClient(), log, nil, sources.L2ClientDefaultConfig(sd.RollupCfg, false)) require.NoError(t, err) _, err = l2Cl.InfoByLabel(t.Ctx(), eth.Unsafe) - require.ErrorContains(t, err, errStr) + require.ErrorIs(t, err, mockErr) head, err := l2Cl.InfoByLabel(t.Ctx(), eth.Unsafe) require.NoError(t, err) require.Equal(gt, sd.L2Cfg.ToBlock().Hash(), head.Hash(), "expecting engine to start at genesis") From 3176f3dbbb24698e2c4e668c09e66d0e55d4d1ac Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Tue, 30 Jan 2024 16:22:58 +0900 Subject: [PATCH 09/11] op-node: Refactor code for trying backupUnsafe reorg --- op-node/rollup/derive/engine_controller.go | 33 ++++++++++++++-------- op-node/rollup/derive/engine_queue.go | 8 +++--- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index 8143974919a2..d9103fcd5cbc 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -33,7 +33,6 @@ const ( ) var errNoFCUNeeded = errors.New("no FCU call was needed") -var errNoBackupUnsafeReorgNeeded = errors.New("no BackupUnsafeReorg was needed") var _ EngineControl = (*EngineController)(nil) var _ LocalEngineControl = (*EngineController)(nil) @@ -394,21 +393,31 @@ func (e *EngineController) InsertUnsafePayload(ctx context.Context, envelope *et return nil } -// TryBackupUnsafeReorg attempts to reorg(restore) unsafe head to backupUnsafeHead. -// If succeeds, update current forkchoice state to the rollup node. -func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { +// shouldTryBackupUnsafeReorg checks reorging(restoring) unsafe head to backupUnsafeHead is needed. +// Returns boolean which decides to trigger FCU. +func (e *EngineController) shouldTryBackupUnsafeReorg() bool { if !e.needFCUCallForBackupUnsafeReorg { - return errNoBackupUnsafeReorgNeeded + return false } // This method must be never called when EL sync. If EL sync is in progress, early return. if e.IsEngineSyncing() { e.log.Warn("Attempting to unsafe reorg using backupUnsafe while EL syncing") - return errNoBackupUnsafeReorgNeeded + return false } if e.BackupUnsafeL2Head() == (eth.L2BlockRef{}) { // sanity check backupUnsafeHead is there e.log.Warn("Attempting to unsafe reorg using backupUnsafe even though it is empty") e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) - return errNoBackupUnsafeReorgNeeded + return false + } + return true +} + +// TryBackupUnsafeReorg attempts to reorg(restore) unsafe head to backupUnsafeHead. +// If succeeds, update current forkchoice state to the rollup node. +func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) (bool, error) { + if !e.shouldTryBackupUnsafeReorg() { + // Do not need to perform FCU. + return false, nil } // Only try FCU once because execution engine may forgot backupUnsafeHead // or backupUnsafeHead is not part of the chain. @@ -428,15 +437,15 @@ func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) switch inputErr.Code { case eth.InvalidForkchoiceState: - return NewResetError(fmt.Errorf("forkchoice update was inconsistent with engine, need reset to resolve: %w", inputErr.Unwrap())) + return true, NewResetError(fmt.Errorf("forkchoice update was inconsistent with engine, need reset to resolve: %w", inputErr.Unwrap())) default: - return NewTemporaryError(fmt.Errorf("unexpected error code in forkchoice-updated response: %w", err)) + return true, NewTemporaryError(fmt.Errorf("unexpected error code in forkchoice-updated response: %w", err)) } } else { // Retry when forkChoiceUpdate returns non-input error. // Do not reset backupUnsafeHead because it will be used again. e.needFCUCallForBackupUnsafeReorg = true - return NewTemporaryError(fmt.Errorf("failed to sync forkchoice with engine: %w", err)) + return true, NewTemporaryError(fmt.Errorf("failed to sync forkchoice with engine: %w", err)) } } if fcRes.PayloadStatus.Status == eth.ExecutionValid { @@ -444,11 +453,11 @@ func (e *EngineController) TryBackupUnsafeReorg(ctx context.Context) error { e.log.Info("successfully reorged unsafe head using backupUnsafe", "unsafe", e.backupUnsafeHead.ID()) e.SetUnsafeHead(e.BackupUnsafeL2Head()) e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) - return nil + return true, nil } e.SetBackupUnsafeL2Head(eth.L2BlockRef{}, false) // Execution engine could not reorg back to previous unsafe head. - return NewTemporaryError(fmt.Errorf("cannot restore unsafe chain using backupUnsafe: err: %w", + return true, NewTemporaryError(fmt.Errorf("cannot restore unsafe chain using backupUnsafe: err: %w", eth.ForkchoiceUpdateErr(fcRes.PayloadStatus))) } diff --git a/op-node/rollup/derive/engine_queue.go b/op-node/rollup/derive/engine_queue.go index 1023b3133dc2..4179e851cea1 100644 --- a/op-node/rollup/derive/engine_queue.go +++ b/op-node/rollup/derive/engine_queue.go @@ -82,7 +82,7 @@ type LocalEngineControl interface { ResetBuildingState() IsEngineSyncing() bool TryUpdateEngine(ctx context.Context) error - TryBackupUnsafeReorg(ctx context.Context) error + TryBackupUnsafeReorg(ctx context.Context) (bool, error) InsertUnsafePayload(ctx context.Context, payload *eth.ExecutionPayloadEnvelope, ref eth.L2BlockRef) error PendingSafeL2Head() eth.L2BlockRef @@ -270,9 +270,9 @@ func (eq *EngineQueue) isEngineSyncing() bool { func (eq *EngineQueue) Step(ctx context.Context) error { // If we don't need to call FCU to restore unsafeHead using backupUnsafe, keep going b/c - // this was a no-op(except correcting invalid state when backupUnsafe is empty but reorg triggered). - // If we needed to perform a network call, then we should yield even if we did not encounter an error. - if err := eq.ec.TryBackupUnsafeReorg(ctx); !errors.Is(err, errNoBackupUnsafeReorgNeeded) { + // this was a no-op(except correcting invalid state when backupUnsafe is empty but TryBackupUnsafeReorg called). + if FCUcalled, err := eq.ec.TryBackupUnsafeReorg(ctx); FCUcalled { + // If we needed to perform a network call, then we should yield even if we did not encounter an error. return err } // If we don't need to call FCU, keep going b/c this was a no-op. If we needed to From da64a2b5c7b64c99d7407cee829ddf4343d2fe67 Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Fri, 1 Mar 2024 11:19:47 -0700 Subject: [PATCH 10/11] op-node: Better variable name --- op-node/rollup/derive/engine_queue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/op-node/rollup/derive/engine_queue.go b/op-node/rollup/derive/engine_queue.go index 4179e851cea1..f2c15d5336d5 100644 --- a/op-node/rollup/derive/engine_queue.go +++ b/op-node/rollup/derive/engine_queue.go @@ -271,7 +271,7 @@ func (eq *EngineQueue) isEngineSyncing() bool { func (eq *EngineQueue) Step(ctx context.Context) error { // If we don't need to call FCU to restore unsafeHead using backupUnsafe, keep going b/c // this was a no-op(except correcting invalid state when backupUnsafe is empty but TryBackupUnsafeReorg called). - if FCUcalled, err := eq.ec.TryBackupUnsafeReorg(ctx); FCUcalled { + if fcuCalled, err := eq.ec.TryBackupUnsafeReorg(ctx); fcuCalled { // If we needed to perform a network call, then we should yield even if we did not encounter an error. return err } From d86a8d4a79e7197dc23a0a9e339f385aa9711cac Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Fri, 1 Mar 2024 11:24:12 -0700 Subject: [PATCH 11/11] op-e2e: Remove global variable Test are run concurrently so accessing shared global object is problematic --- op-e2e/actions/sync_test.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/op-e2e/actions/sync_test.go b/op-e2e/actions/sync_test.go index 13c5839ce4c6..309b10489ec2 100644 --- a/op-e2e/actions/sync_test.go +++ b/op-e2e/actions/sync_test.go @@ -9,7 +9,6 @@ import ( "github.com/ethereum-optimism/optimism/op-batcher/compressor" "github.com/ethereum-optimism/optimism/op-e2e/e2eutils" - "github.com/ethereum-optimism/optimism/op-node/rollup" "github.com/ethereum-optimism/optimism/op-node/rollup/derive" "github.com/ethereum-optimism/optimism/op-node/rollup/sync" "github.com/ethereum-optimism/optimism/op-service/eth" @@ -27,10 +26,6 @@ import ( "github.com/stretchr/testify/require" ) -var ( - rollupCfg rollup.Config -) - // TestSyncBatchType run each sync test case in singular batch mode and span batch mode. func TestSyncBatchType(t *testing.T) { tests := []struct { @@ -252,7 +247,7 @@ func TestBackupUnsafe(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(&rollupCfg, block) + _, err = channelOut.AddBlock(sd.RollupCfg, block) require.NoError(t, err) } @@ -424,7 +419,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(&rollupCfg, block) + _, err = channelOut.AddBlock(sd.RollupCfg, block) require.NoError(t, err) } @@ -572,7 +567,7 @@ func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) { block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{}) } // Add A1, B2, B3, B4, B5 into the channel - _, err = channelOut.AddBlock(&rollupCfg, block) + _, err = channelOut.AddBlock(sd.RollupCfg, block) require.NoError(t, err) }