diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index b85218d67..ba20e0475 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -13,6 +13,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [config] \#5728 `fast_sync = "v1"` is no longer supported (@melekes) - [cli] \#5772 `gen_node_key` prints JSON-encoded `NodeKey` rather than ID and does not save it to `node_key.json` (@melekes) - [cli] \#5777 use hyphen-case instead of snake_case for all cli commands and config parameters (@cmwaters) + - [rpc] \#6019 standardise RPC errors and return the correct status code (@bipulprasad & @cmwaters) - Apps - [ABCI] \#5447 Remove `SetOption` method from `ABCI.Client` interface diff --git a/light/provider/http/http.go b/light/provider/http/http.go index 56c7f53e6..3517a639b 100644 --- a/light/provider/http/http.go +++ b/light/provider/http/http.go @@ -2,27 +2,24 @@ package http import ( "context" + "errors" "fmt" "math/rand" - "regexp" "strings" "time" "github.com/tendermint/tendermint/light/provider" rpcclient "github.com/tendermint/tendermint/rpc/client" rpchttp "github.com/tendermint/tendermint/rpc/client/http" + ctypes "github.com/tendermint/tendermint/rpc/core/types" + rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" "github.com/tendermint/tendermint/types" ) -var ( - // This is very brittle, see: https://github.com/tendermint/tendermint/issues/4740 - regexpMissingHeight = regexp.MustCompile(`height \d+ (must be less than or equal to|is not available)`) - - defaultOptions = Options{ - MaxRetryAttempts: 10, - Timeout: 5 * time.Second, - } -) +var defaultOptions = Options{ + MaxRetryAttempts: 10, + Timeout: 5 * time.Second, +} // http provider uses an RPC client to obtain the necessary information. type http struct { @@ -134,34 +131,48 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato // is negative we will keep repeating. for attempt := 0; attempt != p.maxRetryAttempts+1; attempt++ { res, err := p.client.Validators(ctx, height, &page, &perPage) - if err != nil { - // TODO: standardize errors on the RPC side - if regexpMissingHeight.MatchString(err.Error()) { - return nil, provider.ErrLightBlockNotFound + switch e := err.(type) { + case nil: // success!! Now we validate the response + if len(res.Validators) == 0 { + return nil, provider.ErrBadLightBlock{ + Reason: fmt.Errorf("validator set is empty (height: %d, page: %d, per_page: %d)", + height, page, perPage), + } } - // if we have exceeded retry attempts then return no response error - if attempt == p.maxRetryAttempts { - return nil, provider.ErrNoResponse + if res.Total <= 0 { + return nil, provider.ErrBadLightBlock{ + Reason: fmt.Errorf("total number of vals is <= 0: %d (height: %d, page: %d, per_page: %d)", + res.Total, height, page, perPage), + } } - // else we wait and try again with exponential backoff - time.Sleep(backoffTimeout(uint16(attempt))) - continue - } - // Validate response. - if len(res.Validators) == 0 { - return nil, provider.ErrBadLightBlock{ - Reason: fmt.Errorf("validator set is empty (height: %d, page: %d, per_page: %d)", - height, page, perPage), + case *rpctypes.RPCError: + // Check if we got something other than internal error. This shouldn't happen unless the RPC module + // or light client has been tampered with. If we do get this error, stop the connection with the + // peer and return an error + if e.Code != -32603 { + return nil, provider.ErrBadLightBlock{Reason: errors.New(e.Data)} } - } - if res.Total <= 0 { - return nil, provider.ErrBadLightBlock{ - Reason: fmt.Errorf("total number of vals is <= 0: %d (height: %d, page: %d, per_page: %d)", - res.Total, height, page, perPage), + + // check if the error indicates that the peer doesn't have the block + if strings.Contains(e.Data, ctypes.ErrHeightNotAvailable.Error()) || + strings.Contains(e.Data, ctypes.ErrHeightExceedsChainHead.Error()) { + return nil, provider.ErrLightBlockNotFound } + + // we wait and try again with exponential backoff + // TODO: If we can, we should check if the error is purely because the node failed to respond in + // time. If this is the case then we continue, else we should stop straight away + time.Sleep(backoffTimeout(uint16(attempt))) + continue + + default: + // something has happened to the RPC module if we are not receiving errors of type RPCError + panic(fmt.Errorf("unexpected error type: %w", err)) } + // update the total and increment the page index so we can fetch the + // next page of validators if need be total = res.Total vals = append(vals, res.Validators...) page++ @@ -181,16 +192,34 @@ func (p *http) signedHeader(ctx context.Context, height *int64) (*types.SignedHe // is negative we will keep repeating. for attempt := 0; attempt != p.maxRetryAttempts+1; attempt++ { commit, err := p.client.Commit(ctx, height) - if err != nil { - // TODO: standardize errors on the RPC side - if regexpMissingHeight.MatchString(err.Error()) { + switch e := err.(type) { + case nil: // success!! + return &commit.SignedHeader, nil + + case *rpctypes.RPCError: + // Check if we got something other than internal error. This shouldn't happen unless the RPC module + // or light client has been tampered with. If we do get this error, stop the connection with the + // peer and return an error + if e.Code != -32603 { + return nil, provider.ErrBadLightBlock{Reason: errors.New(e.Data)} + } + + // check if the error indicates that the peer doesn't have the block + if strings.Contains(err.Error(), ctypes.ErrHeightNotAvailable.Error()) || + strings.Contains(err.Error(), ctypes.ErrHeightExceedsChainHead.Error()) { return nil, provider.ErrLightBlockNotFound } + // we wait and try again with exponential backoff + // TODO: If we can, we should check if the error is purely because the node failed to respond in + // time. If this is the case then we continue, else we should stop straight away time.Sleep(backoffTimeout(uint16(attempt))) continue + + default: + // something has happened to the RPC module if we are not receiving errors of type RPCError + panic(fmt.Errorf("unexpected error type: %w", err)) } - return &commit.SignedHeader, nil } return nil, provider.ErrNoResponse } diff --git a/light/provider/http/http_test.go b/light/provider/http/http_test.go index b6b3989a8..da6b19e9e 100644 --- a/light/provider/http/http_test.go +++ b/light/provider/http/http_test.go @@ -79,7 +79,7 @@ func TestProvider(t *testing.T) { require.NoError(t, err) assert.Equal(t, lower, sh.Height) - // fetching missing heights (both future and pruned) should return appropriate errors + // // fetching missing heights (both future and pruned) should return appropriate errors _, err = p.LightBlock(context.Background(), 1000) require.Error(t, err) assert.Equal(t, provider.ErrLightBlockNotFound, err) diff --git a/light/rpc/client.go b/light/rpc/client.go index 7c403aba7..29f2a638a 100644 --- a/light/rpc/client.go +++ b/light/rpc/client.go @@ -20,8 +20,6 @@ import ( "github.com/tendermint/tendermint/types" ) -var errNegOrZeroHeight = errors.New("negative or zero height") - // KeyPathFunc builds a merkle path out of the given path and key. type KeyPathFunc func(path string, key []byte) (merkle.KeyPath, error) @@ -127,7 +125,7 @@ func (c *Client) ABCIQueryWithOptions(ctx context.Context, path string, data tmb return nil, errors.New("no proof ops") } if resp.Height <= 0 { - return nil, errNegOrZeroHeight + return nil, ctypes.ErrZeroOrNegativeHeight } // Update the light client if we're behind. @@ -212,7 +210,7 @@ func (c *Client) ConsensusParams(ctx context.Context, height *int64) (*ctypes.Re return nil, err } if res.BlockHeight <= 0 { - return nil, errNegOrZeroHeight + return nil, ctypes.ErrZeroOrNegativeHeight } // Update the light client if we're behind. @@ -370,7 +368,7 @@ func (c *Client) BlockResults(ctx context.Context, height *int64) (*ctypes.Resul // Validate res. if res.Height <= 0 { - return nil, errNegOrZeroHeight + return nil, ctypes.ErrZeroOrNegativeHeight } // Update the light client if we're behind. @@ -435,7 +433,7 @@ func (c *Client) Tx(ctx context.Context, hash []byte, prove bool) (*ctypes.Resul // Validate res. if res.Height <= 0 { - return nil, errNegOrZeroHeight + return nil, ctypes.ErrZeroOrNegativeHeight } // Update the light client if we're behind. @@ -576,7 +574,7 @@ const ( func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { if perPage < 1 { - panic(fmt.Sprintf("zero or negative perPage: %d", perPage)) + panic(fmt.Errorf("%w (%d)", ctypes.ErrZeroOrNegativePerPage, perPage)) } if pagePtr == nil { // no page parameter @@ -589,7 +587,7 @@ func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { } page := *pagePtr if page <= 0 || page > pages { - return 1, fmt.Errorf("page should be within [1, %d] range, given %d", pages, page) + return 1, fmt.Errorf("%w expected range: [1, %d], given %d", ctypes.ErrPageOutOfRange, pages, page) } return page, nil diff --git a/rpc/core/blocks.go b/rpc/core/blocks.go index c5b2d4928..e846f0a89 100644 --- a/rpc/core/blocks.go +++ b/rpc/core/blocks.go @@ -53,7 +53,7 @@ func BlockchainInfo(ctx *rpctypes.Context, minHeight, maxHeight int64) (*ctypes. func filterMinMax(base, height, min, max, limit int64) (int64, int64, error) { // filter negatives if min < 0 || max < 0 { - return min, max, fmt.Errorf("heights must be non-negative") + return min, max, ctypes.ErrZeroOrNegativeHeight } // adjust for default values @@ -75,7 +75,8 @@ func filterMinMax(base, height, min, max, limit int64) (int64, int64, error) { min = tmmath.MaxInt64(min, max-limit+1) if min > max { - return min, max, fmt.Errorf("min height %d can't be greater than max height %d", min, max) + return min, max, fmt.Errorf("%w: min height %d can't be greater than max height %d", + ctypes.ErrInvalidRequest, min, max) } return min, max, nil } diff --git a/rpc/core/env.go b/rpc/core/env.go index ccc46d5a3..7584023ca 100644 --- a/rpc/core/env.go +++ b/rpc/core/env.go @@ -11,6 +11,7 @@ import ( mempl "github.com/tendermint/tendermint/mempool" "github.com/tendermint/tendermint/p2p" "github.com/tendermint/tendermint/proxy" + ctypes "github.com/tendermint/tendermint/rpc/core/types" sm "github.com/tendermint/tendermint/state" "github.com/tendermint/tendermint/state/txindex" "github.com/tendermint/tendermint/types" @@ -94,8 +95,9 @@ type Environment struct { //---------------------------------------------- func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { + // this can only happen if we haven't first run validatePerPage if perPage < 1 { - panic(fmt.Sprintf("zero or negative perPage: %d", perPage)) + panic(fmt.Errorf("%w (%d)", ctypes.ErrZeroOrNegativePerPage, perPage)) } if pagePtr == nil { // no page parameter @@ -108,7 +110,7 @@ func validatePage(pagePtr *int, perPage, totalCount int) (int, error) { } page := *pagePtr if page <= 0 || page > pages { - return 1, fmt.Errorf("page should be within [1, %d] range, given %d", pages, page) + return 1, fmt.Errorf("%w expected range: [1, %d], given %d", ctypes.ErrPageOutOfRange, pages, page) } return page, nil @@ -142,16 +144,15 @@ func getHeight(latestHeight int64, heightPtr *int64) (int64, error) { if heightPtr != nil { height := *heightPtr if height <= 0 { - return 0, fmt.Errorf("height must be greater than 0, but got %d", height) + return 0, fmt.Errorf("%w (requested height: %d)", ctypes.ErrZeroOrNegativeHeight, height) } if height > latestHeight { - return 0, fmt.Errorf("height %d must be less than or equal to the current blockchain height %d", - height, latestHeight) + return 0, fmt.Errorf("%w (requested height: %d, blockchain height: %d)", + ctypes.ErrHeightExceedsChainHead, height, latestHeight) } base := env.BlockStore.Base() if height < base { - return 0, fmt.Errorf("height %v is not available, lowest height is %v", - height, base) + return 0, fmt.Errorf("%w (requested height: %d, base height: %d)", ctypes.ErrHeightExceedsChainHead, height, base) } return height, nil } diff --git a/rpc/core/evidence.go b/rpc/core/evidence.go index 52990526d..fa4f8256a 100644 --- a/rpc/core/evidence.go +++ b/rpc/core/evidence.go @@ -1,7 +1,6 @@ package core import ( - "errors" "fmt" ctypes "github.com/tendermint/tendermint/rpc/core/types" @@ -13,7 +12,7 @@ import ( // More: https://docs.tendermint.com/master/rpc/#/Evidence/broadcast_evidence func BroadcastEvidence(ctx *rpctypes.Context, ev types.Evidence) (*ctypes.ResultBroadcastEvidence, error) { if ev == nil { - return nil, errors.New("no evidence was provided") + return nil, fmt.Errorf("%w: no evidence was provided", ctypes.ErrInvalidRequest) } if err := ev.ValidateBasic(); err != nil { diff --git a/rpc/core/net.go b/rpc/core/net.go index 45d096ae5..6ec573812 100644 --- a/rpc/core/net.go +++ b/rpc/core/net.go @@ -1,7 +1,7 @@ package core import ( - "errors" + "fmt" "strings" "github.com/tendermint/tendermint/p2p" @@ -36,7 +36,7 @@ func NetInfo(ctx *rpctypes.Context) (*ctypes.ResultNetInfo, error) { // UnsafeDialSeeds dials the given seeds (comma-separated id@IP:PORT). func UnsafeDialSeeds(ctx *rpctypes.Context, seeds []string) (*ctypes.ResultDialSeeds, error) { if len(seeds) == 0 { - return &ctypes.ResultDialSeeds{}, errors.New("no seeds provided") + return &ctypes.ResultDialSeeds{}, fmt.Errorf("%w: no seeds provided", ctypes.ErrInvalidRequest) } env.Logger.Info("DialSeeds", "seeds", seeds) if err := env.P2PPeers.DialPeersAsync(seeds); err != nil { @@ -50,7 +50,7 @@ func UnsafeDialSeeds(ctx *rpctypes.Context, seeds []string) (*ctypes.ResultDialS func UnsafeDialPeers(ctx *rpctypes.Context, peers []string, persistent, unconditional, private bool) ( *ctypes.ResultDialPeers, error) { if len(peers) == 0 { - return &ctypes.ResultDialPeers{}, errors.New("no peers provided") + return &ctypes.ResultDialPeers{}, fmt.Errorf("%w: no peers provided", ctypes.ErrInvalidRequest) } ids, err := getIDs(peers) diff --git a/rpc/core/tx.go b/rpc/core/tx.go index 53e32d9ef..415176552 100644 --- a/rpc/core/tx.go +++ b/rpc/core/tx.go @@ -88,7 +88,7 @@ func TxSearch(ctx *rpctypes.Context, query string, prove bool, pagePtr, perPageP return results[i].Height < results[j].Height }) default: - return nil, errors.New("expected order_by to be either `asc` or `desc` or empty") + return nil, fmt.Errorf("%w: expected order_by to be either `asc` or `desc` or empty", ctypes.ErrInvalidRequest) } // paginate results diff --git a/rpc/core/types/responses.go b/rpc/core/types/responses.go index e819cd330..3390c6502 100644 --- a/rpc/core/types/responses.go +++ b/rpc/core/types/responses.go @@ -2,6 +2,7 @@ package coretypes import ( "encoding/json" + "errors" "time" abci "github.com/tendermint/tendermint/abci/types" @@ -12,6 +13,18 @@ import ( "github.com/tendermint/tendermint/types" ) +// List of standardized errors used across RPC +var ( + ErrZeroOrNegativePerPage = errors.New("zero or negative per_page") + ErrPageOutOfRange = errors.New("page should be within range") + ErrZeroOrNegativeHeight = errors.New("height must be greater than zero") + ErrHeightExceedsChainHead = errors.New("height must be less than or equal to the head of the node's blockchain") + ErrHeightNotAvailable = errors.New("height is not available") + // ErrInvalidRequest is used as a wrapper to cover more specific cases where the user has + // made an invalid request + ErrInvalidRequest = errors.New("invalid request") +) + // List of blocks type ResultBlockchainInfo struct { LastHeight int64 `json:"last_height"` diff --git a/rpc/jsonrpc/client/http_json_client.go b/rpc/jsonrpc/client/http_json_client.go index 59727390a..9041b210e 100644 --- a/rpc/jsonrpc/client/http_json_client.go +++ b/rpc/jsonrpc/client/http_json_client.go @@ -173,7 +173,7 @@ func (c *Client) Call( requestBuf := bytes.NewBuffer(requestBytes) httpRequest, err := http.NewRequestWithContext(ctx, http.MethodPost, c.address, requestBuf) if err != nil { - return nil, fmt.Errorf("request failed: %w", err) + return nil, fmt.Errorf("request setup failed: %w", err) } httpRequest.Header.Set("Content-Type", "application/json") @@ -183,8 +183,11 @@ func (c *Client) Call( } httpResponse, err := c.client.Do(httpRequest) + if e, ok := err.(*url.Error); ok && e.Timeout() { + panic("Hello world") + } if err != nil { - return nil, fmt.Errorf("post failed: %w", err) + return nil, fmt.Errorf("request failed: %w", err) } defer httpResponse.Body.Close() diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index 93a3a124e..6731384f9 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/rpc/jsonrpc/server/http_json_handler.go @@ -3,6 +3,7 @@ package server import ( "bytes" "encoding/json" + "errors" "fmt" "io/ioutil" "net/http" @@ -11,7 +12,8 @@ import ( tmjson "github.com/tendermint/tendermint/libs/json" "github.com/tendermint/tendermint/libs/log" - types "github.com/tendermint/tendermint/rpc/jsonrpc/types" + ctypes "github.com/tendermint/tendermint/rpc/core/types" + "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) // HTTP + JSON handler @@ -23,7 +25,6 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han if err != nil { WriteRPCResponseHTTPError( w, - http.StatusBadRequest, types.RPCInvalidRequestError( nil, fmt.Errorf("error reading request body: %w", err), @@ -50,7 +51,6 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han if err := json.Unmarshal(b, &request); err != nil { WriteRPCResponseHTTPError( w, - http.StatusInternalServerError, types.RPCParseError( fmt.Errorf("error unmarshaling request: %w", err), ), @@ -100,11 +100,27 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han returns := rpcFunc.f.Call(args) logger.Info("HTTPJSONRPC", "method", request.Method, "args", args, "returns", returns) result, err := unreflectResult(returns) - if err != nil { - responses = append(responses, types.RPCInternalError(request.ID, err)) - continue + switch e := err.(type) { + // if no error then return a success response + case nil: + responses = append(responses, types.NewRPCSuccessResponse(request.ID, result)) + + // if this already of type RPC error then forward that error + case *types.RPCError: + responses = append(responses, types.NewRPCErrorResponse(request.ID, e.Code, e.Message, e.Data)) + + default: // we need to unwrap the error and parse it accordingly + switch errors.Unwrap(err) { + // check if the error was due to an invald request + case ctypes.ErrZeroOrNegativeHeight, ctypes.ErrZeroOrNegativePerPage, + ctypes.ErrPageOutOfRange, ctypes.ErrInvalidRequest: + responses = append(responses, types.RPCInvalidRequestError(request.ID, err)) + + // lastly default all remaining errors as internal errors + default: // includes ctypes.ErrHeightNotAvailable and ctypes.ErrHeightExceedsChainHead + responses = append(responses, types.RPCInternalError(request.ID, err)) + } } - responses = append(responses, types.NewRPCSuccessResponse(request.ID, result)) } if len(responses) > 0 { WriteRPCResponseHTTP(w, responses...) diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go index b323d46fd..14238bf8a 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/rpc/jsonrpc/server/http_server.go @@ -91,17 +91,42 @@ func ServeTLS( // WriteRPCResponseHTTPError marshals res as JSON and writes it to w. // +// Maps JSON RPC error codes to HTTP Status codes as follows: +// +// HTTP Status code message +// 500 -32700 Parse error. +// 400 -32600 Invalid Request. +// 404 -32601 Method not found. +// 500 -32602 Invalid params. +// 500 -32603 Internal error. +// 500 -32099..-32000 Server error. +// +// source: https://www.jsonrpc.org/historical/json-rpc-over-http.html +// // Panics if it can't Marshal res or write to w. func WriteRPCResponseHTTPError( w http.ResponseWriter, - httpCode int, res types.RPCResponse, ) { + if res.Error == nil { + panic("tried to write http error response without RPC error") + } + jsonBytes, err := json.MarshalIndent(res, "", " ") if err != nil { panic(err) } + var httpCode int + switch res.Error.Code { + case -32600: + httpCode = http.StatusBadRequest + case -32601: + httpCode = http.StatusNotFound + default: + httpCode = http.StatusInternalServerError + } + w.Header().Set("Content-Type", "application/json") w.WriteHeader(httpCode) if _, err := w.Write(jsonBytes); err != nil { @@ -186,7 +211,6 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler ) WriteRPCResponseHTTPError( rww, - http.StatusInternalServerError, types.RPCInternalError(types.JSONRPCIntID(-1), err), ) } diff --git a/rpc/jsonrpc/server/http_server_test.go b/rpc/jsonrpc/server/http_server_test.go index 60f3ce126..98e1adf25 100644 --- a/rpc/jsonrpc/server/http_server_test.go +++ b/rpc/jsonrpc/server/http_server_test.go @@ -159,9 +159,7 @@ func TestWriteRPCResponseHTTP(t *testing.T) { func TestWriteRPCResponseHTTPError(t *testing.T) { w := httptest.NewRecorder() - WriteRPCResponseHTTPError(w, - http.StatusInternalServerError, - types.RPCInternalError(types.JSONRPCIntID(-1), errors.New("foo"))) + WriteRPCResponseHTTPError(w, types.RPCInternalError(types.JSONRPCIntID(-1), errors.New("foo"))) resp := w.Result() body, err := ioutil.ReadAll(resp.Body) _ = resp.Body.Close() diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index 3e6250183..d5c2a44d8 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -2,6 +2,7 @@ package server import ( "encoding/hex" + "errors" "fmt" "net/http" "reflect" @@ -10,6 +11,7 @@ import ( tmjson "github.com/tendermint/tendermint/libs/json" "github.com/tendermint/tendermint/libs/log" + ctypes "github.com/tendermint/tendermint/rpc/core/types" types "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) @@ -25,8 +27,7 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit // Exception for websocket endpoints if rpcFunc.ws { return func(w http.ResponseWriter, r *http.Request) { - WriteRPCResponseHTTPError(w, http.StatusNotFound, - types.RPCMethodNotFoundError(dummyID)) + WriteRPCResponseHTTPError(w, types.RPCMethodNotFoundError(dummyID)) } } @@ -41,7 +42,6 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit if err != nil { WriteRPCResponseHTTPError( w, - http.StatusInternalServerError, types.RPCInvalidParamsError( dummyID, fmt.Errorf("error converting http params to arguments: %w", err), @@ -55,12 +55,27 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit logger.Debug("HTTPRestRPC", "method", r.URL.Path, "args", args, "returns", returns) result, err := unreflectResult(returns) - if err != nil { - WriteRPCResponseHTTPError(w, http.StatusInternalServerError, - types.RPCInternalError(dummyID, err)) - return + switch e := err.(type) { + // if no error then return a success response + case nil: + WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)) + + // if this already of type RPC error then forward that error. + case *types.RPCError: + WriteRPCResponseHTTPError(w, types.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data)) + + default: // we need to unwrap the error and parse it accordingly + + switch errors.Unwrap(err) { + case ctypes.ErrZeroOrNegativeHeight, ctypes.ErrZeroOrNegativePerPage, + ctypes.ErrPageOutOfRange, ctypes.ErrInvalidRequest: + WriteRPCResponseHTTPError(w, types.RPCInvalidRequestError(dummyID, err)) + + default: // ctypes.ErrHeightNotAvailable, ctypes.ErrHeightExceedsChainHead: + WriteRPCResponseHTTPError(w, types.RPCInternalError(dummyID, err)) + } } - WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)) + } } diff --git a/rpc/jsonrpc/server/rpc_func.go b/rpc/jsonrpc/server/rpc_func.go index e5855c314..17e72cb10 100644 --- a/rpc/jsonrpc/server/rpc_func.go +++ b/rpc/jsonrpc/server/rpc_func.go @@ -1,7 +1,6 @@ package server import ( - "fmt" "net/http" "reflect" "strings" @@ -86,8 +85,8 @@ func funcReturnTypes(f interface{}) []reflect.Type { // NOTE: assume returns is result struct and error. If error is not nil, return it func unreflectResult(returns []reflect.Value) (interface{}, error) { errV := returns[1] - if errV.Interface() != nil { - return nil, fmt.Errorf("%v", errV.Interface()) + if err, ok := errV.Interface().(error); ok && err != nil { + return nil, err } rv := returns[0] // the result is a registered interface, diff --git a/rpc/jsonrpc/server/ws_handler.go b/rpc/jsonrpc/server/ws_handler.go index f8a89714e..a7b77dbd3 100644 --- a/rpc/jsonrpc/server/ws_handler.go +++ b/rpc/jsonrpc/server/ws_handler.go @@ -14,6 +14,7 @@ import ( "github.com/tendermint/tendermint/libs/log" "github.com/tendermint/tendermint/libs/service" + ctypes "github.com/tendermint/tendermint/rpc/core/types" types "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) @@ -369,7 +370,7 @@ func (wsc *wsConnection) readRoutine() { fnArgs, err := jsonParamsToArgs(rpcFunc, request.Params) if err != nil { if err := wsc.WriteRPCResponse(writeCtx, - types.RPCInternalError(request.ID, fmt.Errorf("error converting json params to arguments: %w", err)), + types.RPCInvalidParamsError(request.ID, fmt.Errorf("error converting json params to arguments: %w", err)), ); err != nil { wsc.Logger.Error("Error writing RPC response", "err", err) } @@ -383,17 +384,34 @@ func (wsc *wsConnection) readRoutine() { // TODO: Need to encode args/returns to string if we want to log them wsc.Logger.Info("WSJSONRPC", "method", request.Method) + var resp types.RPCResponse result, err := unreflectResult(returns) - if err != nil { - if err := wsc.WriteRPCResponse(writeCtx, types.RPCInternalError(request.ID, err)); err != nil { - wsc.Logger.Error("Error writing RPC response", "err", err) + switch e := err.(type) { + // if no error then return a success response + case nil: + resp = types.NewRPCSuccessResponse(request.ID, result) + + // if this already of type RPC error then forward that error + case *types.RPCError: + resp = types.NewRPCErrorResponse(request.ID, e.Code, e.Message, e.Data) + + default: // we need to unwrap the error and parse it accordingly + switch errors.Unwrap(err) { + // check if the error was due to an invald request + case ctypes.ErrZeroOrNegativeHeight, ctypes.ErrZeroOrNegativePerPage, + ctypes.ErrPageOutOfRange, ctypes.ErrInvalidRequest: + resp = types.RPCInvalidRequestError(request.ID, err) + + // lastly default all remaining errors as internal errors + default: // includes ctypes.ErrHeightNotAvailable and ctypes.ErrHeightExceedsChainHead + resp = types.RPCInternalError(request.ID, err) } - continue } - if err := wsc.WriteRPCResponse(writeCtx, types.NewRPCSuccessResponse(request.ID, result)); err != nil { + if err := wsc.WriteRPCResponse(writeCtx, resp); err != nil { wsc.Logger.Error("Error writing RPC response", "err", err) } + } } } diff --git a/rpc/jsonrpc/types/types.go b/rpc/jsonrpc/types/types.go index fb1ecd10b..3ed2b375d 100644 --- a/rpc/jsonrpc/types/types.go +++ b/rpc/jsonrpc/types/types.go @@ -214,7 +214,7 @@ func (resp RPCResponse) String() string { // If there was an error in detecting the id in the Request object (e.g. Parse // error/Invalid Request), it MUST be Null. func RPCParseError(err error) RPCResponse { - return NewRPCErrorResponse(nil, -32700, "Parse error. Invalid JSON", err.Error()) + return NewRPCErrorResponse(nil, -32700, "Parse error", err.Error()) } // From the JSON-RPC 2.0 spec: diff --git a/rpc/jsonrpc/types/types_test.go b/rpc/jsonrpc/types/types_test.go index 8434f208b..d57a0403d 100644 --- a/rpc/jsonrpc/types/types_test.go +++ b/rpc/jsonrpc/types/types_test.go @@ -40,7 +40,7 @@ func TestResponses(t *testing.T) { d := RPCParseError(errors.New("hello world")) e, _ := json.Marshal(d) - f := `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error. Invalid JSON","data":"hello world"}}` + f := `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"hello world"}}` assert.Equal(f, string(e)) g := RPCMethodNotFoundError(jsonid)