From c4778d8eec00917b601662066ec3e9da8a76e7fa Mon Sep 17 00:00:00 2001 From: pcw109550 Date: Tue, 30 Jan 2024 16:22:58 +0900 Subject: [PATCH] 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 8287d7a0b6f5..cc866667eaea 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -32,7 +32,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) @@ -386,21 +385,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. @@ -420,15 +429,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 { @@ -436,11 +445,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 5d92f59878cd..0d30a481a332 100644 --- a/op-node/rollup/derive/engine_queue.go +++ b/op-node/rollup/derive/engine_queue.go @@ -81,7 +81,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 @@ -251,9 +251,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