Fold EngDeriver into EngController

op-e2e/actions/helpers/l2_verifier.go

@@ -18,6 +18,7 @@ import (
 	gnode "github.com/ethereum/go-ethereum/node"
 	"github.com/ethereum/go-ethereum/rpc"
 
+	opnodemetrics "github.com/ethereum-optimism/optimism/op-node/metrics"
 	"github.com/ethereum-optimism/optimism/op-node/node"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/rollup/attributes"
@@ -144,8 +145,7 @@ func NewL2Verifier(t Testing, log log.Logger, l1 derive.L1Fetcher,
 	}
 
 	metrics := &testutils.TestDerivationMetrics{}
-	ec := engine.NewEngineController(eng, log, metrics, cfg, syncCfg,
-		sys.Register("engine-controller", nil, opts))
+	ec := engine.NewEngineController(ctx, eng, log, opnodemetrics.NoopMetrics, cfg, syncCfg, sys.Register("engine-controller", nil, opts))
 
 	sys.Register("engine-reset",
 		engine.NewEngineResetDeriver(ctx, log, cfg, l1, eng, syncCfg), opts)
@@ -161,7 +161,7 @@ func NewL2Verifier(t Testing, log log.Logger, l1 derive.L1Fetcher,
 	}
 	sys.Register("finalizer", finalizer, opts)
 
-	attrHandler := attributes.NewAttributesHandler(log, cfg, ctx, eng)
+	attrHandler := attributes.NewAttributesHandler(log, cfg, ctx, eng, ec)
 	sys.Register("attributes-handler", attrHandler, opts)
 
 	indexingMode := interopSys != nil
@@ -192,9 +192,7 @@ func NewL2Verifier(t Testing, log log.Logger, l1 derive.L1Fetcher,
 	//  Couple EngDeriver and NewAttributesHandler for event refactoring
 	ec.SyncDeriver = syncDeriver
 	sys.Register("sync", syncDeriver, opts)
-	engDeriver := engine.NewEngDeriver(log, ctx, cfg, metrics, ec)
-	sys.Register("engine", engDeriver, opts)
-	attrHandler.EngDeriver = engDeriver
+	sys.Register("engine", ec, opts)
 
 	rollupNode := &L2Verifier{
 		eventSys:          sys,

op-node/rollup/attributes/attributes.go

@@ -17,6 +17,15 @@ import (
 	"github.com/ethereum-optimism/optimism/op-service/event"
 )
 
+// EngineController provides direct calls into the EngineController that
+// external components can use instead of emitting events.
+type EngineController interface {
+	// TryUpdatePendingSafe updates the pending safe head if the new reference is newer
+	TryUpdatePendingSafe(ctx context.Context, ref eth.L2BlockRef, concluding bool, source eth.L1BlockRef)
+	// TryUpdateLocalSafe updates the local safe head if the new reference is newer and concluding
+	TryUpdateLocalSafe(ctx context.Context, ref eth.L2BlockRef, concluding bool, source eth.L1BlockRef)
+}
+
 type L2 interface {
 	PayloadByNumber(context.Context, uint64) (*eth.ExecutionPayloadEnvelope, error)
 }
@@ -37,16 +46,20 @@ type AttributesHandler struct {
 	attributes     *derive.AttributesWithParent
 	sentAttributes bool
 
-	EngDeriver engine.EngDeriverInterface
+	engineController EngineController
 }
 
-func NewAttributesHandler(log log.Logger, cfg *rollup.Config, ctx context.Context, l2 L2) *AttributesHandler {
+func NewAttributesHandler(log log.Logger, cfg *rollup.Config, ctx context.Context, l2 L2, engController EngineController) *AttributesHandler {
+	if engController == nil {
+		panic("engController cannot be nil")
+	}
 	return &AttributesHandler{
-		log:        log,
-		cfg:        cfg,
-		ctx:        ctx,
-		l2:         l2,
-		attributes: nil,
+		log:              log,
+		cfg:              cfg,
+		ctx:              ctx,
+		l2:               l2,
+		engineController: engController,
+		attributes:       nil,
 	}
 }
 
@@ -202,8 +215,8 @@ func (eq *AttributesHandler) consolidateNextSafeAttributes(attributes *derive.At
 			eq.log.Error("Failed to compute block-ref from execution payload")
 			return
 		}
-		eq.EngDeriver.TryUpdatePendingSafe(eq.ctx, ref, attributes.Concluding, attributes.DerivedFrom)
-		eq.EngDeriver.TryUpdateLocalSafe(eq.ctx, ref, attributes.Concluding, attributes.DerivedFrom)
+		eq.engineController.TryUpdatePendingSafe(eq.ctx, ref, attributes.Concluding, attributes.DerivedFrom)
+		eq.engineController.TryUpdateLocalSafe(eq.ctx, ref, attributes.Concluding, attributes.DerivedFrom)
 	}
 
 	// unsafe head stays the same, we did not reorg the chain.

op-node/rollup/attributes/attributes_test.go

@@ -170,10 +170,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		emitter.ExpectOnce(derive.ConfirmReceivedAttributesEvent{})
 		emitter.ExpectOnce(engine.PendingSafeRequestEvent{})
@@ -194,10 +193,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		emitter.ExpectOnce(derive.ConfirmReceivedAttributesEvent{})
 		emitter.ExpectOnce(engine.PendingSafeRequestEvent{})
@@ -221,10 +219,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		emitter.ExpectOnce(derive.ConfirmReceivedAttributesEvent{})
 		emitter.ExpectOnce(engine.PendingSafeRequestEvent{})
@@ -249,10 +246,9 @@ func TestAttributesHandler(t *testing.T) {
 			logger := testlog.Logger(t, log.LevelInfo)
 			l2 := &testutils.MockL2Client{}
 			emitter := &testutils.MockEmitter{}
-			engDeriver := &testutils.MockEngDeriver{}
-			ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+			engDeriver := &MockEngineController{}
+			ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 			ah.AttachEmitter(emitter)
-			ah.EngDeriver = engDeriver
 
 			// attrA1Alt does not match block A1, so will cause force-reorg.
 			emitter.ExpectOnce(derive.ConfirmReceivedAttributesEvent{})
@@ -288,10 +284,9 @@ func TestAttributesHandler(t *testing.T) {
 				logger := testlog.Logger(t, log.LevelInfo)
 				l2 := &testutils.MockL2Client{}
 				emitter := &testutils.MockEmitter{}
-				engDeriver := &testutils.MockEngDeriver{}
-				ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+				engDeriver := &MockEngineController{}
+				ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 				ah.AttachEmitter(emitter)
-				ah.EngDeriver = engDeriver
 
 				attr := &derive.AttributesWithParent{
 					Attributes:  attrA1.Attributes, // attributes will match, passing consolidation
@@ -344,10 +339,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		emitter.ExpectOnce(derive.ConfirmReceivedAttributesEvent{})
 		emitter.ExpectOnce(engine.PendingSafeRequestEvent{})
@@ -383,10 +377,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		emitter.ExpectOnceType("ResetEvent")
 		ah.OnEvent(context.Background(), engine.PendingSafeUpdateEvent{
@@ -401,10 +394,9 @@ func TestAttributesHandler(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelInfo)
 		l2 := &testutils.MockL2Client{}
 		emitter := &testutils.MockEmitter{}
-		engDeriver := &testutils.MockEngDeriver{}
-		ah := NewAttributesHandler(logger, cfg, context.Background(), l2)
+		engDeriver := &MockEngineController{}
+		ah := NewAttributesHandler(logger, cfg, context.Background(), l2, engDeriver)
 		ah.AttachEmitter(emitter)
-		ah.EngDeriver = engDeriver
 
 		// If there are no attributes, we expect the pipeline to be requested to generate attributes.
 		emitter.ExpectOnce(derive.PipelineStepEvent{PendingSafe: refA1})

op-node/rollup/attributes/testutils.go

@@ -0,0 +1,22 @@
+package attributes
+
+import (
+	"context"
+
+	"github.com/ethereum-optimism/optimism/op-service/eth"
+	"github.com/stretchr/testify/mock"
+)
+
+type MockEngineController struct {
+	mock.Mock
+}
+
+var _ EngineController = (*MockEngineController)(nil)
+
+func (m *MockEngineController) TryUpdatePendingSafe(ctx context.Context, ref eth.L2BlockRef, concluding bool, source eth.L1BlockRef) {
+	m.Mock.MethodCalled("TryUpdatePendingSafe", ctx, ref, concluding, source)
+}
+
+func (m *MockEngineController) TryUpdateLocalSafe(ctx context.Context, ref eth.L2BlockRef, concluding bool, source eth.L1BlockRef) {
+	m.Mock.MethodCalled("TryUpdateLocalSafe", ctx, ref, concluding, source)
+}

op-node/rollup/driver/driver.go

@@ -7,6 +7,7 @@ import (
 	"github.com/ethereum/go-ethereum/log"
 
 	altda "github.com/ethereum-optimism/optimism/op-alt-da"
+	opnodemetrics "github.com/ethereum-optimism/optimism/op-node/metrics"
 	"github.com/ethereum-optimism/optimism/op-node/metrics/metered"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/rollup/async"
@@ -51,7 +52,7 @@ type Metrics interface {
 
 	RecordL1ReorgDepth(d uint64)
 
-	engine.Metrics
+	opnodemetrics.Metricer
 	metered.L1FetcherMetrics
 	event.Metrics
 	sequencing.Metrics
@@ -190,8 +191,7 @@ func NewDriver(
 	l1 = metered.NewMeteredL1Fetcher(l1Tracker, metrics)
 	verifConfDepth := confdepth.NewConfDepth(driverCfg.VerifierConfDepth, statusTracker.L1Head, l1)
 
-	ec := engine.NewEngineController(l2, log, metrics, cfg, syncCfg,
-		sys.Register("engine-controller", nil))
+	ec := engine.NewEngineController(driverCtx, l2, log, metrics, cfg, syncCfg, sys.Register("engine-controller", nil))
 
 	sys.Register("engine-reset",
 		engine.NewEngineResetDeriver(driverCtx, log, cfg, l1, l2, syncCfg))
@@ -207,7 +207,7 @@ func NewDriver(
 	}
 	sys.Register("finalizer", finalizer)
 
-	attrHandler := attributes.NewAttributesHandler(log, cfg, driverCtx, l2)
+	attrHandler := attributes.NewAttributesHandler(log, cfg, driverCtx, l2, ec)
 	sys.Register("attributes-handler", attrHandler)
 
 	derivationPipeline := derive.NewDerivationPipeline(log, cfg, depSet, verifConfDepth, l1Blobs, altDA, l2, metrics, indexingMode)
@@ -234,9 +234,7 @@ func NewDriver(
 	//  Couple EngDeriver and NewAttributesHandler for event refactoring
 	ec.SyncDeriver = syncDeriver
 	sys.Register("sync", syncDeriver)
-	engDeriver := engine.NewEngDeriver(log, driverCtx, cfg, metrics, ec)
-	sys.Register("engine", engDeriver)
-	attrHandler.EngDeriver = engDeriver
+	sys.Register("engine", ec)
 
 	schedDeriv := NewStepSchedulingDeriver(log)
 	sys.Register("step-scheduler", schedDeriv)

op-node/rollup/driver/state.go

@@ -209,7 +209,7 @@ type SyncDeriver struct {
 
 	// The engine controller is used by the sequencer & Derivation components.
 	// We will also use it for EL sync in a future PR.
-	Engine EngineController
+	Engine *engine.EngineController
 
 	// Sync Mod Config
 	SyncCfg *sync.Config

op-node/rollup/engine/build_cancel.go

@@ -19,12 +19,12 @@ func (ev BuildCancelEvent) String() string {
 	return "build-cancel"
 }
 
-func (eq *EngDeriver) onBuildCancel(ctx context.Context, ev BuildCancelEvent) {
+func (eq *EngineController) onBuildCancel(ctx context.Context, ev BuildCancelEvent) {
 	rpcCtx, cancel := context.WithTimeout(eq.ctx, buildCancelTimeout)
 	defer cancel()
 	// the building job gets wrapped up as soon as the payload is retrieved, there's no explicit cancel in the Engine API
 	eq.log.Warn("cancelling old block building job", "info", ev.Info)
-	_, err := eq.ec.engine.GetPayload(rpcCtx, ev.Info)
+	_, err := eq.engine.GetPayload(rpcCtx, ev.Info)
 	if err != nil {
 		var rpcErr rpc.Error
 		if errors.As(err, &rpcErr) && eth.ErrorCode(rpcErr.ErrorCode()) == eth.UnknownPayload {

op-node/rollup/engine/build_invalid.go

@@ -30,7 +30,7 @@ func (ev InvalidPayloadAttributesEvent) String() string {
 	return "invalid-payload-attributes"
 }
 
-func (eq *EngDeriver) onBuildInvalid(ctx context.Context, ev BuildInvalidEvent) {
+func (eq *EngineController) onBuildInvalid(ctx context.Context, ev BuildInvalidEvent) {
 	eq.log.Warn("could not process payload attributes", "err", ev.Err)
 
 	// Deposit transaction execution errors are suppressed in the execution engine, but if the
@@ -43,27 +43,27 @@ func (eq *EngDeriver) onBuildInvalid(ctx context.Context, ev BuildInvalidEvent)
 		return
 	}
 
-	if ev.Attributes.IsDerived() && eq.cfg.IsHolocene(ev.Attributes.DerivedFrom.Time) {
+	if ev.Attributes.IsDerived() && eq.rollupCfg.IsHolocene(ev.Attributes.DerivedFrom.Time) {
 		eq.emitDepositsOnlyPayloadAttributesRequest(ctx, ev.Attributes.Parent.ID(), ev.Attributes.DerivedFrom)
 		return
 	}
 
 	// Revert the pending safe head to the safe head.
-	eq.ec.SetPendingSafeL2Head(eq.ec.SafeL2Head())
+	eq.SetPendingSafeL2Head(eq.SafeL2Head())
 	// 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.
 
 	// Try to restore to previous known unsafe chain.
-	eq.ec.SetBackupUnsafeL2Head(eq.ec.BackupUnsafeL2Head(), true)
+	eq.SetBackupUnsafeL2Head(eq.BackupUnsafeL2Head(), true)
 
 	// drop the payload without inserting it into the engine
 
 	// Signal that we deemed the attributes as unfit
 	eq.emitter.Emit(ctx, InvalidPayloadAttributesEvent(ev))
 }
 
-func (eq *EngDeriver) emitDepositsOnlyPayloadAttributesRequest(ctx context.Context, parent eth.BlockID, derivedFrom eth.L1BlockRef) {
+func (eq *EngineController) emitDepositsOnlyPayloadAttributesRequest(ctx context.Context, parent eth.BlockID, derivedFrom eth.L1BlockRef) {
 	eq.log.Warn("Holocene active, requesting deposits-only attributes", "parent", parent, "derived_from", derivedFrom)
 	// request deposits-only version
 	eq.emitter.Emit(ctx, derive.DepositsOnlyPayloadAttributesRequestEvent{

op-node/rollup/engine/build_seal.go

@@ -54,12 +54,12 @@ func (ev BuildSealEvent) String() string {
 	return "build-seal"
 }
 
-func (eq *EngDeriver) onBuildSeal(ctx context.Context, ev BuildSealEvent) {
+func (eq *EngineController) onBuildSeal(ctx context.Context, ev BuildSealEvent) {
 	rpcCtx, cancel := context.WithTimeout(eq.ctx, buildSealTimeout)
 	defer cancel()
 
 	sealingStart := time.Now()
-	envelope, err := eq.ec.engine.GetPayload(rpcCtx, ev.Info)
+	envelope, err := eq.engine.GetPayload(rpcCtx, ev.Info)
 	if err != nil {
 		var rpcErr rpc.Error
 		if errors.As(err, &rpcErr) && eth.ErrorCode(rpcErr.ErrorCode()) == eth.UnknownPayload {
@@ -92,7 +92,7 @@ func (eq *EngDeriver) onBuildSeal(ctx context.Context, ev BuildSealEvent) {
 		return
 	}
 
-	ref, err := derive.PayloadToBlockRef(eq.cfg, envelope.ExecutionPayload)
+	ref, err := derive.PayloadToBlockRef(eq.rollupCfg, envelope.ExecutionPayload)
 	if err != nil {
 		eq.emitter.Emit(ctx, PayloadSealInvalidEvent{
 			Info:        ev.Info,
@@ -107,7 +107,7 @@ func (eq *EngDeriver) onBuildSeal(ctx context.Context, ev BuildSealEvent) {
 	sealTime := now.Sub(sealingStart)
 	buildTime := now.Sub(ev.BuildStarted)
 	eq.metrics.RecordSequencerSealingTime(sealTime)
-	eq.metrics.RecordSequencerBuildingDiffTime(buildTime - time.Duration(eq.cfg.BlockTime)*time.Second)
+	eq.metrics.RecordSequencerBuildingDiffTime(buildTime - time.Duration(eq.rollupCfg.BlockTime)*time.Second)
 
 	txnCount := len(envelope.ExecutionPayload.Transactions)
 	depositCount, _ := lastDeposit(envelope.ExecutionPayload.Transactions)

op-node/rollup/engine/build_sealed.go

@@ -25,7 +25,7 @@ func (ev BuildSealedEvent) String() string {
 	return "build-sealed"
 }
 
-func (eq *EngDeriver) onBuildSealed(ctx context.Context, ev BuildSealedEvent) {
+func (eq *EngineController) onBuildSealed(ctx context.Context, ev BuildSealedEvent) {
 	// If a (pending) safe block, immediately process the block
 	if ev.DerivedFrom != (eth.L1BlockRef{}) {
 		eq.emitter.Emit(ctx, PayloadProcessEvent{