rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests

Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.

Author: pinheadmz

src/httprpc.cpp

@@ -75,6 +75,9 @@ static bool g_rpc_whitelist_default = false;
 
 static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCRequest& jreq)
 {
+    // Sending HTTP errors is a legacy JSON-RPC behavior.
+    Assume(jreq.m_json_version != JSONRPCVersion::V2);
+
     // Send error reply from json-rpc error object
     int nStatus = HTTP_INTERNAL_SERVER_ERROR;
     int code = objError.find_value("code").getInt<int>();
@@ -201,7 +204,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 return false;
             }
 
-            reply = JSONRPCExec(jreq);
+            // Legacy 1.0/1.1 behavior is for failed requests to throw
+            // exceptions which return HTTP errors and RPC errors to the client.
+            // 2.0 behavior is to catch exceptions and return HTTP success with
+            // RPC errors, as long as there is not an actual HTTP server error.
+            const bool catch_errors{jreq.m_json_version == JSONRPCVersion::V2};
+            reply = JSONRPCExec(jreq, catch_errors);
 
         // array of requests
         } else if (valRequest.isArray()) {
@@ -226,10 +234,11 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             // Execute each request
             reply = UniValue::VARR;
             for (size_t i{0}; i < valRequest.size(); ++i) {
-                // Batches include errors in the batch response, they do not throw
+                // Batches never throw HTTP errors, they are always just included
+                // in "HTTP OK" responses.
                 try {
                     jreq.parse(valRequest[i]);
-                    reply.push_back(JSONRPCExec(jreq));
+                    reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
                 } catch (UniValue& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
                 } catch (const std::exception& e) {

src/rpc/server.cpp

@@ -358,11 +358,20 @@ bool IsDeprecatedRPCEnabled(const std::string& method)
     return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end();
 }
 
-UniValue JSONRPCExec(const JSONRPCRequest& jreq)
-{
-    // Might throw exception. Single requests will throw and send HTTP error codes
-    // but inside a batch, we just include the error object and return HTTP 200
-    UniValue result = tableRPC.execute(jreq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors)
+{
+    UniValue result;
+    if (catch_errors) {
+        try {
+            result = tableRPC.execute(jreq);
+        } catch (UniValue& e) {
+            return JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version);
+        } catch (const std::exception& e) {
+            return JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_MISC_ERROR, e.what()), jreq.id, jreq.m_json_version);
+        }
+    } else {
+        result = tableRPC.execute(jreq);
+    }
 
     return JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id, jreq.m_json_version);
 }

src/rpc/server.h

@@ -181,7 +181,7 @@ extern CRPCTable tableRPC;
 void StartRPC();
 void InterruptRPC();
 void StopRPC();
-UniValue JSONRPCExec(const JSONRPCRequest& jreq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors);
 
 // Drop witness when serializing for RPC?
 bool RPCSerializationWithoutWitness();

test/functional/interface_rpc.py

@@ -188,9 +188,9 @@ def test_http_status_codes(self):
         self.log.info("Testing HTTP status codes for JSON-RPC 2.0 requests...")
         # OK
         expect_http_rpc_status(200, None,                   self.nodes[0], "getblockhash", [0],  2, False)
-        # RPC errors and HTTP errors
-        expect_http_rpc_status(404, RPC_METHOD_NOT_FOUND,   self.nodes[0], "invalidmethod", [],  2, False)
-        expect_http_rpc_status(500, RPC_INVALID_PARAMETER,  self.nodes[0], "getblockhash", [42], 2, False)
+        # RPC errors but not HTTP errors
+        expect_http_rpc_status(200, RPC_METHOD_NOT_FOUND,   self.nodes[0], "invalidmethod", [],  2, False)
+        expect_http_rpc_status(200, RPC_INVALID_PARAMETER,  self.nodes[0], "getblockhash", [42], 2, False)
         # force-send invalidly formatted requests
         response, status = send_json_rpc(self.nodes[0], {"jsonrpc": 2, "method": "getblockcount"})
         assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "jsonrpc field must be a string"}})
@@ -212,7 +212,7 @@ def test_http_status_codes(self):
         expect_http_rpc_status(200, None,                   self.nodes[0], "generatetoaddress", [1, "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"],  2, True)
         # The command worked even though there was no response
         assert_equal(block_count + 1, self.nodes[0].getblockcount())
-        expect_http_rpc_status(500, RPC_INVALID_ADDRESS_OR_KEY, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
+        expect_http_rpc_status(200, RPC_INVALID_ADDRESS_OR_KEY, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
         # Sanity check: command was not executed
         assert_equal(block_count + 1, self.nodes[0].getblockcount())