diff --git a/arbnode/consensus_execution_syncer.go b/arbnode/consensus_execution_syncer.go index dee4b7569a5..b899aea8ef0 100644 --- a/arbnode/consensus_execution_syncer.go +++ b/arbnode/consensus_execution_syncer.go @@ -14,7 +14,6 @@ import ( "github.com/offchainlabs/nitro/arbutil" "github.com/offchainlabs/nitro/execution" - "github.com/offchainlabs/nitro/execution/gethexec" "github.com/offchainlabs/nitro/staker" "github.com/offchainlabs/nitro/util" "github.com/offchainlabs/nitro/util/headerreader" @@ -108,7 +107,7 @@ func (c *ConsensusExecutionSyncer) getFinalityData( } msgIdx := msgCount - 1 msgResult, err := c.txStreamer.ResultAtMessageIndex(msgIdx) - if errors.Is(err, gethexec.ResultNotFound) { + if errors.Is(err, execution.ErrResultNotFound) { log.Debug("Message result not found, node out of sync", "msgIdx", msgIdx, "err", err) return nil, nil } else if err != nil { diff --git a/changelog/pmikolajczyk-nit-4738.md b/changelog/pmikolajczyk-nit-4738.md new file mode 100644 index 00000000000..e6c56fe1f7e --- /dev/null +++ b/changelog/pmikolajczyk-nit-4738.md @@ -0,0 +1,2 @@ +### Internal +- Replace string matching with JSON-RPC error codes in Consensus/Execution RPC clients diff --git a/consensus/consensusrpcclient/client.go b/consensus/consensusrpcclient/client.go index ec10f97c1da..ef03734a5c9 100644 --- a/consensus/consensusrpcclient/client.go +++ b/consensus/consensusrpcclient/client.go @@ -4,7 +4,6 @@ package consensusrpcclient import ( "context" - "strings" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/node" @@ -40,22 +39,11 @@ func (c *ConsensusRPCClient) StopAndWait() { c.StopWaiter.StopAndWait() } -func convertError(err error) error { - if err == nil { - return nil - } - errStr := err.Error() - if strings.Contains(errStr, execution.ErrSequencerInsertLockTaken.Error()) { - return execution.ErrSequencerInsertLockTaken - } - return err -} - func (c *ConsensusRPCClient) GetL1Confirmations(msgIdx arbutil.MessageIndex) containers.PromiseInterface[uint64] { return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (uint64, error) { var res uint64 err := c.client.CallContext(ctx, &res, consensus.RPCNamespace+"_getL1Confirmations", msgIdx) - return res, convertError(err) + return res, err }) } @@ -63,7 +51,7 @@ func (c *ConsensusRPCClient) FindBatchContainingMessage(msgIdx arbutil.MessageIn return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (uint64, error) { var res uint64 err := c.client.CallContext(ctx, &res, consensus.RPCNamespace+"_findBatchContainingMessage", msgIdx) - return res, convertError(err) + return res, err }) } @@ -71,7 +59,7 @@ func (c *ConsensusRPCClient) BlockMetadataAtMessageIndex(msgIdx arbutil.MessageI return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (common.BlockMetadata, error) { var res common.BlockMetadata err := c.client.CallContext(ctx, &res, consensus.RPCNamespace+"_blockMetadataAtMessageIndex", msgIdx) - return res, convertError(err) + return res, err }) } @@ -79,7 +67,7 @@ func (c *ConsensusRPCClient) WriteMessageFromSequencer(msgIdx arbutil.MessageInd return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (struct{}, error) { var res struct{} err := c.client.CallContext(ctx, &res, consensus.RPCNamespace+"_writeMessageFromSequencer", msgIdx, msgWithMeta, msgResult, blockMetadata) - return res, convertError(err) + return res, err }) } @@ -87,6 +75,6 @@ func (c *ConsensusRPCClient) ExpectChosenSequencer() containers.PromiseInterface return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (struct{}, error) { var res struct{} err := c.client.CallContext(ctx, &res, consensus.RPCNamespace+"_expectChosenSequencer") - return res, convertError(err) + return res, err }) } diff --git a/consensus/consensusrpcclient/client_test.go b/consensus/consensusrpcclient/client_test.go new file mode 100644 index 00000000000..ccda11a3c53 --- /dev/null +++ b/consensus/consensusrpcclient/client_test.go @@ -0,0 +1,169 @@ +// Copyright 2026, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md + +package consensusrpcclient + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/node" + "github.com/ethereum/go-ethereum/rpc" + + "github.com/offchainlabs/nitro/arbos/arbostypes" + "github.com/offchainlabs/nitro/arbutil" + "github.com/offchainlabs/nitro/consensus" + "github.com/offchainlabs/nitro/consensus/consensusrpcserver" + "github.com/offchainlabs/nitro/execution" + "github.com/offchainlabs/nitro/util/containers" + utilrpc "github.com/offchainlabs/nitro/util/rpcclient" + "github.com/offchainlabs/nitro/util/testhelpers" +) + +// mockConsensusService implements consensus.FullConsensusClient for testing. +type mockConsensusService struct { + err error +} + +func (m *mockConsensusService) GetL1Confirmations(_ arbutil.MessageIndex) containers.PromiseInterface[uint64] { + return containers.NewReadyPromise[uint64](0, m.err) +} + +func (m *mockConsensusService) FindBatchContainingMessage(_ arbutil.MessageIndex) containers.PromiseInterface[uint64] { + return containers.NewReadyPromise[uint64](0, m.err) +} + +func (m *mockConsensusService) BlockMetadataAtMessageIndex(_ arbutil.MessageIndex) containers.PromiseInterface[common.BlockMetadata] { + return containers.NewReadyPromise[common.BlockMetadata](nil, m.err) +} + +func (m *mockConsensusService) WriteMessageFromSequencer(_ arbutil.MessageIndex, _ arbostypes.MessageWithMetadata, _ execution.MessageResult, _ common.BlockMetadata) containers.PromiseInterface[struct{}] { + return containers.NewReadyPromise[struct{}](struct{}{}, m.err) +} + +func (m *mockConsensusService) ExpectChosenSequencer() containers.PromiseInterface[struct{}] { + return containers.NewReadyPromise[struct{}](struct{}{}, m.err) +} + +func createMockConsensusNode(t *testing.T, errToReturn error) *node.Node { + t.Helper() + stackConf := node.DefaultConfig + stackConf.HTTPPort = 0 + stackConf.DataDir = "" + stackConf.WSHost = "127.0.0.1" + stackConf.WSPort = 0 + stackConf.WSModules = []string{consensus.RPCNamespace} + stackConf.P2P.NoDiscovery = true + stackConf.P2P.ListenAddr = "" + + stack, err := node.New(&stackConf) + testhelpers.RequireImpl(t, err) + + stack.RegisterAPIs([]rpc.API{{ + Namespace: consensus.RPCNamespace, + Service: consensusrpcserver.NewConsensusRPCServer(&mockConsensusService{err: errToReturn}), + Public: true, + }}) + + testhelpers.RequireImpl(t, stack.Start()) + t.Cleanup(func() { _ = stack.Close() }) + return stack +} + +func TestConsensusClientErrorHandling(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + tests := []struct { + name string + serverErr error + expectedErr error + }{ + { + name: "ErrSequencerInsertLockTaken", + serverErr: execution.ErrSequencerInsertLockTaken, + expectedErr: execution.ErrSequencerInsertLockTaken, + }, + { + name: "ErrSequencerInsertLockTaken wrapped", + serverErr: fmt.Errorf("consensus context: %w", execution.ErrSequencerInsertLockTaken), + expectedErr: execution.ErrSequencerInsertLockTaken, + }, + { + name: "ErrRetrySequencer", + serverErr: execution.ErrRetrySequencer, + expectedErr: execution.ErrRetrySequencer, + }, + { + name: "ErrRetrySequencer wrapped", + serverErr: fmt.Errorf("consensus context: %w", execution.ErrRetrySequencer), + expectedErr: execution.ErrRetrySequencer, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + stack := createMockConsensusNode(t, tc.serverErr) + + config := &utilrpc.ClientConfig{ + URL: "self", + Timeout: 5 * time.Second, + } + testhelpers.RequireImpl(t, config.Validate()) + + client := NewConsensusRPCClient(func() *utilrpc.ClientConfig { return config }, stack) + testhelpers.RequireImpl(t, client.Start(ctx)) + defer client.StopAndWait() + + _, err := client.ExpectChosenSequencer().Await(ctx) + + if err == nil { + t.Fatal("expected an error from server, got nil") + } + if !errors.Is(err, tc.expectedErr) { + t.Errorf("expected %v, got %v", tc.expectedErr, err) + } + }) + } +} + +// TestConsensusClientErrorNoFalsePositives verifies that a plain server error +// (arriving with the default JSON-RPC code -32000) does not match any sentinel. +func TestConsensusClientErrorNoFalsePositives(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + stack := createMockConsensusNode(t, errors.New("some unrelated failure")) + + config := &utilrpc.ClientConfig{ + URL: "self", + Timeout: 5 * time.Second, + } + testhelpers.RequireImpl(t, config.Validate()) + + client := NewConsensusRPCClient(func() *utilrpc.ClientConfig { return config }, stack) + testhelpers.RequireImpl(t, client.Start(ctx)) + defer client.StopAndWait() + + _, err := client.ExpectChosenSequencer().Await(ctx) + if err == nil { + t.Fatal("expected an error from server, got nil") + } + + allSentinels := []error{ + execution.ErrResultNotFound, + execution.ErrRetrySequencer, + execution.ErrSequencerInsertLockTaken, + } + for _, sentinel := range allSentinels { + if errors.Is(err, sentinel) { + t.Errorf("plain error should not match sentinel %v", sentinel) + } + } +} diff --git a/execution/gethexec/executionengine.go b/execution/gethexec/executionengine.go index 135c7531db1..03439ebe3b6 100644 --- a/execution/gethexec/executionengine.go +++ b/execution/gethexec/executionengine.go @@ -71,7 +71,6 @@ var ( var ( ExecutionEngineBlockCreationStopped = errors.New("block creation stopped in execution engine") - ResultNotFound = errors.New("result not found") BlockNumBeforeGenesis = errors.New("block number is before genesis") ) @@ -1024,7 +1023,7 @@ func (s *ExecutionEngine) appendBlock(block *types.Block, statedb *state.StateDB func (s *ExecutionEngine) resultFromHeader(header *types.Header) (*execution.MessageResult, error) { if header == nil { - return nil, ResultNotFound + return nil, execution.ErrResultNotFound } info := types.DeserializeHeaderExtraInformation(header) return &execution.MessageResult{ diff --git a/execution/interface.go b/execution/interface.go index c084875f4da..fc75dd183dc 100644 --- a/execution/interface.go +++ b/execution/interface.go @@ -4,7 +4,6 @@ package execution import ( "context" - "errors" "time" "github.com/ethereum/go-ethereum/common" @@ -42,9 +41,6 @@ type ConsensusSyncData struct { UpdatedAt time.Time } -var ErrRetrySequencer = errors.New("please retry transaction") -var ErrSequencerInsertLockTaken = errors.New("insert lock taken") - // always needed type ExecutionClient interface { ArbOSVersionGetter diff --git a/execution/rpcclient/client.go b/execution/rpcclient/client.go index 2692d5d9968..de27f56970e 100644 --- a/execution/rpcclient/client.go +++ b/execution/rpcclient/client.go @@ -5,7 +5,6 @@ package rpcclient import ( "context" - "strings" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/node" @@ -13,7 +12,6 @@ import ( "github.com/offchainlabs/nitro/arbos/arbostypes" "github.com/offchainlabs/nitro/arbutil" "github.com/offchainlabs/nitro/execution" - "github.com/offchainlabs/nitro/execution/gethexec" "github.com/offchainlabs/nitro/util/containers" "github.com/offchainlabs/nitro/util/rpcclient" "github.com/offchainlabs/nitro/util/stopwaiter" @@ -41,24 +39,11 @@ func (c *Client) StopAndWait() { c.StopWaiter.StopAndWait() } -func convertError(err error) error { - if err == nil { - return nil - } - errStr := err.Error() - if strings.Contains(errStr, execution.ErrRetrySequencer.Error()) { - return execution.ErrRetrySequencer - } else if strings.Contains(errStr, gethexec.ResultNotFound.Error()) { - return gethexec.ResultNotFound - } - return err -} - func sendRequest[T any](c *Client, method string, args ...any) containers.PromiseInterface[T] { return stopwaiter.LaunchPromiseThread(c, func(ctx context.Context) (T, error) { var res T err := c.client.CallContext(ctx, &res, execution.RPCNamespace+method, args...) - return res, convertError(err) + return res, err }) } diff --git a/execution/rpcclient/client_test.go b/execution/rpcclient/client_test.go index b4a4e1582dd..4a0ed2ef9bf 100644 --- a/execution/rpcclient/client_test.go +++ b/execution/rpcclient/client_test.go @@ -15,7 +15,6 @@ import ( "github.com/offchainlabs/nitro/arbutil" "github.com/offchainlabs/nitro/execution" - "github.com/offchainlabs/nitro/execution/gethexec" utilrpc "github.com/offchainlabs/nitro/util/rpcclient" "github.com/offchainlabs/nitro/util/testhelpers" ) @@ -54,6 +53,12 @@ func createMockExecutionNode(t *testing.T, errToReturn error) *node.Node { return stack } +var allSentinels = []error{ + execution.ErrResultNotFound, + execution.ErrRetrySequencer, + execution.ErrSequencerInsertLockTaken, +} + func TestClientErrorHandling(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -65,29 +70,34 @@ func TestClientErrorHandling(t *testing.T) { expectedErr error }{ { - name: "ResultNotFound mapped to sentinel", - serverErr: gethexec.ResultNotFound, - expectedErr: gethexec.ResultNotFound, + name: "ResultNotFound", + serverErr: execution.ErrResultNotFound, + expectedErr: execution.ErrResultNotFound, }, { - name: "ResultNotFound wrapped in longer message mapped to sentinel", - serverErr: fmt.Errorf("execution context: %w", gethexec.ResultNotFound), - expectedErr: gethexec.ResultNotFound, + name: "ResultNotFound wrapped", + serverErr: fmt.Errorf("execution context: %w", execution.ErrResultNotFound), + expectedErr: execution.ErrResultNotFound, }, { - name: "ErrRetrySequencer mapped to sentinel", + name: "ErrRetrySequencer", serverErr: execution.ErrRetrySequencer, expectedErr: execution.ErrRetrySequencer, }, { - name: "ErrRetrySequencer wrapped in longer message mapped to sentinel", + name: "ErrRetrySequencer wrapped", serverErr: fmt.Errorf("rpc context: %w", execution.ErrRetrySequencer), expectedErr: execution.ErrRetrySequencer, }, { - name: "generic error message is preserved", - serverErr: errors.New("unexpected failure"), - expectedErr: errors.New("unexpected failure"), + name: "ErrSequencerInsertLockTaken", + serverErr: execution.ErrSequencerInsertLockTaken, + expectedErr: execution.ErrSequencerInsertLockTaken, + }, + { + name: "ErrSequencerInsertLockTaken wrapped", + serverErr: fmt.Errorf("consensus context: %w", execution.ErrSequencerInsertLockTaken), + expectedErr: execution.ErrSequencerInsertLockTaken, }, } @@ -110,16 +120,39 @@ func TestClientErrorHandling(t *testing.T) { if err == nil { t.Fatal("expected an error from server, got nil") } - switch { - case errors.Is(tc.expectedErr, gethexec.ResultNotFound), errors.Is(tc.expectedErr, execution.ErrRetrySequencer): - if !errors.Is(err, tc.expectedErr) { - t.Errorf("expected sentinel error %v, got %v", tc.expectedErr, err) - } - default: - if err.Error() != tc.expectedErr.Error() { - t.Errorf("expected error message %q, got %q", tc.expectedErr.Error(), err.Error()) - } + if !errors.Is(err, tc.expectedErr) { + t.Errorf("expected %v, got %v", tc.expectedErr, err) } }) } } + +// TestClientErrorNoFalsePositives verifies that a plain server error (which +// arrives with the default JSON-RPC code -32000) does not match any sentinel. +func TestClientErrorNoFalsePositives(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + stack := createMockExecutionNode(t, errors.New("some unrelated failure")) + + config := &utilrpc.ClientConfig{ + URL: "self", + Timeout: 5 * time.Second, + } + testhelpers.RequireImpl(t, config.Validate()) + + client := NewClient(func() *utilrpc.ClientConfig { return config }, stack) + testhelpers.RequireImpl(t, client.Start(ctx)) + defer client.StopAndWait() + + _, err := client.HeadMessageIndex().Await(ctx) + if err == nil { + t.Fatal("expected an error from server, got nil") + } + for _, sentinel := range allSentinels { + if errors.Is(err, sentinel) { + t.Errorf("plain error should not match sentinel %v", sentinel) + } + } +} diff --git a/execution/rpcerror.go b/execution/rpcerror.go new file mode 100644 index 00000000000..6e7618540d4 --- /dev/null +++ b/execution/rpcerror.go @@ -0,0 +1,50 @@ +// Copyright 2026, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md + +package execution + +import "errors" + +// Application-level JSON-RPC error codes for Consensus/Execution communication. +// These are below -32768, outside the JSON-RPC 2.0 spec reserved range +// (-32768 to -32000), so they cannot collide with standard or go-ethereum codes. +const ( + ErrCodeRetrySequencer = -33000 + ErrCodeInsertLockTaken = -33001 + ErrCodeResultNotFound = -33002 +) + +// RPCError is an error that carries a JSON-RPC error code. +// +// On the server side it implements the rpc.Error interface, which causes +// go-ethereum's framework to include the code in the JSON-RPC error response +// instead of using the default -32000. +// +// On the client side the Is method enables code-based comparison via +// errors.Is, so callers do not need to inspect error message strings. +type RPCError struct { + code int + msg string +} + +func (e *RPCError) Error() string { return e.msg } +func (e *RPCError) ErrorCode() int { return e.code } + +// Sentinel errors returned across the Consensus/Execution RPC boundary. +var ( + ErrRetrySequencer = &RPCError{code: ErrCodeRetrySequencer, msg: "please retry transaction"} + ErrSequencerInsertLockTaken = &RPCError{code: ErrCodeInsertLockTaken, msg: "insert lock taken"} + ErrResultNotFound = &RPCError{code: ErrCodeResultNotFound, msg: "result not found"} +) + +// Is reports whether target is an RPCError with the same code. +// This makes errors.Is(receivedErr, sentinel) return true whenever the +// received error carries the same code as the sentinel, regardless of whether +// it is the exact same pointer or a jsonError reconstructed from the wire. +func (e *RPCError) Is(target error) bool { + var t *RPCError + if errors.As(target, &t) { + return t.code == e.code + } + return false +} diff --git a/execution_consensus/init.go b/execution_consensus/init.go index 08f8170aa1e..03db931a194 100644 --- a/execution_consensus/init.go +++ b/execution_consensus/init.go @@ -1,5 +1,6 @@ // Copyright 2025-2026, Offchain Labs, Inc. // For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md + package execution_consensus import ( diff --git a/go-ethereum b/go-ethereum index 15b10fd566e..4f62d14421d 160000 --- a/go-ethereum +++ b/go-ethereum @@ -1 +1 @@ -Subproject commit 15b10fd566ec60b2d954db442f182c0a78809bad +Subproject commit 4f62d14421d1574a97d3ef08104ab855e49c2ad5