diff --git a/.circleci/config.yml b/.circleci/config.yml index 8484806f83a1c..464bffae10d01 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -2316,17 +2316,7 @@ workflows: not: equal: [scheduled_pipeline, << pipeline.trigger_source >>] jobs: - # Wait for approval on the release - - hold: - type: approval - filters: - tags: - only: /^(da-server|cannon|ufm-[a-z0-9\-]*|op-[a-z0-9\-]*)\/v.*/ - branches: - ignore: /.*/ - initialize: - requires: - - hold context: - circleci-repo-readonly-authenticated-github-token filters: diff --git a/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/elsync_test.go b/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/elsync_test.go new file mode 100644 index 0000000000000..4de948305c5a2 --- /dev/null +++ b/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/elsync_test.go @@ -0,0 +1,62 @@ +package sync_tester_elsync + +import ( + "testing" + + "github.com/ethereum-optimism/optimism/op-devstack/devtest" + "github.com/ethereum-optimism/optimism/op-devstack/dsl" + "github.com/ethereum-optimism/optimism/op-devstack/presets" + "github.com/ethereum-optimism/optimism/op-supervisor/supervisor/types" +) + +func TestSyncTesterELSync(gt *testing.T) { + t := devtest.SerialT(gt) + sys := presets.NewSimpleWithSyncTester(t) + require := t.Require() + logger := t.Logger() + ctx := t.Ctx() + + target := uint64(5) + dsl.CheckAll(t, + sys.L2CL.AdvancedFn(types.LocalUnsafe, target, 30), + sys.L2CL2.AdvancedFn(types.LocalUnsafe, target, 30), + ) + + // Stop L2CL2 attached to Sync Tester EL Endpoint + sys.L2CL2.Stop() + + // Reset Sync Tester EL + sessionIDs := sys.SyncTester.ListSessions() + require.GreaterOrEqual(len(sessionIDs), 1, "at least one session") + sessionID := sessionIDs[0] + logger.Info("SyncTester EL", "sessionID", sessionID) + syncTesterClient := sys.SyncTester.Escape().APIWithSession(sessionID) + require.NoError(syncTesterClient.ResetSession(ctx)) + + // Wait for L2CL to advance more unsafe blocks + sys.L2CL.Advanced(types.LocalUnsafe, target+5, 30) + + // EL Sync not done yet + session, err := syncTesterClient.GetSession(ctx) + require.NoError(err) + require.True(session.ELSyncActive) + + // Restarting will trigger EL sync since unsafe head payload will arrive to L2CL2 via P2P + sys.L2CL2.Start() + + // Wait until P2P is connected + sys.L2CL2.IsP2PConnected(sys.L2CL) + + // Reaches EL Sync Target and advances + target = uint64(40) + sys.L2CL2.Reached(types.LocalUnsafe, target, 30) + + session, err = syncTesterClient.GetSession(ctx) + require.NoError(err) + require.False(session.ELSyncActive) + + // Check CL2 view is consistent with read only EL + unsafeHead := sys.L2CL2.SyncStatus().UnsafeL2 + require.GreaterOrEqual(unsafeHead.Number, target) + require.Equal(sys.L2EL.BlockRefByNumber(unsafeHead.Number).Hash, unsafeHead.Hash) +} diff --git a/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/init_test.go b/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/init_test.go new file mode 100644 index 0000000000000..3b90eb66724f0 --- /dev/null +++ b/op-acceptance-tests/tests/sync_tester/sync_tester_elsync/init_test.go @@ -0,0 +1,17 @@ +package sync_tester_elsync + +import ( + "testing" + + "github.com/ethereum-optimism/optimism/op-devstack/compat" + "github.com/ethereum-optimism/optimism/op-devstack/presets" +) + +func TestMain(m *testing.M) { + presets.DoMain(m, + presets.WithExecutionLayerSyncOnVerifiers(), + presets.WithSimpleWithSyncTester(), + presets.WithELSyncTarget(35), + presets.WithCompatibleTypes(compat.SysGo), + ) +} diff --git a/op-deployer/pkg/deployer/forge/binary_test.go b/op-deployer/pkg/deployer/forge/binary_test.go index 2d1fffb1e4e42..799a7fa76a973 100644 --- a/op-deployer/pkg/deployer/forge/binary_test.go +++ b/op-deployer/pkg/deployer/forge/binary_test.go @@ -44,7 +44,7 @@ func TestStandardBinary_ForgeBins(t *testing.T) { ) require.NoError(t, err) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() require.NoError(t, bin.Ensure(ctx)) }) diff --git a/op-deployer/pkg/deployer/forge/client.go b/op-deployer/pkg/deployer/forge/client.go index d612077191c63..f9f6ef9be6e17 100644 --- a/op-deployer/pkg/deployer/forge/client.go +++ b/op-deployer/pkg/deployer/forge/client.go @@ -40,7 +40,7 @@ func NewClient(binary Binary) *Client { func (c *Client) Version(ctx context.Context) (VersionInfo, error) { buf := new(bytes.Buffer) if err := c.execCmd(ctx, buf, io.Discard, "--version"); err != nil { - return VersionInfo{}, fmt.Errorf("failed to execute command: %w", err) + return VersionInfo{}, fmt.Errorf("failed to run forge version command: %w", err) } outputStr := buf.String() matches := versionRegexp.FindAllStringSubmatch(outputStr, -1) @@ -67,7 +67,7 @@ func (c *Client) RunScript(ctx context.Context, script string, sig string, args cliOpts = append(cliOpts, opts...) cliOpts = append(cliOpts, "--sig", sig, script, "0x"+hex.EncodeToString(args)) if err := c.execCmd(ctx, buf, io.Discard, cliOpts...); err != nil { - return "", fmt.Errorf("failed to execute command: %w", err) + return "", fmt.Errorf("failed to execute forge script: %w", err) } return buf.String(), nil } @@ -93,7 +93,7 @@ func (c *Client) execCmd(ctx context.Context, stdout io.Writer, stderr io.Writer cmd.Stderr = mwStderr cmd.Dir = c.Wd if err := cmd.Run(); err != nil { - return fmt.Errorf("failed to execute forge: %w", err) + return fmt.Errorf("failed to run forge command: %w", err) } return nil } @@ -106,6 +106,11 @@ type ScriptCallDecoder[O any] interface { Decode(raw []byte) (O, error) } +// ScriptCaller is a function that calls a forge script +// Ouputs: +// - Return value of the script (decoded into go type) +// - Bool indicating if the script was recompiled (mostly used for testing) +// - Error if the script fails to run type ScriptCaller[I any, O any] func(ctx context.Context, input I, opts ...string) (O, bool, error) func NewScriptCaller[I any, O any](client *Client, script string, sig string, encoder ScriptCallEncoder[I], decoder ScriptCallDecoder[O]) ScriptCaller[I, O] { @@ -117,7 +122,7 @@ func NewScriptCaller[I any, O any](client *Client, script string, sig string, en } rawOut, err := client.RunScript(ctx, script, sig, encArgs, opts...) if err != nil { - return out, false, fmt.Errorf("failed to execute forge: %w", err) + return out, false, fmt.Errorf("failed to run forge script: %w", err) } sigilMatches := sigilRegexp.FindAllStringSubmatch(rawOut, -1) if len(sigilMatches) != 1 || len(sigilMatches[0]) != 2 { diff --git a/op-deployer/pkg/deployer/forge/client_test.go b/op-deployer/pkg/deployer/forge/client_test.go index e26203b0a0d06..42020fa82c8d8 100644 --- a/op-deployer/pkg/deployer/forge/client_test.go +++ b/op-deployer/pkg/deployer/forge/client_test.go @@ -3,26 +3,24 @@ package forge import ( "bytes" "context" - "fmt" "io" "os" "path" "path/filepath" - "reflect" "regexp" "runtime" "strings" "testing" "time" - "github.com/ethereum/go-ethereum/accounts/abi" - "github.com/stretchr/testify/require" ) type ioStruct struct { - ID uint8 - Data []byte + ID uint8 + Data []byte + Slice []uint32 + Array [3]uint64 } func TestMinimalSources(t *testing.T) { @@ -44,19 +42,28 @@ func TestMinimalSources(t *testing.T) { // Then see if we can successfully run a script cl.Wd = tmpDir - encDec := new(testEncoderDecoder) - caller := NewScriptCaller[ioStruct, ioStruct](cl, "script/Test.s.sol:TestScript", "run(bytes)", encDec, encDec) + caller := NewScriptCaller( + cl, + "script/Test.s.sol:TestScript", + "runEncoded(bytes)", + &BytesScriptEncoder[ioStruct]{TypeName: "ioStruct"}, + &BytesScriptDecoder[ioStruct]{TypeName: "ioStruct"}, + ) // It should not recompile since we included the cache. in := ioStruct{ - ID: 1, - Data: []byte{0x01, 0x02, 0x03, 0x04}, + ID: 1, + Data: []byte{0x01, 0x02, 0x03, 0x04}, + Slice: []uint32{0x01, 0x02, 0x03, 0x04}, + Array: [3]uint64{0x01, 0x02, 0x03}, } out, changed, err := caller(ctx, in) require.NoError(t, err) require.False(t, changed) require.EqualValues(t, ioStruct{ - ID: 2, - Data: in.Data, + ID: 2, + Data: in.Data, + Slice: in.Slice, + Array: in.Array, }, out) } @@ -90,65 +97,40 @@ func TestScriptCaller(t *testing.T) { cl.Wd = projDir(t) require.NoError(t, cl.Clean(ctx)) - encDec := new(testEncoderDecoder) - caller := NewScriptCaller[ioStruct, ioStruct](cl, "script/Test.s.sol:TestScript", "run(bytes)", encDec, encDec) + caller := NewScriptCaller( + cl, + "script/Test.s.sol:TestScript", + "runEncoded(bytes)", + &BytesScriptEncoder[ioStruct]{TypeName: "ioStruct"}, + &BytesScriptDecoder[ioStruct]{TypeName: "ioStruct"}, + ) + in := ioStruct{ - ID: 1, - Data: []byte{0x01, 0x02}, + ID: 1, + Data: []byte{0x01, 0x02}, + Slice: []uint32{0x01, 0x02, 0x03, 0x04}, + Array: [3]uint64{0x01, 0x02, 0x03}, } out, recompiled, err := caller(context.Background(), in) require.NoError(t, err) require.True(t, recompiled) require.EqualValues(t, ioStruct{ - ID: 2, - Data: []byte{0x01, 0x02}, + ID: 2, + Data: in.Data, + Slice: in.Slice, + Array: in.Array, }, out) out, recompiled, err = caller(context.Background(), in) require.NoError(t, err) require.False(t, recompiled) require.EqualValues(t, ioStruct{ - ID: 2, - Data: []byte{0x01, 0x02}, + ID: 2, + Data: in.Data, + Slice: in.Slice, + Array: in.Array, }, out) } -var ( - runArgs = abi.Arguments{{Type: mustTuple()}} -) - -func mustTuple() abi.Type { - t, err := abi.NewType("tuple", "", []abi.ArgumentMarshaling{ - {Name: "ID", Type: "uint8"}, - {Name: "Data", Type: "bytes"}, - }) - if err != nil { - panic(err) - } - return t -} - -type testEncoderDecoder struct { -} - -func (t *testEncoderDecoder) Encode(in ioStruct) ([]byte, error) { - return runArgs.Pack(in) -} - -func (t *testEncoderDecoder) Decode(v []byte) (ioStruct, error) { - var out ioStruct - decoded, err := runArgs.Unpack(v) - if err != nil { - return out, fmt.Errorf("error unpacking args: %w", err) - } - // Geth's ABI decoding library returns an anonymous strut - // which requires reflection to parse. - anonStruct := decoded[0] - val := reflect.ValueOf(anonStruct) - out.ID = uint8(val.FieldByName("ID").Uint()) - out.Data = val.FieldByName("Data").Bytes() - return out, nil -} - func copyDir(src, dst string) error { return filepath.Walk(src, func(path string, info os.FileInfo, err error) error { if err != nil { diff --git a/op-deployer/pkg/deployer/forge/encoding.go b/op-deployer/pkg/deployer/forge/encoding.go new file mode 100644 index 0000000000000..c5ab9302b5dc1 --- /dev/null +++ b/op-deployer/pkg/deployer/forge/encoding.go @@ -0,0 +1,170 @@ +package forge + +import ( + "fmt" + "math/big" + "reflect" + + "github.com/ethereum/go-ethereum/accounts/abi" + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/params" +) + +func GoStructToABITuple(structType reflect.Type, tupleName string) (abi.Type, error) { + var components []abi.ArgumentMarshaling + + for i := 0; i < structType.NumField(); i++ { + field := structType.Field(i) + + abiType, err := GoTypeToABIType(field.Type) + if err != nil { + return abi.Type{}, fmt.Errorf("unsupported field type %s: %w", field.Type, err) + } + + components = append(components, abi.ArgumentMarshaling{ + Name: field.Name, + Type: abiType, + }) + } + + return abi.NewType("tuple", tupleName, components) +} + +func GoTypeToABIType(goType reflect.Type) (string, error) { + // handle pointers by dereferencing + if goType.Kind() == reflect.Ptr { + goType = goType.Elem() + } + + // standard go types + switch goType.Kind() { + case reflect.Slice: + elemType := goType.Elem() + + // special case: []byte -> "bytes" + if elemType.Kind() == reflect.Uint8 { + return "bytes", nil + } + + // recursive: []T -> "T[]" + elemABI, err := GoTypeToABIType(elemType) + if err != nil { + return "", fmt.Errorf("unsupported slice element type: %w", err) + } + return elemABI + "[]", nil + + case reflect.Array: + elemType := goType.Elem() + arrayLen := goType.Len() + + // recursive: [N]T -> "T[N]" + elemABI, err := GoTypeToABIType(elemType) + if err != nil { + return "", fmt.Errorf("unsupported array element type: %w", err) + } + return fmt.Sprintf("%s[%d]", elemABI, arrayLen), nil + + case reflect.String: + return "string", nil + case reflect.Bool: + return "bool", nil + case reflect.Uint8: + return "uint8", nil + case reflect.Uint16: + return "uint16", nil + case reflect.Uint32: + return "uint32", nil + case reflect.Uint64, reflect.Uint: + return "uint64", nil + case reflect.Int8: + return "int8", nil + case reflect.Int16: + return "int16", nil + case reflect.Int32: + return "int32", nil + case reflect.Int64, reflect.Int: + return "int64", nil + } + + // non-standard go types + switch goType { + case reflect.TypeOf(common.Address{}): + return "address", nil + case reflect.TypeOf(common.Hash{}): + return "bytes32", nil + case reflect.TypeOf(params.ProtocolVersion{}): + return "bytes32", nil + case reflect.TypeOf(big.NewInt(0)).Elem(): + return "uint256", nil + } + + return "", fmt.Errorf("unable to convert go type to abi type: %s", goType) +} + +func ConvertAnonStructToTyped[T any](anonStruct interface{}) (T, error) { + var result T + + srcVal := reflect.ValueOf(anonStruct) + destVal := reflect.ValueOf(&result).Elem() + + // Ensure both are structs + if srcVal.Kind() != reflect.Struct || destVal.Kind() != reflect.Struct { + return result, fmt.Errorf("both source and destination must be structs") + } + + // Check field count matches + if srcVal.NumField() != destVal.NumField() { + return result, fmt.Errorf("field count mismatch: source has %d, destination has %d", + srcVal.NumField(), destVal.NumField()) + } + + // Copy fields by index (assumes same field order) + for i := 0; i < srcVal.NumField(); i++ { + srcField := srcVal.Field(i) + destField := destVal.Field(i) + + if destField.CanSet() { + destField.Set(srcField) + } + } + + return result, nil +} + +type BytesScriptEncoder[T any] struct { + TypeName string // e.g., "DeploySuperchainInput" +} + +func (e *BytesScriptEncoder[T]) Encode(input T) ([]byte, error) { + inputType, err := GoStructToABITuple(reflect.TypeOf(input), e.TypeName) + if err != nil { + return nil, fmt.Errorf("failed to create input type: %w", err) + } + + args := abi.Arguments{{Type: inputType}} + return args.Pack(input) +} + +type BytesScriptDecoder[T any] struct { + TypeName string // e.g., "DeploySuperchainOutput" +} + +func (d *BytesScriptDecoder[T]) Decode(rawOutput []byte) (T, error) { + var zero T + outputType, err := GoStructToABITuple(reflect.TypeOf(zero), d.TypeName) + if err != nil { + return zero, fmt.Errorf("failed to create output type: %w", err) + } + + args := abi.Arguments{{Type: outputType}} + unpacked, err := args.Unpack(rawOutput) + if err != nil { + return zero, fmt.Errorf("failed to unpack output: %w", err) + } + + if len(unpacked) != 1 { + return zero, fmt.Errorf("expected 1 unpacked value, got %d", len(unpacked)) + } + + return ConvertAnonStructToTyped[T](unpacked[0]) +} diff --git a/op-deployer/pkg/deployer/forge/testdata/testproject/script/Test.s.sol b/op-deployer/pkg/deployer/forge/testdata/testproject/script/Test.s.sol index 884252c220a4f..4d6c2e11cd2a8 100644 --- a/op-deployer/pkg/deployer/forge/testdata/testproject/script/Test.s.sol +++ b/op-deployer/pkg/deployer/forge/testdata/testproject/script/Test.s.sol @@ -5,18 +5,22 @@ contract TestScript { struct Input { uint8 id; bytes data; + uint32[] slice; + uint256[3] array; } struct Output { uint8 id; bytes data; + uint32[] slice; + uint256[3] array; } function _run(Input memory _input) public pure returns (Output memory) { - return Output({ id: 0x02, data: _input.data }); + return Output({ id: 0x02, data: _input.data, slice: _input.slice, array: _input.array }); } - function run(bytes memory _input) public pure returns (bytes memory) { + function runEncoded(bytes memory _input) public pure returns (bytes memory) { Input memory input = abi.decode(_input, (Input)); Output memory output = _run(input); return abi.encode(output); diff --git a/op-devstack/dsl/l2_cl.go b/op-devstack/dsl/l2_cl.go index 79f7d38a051dc..42bf3dd038cdd 100644 --- a/op-devstack/dsl/l2_cl.go +++ b/op-devstack/dsl/l2_cl.go @@ -316,6 +316,20 @@ func (cl *L2CLNode) ConnectPeer(peer *L2CLNode) { cl.require.NoError(err, "failed to connect peer") } +func (cl *L2CLNode) IsP2PConnected(peer *L2CLNode) { + myInfo := cl.PeerInfo() + strategy := &retry.ExponentialStrategy{Min: 10 * time.Second, Max: 30 * time.Second, MaxJitter: 250 * time.Millisecond} + err := retry.Do0(cl.ctx, 5, strategy, func() error { + for _, p := range peer.Peers().Peers { + if p.PeerID == myInfo.PeerID { + return nil + } + } + return errors.New("peer not connected yet") + }) + cl.require.NoError(err, "peer not connected") +} + type safeHeadDbMatchOpts struct { minRequiredL2Block *uint64 } diff --git a/op-devstack/presets/sync_tester_config.go b/op-devstack/presets/sync_tester_config.go index fd436d7cbf2e8..8687952a00599 100644 --- a/op-devstack/presets/sync_tester_config.go +++ b/op-devstack/presets/sync_tester_config.go @@ -19,7 +19,7 @@ func WithELSyncTarget(elSyncTarget uint64) stack.CommonOption { return stack.MakeCommon( sysgo.WithGlobalSyncTesterELOption(sysgo.SyncTesterELOptionFn( func(_ devtest.P, id stack.L2ELNodeID, cfg *sysgo.SyncTesterELConfig) { - cfg.ELSyncEnabled = true + cfg.ELSyncActive = true cfg.ELSyncTarget = elSyncTarget }))) } diff --git a/op-devstack/sysgo/l2_el_synctester.go b/op-devstack/sysgo/l2_el_synctester.go index 67eca6d69ae95..5ee34116b0816 100644 --- a/op-devstack/sysgo/l2_el_synctester.go +++ b/op-devstack/sysgo/l2_el_synctester.go @@ -36,14 +36,14 @@ type SyncTesterEL struct { } type SyncTesterELConfig struct { - FCUState eth.FCUState - ELSyncEnabled bool - ELSyncTarget uint64 + FCUState eth.FCUState + ELSyncActive bool + ELSyncTarget uint64 } func (cfg *SyncTesterELConfig) Path() string { path := fmt.Sprintf("?latest=%d&safe=%d&finalized=%d", cfg.FCUState.Latest, cfg.FCUState.Safe, cfg.FCUState.Finalized) - if cfg.ELSyncEnabled { + if cfg.ELSyncActive { path += fmt.Sprintf("&el_sync_target=%d", cfg.ELSyncTarget) } return path @@ -51,9 +51,9 @@ func (cfg *SyncTesterELConfig) Path() string { func DefaultSyncTesterELConfig() *SyncTesterELConfig { return &SyncTesterELConfig{ - FCUState: eth.FCUState{Latest: 0, Safe: 0, Finalized: 0}, - ELSyncEnabled: false, - ELSyncTarget: 0, + FCUState: eth.FCUState{Latest: 0, Safe: 0, Finalized: 0}, + ELSyncActive: false, + ELSyncTarget: 0, } } diff --git a/op-devstack/sysgo/system_synctester.go b/op-devstack/sysgo/system_synctester.go index 4b644e4171893..d5e6d6f9c0ff0 100644 --- a/op-devstack/sysgo/system_synctester.go +++ b/op-devstack/sysgo/system_synctester.go @@ -66,6 +66,9 @@ func DefaultSimpleSystemWithSyncTester(dest *DefaultSimpleSystemWithSyncTesterID opt.Add(WithSyncTesterL2ELNode(ids.SyncTesterL2EL, ids.L2EL)) opt.Add(WithL2CLNode(ids.L2CL2, ids.L1CL, ids.L1EL, ids.SyncTesterL2EL)) + // P2P Connect CLs to signal unsafe heads + opt.Add(WithL2CLP2PConnection(ids.L2CL, ids.L2CL2)) + opt.Add(stack.Finally(func(orch *Orchestrator) { *dest = ids })) diff --git a/op-node/node/node.go b/op-node/node/node.go index b3fbb0f0cfd70..7c0dc9dc39388 100644 --- a/op-node/node/node.go +++ b/op-node/node/node.go @@ -555,7 +555,7 @@ func (n *OpNode) initP2P(cfg *config.Config) (err error) { } // embed syncDeriver and tracer(optional) to the blockReceiver to handle unsafe payloads via p2p rec := p2p.NewBlockReceiver(n.log, n.metrics, n.l2Driver.SyncDeriver, n.cfg.Tracer) - n.p2pNode, err = p2p.NewNodeP2P(n.resourcesCtx, &cfg.Rollup, n.log, cfg.P2P, rec, n.l2Source, n.runCfg, n.metrics, false) + n.p2pNode, err = p2p.NewNodeP2P(n.resourcesCtx, &cfg.Rollup, n.log, cfg.P2P, rec, n.l2Source, n.runCfg, n.metrics) if err != nil { return } diff --git a/op-node/p2p/host_test.go b/op-node/p2p/host_test.go index 6d55b36a3cc6b..9f461b2fed792 100644 --- a/op-node/p2p/host_test.go +++ b/op-node/p2p/host_test.go @@ -120,7 +120,7 @@ func TestP2PFull(t *testing.T) { runCfgB := &testutils.MockRuntimeConfig{P2PSeqAddress: common.Address{0x42}} logA := testlog.Logger(t, log.LevelError).New("host", "A") - nodeA, err := NewNodeP2P(context.Background(), &rollup.Config{}, logA, &confA, &mockGossipIn{}, nil, runCfgA, metrics.NoopMetrics, false) + nodeA, err := NewNodeP2P(context.Background(), &rollup.Config{}, logA, &confA, &mockGossipIn{}, nil, runCfgA, metrics.NoopMetrics) require.NoError(t, err) defer nodeA.Close() @@ -149,7 +149,7 @@ func TestP2PFull(t *testing.T) { logB := testlog.Logger(t, log.LevelError).New("host", "B") - nodeB, err := NewNodeP2P(context.Background(), &rollup.Config{}, logB, &confB, &mockGossipIn{}, nil, runCfgB, metrics.NoopMetrics, false) + nodeB, err := NewNodeP2P(context.Background(), &rollup.Config{}, logB, &confB, &mockGossipIn{}, nil, runCfgB, metrics.NoopMetrics) require.NoError(t, err) defer nodeB.Close() hostB := nodeB.Host() @@ -321,7 +321,7 @@ func TestDiscovery(t *testing.T) { resourcesCtx, resourcesCancel := context.WithCancel(context.Background()) defer resourcesCancel() - nodeA, err := NewNodeP2P(context.Background(), rollupCfg, logA, &confA, &mockGossipIn{}, nil, runCfgA, metrics.NoopMetrics, false) + nodeA, err := NewNodeP2P(context.Background(), rollupCfg, logA, &confA, &mockGossipIn{}, nil, runCfgA, metrics.NoopMetrics) require.NoError(t, err) defer nodeA.Close() hostA := nodeA.Host() @@ -336,7 +336,7 @@ func TestDiscovery(t *testing.T) { confB.DiscoveryDB = discDBC // Start B - nodeB, err := NewNodeP2P(context.Background(), rollupCfg, logB, &confB, &mockGossipIn{}, nil, runCfgB, metrics.NoopMetrics, false) + nodeB, err := NewNodeP2P(context.Background(), rollupCfg, logB, &confB, &mockGossipIn{}, nil, runCfgB, metrics.NoopMetrics) require.NoError(t, err) defer nodeB.Close() hostB := nodeB.Host() @@ -351,7 +351,7 @@ func TestDiscovery(t *testing.T) { }}) // Start C - nodeC, err := NewNodeP2P(context.Background(), rollupCfg, logC, &confC, &mockGossipIn{}, nil, runCfgC, metrics.NoopMetrics, false) + nodeC, err := NewNodeP2P(context.Background(), rollupCfg, logC, &confC, &mockGossipIn{}, nil, runCfgC, metrics.NoopMetrics) require.NoError(t, err) defer nodeC.Close() hostC := nodeC.Host() diff --git a/op-node/p2p/node.go b/op-node/p2p/node.go index e5c3dccbc31cf..149eec5a8f8e1 100644 --- a/op-node/p2p/node.go +++ b/op-node/p2p/node.go @@ -61,7 +61,6 @@ func NewNodeP2P( l2Chain L2Chain, runCfg GossipRuntimeConfig, metrics metrics.Metricer, - elSyncEnabled bool, ) (*NodeP2P, error) { if setup == nil { return nil, errors.New("p2p node cannot be created without setup") @@ -70,7 +69,7 @@ func NewNodeP2P( return nil, errors.New("SetupP2P.Disabled is true") } var n NodeP2P - if err := n.init(resourcesCtx, rollupCfg, log, setup, gossipIn, l2Chain, runCfg, metrics, elSyncEnabled); err != nil { + if err := n.init(resourcesCtx, rollupCfg, log, setup, gossipIn, l2Chain, runCfg, metrics); err != nil { closeErr := n.Close() if closeErr != nil { log.Error("failed to close p2p after starting with err", "closeErr", closeErr, "err", err) @@ -94,7 +93,6 @@ func (n *NodeP2P) init( l2Chain L2Chain, runCfg GossipRuntimeConfig, metrics metrics.Metricer, - elSyncEnabled bool, ) error { bwc := p2pmetrics.NewBandwidthCounter() @@ -131,7 +129,7 @@ func (n *NodeP2P) init( n.appScorer = &NoopApplicationScorer{} } // Activate the P2P req-resp sync if enabled by feature-flag. - if setup.ReqRespSyncEnabled() && !elSyncEnabled { + if setup.ReqRespSyncEnabled() { n.syncCl = NewSyncClient(log, rollupCfg, n.host, gossipIn.OnUnsafeL2Payload, metrics, n.appScorer) n.host.Network().Notify(&network.NotifyBundle{ ConnectedF: func(nw network.Network, conn network.Conn) { diff --git a/op-service/apis/sync_tester.go b/op-service/apis/sync_tester.go index 1a826470d297c..7e345cb2df881 100644 --- a/op-service/apis/sync_tester.go +++ b/op-service/apis/sync_tester.go @@ -21,6 +21,7 @@ type SyncTester interface { type SyncAPI interface { GetSession(ctx context.Context) (*eth.SyncTesterSession, error) DeleteSession(ctx context.Context) error + ResetSession(ctx context.Context) error ListSessions(ctx context.Context) ([]string, error) } diff --git a/op-service/eth/synctester_session.go b/op-service/eth/synctester_session.go index b8f107494a9a2..4cb4f9377702b 100644 --- a/op-service/eth/synctester_session.go +++ b/op-service/eth/synctester_session.go @@ -14,25 +14,51 @@ type FCUState struct { type SyncTesterSession struct { sync.Mutex - SessionID string `json:"sessionID"` + SessionID string `json:"session_id"` // Non canonical view of the chain Validated uint64 `json:"validated"` // Canonical view of the chain - CurrentState FCUState `json:"currentState"` + CurrentState FCUState `json:"current_state"` // payloads Payloads map[PayloadID]*ExecutionPayloadEnvelope `json:"-"` - InitialState FCUState `json:"initialState"` + ELSyncTarget uint64 `json:"el_sync_target"` + ELSyncActive bool `json:"el_sync_active"` + + InitialState FCUState `json:"initial_state"` + InitialELSyncActive bool `json:"initial_el_sync_active"` } -func (s *SyncTesterSession) UpdateFCUState(latest, safe, finalized uint64) { +func (s *SyncTesterSession) UpdateFCULatest(latest uint64) { s.CurrentState.Latest = latest +} + +func (s *SyncTesterSession) UpdateFCUSafe(safe uint64) { s.CurrentState.Safe = safe +} + +func (s *SyncTesterSession) UpdateFCUFinalized(finalized uint64) { s.CurrentState.Finalized = finalized } -func NewSyncTesterSession(sessionID string, latest, safe, finalized uint64) *SyncTesterSession { +func (s *SyncTesterSession) FinishELSync(target uint64) { + s.ELSyncActive = false + s.Validated = target +} + +func (s *SyncTesterSession) IsELSyncFinished() bool { + return !s.ELSyncActive +} + +func (s *SyncTesterSession) ResetSession() { + s.CurrentState = s.InitialState + s.Validated = s.InitialState.Latest + s.Payloads = make(map[PayloadID]*ExecutionPayloadEnvelope) + s.ELSyncActive = s.InitialELSyncActive +} + +func NewSyncTesterSession(sessionID string, latest, safe, finalized, elSyncTarget uint64, elSyncActive bool) *SyncTesterSession { return &SyncTesterSession{ SessionID: sessionID, Validated: latest, @@ -41,11 +67,14 @@ func NewSyncTesterSession(sessionID string, latest, safe, finalized uint64) *Syn Safe: safe, Finalized: finalized, }, - Payloads: make(map[PayloadID]*ExecutionPayloadEnvelope), + Payloads: make(map[PayloadID]*ExecutionPayloadEnvelope), + ELSyncTarget: elSyncTarget, + ELSyncActive: elSyncActive, InitialState: FCUState{ Latest: latest, Safe: safe, Finalized: finalized, }, + InitialELSyncActive: elSyncActive, } } diff --git a/op-service/sources/sync_tester_client.go b/op-service/sources/sync_tester_client.go index 7285e75e705fd..ba19034537193 100644 --- a/op-service/sources/sync_tester_client.go +++ b/op-service/sources/sync_tester_client.go @@ -41,3 +41,7 @@ func (cl *SyncTesterClient) ListSessions(ctx context.Context) ([]string, error) func (cl *SyncTesterClient) DeleteSession(ctx context.Context) error { return cl.client.CallContext(ctx, nil, "sync_deleteSession") } + +func (cl *SyncTesterClient) ResetSession(ctx context.Context) error { + return cl.client.CallContext(ctx, nil, "sync_resetSession") +} diff --git a/op-sync-tester/example_config.yaml b/op-sync-tester/example_config.yaml index 04092229c8ff5..a690bcb994417 100644 --- a/op-sync-tester/example_config.yaml +++ b/op-sync-tester/example_config.yaml @@ -1,7 +1,7 @@ synctesters: local: chain_id: 2151908 - el_rpc: http://localhost:62654/ + el_rpc: http://localhost:65293/ sepolia: chain_id: 11155420 el_rpc: https://sepolia.optimism.io diff --git a/op-sync-tester/synctester/backend/sync_tester.go b/op-sync-tester/synctester/backend/sync_tester.go index c51060f3c16af..647093309a457 100644 --- a/op-sync-tester/synctester/backend/sync_tester.go +++ b/op-sync-tester/synctester/backend/sync_tester.go @@ -88,6 +88,15 @@ func (s *SyncTester) DeleteSession(ctx context.Context) error { return err } +func (s *SyncTester) ResetSession(ctx context.Context) error { + _, err := session.WithSession(s.sessMgr, ctx, s.log, func(session *eth.SyncTesterSession, logger log.Logger) (any, error) { + logger.Debug("ResetSession") + session.ResetSession() + return struct{}{}, nil + }) + return err +} + func (s *SyncTester) ListSessions(ctx context.Context) ([]string, error) { ids := s.sessMgr.SessionIDs() s.log.Debug("ListSessions", "count", len(ids)) @@ -351,8 +360,10 @@ func (s *SyncTester) forkchoiceUpdated(ctx context.Context, session *eth.SyncTes // Let CL backfill via newPayload return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionSyncing}, PayloadID: nil}, nil } + // Equivalent to SetCanonical + session.UpdateFCULatest(candLatest.NumberU64()) + logger.Debug("Updated FCU State", "latest", session.CurrentState.Latest) // Simulate db check for finalized head - var finalizedNum uint64 if state.FinalizedBlockHash != (common.Hash{}) { // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/paris.md#specification-1 // Spec: MUST return -38002: Invalid forkchoice state error if the payload referenced by forkchoiceState.headBlockHash is VALID and a payload referenced by either forkchoiceState.finalizedBlockHash or forkchoiceState.safeBlockHash does not belong to the chain defined by forkchoiceState.headBlockHash. @@ -360,13 +371,15 @@ func (s *SyncTester) forkchoiceUpdated(ctx context.Context, session *eth.SyncTes if err != nil { return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionInvalid}, PayloadID: nil}, engine.InvalidForkChoiceState.With(errors.New("finalized block not available")) } - finalizedNum = candFinalized.NumberU64() + finalizedNum := candFinalized.NumberU64() if session.CurrentState.Latest < finalizedNum { return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionInvalid}, PayloadID: nil}, engine.InvalidForkChoiceState.With(errors.New("finalized block not canonical")) } + // Equivalent to SetFinalized + session.UpdateFCUFinalized(finalizedNum) + logger.Debug("Updated FCU State", "finalized", session.CurrentState.Finalized) } // Simulate db check for safe head - var safeNum uint64 if state.SafeBlockHash != (common.Hash{}) { // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/paris.md#specification-1 // Spec: MUST return -38002: Invalid forkchoice state error if the payload referenced by forkchoiceState.headBlockHash is VALID and a payload referenced by either forkchoiceState.finalizedBlockHash or forkchoiceState.safeBlockHash does not belong to the chain defined by forkchoiceState.headBlockHash. @@ -374,10 +387,13 @@ func (s *SyncTester) forkchoiceUpdated(ctx context.Context, session *eth.SyncTes if err != nil { return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionInvalid}, PayloadID: nil}, engine.InvalidForkChoiceState.With(errors.New("safe block not available")) } - safeNum = candSafe.NumberU64() + safeNum := candSafe.NumberU64() if session.CurrentState.Latest < safeNum { return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionInvalid}, PayloadID: nil}, engine.InvalidForkChoiceState.With(errors.New("safe block not canonical")) } + // Equivalent to SetSafe + session.UpdateFCUSafe(safeNum) + logger.Debug("Updated FCU State", "safe", session.CurrentState.Safe) } var id *engine.PayloadID if attr != nil { @@ -436,8 +452,6 @@ func (s *SyncTester) forkchoiceUpdated(ctx context.Context, session *eth.SyncTes logger.Debug("Store payload", "payloadID", payloadID) session.Payloads[payloadID] = payloadEnv } - session.UpdateFCUState(candLatest.NumberU64(), safeNum, finalizedNum) - logger.Debug("Updated FCU State") // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/paris.md#specification-1 // Spec: Client software MUST respond to this method call in the following way: {payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: buildProcessId} if the payload is deemed VALID and the build process has begun return ð.ForkchoiceUpdatedResult{PayloadStatus: eth.PayloadStatusV1{Status: eth.ExecutionValid, LatestValidHash: &state.HeadBlockHash}, PayloadID: id}, nil @@ -557,6 +571,44 @@ func (s *SyncTester) NewPayloadV4(ctx context.Context, payload *eth.ExecutionPay }) } +func (s *SyncTester) validatePayload(logger log.Logger, isCanyon, isIsthmus bool, block *types.Block, payload *eth.ExecutionPayload, beaconRoot *common.Hash) (*eth.PayloadStatusV1, error) { + // Already have the block locally or advance single block without setting the head + // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/shanghai.md#specification + // Spec: MUST return {status: INVALID, latestValidHash: null, validationError: errorMessage | null} if the blockHash validation has failed. + blockHash := block.Hash() + config := ¶ms.ChainConfig{} + if isCanyon { + config.CanyonTime = new(uint64) + } + if isIsthmus { + config.IsthmusTime = new(uint64) + } + correctPayload, err := eth.BlockAsPayload(block, config) + if err != nil { + // The failure is from the EL processing so consider as a server error and make CL retry + return ð.PayloadStatusV1{Status: eth.ExecutionInvalid}, engine.GenericServerError.With(wrapSyncTesterError("failed to convert block to payload", err)) + } + // Sanity check parent beacon block root and block hash by recomputation + if !isIsthmus { + // Depopulate withdrawal root field for block hash recomputation + if payload.WithdrawalsRoot != nil { + logger.Warn("Isthmus disabled but withdrawal roots included in payload not nil", "root", payload.WithdrawalsRoot) + } + payload.WithdrawalsRoot = nil + } + // Check given payload matches the payload derived using the read only EL block + if err := correctPayload.CheckEqual(payload); err != nil { + // Consider as block hash validation error when payload mismatch + return s.newPayloadInvalid(fmt.Errorf("payload check mismatch: %w", err), nil), nil + } + execEnvelope := eth.ExecutionPayloadEnvelope{ParentBeaconBlockRoot: beaconRoot, ExecutionPayload: payload} + actual, ok := execEnvelope.CheckBlockHash() + if blockHash != payload.BlockHash || !ok { + return s.newPayloadInvalid(fmt.Errorf("block hash check from execution envelope failed. %s != %s", blockHash, actual), nil), nil + } + return nil, nil +} + // newPayload validates and processes a new execution payload according to the // Engine API rules to simulate consensus-layer to execution-layer interactions // without advancing canonical chain state. @@ -642,51 +694,43 @@ func (s *SyncTester) newPayload(ctx context.Context, session *eth.SyncTesterSess return ð.PayloadStatusV1{Status: eth.ExecutionInvalid}, engine.InvalidParams.With(errors.New("non-nil withdrawals pre-shanghai")) } } - blockHash := block.Hash() + blockNumber := block.NumberU64() // We only attempt to advance non-canonical view of the chain, following the read only EL - if block.NumberU64() <= session.Validated+1 { - // Already have the block locally or advance single block without setting the head - // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/shanghai.md#specification - // Spec: MUST return {status: INVALID, latestValidHash: null, validationError: errorMessage | null} if the blockHash validation has failed. - config := ¶ms.ChainConfig{} - if isCanyon { - config.CanyonTime = new(uint64) + if blockNumber <= session.Validated+1 { + if status, err := s.validatePayload(logger, isCanyon, isIsthmus, block, payload, beaconRoot); status != nil { + return status, err } - if isIsthmus { - config.IsthmusTime = new(uint64) - } - correctPayload, err := eth.BlockAsPayload(block, config) - if err != nil { - // The failure is from the EL processing so consider as a server error and make CL retry - return ð.PayloadStatusV1{Status: eth.ExecutionInvalid}, engine.GenericServerError.With(wrapSyncTesterError("failed to convert block to payload", err)) - } - // Sanity check parent beacon block root and block hash by recomputation - if !isIsthmus { - // Depopulate withdrawal root field for block hash recomputation - if payload.WithdrawalsRoot != nil { - logger.Warn("Isthmus disabled but withdrawal roots included in payload not nil", "root", payload.WithdrawalsRoot) - } - payload.WithdrawalsRoot = nil - } - // Check given payload matches the payload derived using the read only EL block - if err := correctPayload.CheckEqual(payload); err != nil { - // Consider as block hash validation error when payload mismatch - return s.newPayloadInvalid(fmt.Errorf("payload check mismatch: %w", err), nil), nil - } - execEnvelope := eth.ExecutionPayloadEnvelope{ParentBeaconBlockRoot: beaconRoot, ExecutionPayload: payload} - actual, ok := execEnvelope.CheckBlockHash() - if blockHash != payload.BlockHash || !ok { - return s.newPayloadInvalid(fmt.Errorf("block hash check from execution envelope failed. %s != %s", blockHash, actual), nil), nil - } - if block.NumberU64() == session.Validated+1 { + if blockNumber == session.Validated+1 { // Advance single block without setting the head, equivalent to geth InsertBlockWithoutSetHead session.Validated += 1 logger.Debug("Advanced non canonical chain", "validated", session.Validated) } + if !session.IsELSyncFinished() && session.Validated == session.ELSyncTarget { + // Can reach here when not doing EL Sync on CL side but session is configured for EL Sync + logger.Debug("Non canonical chain reached EL Sync target", "validated", session.Validated) + session.FinishELSync(session.Validated) + } // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/paris.md#payload-validation // Spec: If validation succeeds, the response MUST contain {status: VALID, latestValidHash: payload.blockHash} return ð.PayloadStatusV1{Status: eth.ExecutionValid, LatestValidHash: &blockHash}, nil + } else if !session.IsELSyncFinished() { + if blockNumber == session.ELSyncTarget { + logger.Debug("Attempting to finish EL Sync on non canonical chain", "target", session.ELSyncTarget) + if status, err := s.validatePayload(logger, isCanyon, isIsthmus, block, payload, beaconRoot); status != nil { + return status, err + } + session.FinishELSync(blockNumber) + logger.Debug("Finished EL Sync by advancing non canonical chain", "validated", session.Validated) + // https://github.com/ethereum/execution-apis/blob/584905270d8ad665718058060267061ecfd79ca5/src/engine/paris.md#payload-validation + // Spec: If validation succeeds, the response MUST contain {status: VALID, latestValidHash: payload.blockHash} + return ð.PayloadStatusV1{Status: eth.ExecutionValid, LatestValidHash: &blockHash}, nil + } else if blockNumber < session.ELSyncTarget { + logger.Trace("EL Sync on progress", "target", blockNumber) + } else if blockNumber > session.ELSyncTarget { + // L2CL may never reach the EL Sync Target because the current number may keep increasing + logger.Warn("Received payload which has larger block number than EL Sync target", "current", blockNumber, "target", session.ELSyncTarget) + } } // Block not available so mark as syncing return ð.PayloadStatusV1{Status: eth.ExecutionSyncing}, nil diff --git a/op-sync-tester/synctester/frontend/sync.go b/op-sync-tester/synctester/frontend/sync.go index d0663aa803248..e4e8f7beb2fad 100644 --- a/op-sync-tester/synctester/frontend/sync.go +++ b/op-sync-tester/synctester/frontend/sync.go @@ -29,3 +29,7 @@ func (s *SyncFrontend) DeleteSession(ctx context.Context) error { func (s *SyncFrontend) ListSessions(ctx context.Context) ([]string, error) { return s.b.ListSessions(ctx) } + +func (s *SyncFrontend) ResetSession(ctx context.Context) error { + return s.b.ResetSession(ctx) +} diff --git a/op-sync-tester/synctester/middleware.go b/op-sync-tester/synctester/middleware.go index ca624c336fe82..522e37c002227 100644 --- a/op-sync-tester/synctester/middleware.go +++ b/op-sync-tester/synctester/middleware.go @@ -14,6 +14,9 @@ import ( var ErrInvalidSessionIDFormat = errors.New("invalid UUID") var ErrInvalidParams = errors.New("invalid param") +var ErrInvalidELSyncTarget = errors.New("invalid el sync target") + +const ELSyncTargetKey = "el_sync_target" func IsValidSessionID(sessionID string) error { u, err := uuid.Parse(sessionID) @@ -28,7 +31,8 @@ func IsValidSessionID(sessionID string) error { // parseSession inspects the incoming request to determine if it targets a session-specific route. // If the request path matches the pattern `/chain/{chain_id}/synctest/{uuid}`, it attempts to parse -// the UUID and optional query parameters (`latest`, `safe`, `finalized`) used to initialize the session. +// the UUID and optional query parameters (`latest`, `safe`, `finalized`, `el_sync_target`) used to +// initialize the session. // // If parsing succeeds, a backend.Session is attached to the request context, and the URL path is // rewritten to `/chain/{chain_id}/synctest` to enable consistent routing downstream. @@ -71,7 +75,18 @@ func parseSession(r *http.Request) (*http.Request, error) { if err != nil { return r, err } - sess := eth.NewSyncTesterSession(sessionID, latest, safe, finalized) + elSyncTarget, err := parseParam(ELSyncTargetKey) + if err != nil { + return r, err + } + elSyncActive := false + if elSyncTarget != 0 { + if elSyncTarget < latest { + return r, ErrInvalidELSyncTarget + } + elSyncActive = true + } + sess := eth.NewSyncTesterSession(sessionID, latest, safe, finalized, elSyncTarget, elSyncActive) ctx := session.WithSyncTesterSession(r.Context(), sess) // remove uuid path for routing r.URL.Path = "/" + strings.Join(segments[:3], "/") diff --git a/op-sync-tester/synctester/middleware_test.go b/op-sync-tester/synctester/middleware_test.go index 4da5fe7a9071e..f37968ce2b960 100644 --- a/op-sync-tester/synctester/middleware_test.go +++ b/op-sync-tester/synctester/middleware_test.go @@ -3,6 +3,7 @@ package synctester import ( "net/http" "net/url" + "strconv" "testing" "github.com/ethereum-optimism/optimism/op-service/eth" @@ -63,6 +64,31 @@ func TestParseSession_DefaultsToZero(t *testing.T) { require.Equal(t, session.InitialState, session.CurrentState) } +func TestParseSession_ELSyncTarget(t *testing.T) { + id := uuid.New().String() + query := url.Values{} + elSyncTarget := uint64(4) + query.Set(ELSyncTargetKey, strconv.Itoa(int(elSyncTarget))) + + req := newRequest("/chain/1/synctest/"+id, query) + + newReq, err := parseSession(req) + require.NoError(t, err) + require.NotNil(t, newReq) + + session, ok := session.SyncTesterSessionFromContext(newReq.Context()) + require.True(t, ok) + require.NotNil(t, session) + require.Equal(t, id, session.SessionID) + require.Equal(t, uint64(0), session.InitialState.Latest) + require.Equal(t, uint64(0), session.InitialState.Safe) + require.Equal(t, uint64(0), session.InitialState.Finalized) + require.Equal(t, session.InitialState.Latest, session.Validated) + require.Equal(t, session.InitialState, session.CurrentState) + require.True(t, session.ELSyncActive) + require.Equal(t, session.ELSyncTarget, elSyncTarget) +} + func TestParseSession_NoSessionInitialized(t *testing.T) { req := newRequest("/chain/1/synctest", nil) @@ -89,3 +115,16 @@ func TestParseSession_InvalidQueryParam(t *testing.T) { _, err := parseSession(req) require.ErrorIs(t, err, ErrInvalidParams) } + +func TestParseSession_InvalidELSyncTarget(t *testing.T) { + id := uuid.New().String() + query := url.Values{} + latest := 4 + elSyncTarget := latest - 1 + query.Set(eth.Unsafe, strconv.Itoa(latest)) + query.Set(ELSyncTargetKey, strconv.Itoa(elSyncTarget)) + + req := newRequest("/chain/1/synctest/"+id, query) + _, err := parseSession(req) + require.ErrorIs(t, err, ErrInvalidELSyncTarget) +} diff --git a/op-sync-tester/synctester/service.go b/op-sync-tester/synctester/service.go index c0758a99cf37a..7417785606c40 100644 --- a/op-sync-tester/synctester/service.go +++ b/op-sync-tester/synctester/service.go @@ -148,7 +148,7 @@ func (s *Service) initHTTPServer(cfg *config.Config) error { // middleware to initialize session handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { r, err := parseSession(r) - if errors.Is(err, ErrInvalidSessionIDFormat) || errors.Is(err, ErrInvalidParams) { + if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } diff --git a/packages/contracts-bedrock/interfaces/dispute/v2/IPermissionedDisputeGameV2.sol b/packages/contracts-bedrock/interfaces/dispute/v2/IPermissionedDisputeGameV2.sol index 1f8cb3b991903..78eb910d8648d 100644 --- a/packages/contracts-bedrock/interfaces/dispute/v2/IPermissionedDisputeGameV2.sol +++ b/packages/contracts-bedrock/interfaces/dispute/v2/IPermissionedDisputeGameV2.sol @@ -130,13 +130,11 @@ interface IPermissionedDisputeGameV2 is IDisputeGame { error BadAuth(); - function proposer() external view returns (address proposer_); - function challenger() external view returns (address challenger_); + function proposer() external pure returns (address proposer_); + function challenger() external pure returns (address challenger_); function __constructor__( - IFaultDisputeGameV2.GameConstructorParams memory _params, - address _proposer, - address _challenger + IFaultDisputeGameV2.GameConstructorParams memory _params ) external; } diff --git a/packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGameV2.json b/packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGameV2.json index 6e0e7681c7fb0..23dbddadfc085 100644 --- a/packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGameV2.json +++ b/packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGameV2.json @@ -32,16 +32,6 @@ "internalType": "struct FaultDisputeGameV2.GameConstructorParams", "name": "_params", "type": "tuple" - }, - { - "internalType": "address", - "name": "_proposer", - "type": "address" - }, - { - "internalType": "address", - "name": "_challenger", - "type": "address" } ], "stateMutability": "nonpayable", @@ -182,7 +172,7 @@ "type": "address" } ], - "stateMutability": "view", + "stateMutability": "pure", "type": "function" }, { @@ -660,7 +650,7 @@ "type": "address" } ], - "stateMutability": "view", + "stateMutability": "pure", "type": "function" }, { diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 921af92de4831..7e87d700fa8cd 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -196,12 +196,12 @@ "sourceCodeHash": "0x8fdd69d4bcd33a3d8b49a73ff5b6855f9ad5f7e2b7393e67cd755973b127b1e8" }, "src/dispute/v2/FaultDisputeGameV2.sol:FaultDisputeGameV2": { - "initCodeHash": "0x13ef27ad793c95be884dea8259ac06619bf13d10868c5fc9980bc402b59efb8d", - "sourceCodeHash": "0x47c141889e647820759a2eaa84c31be8acdce6427cc4fe7c00a482ba44d62b87" + "initCodeHash": "0xb5a7bfcbfcb445dc57fc88c07d7305191dc32cc0cf5580d50b4897229e4033c1", + "sourceCodeHash": "0x38fd8878a22564cd63e2bdc6af51edead373e2c4a90e631d2774078bbaa54ea6" }, "src/dispute/v2/PermissionedDisputeGameV2.sol:PermissionedDisputeGameV2": { - "initCodeHash": "0x7b1385d347dce625d63f71c13201fd90e63fcaeae17416911bccd9d670b3afc4", - "sourceCodeHash": "0xc5aa308a19fd9600607370693885155a3d6f9f7bf8a7c6ff93a389c37c136555" + "initCodeHash": "0xcff1406639ff8a83a4c948f412329956104bc7ee30eb8da169a2ba5ef6d8848f", + "sourceCodeHash": "0x53fdae5faf97beed5f23d4f285bed06766161ab15c88e3a388f84808471a73c3" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/dispute/v2/FaultDisputeGameV2.sol b/packages/contracts-bedrock/src/dispute/v2/FaultDisputeGameV2.sol index 9417b9649c775..6f50e7c612f39 100644 --- a/packages/contracts-bedrock/src/dispute/v2/FaultDisputeGameV2.sol +++ b/packages/contracts-bedrock/src/dispute/v2/FaultDisputeGameV2.sol @@ -151,9 +151,9 @@ contract FaultDisputeGameV2 is Clone, ISemver { uint256 internal constant HEADER_BLOCK_NUMBER_INDEX = 8; /// @notice Semantic version. - /// @custom:semver 2.0.0 + /// @custom:semver 2.1.0 function version() public pure virtual returns (string memory) { - return "2.0.0"; + return "2.1.0"; } /// @notice The starting timestamp of the game @@ -268,20 +268,7 @@ contract FaultDisputeGameV2 is Clone, ISemver { // This is to prevent adding extra or omitting bytes from to `extraData` that result in a different game UUID // in the factory, but are not used by the game, which would allow for multiple dispute games for the same // output proposal to be created. - // - // Expected length: 246 bytes - // - 4 bytes: selector - // - 2 bytes: CWIA length prefix - // - 20 bytes: creator address - // - 32 bytes: root claim - // - 32 bytes: l1 head - // - 32 bytes: extraData - // - 32 bytes: absolutePrestate - // - 20 bytes: vm address - // - 20 bytes: anchorStateRegistry address - // - 20 bytes: weth address - // - 32 bytes: l2ChainId - if (msg.data.length != 246) revert BadExtraData(); + if (msg.data.length != expectedInitCallDataLength()) revert BadExtraData(); // Grab the latest anchor root. (Hash root, uint256 rootBlockNumber) = anchorStateRegistry().getAnchorRoot(); @@ -340,6 +327,30 @@ contract FaultDisputeGameV2 is Clone, ISemver { GameType.unwrap(anchorStateRegistry().respectedGameType()) == GameType.unwrap(GAME_TYPE); } + /// @notice Returns the expected calldata length for the initialize method + function expectedInitCallDataLength() internal pure returns (uint256) { + // Expected length: 6 bytes + immutable args byte count + // - 4 bytes: selector + // - 2 bytes: CWIA length prefix + // - n bytes: Immutable args data + return 6 + immutableArgsByteCount(); + } + + /// @notice Returns the byte count of the immutable args for this contract. + function immutableArgsByteCount() internal pure virtual returns (uint256) { + // Expected length: 240 bytes + // - 20 bytes: creator address + // - 32 bytes: root claim + // - 32 bytes: l1 head + // - 32 bytes: extraData + // - 32 bytes: absolutePrestate + // - 20 bytes: vm address + // - 20 bytes: anchorStateRegistry address + // - 20 bytes: weth address + // - 32 bytes: l2ChainId + return 240; + } + //////////////////////////////////////////////////////////////// // `IFaultDisputeGame` impl // //////////////////////////////////////////////////////////////// diff --git a/packages/contracts-bedrock/src/dispute/v2/PermissionedDisputeGameV2.sol b/packages/contracts-bedrock/src/dispute/v2/PermissionedDisputeGameV2.sol index a71777012c96b..a1e658eea6877 100644 --- a/packages/contracts-bedrock/src/dispute/v2/PermissionedDisputeGameV2.sol +++ b/packages/contracts-bedrock/src/dispute/v2/PermissionedDisputeGameV2.sol @@ -17,39 +17,22 @@ import { BadAuth } from "src/dispute/lib/Errors.sol"; /// costs that certain networks may not wish to support. This contract can also be used as a fallback mechanism /// in case of a failure in the permissionless fault proof system in the stage one release. contract PermissionedDisputeGameV2 is FaultDisputeGameV2 { - /// @notice The proposer role is allowed to create proposals and participate in the dispute game. - address internal immutable PROPOSER; - - /// @notice The challenger role is allowed to participate in the dispute game. - address internal immutable CHALLENGER; - /// @notice Modifier that gates access to the `challenger` and `proposer` roles. modifier onlyAuthorized() { - if (!(msg.sender == PROPOSER || msg.sender == CHALLENGER)) { + if (!(msg.sender == proposer() || msg.sender == challenger())) { revert BadAuth(); } _; } /// @notice Semantic version. - /// @custom:semver 2.0.0 + /// @custom:semver 2.1.0 function version() public pure override returns (string memory) { - return "2.0.0"; + return "2.1.0"; } /// @param _params Parameters for creating a new FaultDisputeGame. - /// @param _proposer Address that is allowed to create instances of this contract. - /// @param _challenger Address that is allowed to challenge instances of this contract. - constructor( - GameConstructorParams memory _params, - address _proposer, - address _challenger - ) - FaultDisputeGameV2(_params) - { - PROPOSER = _proposer; - CHALLENGER = _challenger; - } + constructor(GameConstructorParams memory _params) FaultDisputeGameV2(_params) { } /// @inheritdoc FaultDisputeGameV2 function step( @@ -86,24 +69,31 @@ contract PermissionedDisputeGameV2 is FaultDisputeGameV2 { /// @notice Initializes the contract. function initialize() public payable override { + super.initialize(); + // The creator of the dispute game must be the proposer EOA. - if (tx.origin != PROPOSER) revert BadAuth(); + if (tx.origin != proposer()) revert BadAuth(); + } - // Fallthrough initialization. - super.initialize(); + function immutableArgsByteCount() internal pure override returns (uint256) { + // Extend expected data length to account for proposer and challenger addresses + // - 20 bytes: proposer address + // - 20 bytes: challenger address + return super.immutableArgsByteCount() + 40; } //////////////////////////////////////////////////////////////// // IMMUTABLE GETTERS // //////////////////////////////////////////////////////////////// - /// @notice Returns the proposer address. - function proposer() external view returns (address proposer_) { - proposer_ = PROPOSER; + /// @notice Returns the proposer address. The proposer role is allowed to create proposals and participate in the + /// dispute game. + function proposer() public pure returns (address proposer_) { + proposer_ = _getArgAddress(super.immutableArgsByteCount()); } - /// @notice Returns the challenger address. - function challenger() external view returns (address challenger_) { - challenger_ = CHALLENGER; + /// @notice Returns the challenger address. The challenger role is allowed to participate in the dispute game. + function challenger() public pure returns (address challenger_) { + challenger_ = _getArgAddress(super.immutableArgsByteCount() + 20); } } diff --git a/packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol b/packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol index 9a87ff81a7391..76f0f2778670f 100644 --- a/packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol +++ b/packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol @@ -129,9 +129,17 @@ contract DisputeGameFactory_TestInit is CommonTest { params_ = abi.decode(args, (ISuperFaultDisputeGame.GameConstructorParams)); } + function _setGame(address _gameImpl, GameType _gameType) internal { + _setGame(_gameImpl, _gameType, false, ""); + } + function _setGame(address _gameImpl, GameType _gameType, bytes memory _implArgs) internal { + _setGame(_gameImpl, _gameType, true, _implArgs); + } + + function _setGame(address _gameImpl, GameType _gameType, bool _hasImplArgs, bytes memory _implArgs) internal { vm.startPrank(disputeGameFactory.owner()); - if (_implArgs.length > 0) { + if (_hasImplArgs) { disputeGameFactory.setImplementation(_gameType, IDisputeGame(_gameImpl), _implArgs); } else { disputeGameFactory.setImplementation(_gameType, IDisputeGame(_gameImpl)); @@ -157,7 +165,7 @@ contract DisputeGameFactory_TestInit is CommonTest { ) }); - _setGame(gameImpl_, GameTypes.SUPER_CANNON, ""); + _setGame(gameImpl_, GameTypes.SUPER_CANNON); } /// @notice Sets up a super permissioned game implementation @@ -185,7 +193,7 @@ contract DisputeGameFactory_TestInit is CommonTest { ) }); - _setGame(gameImpl_, GameTypes.SUPER_PERMISSIONED_CANNON, ""); + _setGame(gameImpl_, GameTypes.SUPER_PERMISSIONED_CANNON); } /// @notice Sets up a fault game implementation @@ -203,7 +211,23 @@ contract DisputeGameFactory_TestInit is CommonTest { ) }); - _setGame(gameImpl_, GameTypes.CANNON, ""); + _setGame(gameImpl_, GameTypes.CANNON); + } + + /// @notice Sets up immutable data for fault game v2 implementation + function getFaultDisputeGameV2ImmutableArgs(Claim _absolutePrestate) + internal + returns (bytes memory immutableArgs_, AlphabetVM vm_, IPreimageOracle preimageOracle_) + { + (vm_, preimageOracle_) = _createVM(_absolutePrestate); + // Encode the implementation args for CWIA (tightly packed) + immutableArgs_ = abi.encodePacked( + _absolutePrestate, // 32 bytes + vm_, // 20 bytes + anchorStateRegistry, // 20 bytes + delayedWeth, // 20 bytes + l2ChainId // 32 bytes (l2ChainId) + ); } /// @notice Sets up a fault game v2 implementation @@ -211,7 +235,12 @@ contract DisputeGameFactory_TestInit is CommonTest { internal returns (address gameImpl_, AlphabetVM vm_, IPreimageOracle preimageOracle_) { - (vm_, preimageOracle_) = _createVM(_absolutePrestate); + bytes memory immutableArgs; + (immutableArgs, vm_, preimageOracle_) = getFaultDisputeGameV2ImmutableArgs(_absolutePrestate); + gameImpl_ = setupFaultDisputeGameV2(immutableArgs); + } + + function setupFaultDisputeGameV2(bytes memory immutableArgs) internal returns (address gameImpl_) { gameImpl_ = DeployUtils.create1({ _name: "FaultDisputeGameV2", _args: DeployUtils.encodeConstructor( @@ -219,16 +248,7 @@ contract DisputeGameFactory_TestInit is CommonTest { ) }); - // Encode the implementation args for CWIA (tightly packed) - bytes memory implArgs = abi.encodePacked( - _absolutePrestate, // 32 bytes - vm_, // 20 bytes - anchorStateRegistry, // 20 bytes - delayedWeth, // 20 bytes - l2ChainId // 32 bytes (l2ChainId) - ); - - _setGame(gameImpl_, GameTypes.CANNON, implArgs); + _setGame(gameImpl_, GameTypes.CANNON, immutableArgs); } function setupPermissionedDisputeGame( @@ -254,7 +274,7 @@ contract DisputeGameFactory_TestInit is CommonTest { ) }); - _setGame(gameImpl_, GameTypes.PERMISSIONED_CANNON, ""); + _setGame(gameImpl_, GameTypes.PERMISSIONED_CANNON); } function changeClaimStatus(Claim _claim, VMStatus _status) public pure returns (Claim out_) { @@ -263,6 +283,30 @@ contract DisputeGameFactory_TestInit is CommonTest { } } + /// @notice Sets up immutable args for PDG v2 implementation + function getPermissionedDisputeGameV2ImmutableArgs( + Claim _absolutePrestate, + address _proposer, + address _challenger + ) + internal + returns (bytes memory implArgs_, AlphabetVM vm_, IPreimageOracle preimageOracle_) + { + (vm_, preimageOracle_) = _createVM(_absolutePrestate); + + // Encode the implementation args for CWIA (tightly packed) + implArgs_ = abi.encodePacked( + _absolutePrestate, // 32 bytes + vm_, // 20 bytes + anchorStateRegistry, // 20 bytes + delayedWeth, // 20 bytes + l2ChainId, // 32 bytes (l2ChainId), + _proposer, // 20 bytes + _challenger // 20 bytes + ); + } + + /// @notice Deploys PDG v2 implementation and sets it on the DGF function setupPermissionedDisputeGameV2( Claim _absolutePrestate, address _proposer, @@ -271,27 +315,25 @@ contract DisputeGameFactory_TestInit is CommonTest { internal returns (address gameImpl_, AlphabetVM vm_, IPreimageOracle preimageOracle_) { - (vm_, preimageOracle_) = _createVM(_absolutePrestate); + bytes memory implArgs; + (implArgs, vm_, preimageOracle_) = + getPermissionedDisputeGameV2ImmutableArgs(_absolutePrestate, _proposer, _challenger); + + gameImpl_ = setupPermissionedDisputeGameV2(implArgs); + } + + /// @notice Deploys PDG v2 implementation and sets it on the DGF + function setupPermissionedDisputeGameV2(bytes memory _implArgs) internal returns (address gameImpl_) { gameImpl_ = DeployUtils.create1({ _name: "PermissionedDisputeGameV2", _args: DeployUtils.encodeConstructor( abi.encodeCall( - IPermissionedDisputeGameV2.__constructor__, - (_getGameConstructorParamsV2(GameTypes.PERMISSIONED_CANNON), _proposer, _challenger) + IPermissionedDisputeGameV2.__constructor__, (_getGameConstructorParamsV2(GameTypes.PERMISSIONED_CANNON)) ) ) }); - // Encode the implementation args for CWIA (tightly packed) - bytes memory implArgs = abi.encodePacked( - _absolutePrestate, // 32 bytes - vm_, // 20 bytes - anchorStateRegistry, // 20 bytes - delayedWeth, // 20 bytes - l2ChainId // 32 bytes (l2ChainId) - ); - - _setGame(gameImpl_, GameTypes.PERMISSIONED_CANNON, implArgs); + _setGame(gameImpl_, GameTypes.PERMISSIONED_CANNON, _implArgs); } } diff --git a/packages/contracts-bedrock/test/dispute/PermissionedDisputeGame.t.sol b/packages/contracts-bedrock/test/dispute/PermissionedDisputeGame.t.sol index d1a4458456cdf..3984b217ffb3e 100644 --- a/packages/contracts-bedrock/test/dispute/PermissionedDisputeGame.t.sol +++ b/packages/contracts-bedrock/test/dispute/PermissionedDisputeGame.t.sol @@ -86,7 +86,7 @@ contract PermissionedDisputeGame_TestInit is DisputeGameFactory_TestInit { assertEq(address(gameProxy.vm()), address(_vm)); // Label the proxy - vm.label(address(gameProxy), "FaultDisputeGame_Clone"); + vm.label(address(gameProxy), "PermissionedDisputeGame_Clone"); } function setUp() public override { diff --git a/packages/contracts-bedrock/test/dispute/v2/FaultDisputeGameV2.t.sol b/packages/contracts-bedrock/test/dispute/v2/FaultDisputeGameV2.t.sol index 9667f9c067413..9908aa2d3dd3e 100644 --- a/packages/contracts-bedrock/test/dispute/v2/FaultDisputeGameV2.t.sol +++ b/packages/contracts-bedrock/test/dispute/v2/FaultDisputeGameV2.t.sol @@ -129,6 +129,14 @@ contract BaseFaultDisputeGameV2_TestInit is DisputeGameFactory_TestInit { fallback() external payable { } receive() external payable { } + + function copyBytes(bytes memory src, bytes memory dest) internal pure returns (bytes memory) { + uint256 byteCount = src.length < dest.length ? src.length : dest.length; + for (uint256 i = 0; i < byteCount; i++) { + dest[i] = src[i]; + } + return dest; + } } /// @title FaultDisputeGameV2_TestInit @@ -393,8 +401,8 @@ contract FaultDisputeGameV2_Initialize_Test is FaultDisputeGameV2_TestInit { } /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length - /// (must be exactly 246 bytes) - function test_initialize_wrongCalldataLength_reverts(uint256 _extraDataLen) public { + /// caused by extraData of the wrong length + function test_initialize_wrongExtradataLength_reverts(uint256 _extraDataLen) public { // The `DisputeGameFactory` will pack the root claim and the extra data into a single // array, which is enforced to be at least 64 bytes long. // We bound the upper end to 23.5KB to ensure that the minimal proxy never surpasses the @@ -421,6 +429,53 @@ contract FaultDisputeGameV2_Initialize_Test is FaultDisputeGameV2_TestInit { ); } + /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length + /// caused by additional immutable args data + function test_initialize_extraImmutableArgsBytes_reverts(uint256 _extraByteCount) public { + (bytes memory correctArgs,,) = getFaultDisputeGameV2ImmutableArgs(absolutePrestate); + + // We bound the upper end to 23.5KB to ensure that the minimal proxy never surpasses the + // contract size limit in this test, as CWIA proxies store the immutable args in their + // bytecode. + _extraByteCount = bound(_extraByteCount, 1, 23_500); + bytes memory immutableArgs = new bytes(_extraByteCount + correctArgs.length); + // Copy correct args into immutable args + copyBytes(correctArgs, immutableArgs); + + // Set up dispute game implementation with target immutableArgs + setupFaultDisputeGameV2(immutableArgs); + + Claim claim = _dummyClaim(); + vm.expectRevert(IFaultDisputeGameV2.BadExtraData.selector); + gameProxy = IFaultDisputeGameV2( + payable( + address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, claim, abi.encode(validL2BlockNumber))) + ) + ); + } + + /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length + /// caused by missing immutable args data + function test_initialize_missingImmutableArgsBytes_reverts(uint256 _truncatedByteCount) public { + (bytes memory correctArgs,,) = getFaultDisputeGameV2ImmutableArgs(absolutePrestate); + + _truncatedByteCount = (_truncatedByteCount % correctArgs.length) + 1; + bytes memory immutableArgs = new bytes(correctArgs.length - _truncatedByteCount); + // Copy correct args into immutable args + copyBytes(correctArgs, immutableArgs); + + // Set up dispute game implementation with target immutableArgs + setupFaultDisputeGameV2(immutableArgs); + + Claim claim = _dummyClaim(); + vm.expectRevert(IFaultDisputeGameV2.BadExtraData.selector); + gameProxy = IFaultDisputeGameV2( + payable( + address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, claim, abi.encode(validL2BlockNumber))) + ) + ); + } + /// @notice Tests that the game is initialized with the correct data. function test_initialize_correctData_succeeds() public view { // Assert that the root claim is initialized correctly. diff --git a/packages/contracts-bedrock/test/dispute/v2/PermissionedDisputeGameV2.t.sol b/packages/contracts-bedrock/test/dispute/v2/PermissionedDisputeGameV2.t.sol index e256ba17fa2ff..f6a59b81bce71 100644 --- a/packages/contracts-bedrock/test/dispute/v2/PermissionedDisputeGameV2.t.sol +++ b/packages/contracts-bedrock/test/dispute/v2/PermissionedDisputeGameV2.t.sol @@ -4,12 +4,14 @@ pragma solidity ^0.8.15; // Testing import { DisputeGameFactory_TestInit } from "test/dispute/DisputeGameFactory.t.sol"; import { AlphabetVM } from "test/mocks/AlphabetVM.sol"; + // Libraries import "src/dispute/lib/Types.sol"; import "src/dispute/lib/Errors.sol"; // Interfaces import { IPermissionedDisputeGameV2 } from "interfaces/dispute/v2/IPermissionedDisputeGameV2.sol"; +import { IFaultDisputeGameV2 } from "interfaces/dispute/v2/IFaultDisputeGameV2.sol"; /// @title PermissionedDisputeGameV2_TestInit /// @notice Reusable test initialization for `PermissionedDisputeGame` tests. @@ -21,6 +23,9 @@ contract PermissionedDisputeGameV2_TestInit is DisputeGameFactory_TestInit { /// @notice Mock challenger key address internal constant CHALLENGER = address(0xfacadec); + /// @dev The initial bond for the game. + uint256 internal initBond; + /// @notice The implementation of the game. IPermissionedDisputeGameV2 internal gameImpl; /// @notice The `Clone` proxy of the game. @@ -61,7 +66,7 @@ contract PermissionedDisputeGameV2_TestInit is DisputeGameFactory_TestInit { gameImpl = IPermissionedDisputeGameV2(_impl); // Create a new game. - uint256 bondAmount = disputeGameFactory.initBonds(GAME_TYPE); + initBond = disputeGameFactory.initBonds(GAME_TYPE); vm.mockCall( address(anchorStateRegistry), abi.encodeCall(anchorStateRegistry.anchors, (GAME_TYPE)), @@ -69,7 +74,7 @@ contract PermissionedDisputeGameV2_TestInit is DisputeGameFactory_TestInit { ); vm.prank(PROPOSER, PROPOSER); gameProxy = IPermissionedDisputeGameV2( - payable(address(disputeGameFactory.create{ value: bondAmount }(GAME_TYPE, _rootClaim, extraData))) + payable(address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, _rootClaim, extraData))) ); // Check immutables @@ -88,7 +93,7 @@ contract PermissionedDisputeGameV2_TestInit is DisputeGameFactory_TestInit { assertEq(gameProxy.l2ChainId(), l2ChainId); // Label the proxy - vm.label(address(gameProxy), "FaultDisputeGame_Clone"); + vm.label(address(gameProxy), "PermissionedDisputeGame_Clone"); } function setUp() public override { @@ -126,6 +131,14 @@ contract PermissionedDisputeGameV2_TestInit is DisputeGameFactory_TestInit { fallback() external payable { } receive() external payable { } + + function copyBytes(bytes memory src, bytes memory dest) internal pure returns (bytes memory) { + uint256 byteCount = src.length < dest.length ? src.length : dest.length; + for (uint256 i = 0; i < byteCount; i++) { + dest[i] = src[i]; + } + return dest; + } } /// @title PermissionedDisputeGameV2_Version_Test @@ -140,13 +153,59 @@ contract PermissionedDisputeGameV2_Version_Test is PermissionedDisputeGameV2_Tes /// @title PermissionedDisputeGameV2_Step_Test /// @notice Tests the `step` function of the `PermissionedDisputeGame` contract. contract PermissionedDisputeGameV2_Step_Test is PermissionedDisputeGameV2_TestInit { - /// @notice Tests that step works properly. - function test_step_succeeds() public { - // Give the test contract some ether + /// @notice Tests that step works properly for the challenger. + function test_step_fromChallenger_succeeds() public { + validateStepForActor(CHALLENGER); + } + + /// @notice Tests that step works properly for the proposer. + function test_step_fromProposer_succeeds() public { + validateStepForActor(PROPOSER); + } + + function validateStepForActor(address actor) internal { + vm.deal(actor, 1_000 ether); + vm.startPrank(actor, actor); + + // Set up and perform the step + setupGameForStep(); + performStep(); + assertEq(gameProxy.claimDataLen(), 9); + + // Resolve the game and check that the expected actor countered the root claim + resolveGame(); + assertEq(uint256(gameProxy.status()), uint256(GameStatus.CHALLENGER_WINS)); + assertEq(gameProxy.resolvedAt().raw(), block.timestamp); + (, address counteredBy,,,,,) = gameProxy.claimData(0); + assertEq(counteredBy, actor); + + vm.stopPrank(); + } + + /// @notice Tests that step reverts for unauthorized addresses. + function test_step_notAuthorized_reverts(address _unauthorized) internal { + vm.assume(_unauthorized != PROPOSER && _unauthorized != CHALLENGER); + vm.deal(_unauthorized, 1_000 ether); vm.deal(CHALLENGER, 1_000 ether); + // Set up for the step using an authorized actor vm.startPrank(CHALLENGER, CHALLENGER); + setupGameForStep(); + vm.stopPrank(); + + // Perform step with the unauthorized actor + vm.startPrank(_unauthorized, _unauthorized); + vm.expectRevert(BadAuth.selector); + performStep(); + + // Game should still be in progress, leaf claim should be missing + assertEq(uint256(gameProxy.status()), uint256(GameStatus.CHALLENGER_WINS)); + assertEq(gameProxy.claimDataLen(), 8); + vm.stopPrank(); + } + + function setupGameForStep() internal { // Make claims all the way down the tree. (,,,, Claim disputed,,) = gameProxy.claimData(0); gameProxy.attack{ value: _getRequiredBond(0) }(disputed, 0, _dummyClaim()); @@ -165,12 +224,16 @@ contract PermissionedDisputeGameV2_Step_Test is PermissionedDisputeGameV2_TestIn (,,,, disputed,,) = gameProxy.claimData(7); gameProxy.attack{ value: _getRequiredBond(7) }(disputed, 7, _dummyClaim()); - // Verify game state before step + // Verify game state and add local data assertEq(uint256(gameProxy.status()), uint256(GameStatus.IN_PROGRESS)); - gameProxy.addLocalData(LocalPreimageKey.DISPUTED_L2_BLOCK_NUMBER, 8, 0); + } + + function performStep() internal { gameProxy.step(8, true, absolutePrestateData, hex""); + } + function resolveGame() internal { vm.warp(block.timestamp + gameProxy.maxClockDuration().raw() + 1); gameProxy.resolveClaim(8, 0); gameProxy.resolveClaim(7, 0); @@ -183,11 +246,89 @@ contract PermissionedDisputeGameV2_Step_Test is PermissionedDisputeGameV2_TestIn gameProxy.resolveClaim(0, 0); gameProxy.resolve(); + } +} - assertEq(uint256(gameProxy.status()), uint256(GameStatus.CHALLENGER_WINS)); - assertEq(gameProxy.resolvedAt().raw(), block.timestamp); - (, address counteredBy,,,,,) = gameProxy.claimData(0); - assertEq(counteredBy, CHALLENGER); +/// @title PermissionedDisputeGame_Initialize_Test +/// @notice Tests the initialization of the `PermissionedDisputeGame` contract. +contract PermissionedDisputeGameV2_Initialize_Test is PermissionedDisputeGameV2_TestInit { + /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length + /// caused by extraData of the wrong length + function test_initialize_wrongExtradataLength_reverts(uint256 _extraDataLen) public { + // The `DisputeGameFactory` will pack the root claim and the extra data into a single + // array, which is enforced to be at least 64 bytes long. + // We bound the upper end to 23.5KB to ensure that the minimal proxy never surpasses the + // contract size limit in this test, as CWIA proxies store the immutable args in their + // bytecode. + // [0 bytes, 31 bytes] u [33 bytes, 23.5 KB] + _extraDataLen = bound(_extraDataLen, 0, 23_500); + if (_extraDataLen == 32) { + _extraDataLen++; + } + bytes memory _extraData = new bytes(_extraDataLen); + + // Assign the first 32 bytes in `extraData` to a valid L2 block number passed the starting + // block. + (, uint256 startingL2Block) = gameProxy.startingOutputRoot(); + assembly { + mstore(add(_extraData, 0x20), add(startingL2Block, 1)) + } + + Claim claim = _dummyClaim(); + vm.prank(PROPOSER, PROPOSER); + vm.expectRevert(IFaultDisputeGameV2.BadExtraData.selector); + gameProxy = IPermissionedDisputeGameV2( + payable(address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, claim, _extraData))) + ); + } + + /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length + /// caused by additional immutable args data + function test_initialize_extraImmutableArgsBytes_reverts(uint256 _extraByteCount) public { + (bytes memory correctArgs,,) = getPermissionedDisputeGameV2ImmutableArgs(absolutePrestate, PROPOSER, CHALLENGER); + + // We bound the upper end to 23.5KB to ensure that the minimal proxy never surpasses the + // contract size limit in this test, as CWIA proxies store the immutable args in their + // bytecode. + _extraByteCount = bound(_extraByteCount, 1, 23_500); + bytes memory immutableArgs = new bytes(_extraByteCount + correctArgs.length); + // Copy correct args into immutable args + copyBytes(correctArgs, immutableArgs); + + // Set up dispute game implementation with target immutableArgs + setupPermissionedDisputeGameV2(immutableArgs); + + Claim claim = _dummyClaim(); + vm.prank(PROPOSER, PROPOSER); + vm.expectRevert(IFaultDisputeGameV2.BadExtraData.selector); + gameProxy = IPermissionedDisputeGameV2( + payable( + address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, claim, abi.encode(validL2BlockNumber))) + ) + ); + } + + /// @notice Tests that the game cannot be initialized with incorrect CWIA calldata length + /// caused by missing immutable args data + function test_initialize_missingImmutableArgsBytes_reverts(uint256 _truncatedByteCount) public { + (bytes memory correctArgs,,) = getPermissionedDisputeGameV2ImmutableArgs(absolutePrestate, PROPOSER, CHALLENGER); + + _truncatedByteCount = (_truncatedByteCount % correctArgs.length) + 1; + bytes memory immutableArgs = new bytes(correctArgs.length - _truncatedByteCount); + // Copy correct args into immutable args + copyBytes(correctArgs, immutableArgs); + + // Set up dispute game implementation with target immutableArgs + setupPermissionedDisputeGameV2(immutableArgs); + + Claim claim = _dummyClaim(); + vm.prank(PROPOSER, PROPOSER); + vm.expectRevert(IFaultDisputeGameV2.BadExtraData.selector); + gameProxy = IPermissionedDisputeGameV2( + payable( + address(disputeGameFactory.create{ value: initBond }(GAME_TYPE, claim, abi.encode(validL2BlockNumber))) + ) + ); } } @@ -197,9 +338,16 @@ contract PermissionedDisputeGameV2_Step_Test is PermissionedDisputeGameV2_TestIn contract PermissionedDisputeGameV2_Unclassified_Test is PermissionedDisputeGameV2_TestInit { /// @notice Tests that the proposer can create a permissioned dispute game. function test_createGame_proposer_succeeds() public { - uint256 bondAmount = disputeGameFactory.initBonds(GAME_TYPE); vm.prank(PROPOSER, PROPOSER); - disputeGameFactory.create{ value: bondAmount }(GAME_TYPE, arbitaryRootClaim, abi.encode(validL2BlockNumber)); + disputeGameFactory.create{ value: initBond }(GAME_TYPE, arbitaryRootClaim, abi.encode(validL2BlockNumber)); + } + + /// @notice Tests that the permissioned game cannot be created by the challenger. + function test_createGame_challenger_reverts() public { + vm.deal(CHALLENGER, initBond); + vm.prank(CHALLENGER, CHALLENGER); + vm.expectRevert(BadAuth.selector); + disputeGameFactory.create{ value: initBond }(GAME_TYPE, arbitaryRootClaim, abi.encode(validL2BlockNumber)); } /// @notice Tests that the permissioned game cannot be created by any address other than the @@ -207,11 +355,10 @@ contract PermissionedDisputeGameV2_Unclassified_Test is PermissionedDisputeGameV function testFuzz_createGame_notProposer_reverts(address _p) public { vm.assume(_p != PROPOSER); - uint256 bondAmount = disputeGameFactory.initBonds(GAME_TYPE); - vm.deal(_p, bondAmount); + vm.deal(_p, initBond); vm.prank(_p, _p); vm.expectRevert(BadAuth.selector); - disputeGameFactory.create{ value: bondAmount }(GAME_TYPE, arbitaryRootClaim, abi.encode(validL2BlockNumber)); + disputeGameFactory.create{ value: initBond }(GAME_TYPE, arbitaryRootClaim, abi.encode(validL2BlockNumber)); } /// @notice Tests that the challenger can participate in a permissioned dispute game.