nspcc-dev / neo-go

Go Node and SDK for the NEO blockchain
MIT License
120 stars 78 forks source link

discrepancy in http return codes for RPC server between C# node and Go node on error #3586

Open ixje opened 6 days ago

ixje commented 6 days ago

Current Behaviour

curl -i --location 'https://rpc10.n3.nspcc.ru:10331' --header 'Content-Type: text/plain' --data '{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "getapplicationlog",
  "params": ["0xacaa380861ecbd458a924256f3ab798d89263d43d3340014002eccab2542da31"]
}'
**HTTP/2 422**

Expected Behavior

status code 200 like C#

curl -i --location 'http://seed1.neo.org:10332' --header 'Content-Type: text/plain' --data '{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "getapplicationlog",
  "params": ["0xacaa380861ecbd458a924256f3ab798d89263d43d3340014002eccab2542da31"]
}'
**HTTP/1.1 200 OK**

Similarly an invalid invokescript returns 200 on C# and 400 with neo-go.

Possible Solution

change status code to 200

From https://www.rfc-editor.org/rfc/rfc4918#section-11.2

The 422 (Unprocessable Content) status code indicates that the server understands the content type of the request content (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request content is correct, but it was unable to process the contained instructions.

I assume the reason came from the bold part, but is that really fair? It actually is able to process the instructions in the request, its just that it couldn't find what was asked for. This feels like throwing a 422 just because you called getnep17balances expecting there to be some but there were none

Your Environment

0.106.3

roman-khimov commented 6 days ago

This behavior goes back to 19a430b262dc418a17a781200c0ba7ca5a2148b1. Not that we've never seen it, it was always suspicious, but it worked well and I'm still not sure what can be affected if we're to change it. Also, JSON-RPC itself doesn't specify how to use it over HTTP.

Let's review other codes as well while we're here and try to pick the best practices. This spec is outdated, but still has some ideas for HTTP codes: https://www.jsonrpc.org/historical/json-rpc-over-http.html, also https://stackoverflow.com/questions/76860816/json-rpc-v2-api-http-layer-what-are-the-appropriate-http-layer-status-codes-fo.

ixje commented 6 days ago

from the comment on the last post

The gist of both of these is that non-200 codes are for indicating a problem with the HTTP transport, and not for errors that are related to the semantics of the method call.

That seems to be inline with what C# is doing. Most HTTP request libraries also suggest some flow of

  1. do request
  2. check status code of response
  3. process response if status OK

The way neo-go works now means I need to remove the 2nd step (which sometimes is even done implicitly by libraries) or I can't get to the point that processes the actual error from the JSON-RPC error.