From 23ddf65b11ecd1c7a8078dcfde2c79ae7bae54e0 Mon Sep 17 00:00:00 2001 From: Tei Im Date: Tue, 30 Jan 2024 18:56:51 +0900 Subject: [PATCH 1/8] Use engine API that matches HF --- op-e2e/actions/l2_engine_test.go | 2 +- op-e2e/op_geth.go | 2 +- op-e2e/op_geth_test.go | 2 +- op-node/rollup/derive/engine_controller.go | 28 +++++++++++----------- op-node/rollup/derive/engine_update.go | 4 ++-- op-program/client/l2/engine.go | 19 +++++++++++++-- op-service/eth/types.go | 4 ++++ op-service/sources/engine_client.go | 26 ++++++++++++++++---- op-service/testutils/mock_engine.go | 4 ++-- 9 files changed, 63 insertions(+), 28 deletions(-) diff --git a/op-e2e/actions/l2_engine_test.go b/op-e2e/actions/l2_engine_test.go index c497f7f5a34c..cd59763cd59a 100644 --- a/op-e2e/actions/l2_engine_test.go +++ b/op-e2e/actions/l2_engine_test.go @@ -154,7 +154,7 @@ func TestL2EngineAPIBlockBuilding(gt *testing.T) { engine.ActL2IncludeTx(dp.Addresses.Alice)(t) } - envelope, err := l2Cl.GetPayload(t.Ctx(), *fcRes.PayloadID) + envelope, err := l2Cl.GetPayload(t.Ctx(), eth.PayloadInfo{ID: *fcRes.PayloadID, Timestamp: uint64(nextBlockTime)}) payload := envelope.ExecutionPayload require.NoError(t, err) require.Equal(t, parent.Hash(), payload.ParentHash, "block builds on parent block") diff --git a/op-e2e/op_geth.go b/op-e2e/op_geth.go index aed070af1b59..3dbd0add55c1 100644 --- a/op-e2e/op_geth.go +++ b/op-e2e/op_geth.go @@ -146,7 +146,7 @@ func (d *OpGeth) AddL2Block(ctx context.Context, txs ...*types.Transaction) (*et return nil, err } - envelope, err := d.l2Engine.GetPayload(ctx, *res.PayloadID) + envelope, err := d.l2Engine.GetPayload(ctx, eth.PayloadInfo{ID: *res.PayloadID, Timestamp: uint64(attrs.Timestamp)}) payload := envelope.ExecutionPayload if err != nil { diff --git a/op-e2e/op_geth_test.go b/op-e2e/op_geth_test.go index 9f69113675e7..03a1f8c4234e 100644 --- a/op-e2e/op_geth_test.go +++ b/op-e2e/op_geth_test.go @@ -196,7 +196,7 @@ func TestGethOnlyPendingBlockIsLatest(t *testing.T) { time.Sleep(time.Second * 4) // conservatively wait 4 seconds, CI might lag during block building. // retrieve the block - envelope, err := opGeth.l2Engine.GetPayload(ctx, *res.PayloadID) + envelope, err := opGeth.l2Engine.GetPayload(ctx, eth.PayloadInfo{ID: *res.PayloadID, Timestamp: uint64(attrs.Timestamp)}) require.NoError(t, err) payload := envelope.ExecutionPayload diff --git a/op-node/rollup/derive/engine_controller.go b/op-node/rollup/derive/engine_controller.go index 8ffb1f872b85..10112e2f1a79 100644 --- a/op-node/rollup/derive/engine_controller.go +++ b/op-node/rollup/derive/engine_controller.go @@ -38,7 +38,7 @@ var _ EngineControl = (*EngineController)(nil) var _ LocalEngineControl = (*EngineController)(nil) type ExecEngine interface { - GetPayload(ctx context.Context, payloadId eth.PayloadID) (*eth.ExecutionPayloadEnvelope, error) + GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) L2BlockRefByLabel(ctx context.Context, label eth.BlockLabel) (eth.L2BlockRef, error) @@ -63,7 +63,7 @@ type EngineController struct { // Building State buildingOnto eth.L2BlockRef - buildingID eth.PayloadID + buildingInfo eth.PayloadInfo buildingSafe bool safeAttrs *AttributesWithParent } @@ -104,7 +104,7 @@ func (e *EngineController) Finalized() eth.L2BlockRef { } func (e *EngineController) BuildingPayload() (eth.L2BlockRef, eth.PayloadID, bool) { - return e.buildingOnto, e.buildingID, e.buildingSafe + return e.buildingOnto, e.buildingInfo.ID, e.buildingSafe } func (e *EngineController) IsEngineSyncing() bool { @@ -146,8 +146,8 @@ func (e *EngineController) StartPayload(ctx context.Context, parent eth.L2BlockR if e.IsEngineSyncing() { return BlockInsertTemporaryErr, fmt.Errorf("engine is in progess of p2p sync") } - if e.buildingID != (eth.PayloadID{}) { - e.log.Warn("did not finish previous block building, starting new building now", "prev_onto", e.buildingOnto, "prev_payload_id", e.buildingID, "new_onto", parent) + if e.buildingInfo != (eth.PayloadInfo{}) { + e.log.Warn("did not finish previous block building, starting new building now", "prev_onto", e.buildingOnto, "prev_payload_id", e.buildingInfo.ID, "new_onto", parent) // TODO(8841): maybe worth it to force-cancel the old payload ID here. } fc := eth.ForkchoiceState{ @@ -161,7 +161,7 @@ func (e *EngineController) StartPayload(ctx context.Context, parent eth.L2BlockR return errTyp, err } - e.buildingID = id + e.buildingInfo = eth.PayloadInfo{ID: id, Timestamp: uint64(attrs.attributes.Timestamp)} e.buildingSafe = updateSafe e.buildingOnto = parent if updateSafe { @@ -172,7 +172,7 @@ func (e *EngineController) StartPayload(ctx context.Context, parent eth.L2BlockR } func (e *EngineController) ConfirmPayload(ctx context.Context, agossip async.AsyncGossiper, sequencerConductor conductor.SequencerConductor) (out *eth.ExecutionPayloadEnvelope, errTyp BlockInsertionErrType, err error) { - if e.buildingID == (eth.PayloadID{}) { + if e.buildingInfo == (eth.PayloadInfo{}) { return nil, BlockInsertPrestateErr, fmt.Errorf("cannot complete payload building: not currently building a payload") } if e.buildingOnto.Hash != e.unsafeHead.Hash { // E.g. when safe-attributes consolidation fails, it will drop the existing work. @@ -185,9 +185,9 @@ func (e *EngineController) ConfirmPayload(ctx context.Context, agossip async.Asy } // Update the safe head if the payload is built with the last attributes in the batch. updateSafe := e.buildingSafe && e.safeAttrs != nil && e.safeAttrs.isLastInSpan - envelope, errTyp, err := confirmPayload(ctx, e.log, e.engine, fc, e.buildingID, updateSafe, agossip, sequencerConductor) + envelope, errTyp, err := confirmPayload(ctx, e.log, e.engine, fc, e.buildingInfo, updateSafe, agossip, sequencerConductor) if err != nil { - return nil, errTyp, fmt.Errorf("failed to complete building on top of L2 chain %s, id: %s, error (%d): %w", e.buildingOnto, e.buildingID, errTyp, err) + return nil, errTyp, fmt.Errorf("failed to complete building on top of L2 chain %s, id: %s, error (%d): %w", e.buildingOnto, e.buildingInfo.ID, errTyp, err) } ref, err := PayloadToBlockRef(e.rollupCfg, envelope.ExecutionPayload) if err != nil { @@ -211,14 +211,14 @@ func (e *EngineController) ConfirmPayload(ctx context.Context, agossip async.Asy } func (e *EngineController) CancelPayload(ctx context.Context, force bool) error { - if e.buildingID == (eth.PayloadID{}) { // only cancel if there is something to cancel. + if e.buildingInfo == (eth.PayloadInfo{}) { // only cancel if there is something to cancel. return nil } // the building job gets wrapped up as soon as the payload is retrieved, there's no explicit cancel in the Engine API - e.log.Error("cancelling old block sealing job", "payload", e.buildingID) - _, err := e.engine.GetPayload(ctx, e.buildingID) + e.log.Error("cancelling old block sealing job", "payload", e.buildingInfo.ID) + _, err := e.engine.GetPayload(ctx, e.buildingInfo) if err != nil { - e.log.Error("failed to cancel block building job", "payload", e.buildingID, "err", err) + e.log.Error("failed to cancel block building job", "payload", e.buildingInfo.ID, "err", err) if !force { return err } @@ -228,7 +228,7 @@ func (e *EngineController) CancelPayload(ctx context.Context, force bool) error } func (e *EngineController) resetBuildingState() { - e.buildingID = eth.PayloadID{} + e.buildingInfo = eth.PayloadInfo{} e.buildingOnto = eth.L2BlockRef{} e.buildingSafe = false e.safeAttrs = nil diff --git a/op-node/rollup/derive/engine_update.go b/op-node/rollup/derive/engine_update.go index 5408ef495ec3..b23a27004004 100644 --- a/op-node/rollup/derive/engine_update.go +++ b/op-node/rollup/derive/engine_update.go @@ -124,7 +124,7 @@ func confirmPayload( log log.Logger, eng ExecEngine, fc eth.ForkchoiceState, - id eth.PayloadID, + payloadInfo eth.PayloadInfo, updateSafe bool, agossip async.AsyncGossiper, sequencerConductor conductor.SequencerConductor, @@ -140,7 +140,7 @@ func confirmPayload( "parent", envelope.ExecutionPayload.ParentHash, "txs", len(envelope.ExecutionPayload.Transactions)) } else { - envelope, err = eng.GetPayload(ctx, id) + envelope, err = eng.GetPayload(ctx, payloadInfo) } if err != nil { // even if it is an input-error (unknown payload ID), it is temporary, since we will re-attempt the full payload building, not just the retrieval of the payload. diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index 260561bccee3..be178f274f99 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -50,8 +50,14 @@ func (o *OracleEngine) L2OutputRoot(l2ClaimBlockNum uint64) (eth.Bytes32, error) return rollup.ComputeL2OutputRootV0(eth.HeaderBlockInfo(outBlock), withdrawalsTrie.Hash()) } -func (o *OracleEngine) GetPayload(ctx context.Context, payloadId eth.PayloadID) (*eth.ExecutionPayloadEnvelope, error) { - res, err := o.api.GetPayloadV3(ctx, payloadId) +func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) { + var res *eth.ExecutionPayloadEnvelope + var err error + if o.rollupCfg.IsEcotone(payloadInfo.Timestamp) { + res, err = o.api.GetPayloadV3(ctx, payloadInfo.ID) + } else { + res, err = o.api.GetPayloadV2(ctx, payloadInfo.ID) + } if err != nil { return nil, err } @@ -59,6 +65,15 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadId eth.PayloadID) } func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { + if attr != nil { + if o.rollupCfg.IsEcotone(uint64(attr.Timestamp)) { + return o.api.ForkchoiceUpdatedV3(ctx, state, attr) + } else if o.rollupCfg.IsCanyon(uint64(attr.Timestamp)) { + return o.api.ForkchoiceUpdatedV2(ctx, state, attr) + } else { + return o.api.ForkchoiceUpdatedV1(ctx, state, attr) + } + } return o.api.ForkchoiceUpdatedV3(ctx, state, attr) } diff --git a/op-service/eth/types.go b/op-service/eth/types.go index bb7e4c6cdd14..30c052c4208a 100644 --- a/op-service/eth/types.go +++ b/op-service/eth/types.go @@ -153,6 +153,10 @@ type Uint256Quantity = uint256.Int type Data = hexutil.Bytes type PayloadID = engine.PayloadID +type PayloadInfo struct { + ID PayloadID + Timestamp uint64 +} type ExecutionPayloadEnvelope struct { ParentBeaconBlockRoot *common.Hash `json:"parentBeaconBlockRoot,omitempty"` diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index e80280a317b8..3fbd70354cc5 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -58,7 +58,17 @@ func (s *EngineClient) ForkchoiceUpdate(ctx context.Context, fc *eth.ForkchoiceS fcCtx, cancel := context.WithTimeout(ctx, time.Second*5) defer cancel() var result eth.ForkchoiceUpdatedResult - err := s.client.CallContext(fcCtx, &result, "engine_forkchoiceUpdatedV3", fc, attributes) + method := "engine_forkchoiceUpdatedV3" + if attributes != nil { + ts := uint64(attributes.Timestamp) + if s.rollupCfg.IsEcotone(ts) { + } else if s.rollupCfg.IsCanyon(ts) { + method = "engine_forkchoiceUpdatedV2" + } else if !s.rollupCfg.IsCanyon(ts) { + method = "engine_forkchoiceUpdatedV1" + } + } + err := s.client.CallContext(fcCtx, &result, method, fc, attributes) if err == nil { tlog.Trace("Shared forkchoice-updated signal") if attributes != nil { // block building is optional, we only get a payload ID if we are building a block @@ -113,13 +123,19 @@ func (s *EngineClient) NewPayload(ctx context.Context, payload *eth.ExecutionPay // There may be two types of error: // 1. `error` as eth.InputError: the payload ID may be unknown // 2. Other types of `error`: temporary RPC errors, like timeouts. -func (s *EngineClient) GetPayload(ctx context.Context, payloadId eth.PayloadID) (*eth.ExecutionPayloadEnvelope, error) { - e := s.log.New("payload_id", payloadId) +func (s *EngineClient) GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) { + e := s.log.New("payload_id", payloadInfo.ID) e.Trace("getting payload") var result eth.ExecutionPayloadEnvelope - err := s.client.CallContext(ctx, &result, "engine_getPayloadV3", payloadId) + var method string + if s.rollupCfg.IsEcotone(payloadInfo.Timestamp) { + method = "engine_getPayloadV3" + } else { + method = "engine_getPayloadV2" + } + err := s.client.CallContext(ctx, &result, method, payloadInfo.ID) if err != nil { - e.Warn("Failed to get payload", "payload_id", payloadId, "err", err) + e.Warn("Failed to get payload", "payload_id", payloadInfo.ID, "err", err) if rpcErr, ok := err.(rpc.Error); ok { code := eth.ErrorCode(rpcErr.ErrorCode()) switch code { diff --git a/op-service/testutils/mock_engine.go b/op-service/testutils/mock_engine.go index b6bfa5d98f5b..99dadf534ec4 100644 --- a/op-service/testutils/mock_engine.go +++ b/op-service/testutils/mock_engine.go @@ -12,8 +12,8 @@ type MockEngine struct { MockL2Client } -func (m *MockEngine) GetPayload(ctx context.Context, payloadId eth.PayloadID) (*eth.ExecutionPayloadEnvelope, error) { - out := m.Mock.Called(payloadId) +func (m *MockEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) { + out := m.Mock.Called(payloadInfo.ID) return out.Get(0).(*eth.ExecutionPayloadEnvelope), out.Error(1) } From 789b516d06bac80325391458e0349f6dfe211f1c Mon Sep 17 00:00:00 2001 From: Tei Im Date: Thu, 1 Feb 2024 11:55:01 +0900 Subject: [PATCH 2/8] Simplify engine client code by using EngineAPIVersion --- op-node/rollup/types.go | 43 +++++++++++++++++++++++++++++ op-program/client/l2/engine.go | 17 +++++++----- op-service/sources/engine_client.go | 24 ++++++---------- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/op-node/rollup/types.go b/op-node/rollup/types.go index f47bf0a0fa90..a11a97449d92 100644 --- a/op-node/rollup/types.go +++ b/op-node/rollup/types.go @@ -353,6 +353,41 @@ func (c *Config) IsInterop(timestamp uint64) bool { return c.InteropTime != nil && timestamp >= *c.InteropTime } +// ForkchoiceUpdatedVersion returns the EngineAPIVersion suitable for the chain hard fork version. +func (c *Config) ForkchoiceUpdatedVersion(timestamp uint64) EngineAPIVersion { + if c.IsEcotone(timestamp) { + // Cancun + return EngineAPIV3 + } else if c.IsCanyon(timestamp) { + // Shanghai + return EngineAPIV2 + } else { + // According to Ethereum engine API spec, we can use fcuV2 here, + // but upstream Geth v1.13.11 does not accept V2 before Shanghai. + return EngineAPIV1 + } +} + +// NewPayloadVersion returns the EngineAPIVersion suitable for the chain hard fork version. +func (c *Config) NewPayloadVersion(timestamp uint64) EngineAPIVersion { + if c.IsEcotone(timestamp) { + // Cancun + return EngineAPIV3 + } else { + return EngineAPIV2 + } +} + +// GetPayloadVersion returns the EngineAPIVersion suitable for the chain hard fork version. +func (c *Config) GetPayloadVersion(timestamp uint64) EngineAPIVersion { + if c.IsEcotone(timestamp) { + // Cancun + return EngineAPIV3 + } else { + return EngineAPIV2 + } +} + // Description outputs a banner describing the important parts of rollup configuration in a human-readable form. // Optionally provide a mapping of L2 chain IDs to network names to label the L2 chain with if not unknown. // The config should be config.Check()-ed before creating a description. @@ -433,3 +468,11 @@ func fmtTime(v uint64) string { } type Epoch uint64 + +type EngineAPIVersion string + +const ( + EngineAPIV1 EngineAPIVersion = "V1" + EngineAPIV2 EngineAPIVersion = "V2" + EngineAPIV3 EngineAPIVersion = "V3" +) diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index be178f274f99..7813890c972d 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -53,9 +53,10 @@ func (o *OracleEngine) L2OutputRoot(l2ClaimBlockNum uint64) (eth.Bytes32, error) func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) { var res *eth.ExecutionPayloadEnvelope var err error - if o.rollupCfg.IsEcotone(payloadInfo.Timestamp) { + switch o.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) { + case rollup.EngineAPIV3: res, err = o.api.GetPayloadV3(ctx, payloadInfo.ID) - } else { + default: res, err = o.api.GetPayloadV2(ctx, payloadInfo.ID) } if err != nil { @@ -66,11 +67,12 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { if attr != nil { - if o.rollupCfg.IsEcotone(uint64(attr.Timestamp)) { + switch o.rollupCfg.ForkchoiceUpdatedVersion(uint64(attr.Timestamp)) { + case rollup.EngineAPIV3: return o.api.ForkchoiceUpdatedV3(ctx, state, attr) - } else if o.rollupCfg.IsCanyon(uint64(attr.Timestamp)) { + case rollup.EngineAPIV2: return o.api.ForkchoiceUpdatedV2(ctx, state, attr) - } else { + default: return o.api.ForkchoiceUpdatedV1(ctx, state, attr) } } @@ -78,9 +80,10 @@ func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.Forkchoi } func (o *OracleEngine) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) { - if o.rollupCfg.IsEcotone(uint64(payload.Timestamp)) { + switch o.rollupCfg.GetPayloadVersion(uint64(payload.Timestamp)) { + case rollup.EngineAPIV3: return o.api.NewPayloadV3(ctx, payload, []common.Hash{}, parentBeaconBlockRoot) - } else { + default: return o.api.NewPayloadV2(ctx, payload) } } diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index 3fbd70354cc5..10a63f8e7edb 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -58,16 +58,11 @@ func (s *EngineClient) ForkchoiceUpdate(ctx context.Context, fc *eth.ForkchoiceS fcCtx, cancel := context.WithTimeout(ctx, time.Second*5) defer cancel() var result eth.ForkchoiceUpdatedResult - method := "engine_forkchoiceUpdatedV3" + engineAPIVersion := rollup.EngineAPIV3 if attributes != nil { - ts := uint64(attributes.Timestamp) - if s.rollupCfg.IsEcotone(ts) { - } else if s.rollupCfg.IsCanyon(ts) { - method = "engine_forkchoiceUpdatedV2" - } else if !s.rollupCfg.IsCanyon(ts) { - method = "engine_forkchoiceUpdatedV1" - } + engineAPIVersion = s.rollupCfg.ForkchoiceUpdatedVersion(uint64(attributes.Timestamp)) } + method := fmt.Sprintf("engine_forkchoiceUpdated%s", engineAPIVersion) err := s.client.CallContext(fcCtx, &result, method, fc, attributes) if err == nil { tlog.Trace("Shared forkchoice-updated signal") @@ -105,9 +100,10 @@ func (s *EngineClient) NewPayload(ctx context.Context, payload *eth.ExecutionPay var result eth.PayloadStatusV1 var err error - if s.rollupCfg.IsEcotone(uint64(payload.Timestamp)) { + switch s.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)) { + case rollup.EngineAPIV3: err = s.client.CallContext(execCtx, &result, "engine_newPayloadV3", payload, []common.Hash{}, parentBeaconBlockRoot) - } else { + default: err = s.client.CallContext(execCtx, &result, "engine_newPayloadV2", payload) } @@ -127,12 +123,8 @@ func (s *EngineClient) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn e := s.log.New("payload_id", payloadInfo.ID) e.Trace("getting payload") var result eth.ExecutionPayloadEnvelope - var method string - if s.rollupCfg.IsEcotone(payloadInfo.Timestamp) { - method = "engine_getPayloadV3" - } else { - method = "engine_getPayloadV2" - } + engineAPIVersion := s.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) + method := fmt.Sprintf("engine_getPayload%s", engineAPIVersion) err := s.client.CallContext(ctx, &result, method, payloadInfo.ID) if err != nil { e.Warn("Failed to get payload", "payload_id", payloadInfo.ID, "err", err) From 71c96523cac12f31a26acfbf81a7a96994f5dad7 Mon Sep 17 00:00:00 2001 From: Tei Im Date: Fri, 2 Feb 2024 09:50:49 +0900 Subject: [PATCH 3/8] Return full method string from engine api version methods --- op-node/rollup/types.go | 34 +++++++++++------------------ op-program/client/l2/engine.go | 8 +++---- op-service/eth/types.go | 14 ++++++++++++ op-service/sources/engine_client.go | 20 ++++++++--------- 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/op-node/rollup/types.go b/op-node/rollup/types.go index a11a97449d92..4a991a5c7a4d 100644 --- a/op-node/rollup/types.go +++ b/op-node/rollup/types.go @@ -353,38 +353,38 @@ func (c *Config) IsInterop(timestamp uint64) bool { return c.InteropTime != nil && timestamp >= *c.InteropTime } -// ForkchoiceUpdatedVersion returns the EngineAPIVersion suitable for the chain hard fork version. -func (c *Config) ForkchoiceUpdatedVersion(timestamp uint64) EngineAPIVersion { +// ForkchoiceUpdatedVersion returns the EngineAPIMethod suitable for the chain hard fork version. +func (c *Config) ForkchoiceUpdatedVersion(timestamp uint64) eth.EngineAPIMethod { if c.IsEcotone(timestamp) { // Cancun - return EngineAPIV3 + return eth.FCUV3 } else if c.IsCanyon(timestamp) { // Shanghai - return EngineAPIV2 + return eth.FCUV2 } else { // According to Ethereum engine API spec, we can use fcuV2 here, // but upstream Geth v1.13.11 does not accept V2 before Shanghai. - return EngineAPIV1 + return eth.FCUV1 } } -// NewPayloadVersion returns the EngineAPIVersion suitable for the chain hard fork version. -func (c *Config) NewPayloadVersion(timestamp uint64) EngineAPIVersion { +// NewPayloadVersion returns the EngineAPIMethod suitable for the chain hard fork version. +func (c *Config) NewPayloadVersion(timestamp uint64) eth.EngineAPIMethod { if c.IsEcotone(timestamp) { // Cancun - return EngineAPIV3 + return eth.NewPayloadV3 } else { - return EngineAPIV2 + return eth.NewPayloadV2 } } -// GetPayloadVersion returns the EngineAPIVersion suitable for the chain hard fork version. -func (c *Config) GetPayloadVersion(timestamp uint64) EngineAPIVersion { +// GetPayloadVersion returns the EngineAPIMethod suitable for the chain hard fork version. +func (c *Config) GetPayloadVersion(timestamp uint64) eth.EngineAPIMethod { if c.IsEcotone(timestamp) { // Cancun - return EngineAPIV3 + return eth.GetPayloadV3 } else { - return EngineAPIV2 + return eth.GetPayloadV2 } } @@ -468,11 +468,3 @@ func fmtTime(v uint64) string { } type Epoch uint64 - -type EngineAPIVersion string - -const ( - EngineAPIV1 EngineAPIVersion = "V1" - EngineAPIV2 EngineAPIVersion = "V2" - EngineAPIV3 EngineAPIVersion = "V3" -) diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index 7813890c972d..bf1a4cfbdc91 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -54,7 +54,7 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn var res *eth.ExecutionPayloadEnvelope var err error switch o.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) { - case rollup.EngineAPIV3: + case eth.GetPayloadV3: res, err = o.api.GetPayloadV3(ctx, payloadInfo.ID) default: res, err = o.api.GetPayloadV2(ctx, payloadInfo.ID) @@ -68,9 +68,9 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { if attr != nil { switch o.rollupCfg.ForkchoiceUpdatedVersion(uint64(attr.Timestamp)) { - case rollup.EngineAPIV3: + case eth.FCUV3: return o.api.ForkchoiceUpdatedV3(ctx, state, attr) - case rollup.EngineAPIV2: + case eth.FCUV2: return o.api.ForkchoiceUpdatedV2(ctx, state, attr) default: return o.api.ForkchoiceUpdatedV1(ctx, state, attr) @@ -81,7 +81,7 @@ func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.Forkchoi func (o *OracleEngine) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) { switch o.rollupCfg.GetPayloadVersion(uint64(payload.Timestamp)) { - case rollup.EngineAPIV3: + case eth.NewPayloadV3: return o.api.NewPayloadV3(ctx, payload, []common.Hash{}, parentBeaconBlockRoot) default: return o.api.NewPayloadV2(ctx, payload) diff --git a/op-service/eth/types.go b/op-service/eth/types.go index 30c052c4208a..2c39e5d333e1 100644 --- a/op-service/eth/types.go +++ b/op-service/eth/types.go @@ -458,3 +458,17 @@ func (v *Uint64String) UnmarshalText(b []byte) error { *v = Uint64String(n) return nil } + +type EngineAPIMethod string + +const ( + FCUV1 EngineAPIMethod = "engine_forkchoiceUpdatedV1" + FCUV2 EngineAPIMethod = "engine_forkchoiceUpdatedV2" + FCUV3 EngineAPIMethod = "engine_forkchoiceUpdatedV3" + + NewPayloadV2 EngineAPIMethod = "engine_newPayloadV2" + NewPayloadV3 EngineAPIMethod = "engine_newPayloadV3" + + GetPayloadV2 EngineAPIMethod = "engine_getPayloadV2" + GetPayloadV3 EngineAPIMethod = "engine_getPayloadV3" +) diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index 10a63f8e7edb..fb30251c8538 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -58,12 +58,11 @@ func (s *EngineClient) ForkchoiceUpdate(ctx context.Context, fc *eth.ForkchoiceS fcCtx, cancel := context.WithTimeout(ctx, time.Second*5) defer cancel() var result eth.ForkchoiceUpdatedResult - engineAPIVersion := rollup.EngineAPIV3 + method := eth.FCUV3 if attributes != nil { - engineAPIVersion = s.rollupCfg.ForkchoiceUpdatedVersion(uint64(attributes.Timestamp)) + method = s.rollupCfg.ForkchoiceUpdatedVersion(uint64(attributes.Timestamp)) } - method := fmt.Sprintf("engine_forkchoiceUpdated%s", engineAPIVersion) - err := s.client.CallContext(fcCtx, &result, method, fc, attributes) + err := s.client.CallContext(fcCtx, &result, string(method), fc, attributes) if err == nil { tlog.Trace("Shared forkchoice-updated signal") if attributes != nil { // block building is optional, we only get a payload ID if we are building a block @@ -100,11 +99,11 @@ func (s *EngineClient) NewPayload(ctx context.Context, payload *eth.ExecutionPay var result eth.PayloadStatusV1 var err error - switch s.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)) { - case rollup.EngineAPIV3: - err = s.client.CallContext(execCtx, &result, "engine_newPayloadV3", payload, []common.Hash{}, parentBeaconBlockRoot) + switch method := s.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)); method { + case eth.NewPayloadV3: + err = s.client.CallContext(execCtx, &result, string(method), payload, []common.Hash{}, parentBeaconBlockRoot) default: - err = s.client.CallContext(execCtx, &result, "engine_newPayloadV2", payload) + err = s.client.CallContext(execCtx, &result, string(method), payload) } e.Trace("Received payload execution result", "status", result.Status, "latestValidHash", result.LatestValidHash, "message", result.ValidationError) @@ -123,9 +122,8 @@ func (s *EngineClient) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn e := s.log.New("payload_id", payloadInfo.ID) e.Trace("getting payload") var result eth.ExecutionPayloadEnvelope - engineAPIVersion := s.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) - method := fmt.Sprintf("engine_getPayload%s", engineAPIVersion) - err := s.client.CallContext(ctx, &result, method, payloadInfo.ID) + method := s.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) + err := s.client.CallContext(ctx, &result, string(method), payloadInfo.ID) if err != nil { e.Warn("Failed to get payload", "payload_id", payloadInfo.ID, "err", err) if rpcErr, ok := err.(rpc.Error); ok { From fd54c2ed44d94ca0a29a3ebc54744c2452bbbade Mon Sep 17 00:00:00 2001 From: Tei Im Date: Fri, 2 Feb 2024 09:53:26 +0900 Subject: [PATCH 4/8] FCUVersion takes payload attributes --- op-node/rollup/types.go | 7 ++++--- op-program/client/l2/engine.go | 2 +- op-service/sources/engine_client.go | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/op-node/rollup/types.go b/op-node/rollup/types.go index 4a991a5c7a4d..c59e1cd47990 100644 --- a/op-node/rollup/types.go +++ b/op-node/rollup/types.go @@ -354,11 +354,12 @@ func (c *Config) IsInterop(timestamp uint64) bool { } // ForkchoiceUpdatedVersion returns the EngineAPIMethod suitable for the chain hard fork version. -func (c *Config) ForkchoiceUpdatedVersion(timestamp uint64) eth.EngineAPIMethod { - if c.IsEcotone(timestamp) { +func (c *Config) ForkchoiceUpdatedVersion(attr *eth.PayloadAttributes) eth.EngineAPIMethod { + ts := uint64(attr.Timestamp) + if c.IsEcotone(ts) { // Cancun return eth.FCUV3 - } else if c.IsCanyon(timestamp) { + } else if c.IsCanyon(ts) { // Shanghai return eth.FCUV2 } else { diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index bf1a4cfbdc91..0837c644d0b3 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -67,7 +67,7 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { if attr != nil { - switch o.rollupCfg.ForkchoiceUpdatedVersion(uint64(attr.Timestamp)) { + switch o.rollupCfg.ForkchoiceUpdatedVersion(attr) { case eth.FCUV3: return o.api.ForkchoiceUpdatedV3(ctx, state, attr) case eth.FCUV2: diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index fb30251c8538..994511bf62c3 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -60,7 +60,7 @@ func (s *EngineClient) ForkchoiceUpdate(ctx context.Context, fc *eth.ForkchoiceS var result eth.ForkchoiceUpdatedResult method := eth.FCUV3 if attributes != nil { - method = s.rollupCfg.ForkchoiceUpdatedVersion(uint64(attributes.Timestamp)) + method = s.rollupCfg.ForkchoiceUpdatedVersion(attributes) } err := s.client.CallContext(fcCtx, &result, string(method), fc, attributes) if err == nil { From d3f3838f1466d53fcbd07398446e20b5eeec7584 Mon Sep 17 00:00:00 2001 From: Tei Im Date: Fri, 2 Feb 2024 10:05:15 +0900 Subject: [PATCH 5/8] Use a proper method --- op-program/client/l2/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index 0837c644d0b3..fa8ed765f73e 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -80,7 +80,7 @@ func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.Forkchoi } func (o *OracleEngine) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) { - switch o.rollupCfg.GetPayloadVersion(uint64(payload.Timestamp)) { + switch o.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)) { case eth.NewPayloadV3: return o.api.NewPayloadV3(ctx, payload, []common.Hash{}, parentBeaconBlockRoot) default: From c56eba8235ae254c9d3903128c813de4fdd59ef0 Mon Sep 17 00:00:00 2001 From: Tei Im Date: Fri, 2 Feb 2024 10:13:10 +0900 Subject: [PATCH 6/8] Handle switch default case explicitly --- op-program/client/l2/engine.go | 18 ++++++++++++------ op-service/sources/engine_client.go | 4 +++- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index fa8ed765f73e..e2ff5b56eff0 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -53,11 +53,13 @@ func (o *OracleEngine) L2OutputRoot(l2ClaimBlockNum uint64) (eth.Bytes32, error) func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadInfo) (*eth.ExecutionPayloadEnvelope, error) { var res *eth.ExecutionPayloadEnvelope var err error - switch o.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp) { + switch method := o.rollupCfg.GetPayloadVersion(payloadInfo.Timestamp); method { case eth.GetPayloadV3: res, err = o.api.GetPayloadV3(ctx, payloadInfo.ID) - default: + case eth.GetPayloadV2: res, err = o.api.GetPayloadV2(ctx, payloadInfo.ID) + default: + return nil, fmt.Errorf("unsupported GetPayload version: %s", method) } if err != nil { return nil, err @@ -67,24 +69,28 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { if attr != nil { - switch o.rollupCfg.ForkchoiceUpdatedVersion(attr) { + switch method := o.rollupCfg.ForkchoiceUpdatedVersion(attr); method { case eth.FCUV3: return o.api.ForkchoiceUpdatedV3(ctx, state, attr) case eth.FCUV2: return o.api.ForkchoiceUpdatedV2(ctx, state, attr) - default: + case eth.FCUV1: return o.api.ForkchoiceUpdatedV1(ctx, state, attr) + default: + return nil, fmt.Errorf("unsupported ForkchoiceUpdated version: %s", method) } } return o.api.ForkchoiceUpdatedV3(ctx, state, attr) } func (o *OracleEngine) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) { - switch o.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)) { + switch method := o.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)); method { case eth.NewPayloadV3: return o.api.NewPayloadV3(ctx, payload, []common.Hash{}, parentBeaconBlockRoot) - default: + case eth.NewPayloadV2: return o.api.NewPayloadV2(ctx, payload) + default: + return nil, fmt.Errorf("unsupported NewPayload version: %s", method) } } diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index 994511bf62c3..132b0acf6887 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -102,8 +102,10 @@ func (s *EngineClient) NewPayload(ctx context.Context, payload *eth.ExecutionPay switch method := s.rollupCfg.NewPayloadVersion(uint64(payload.Timestamp)); method { case eth.NewPayloadV3: err = s.client.CallContext(execCtx, &result, string(method), payload, []common.Hash{}, parentBeaconBlockRoot) - default: + case eth.NewPayloadV2: err = s.client.CallContext(execCtx, &result, string(method), payload) + default: + return nil, fmt.Errorf("unsupported NewPayload version: %s", method) } e.Trace("Received payload execution result", "status", result.Status, "latestValidHash", result.LatestValidHash, "message", result.ValidationError) From 81841564d6dabcc728e64273ed318d3bf06003c7 Mon Sep 17 00:00:00 2001 From: Tei Im Date: Mon, 5 Feb 2024 14:13:31 +0900 Subject: [PATCH 7/8] Return fcuV3 if attrs is nil --- op-node/rollup/types.go | 4 ++++ op-program/client/l2/engine.go | 23 ++++++++++------------- op-service/sources/engine_client.go | 5 +---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/op-node/rollup/types.go b/op-node/rollup/types.go index c59e1cd47990..3562fbcf7435 100644 --- a/op-node/rollup/types.go +++ b/op-node/rollup/types.go @@ -355,6 +355,10 @@ func (c *Config) IsInterop(timestamp uint64) bool { // ForkchoiceUpdatedVersion returns the EngineAPIMethod suitable for the chain hard fork version. func (c *Config) ForkchoiceUpdatedVersion(attr *eth.PayloadAttributes) eth.EngineAPIMethod { + if attr == nil { + // Don't begin payload build process. + return eth.FCUV3 + } ts := uint64(attr.Timestamp) if c.IsEcotone(ts) { // Cancun diff --git a/op-program/client/l2/engine.go b/op-program/client/l2/engine.go index e2ff5b56eff0..d4cb64529a64 100644 --- a/op-program/client/l2/engine.go +++ b/op-program/client/l2/engine.go @@ -68,19 +68,16 @@ func (o *OracleEngine) GetPayload(ctx context.Context, payloadInfo eth.PayloadIn } func (o *OracleEngine) ForkchoiceUpdate(ctx context.Context, state *eth.ForkchoiceState, attr *eth.PayloadAttributes) (*eth.ForkchoiceUpdatedResult, error) { - if attr != nil { - switch method := o.rollupCfg.ForkchoiceUpdatedVersion(attr); method { - case eth.FCUV3: - return o.api.ForkchoiceUpdatedV3(ctx, state, attr) - case eth.FCUV2: - return o.api.ForkchoiceUpdatedV2(ctx, state, attr) - case eth.FCUV1: - return o.api.ForkchoiceUpdatedV1(ctx, state, attr) - default: - return nil, fmt.Errorf("unsupported ForkchoiceUpdated version: %s", method) - } - } - return o.api.ForkchoiceUpdatedV3(ctx, state, attr) + switch method := o.rollupCfg.ForkchoiceUpdatedVersion(attr); method { + case eth.FCUV3: + return o.api.ForkchoiceUpdatedV3(ctx, state, attr) + case eth.FCUV2: + return o.api.ForkchoiceUpdatedV2(ctx, state, attr) + case eth.FCUV1: + return o.api.ForkchoiceUpdatedV1(ctx, state, attr) + default: + return nil, fmt.Errorf("unsupported ForkchoiceUpdated version: %s", method) + } } func (o *OracleEngine) NewPayload(ctx context.Context, payload *eth.ExecutionPayload, parentBeaconBlockRoot *common.Hash) (*eth.PayloadStatusV1, error) { diff --git a/op-service/sources/engine_client.go b/op-service/sources/engine_client.go index 132b0acf6887..70c63c237533 100644 --- a/op-service/sources/engine_client.go +++ b/op-service/sources/engine_client.go @@ -58,10 +58,7 @@ func (s *EngineClient) ForkchoiceUpdate(ctx context.Context, fc *eth.ForkchoiceS fcCtx, cancel := context.WithTimeout(ctx, time.Second*5) defer cancel() var result eth.ForkchoiceUpdatedResult - method := eth.FCUV3 - if attributes != nil { - method = s.rollupCfg.ForkchoiceUpdatedVersion(attributes) - } + method := s.rollupCfg.ForkchoiceUpdatedVersion(attributes) err := s.client.CallContext(fcCtx, &result, string(method), fc, attributes) if err == nil { tlog.Trace("Shared forkchoice-updated signal") From 649e994d46e08e799abeefe0b8e41188c2a4fafe Mon Sep 17 00:00:00 2001 From: Tei Im Date: Mon, 5 Feb 2024 14:13:54 +0900 Subject: [PATCH 8/8] Add test cases for engine api version methods --- op-node/rollup/types_test.go | 115 +++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/op-node/rollup/types_test.go b/op-node/rollup/types_test.go index a5bbc0575e16..17343430f262 100644 --- a/op-node/rollup/types_test.go +++ b/op-node/rollup/types_test.go @@ -508,3 +508,118 @@ func TestTimestampForBlock(t *testing.T) { } } + +func TestForkchoiceUpdatedVersion(t *testing.T) { + config := randConfig() + tests := []struct { + name string + canyonTime uint64 + ecotoneTime uint64 + attrs *eth.PayloadAttributes + expectedMethod eth.EngineAPIMethod + }{ + { + name: "NoAttrs", + canyonTime: 10, + ecotoneTime: 20, + attrs: nil, + expectedMethod: eth.FCUV3, + }, + { + name: "BeforeCanyon", + canyonTime: 10, + ecotoneTime: 20, + attrs: ð.PayloadAttributes{Timestamp: 5}, + expectedMethod: eth.FCUV1, + }, + { + name: "Canyon", + canyonTime: 10, + ecotoneTime: 20, + attrs: ð.PayloadAttributes{Timestamp: 15}, + expectedMethod: eth.FCUV2, + }, + { + name: "Ecotone", + canyonTime: 10, + ecotoneTime: 20, + attrs: ð.PayloadAttributes{Timestamp: 25}, + expectedMethod: eth.FCUV3, + }, + } + + for _, test := range tests { + test := test + t.Run(fmt.Sprintf("TestForkchoiceUpdatedVersion_%s", test.name), func(t *testing.T) { + config.CanyonTime = &test.canyonTime + config.EcotoneTime = &test.ecotoneTime + assert.Equal(t, config.ForkchoiceUpdatedVersion(test.attrs), test.expectedMethod) + }) + } +} + +func TestNewPayloadVersion(t *testing.T) { + config := randConfig() + canyonTime := uint64(0) + config.CanyonTime = &canyonTime + tests := []struct { + name string + ecotoneTime uint64 + payloadTime uint64 + expectedMethod eth.EngineAPIMethod + }{ + { + name: "BeforeEcotone", + ecotoneTime: 10, + payloadTime: 5, + expectedMethod: eth.NewPayloadV2, + }, + { + name: "Ecotone", + ecotoneTime: 10, + payloadTime: 15, + expectedMethod: eth.NewPayloadV3, + }, + } + + for _, test := range tests { + test := test + t.Run(fmt.Sprintf("TestNewPayloadVersion_%s", test.name), func(t *testing.T) { + config.EcotoneTime = &test.ecotoneTime + assert.Equal(t, config.NewPayloadVersion(test.payloadTime), test.expectedMethod) + }) + } +} + +func TestGetPayloadVersion(t *testing.T) { + config := randConfig() + canyonTime := uint64(0) + config.CanyonTime = &canyonTime + tests := []struct { + name string + ecotoneTime uint64 + payloadTime uint64 + expectedMethod eth.EngineAPIMethod + }{ + { + name: "BeforeEcotone", + ecotoneTime: 10, + payloadTime: 5, + expectedMethod: eth.GetPayloadV2, + }, + { + name: "Ecotone", + ecotoneTime: 10, + payloadTime: 15, + expectedMethod: eth.GetPayloadV3, + }, + } + + for _, test := range tests { + test := test + t.Run(fmt.Sprintf("TestGetPayloadVersion_%s", test.name), func(t *testing.T) { + config.EcotoneTime = &test.ecotoneTime + assert.Equal(t, config.GetPayloadVersion(test.payloadTime), test.expectedMethod) + }) + } +}