From 4202c170da37a3203e05a9f39f303d7df19b6d81 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Mon, 27 Feb 2023 13:00:37 -0500
Subject: [PATCH 1/9] test: refactor interface_rpc.py

Refactors the helper functions in the test to provide more
fine-grained control over RPC requests and responses than
the usual AuthProxy methods.

No change in test behavior, the same RPC requests are made.
---
 test/functional/interface_rpc.py | 112 +++++++++++++++++++++++--------
 1 file changed, 83 insertions(+), 29 deletions(-)

diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index e873e2da0bffc..501a841805a83 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -4,22 +4,72 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Tests some generic aspects of the RPC interface."""
 
+import json
 import os
-from test_framework.authproxy import JSONRPCException
+from dataclasses import dataclass
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_greater_than_or_equal
 from threading import Thread
+from typing import Optional
 import subprocess
 
 
-def expect_http_status(expected_http_status, expected_rpc_code,
-                       fcn, *args):
-    try:
-        fcn(*args)
-        raise AssertionError(f"Expected RPC error {expected_rpc_code}, got none")
-    except JSONRPCException as exc:
-        assert_equal(exc.error["code"], expected_rpc_code)
-        assert_equal(exc.http_status, expected_http_status)
+RPC_INVALID_PARAMETER = -8
+RPC_METHOD_NOT_FOUND  = -32601
+RPC_INVALID_REQUEST   = -32600
+
+
+@dataclass
+class BatchOptions:
+    version: Optional[int] = None
+    notification: bool = False
+    request_fields: Optional[dict] = None
+    response_fields: Optional[dict] = None
+
+
+def format_request(options, idx, fields):
+    request = {}
+    if options.version == 1:
+        request.update(version="1.1")
+    elif options.version == 2:
+        request.update(jsonrpc="2.0")
+    elif options.version is not None:
+        raise NotImplementedError(f"Unknown JSONRPC version {options.version}")
+    if not options.notification:
+        request.update(id=idx)
+    request.update(fields)
+    if options.request_fields:
+        request.update(options.request_fields)
+    return request
+
+
+def format_response(options, idx, fields):
+    response = {}
+    response.update(id=None if options.notification else idx)
+    response.update(result=None, error=None)
+    response.update(fields)
+    if options.response_fields:
+        response.update(options.response_fields)
+    return response
+
+
+def send_raw_rpc(node, raw_body: bytes) -> tuple[object, int]:
+    return node._request("POST", "/", raw_body)
+
+
+def send_json_rpc(node, body: object) -> tuple[object, int]:
+    raw = json.dumps(body).encode("utf-8")
+    return send_raw_rpc(node, raw)
+
+
+def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params, version=1, notification=False):
+    req = format_request(BatchOptions(version, notification), 0, {"method": method, "params": params})
+    response, status = send_json_rpc(node, req)
+
+    if expected_rpc_error_code is not None:
+        assert_equal(response["error"]["code"], expected_rpc_error_code)
+
+    assert_equal(status, expected_http_status)
 
 
 def test_work_queue_getblock(node, got_exceeded_error):
@@ -51,34 +101,38 @@ def test_getrpcinfo(self):
     def test_batch_request(self):
         self.log.info("Testing basic JSON-RPC batch request...")
 
-        results = self.nodes[0].batch([
+        calls = [
             # A basic request that will work fine.
-            {"method": "getblockcount", "id": 1},
+            {"method": "getblockcount"},
             # Request that will fail.  The whole batch request should still
             # work fine.
-            {"method": "invalidmethod", "id": 2},
+            {"method": "invalidmethod"},
             # Another call that should succeed.
-            {"method": "getblockhash", "id": 3, "params": [0]},
-        ])
-
-        result_by_id = {}
-        for res in results:
-            result_by_id[res["id"]] = res
-
-        assert_equal(result_by_id[1]['error'], None)
-        assert_equal(result_by_id[1]['result'], 0)
-
-        assert_equal(result_by_id[2]['error']['code'], -32601)
-        assert_equal(result_by_id[2]['result'], None)
-
-        assert_equal(result_by_id[3]['error'], None)
-        assert result_by_id[3]['result'] is not None
+            {"method": "getblockhash", "params": [0]},
+        ]
+        results = [
+            {"result": 0},
+            {"error": {"code": RPC_METHOD_NOT_FOUND, "message": "Method not found"}},
+            {"result": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206"},
+            {"error": {"code": RPC_INVALID_REQUEST, "message": "Missing method"}},
+        ]
+
+        request = []
+        response = []
+        for idx, (call, result) in enumerate(zip(calls, results), 1):
+            options = BatchOptions()
+            request.append(format_request(options, idx, call))
+            response.append(format_response(options, idx, result))
+
+        rpc_response, http_status = send_json_rpc(self.nodes[0], request)
+        assert_equal(http_status, 200)
+        assert_equal(rpc_response, response)
 
     def test_http_status_codes(self):
         self.log.info("Testing HTTP status codes for JSON-RPC requests...")
 
-        expect_http_status(404, -32601, self.nodes[0].invalidmethod)
-        expect_http_status(500, -8, self.nodes[0].getblockhash, 42)
+        expect_http_rpc_status(404, RPC_METHOD_NOT_FOUND,  self.nodes[0], "invalidmethod", [])
+        expect_http_rpc_status(500, RPC_INVALID_PARAMETER, self.nodes[0], "getblockhash", [42])
 
     def test_work_queue_exceeded(self):
         self.log.info("Testing work queue exceeded...")

From 09416f9ec445e4d6bb277400758083b0b4e8b174 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Sun, 31 Dec 2023 07:55:03 -0500
Subject: [PATCH 2/9] test: cover JSONRPC 2.0 requests, batches, and
 notifications

---
 test/functional/interface_rpc.py | 97 ++++++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 10 deletions(-)

diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index 501a841805a83..748d83858ae7f 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -14,9 +14,11 @@
 import subprocess
 
 
-RPC_INVALID_PARAMETER = -8
-RPC_METHOD_NOT_FOUND  = -32601
-RPC_INVALID_REQUEST   = -32600
+RPC_INVALID_ADDRESS_OR_KEY = -5
+RPC_INVALID_PARAMETER      = -8
+RPC_METHOD_NOT_FOUND       = -32601
+RPC_INVALID_REQUEST        = -32600
+RPC_PARSE_ERROR            = -32700
 
 
 @dataclass
@@ -98,9 +100,7 @@ def test_getrpcinfo(self):
         assert_greater_than_or_equal(command['duration'], 0)
         assert_equal(info['logpath'], os.path.join(self.nodes[0].chain_path, 'debug.log'))
 
-    def test_batch_request(self):
-        self.log.info("Testing basic JSON-RPC batch request...")
-
+    def test_batch_request(self, call_options):
         calls = [
             # A basic request that will work fine.
             {"method": "getblockcount"},
@@ -109,6 +109,8 @@ def test_batch_request(self):
             {"method": "invalidmethod"},
             # Another call that should succeed.
             {"method": "getblockhash", "params": [0]},
+            # Invalid request format
+            {"pizza": "sausage"}
         ]
         results = [
             {"result": 0},
@@ -120,7 +122,9 @@ def test_batch_request(self):
         request = []
         response = []
         for idx, (call, result) in enumerate(zip(calls, results), 1):
-            options = BatchOptions()
+            options = call_options(idx)
+            if options is None:
+                continue
             request.append(format_request(options, idx, call))
             response.append(format_response(options, idx, result))
 
@@ -128,11 +132,84 @@ def test_batch_request(self):
         assert_equal(http_status, 200)
         assert_equal(rpc_response, response)
 
-    def test_http_status_codes(self):
-        self.log.info("Testing HTTP status codes for JSON-RPC requests...")
+    def test_batch_requests(self):
+        self.log.info("Testing empty batch request...")
+        self.test_batch_request(lambda idx: None)
+
+        self.log.info("Testing basic JSON-RPC 2.0 batch request...")
+        self.test_batch_request(lambda idx: BatchOptions(version=2))
+
+        self.log.info("Testing JSON-RPC 2.0 batch with notifications...")
+        self.test_batch_request(lambda idx: BatchOptions(version=2, notification=idx < 2))
+
+        self.log.info("Testing JSON-RPC 2.0 batch of ALL notifications...")
+        self.test_batch_request(lambda idx: BatchOptions(version=2, notification=True))
+
+        # JSONRPC 1.1 does not support batch requests, but test them for backwards compatibility.
+        self.log.info("Testing nonstandard JSON-RPC 1.1 batch request...")
+        self.test_batch_request(lambda idx: BatchOptions(version=1))
 
+        self.log.info("Testing nonstandard mixed JSON-RPC 1.1/2.0 batch request...")
+        self.test_batch_request(lambda idx: BatchOptions(version=2 if idx % 2 else 1))
+
+        self.log.info("Testing nonstandard batch request without version numbers...")
+        self.test_batch_request(lambda idx: BatchOptions())
+
+        self.log.info("Testing nonstandard batch request without version numbers or ids...")
+        self.test_batch_request(lambda idx: BatchOptions(notification=True))
+
+        self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
+        self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "1.0"}))
+
+        self.log.info("Testing unrecognized jsonrpc version number is accepted...")
+        self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "2.1"}))
+
+    def test_http_status_codes(self):
+        self.log.info("Testing HTTP status codes for JSON-RPC 1.1 requests...")
+        # OK
+        expect_http_rpc_status(200, None,                  self.nodes[0], "getblockhash", [0])
+        # Errors
         expect_http_rpc_status(404, RPC_METHOD_NOT_FOUND,  self.nodes[0], "invalidmethod", [])
         expect_http_rpc_status(500, RPC_INVALID_PARAMETER, self.nodes[0], "getblockhash", [42])
+        # force-send empty request
+        response, status = send_raw_rpc(self.nodes[0], b"")
+        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_PARSE_ERROR, "message": "Parse error"}})
+        assert_equal(status, 500)
+        # force-send invalidly formatted request
+        response, status = send_raw_rpc(self.nodes[0], b"this is bad")
+        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_PARSE_ERROR, "message": "Parse error"}})
+        assert_equal(status, 500)
+
+        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)
+        # force-send invalidly formatted requests
+        response, status = send_json_rpc(self.nodes[0], {"jsonrpc": 2, "method": "getblockcount"})
+        assert_equal(response, {"error": None, "id": None, "result": 0})
+        assert_equal(status, 200)
+        response, status = send_json_rpc(self.nodes[0], {"jsonrpc": "3.0", "method": "getblockcount"})
+        assert_equal(response, {"error": None, "id": None, "result": 0})
+        assert_equal(status, 200)
+
+        self.log.info("Testing HTTP status codes for JSON-RPC 2.0 notifications...")
+        # Not notification: id exists
+        response, status = send_json_rpc(self.nodes[0], {"jsonrpc": "2.0", "id": None, "method": "getblockcount"})
+        assert_equal(response["result"], 0)
+        assert_equal(status, 200)
+        # Not notification: JSON 1.1
+        expect_http_rpc_status(200, None,                   self.nodes[0], "getblockcount", [],  1)
+        # Not notification: has "id" field
+        expect_http_rpc_status(200, None,                   self.nodes[0], "getblockcount", [],  2, False)
+        block_count = self.nodes[0].getblockcount()
+        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)
+        # Sanity check: command was not executed
+        assert_equal(block_count + 1, self.nodes[0].getblockcount())
 
     def test_work_queue_exceeded(self):
         self.log.info("Testing work queue exceeded...")
@@ -148,7 +225,7 @@ def test_work_queue_exceeded(self):
 
     def run_test(self):
         self.test_getrpcinfo()
-        self.test_batch_request()
+        self.test_batch_requests()
         self.test_http_status_codes()
         self.test_work_queue_exceeded()
 

From df6e3756d6feaf1856e7886820b70874209fd90b Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Tue, 23 Jan 2024 10:33:26 -0500
Subject: [PATCH 3/9] rpc: Avoid copies in JSONRPCReplyObj()

Change parameters from const references to values, so they can be moved into
the reply instead of copied. Also update callers to move instead of copy.
---
 src/bitcoin-cli.cpp |  6 +++---
 src/httprpc.cpp     | 10 +++++-----
 src/rpc/request.cpp | 14 ++++----------
 src/rpc/request.h   |  3 +--
 src/rpc/server.cpp  |  6 +++---
 5 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 45db7a9a66cb1..3591f7248bf89 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -302,7 +302,7 @@ class AddrinfoRequestHandler : public BaseRequestHandler
         }
         addresses.pushKV("total", total);
         result.pushKV("addresses_known", addresses);
-        return JSONRPCReplyObj(result, NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
     }
 };
 
@@ -371,7 +371,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
         }
         result.pushKV("relayfee", batch[ID_NETWORKINFO]["result"]["relayfee"]);
         result.pushKV("warnings", batch[ID_NETWORKINFO]["result"]["warnings"]);
-        return JSONRPCReplyObj(result, NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
     }
 };
 
@@ -709,7 +709,7 @@ class GenerateToAddressRequestHandler : public BaseRequestHandler
         UniValue result(UniValue::VOBJ);
         result.pushKV("address", address_str);
         result.pushKV("blocks", reply.get_obj()["result"]);
-        return JSONRPCReplyObj(result, NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
     }
 protected:
     std::string address_str;
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index c72dbf10bc958..aef20c24fc685 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -73,7 +73,7 @@ static std::vector<std::vector<std::string>> g_rpcauth;
 static std::map<std::string, std::set<std::string>> g_rpc_whitelist;
 static bool g_rpc_whitelist_default = false;
 
-static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id)
+static void JSONErrorReply(HTTPRequest* req, UniValue objError, UniValue id)
 {
     // Send error reply from json-rpc error object
     int nStatus = HTTP_INTERNAL_SERVER_ERROR;
@@ -84,7 +84,7 @@ static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const Uni
     else if (code == RPC_METHOD_NOT_FOUND)
         nStatus = HTTP_NOT_FOUND;
 
-    std::string strReply = JSONRPCReply(NullUniValue, objError, id);
+    std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), std::move(id)).write() + "\n";
 
     req->WriteHeader("Content-Type", "application/json");
     req->WriteReply(nStatus, strReply);
@@ -203,7 +203,7 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             UniValue result = tableRPC.execute(jreq);
 
             // Send reply
-            strReply = JSONRPCReply(result, NullUniValue, jreq.id);
+            strReply = JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id).write() + "\n";
 
         // array of requests
         } else if (valRequest.isArray()) {
@@ -230,8 +230,8 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
 
         req->WriteHeader("Content-Type", "application/json");
         req->WriteReply(HTTP_OK, strReply);
-    } catch (const UniValue& objError) {
-        JSONErrorReply(req, objError, jreq.id);
+    } catch (UniValue& e) {
+        JSONErrorReply(req, std::move(e), jreq.id);
         return false;
     } catch (const std::exception& e) {
         JSONErrorReply(req, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id);
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index b7acd62ee3d4b..12726ecc88599 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -37,24 +37,18 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params,
     return request;
 }
 
-UniValue JSONRPCReplyObj(const UniValue& result, const UniValue& error, const UniValue& id)
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id)
 {
     UniValue reply(UniValue::VOBJ);
     if (!error.isNull())
         reply.pushKV("result", NullUniValue);
     else
-        reply.pushKV("result", result);
-    reply.pushKV("error", error);
-    reply.pushKV("id", id);
+        reply.pushKV("result", std::move(result));
+    reply.pushKV("error", std::move(error));
+    reply.pushKV("id", std::move(id));
     return reply;
 }
 
-std::string JSONRPCReply(const UniValue& result, const UniValue& error, const UniValue& id)
-{
-    UniValue reply = JSONRPCReplyObj(result, error, id);
-    return reply.write() + "\n";
-}
-
 UniValue JSONRPCError(int code, const std::string& message)
 {
     UniValue error(UniValue::VOBJ);
diff --git a/src/rpc/request.h b/src/rpc/request.h
index a682c58d966f0..116ebb8aac15b 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -12,8 +12,7 @@
 #include <univalue.h>
 
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
-UniValue JSONRPCReplyObj(const UniValue& result, const UniValue& error, const UniValue& id);
-std::string JSONRPCReply(const UniValue& result, const UniValue& error, const UniValue& id);
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id);
 UniValue JSONRPCError(int code, const std::string& message);
 
 /** Generate a new RPC authentication cookie and write it to disk */
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 4883341522f88..fd18a7f9ec084 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -366,11 +366,11 @@ static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req)
         jreq.parse(req);
 
         UniValue result = tableRPC.execute(jreq);
-        rpc_result = JSONRPCReplyObj(result, NullUniValue, jreq.id);
+        rpc_result = JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id);
     }
-    catch (const UniValue& objError)
+    catch (UniValue& e)
     {
-        rpc_result = JSONRPCReplyObj(NullUniValue, objError, jreq.id);
+        rpc_result = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id);
     }
     catch (const std::exception& e)
     {

From a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Mon, 27 Feb 2023 13:03:45 -0500
Subject: [PATCH 4/9] rpc: refactor single/batch requests

Simplify the request handling flow so that errors and results only
come from JSONRPCExec()
---
 src/httprpc.cpp    | 24 ++++++++++++++++++------
 src/rpc/server.cpp | 33 +++++----------------------------
 src/rpc/server.h   |  2 +-
 3 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index aef20c24fc685..53f994efd7f0f 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -185,7 +185,7 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
         // Set the URI
         jreq.URI = req->GetURI();
 
-        std::string strReply;
+        UniValue reply;
         bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser);
         if (!user_has_whitelist && g_rpc_whitelist_default) {
             LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser);
@@ -200,13 +200,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 req->WriteReply(HTTP_FORBIDDEN);
                 return false;
             }
-            UniValue result = tableRPC.execute(jreq);
 
-            // Send reply
-            strReply = JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id).write() + "\n";
+            reply = JSONRPCExec(jreq);
 
         // array of requests
         } else if (valRequest.isArray()) {
+            // Check authorization for each request's method
             if (user_has_whitelist) {
                 for (unsigned int reqIdx = 0; reqIdx < valRequest.size(); reqIdx++) {
                     if (!valRequest[reqIdx].isObject()) {
@@ -223,13 +222,26 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                     }
                 }
             }
-            strReply = JSONRPCExecBatch(jreq, valRequest.get_array());
+
+            // 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
+                try {
+                    jreq.parse(valRequest[i]);
+                    reply.push_back(JSONRPCExec(jreq));
+                } catch (UniValue& e) {
+                    reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id));
+                } catch (const std::exception& e) {
+                    reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id));
+                }
+            }
         }
         else
             throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
 
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strReply);
+        req->WriteReply(HTTP_OK, reply.write() + "\n");
     } catch (UniValue& e) {
         JSONErrorReply(req, std::move(e), jreq.id);
         return false;
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index fd18a7f9ec084..f19991409596e 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -358,36 +358,13 @@ bool IsDeprecatedRPCEnabled(const std::string& method)
     return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end();
 }
 
-static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req)
+UniValue JSONRPCExec(const JSONRPCRequest& jreq)
 {
-    UniValue rpc_result(UniValue::VOBJ);
+    // 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);
 
-    try {
-        jreq.parse(req);
-
-        UniValue result = tableRPC.execute(jreq);
-        rpc_result = JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id);
-    }
-    catch (UniValue& e)
-    {
-        rpc_result = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id);
-    }
-    catch (const std::exception& e)
-    {
-        rpc_result = JSONRPCReplyObj(NullUniValue,
-                                     JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id);
-    }
-
-    return rpc_result;
-}
-
-std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq)
-{
-    UniValue ret(UniValue::VARR);
-    for (unsigned int reqIdx = 0; reqIdx < vReq.size(); reqIdx++)
-        ret.push_back(JSONRPCExecOne(jreq, vReq[reqIdx]));
-
-    return ret.write() + "\n";
+    return JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id);
 }
 
 /**
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 9a49d825703a9..2761bcd9db1f7 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -181,7 +181,7 @@ extern CRPCTable tableRPC;
 void StartRPC();
 void InterruptRPC();
 void StopRPC();
-std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq);
 
 // Drop witness when serializing for RPC?
 bool RPCSerializationWithoutWitness();

From 2ca1460ae3a7217eaa8c5972515bf622bedadfce Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Fri, 7 Jul 2023 14:31:18 -0400
Subject: [PATCH 5/9] rpc: identify JSON-RPC 2.0 requests

---
 src/rpc/request.cpp              | 19 +++++++++++++++++++
 src/rpc/request.h                |  6 ++++++
 test/functional/interface_rpc.py | 14 ++++++++------
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index 12726ecc88599..08e0658561bf7 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -167,6 +167,25 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
     // Parse id now so errors from here on will have the id
     id = request.find_value("id");
 
+    // Check for JSON-RPC 2.0 (default 1.1)
+    m_json_version = JSONRPCVersion::V1_LEGACY;
+    const UniValue& jsonrpc_version = request.find_value("jsonrpc");
+    if (!jsonrpc_version.isNull()) {
+        if (!jsonrpc_version.isStr()) {
+            throw JSONRPCError(RPC_INVALID_REQUEST, "jsonrpc field must be a string");
+        }
+        // The "jsonrpc" key was added in the 2.0 spec, but some older documentation
+        // incorrectly included {"jsonrpc":"1.0"} in a request object, so we
+        // maintain that for backwards compatibility.
+        if (jsonrpc_version.get_str() == "1.0") {
+            m_json_version = JSONRPCVersion::V1_LEGACY;
+        } else if (jsonrpc_version.get_str() == "2.0") {
+            m_json_version = JSONRPCVersion::V2;
+        } else {
+            throw JSONRPCError(RPC_INVALID_REQUEST, "JSON-RPC version not supported");
+        }
+    }
+
     // Parse method
     const UniValue& valMethod{request.find_value("method")};
     if (valMethod.isNull())
diff --git a/src/rpc/request.h b/src/rpc/request.h
index 116ebb8aac15b..8b7217269532b 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -11,6 +11,11 @@
 
 #include <univalue.h>
 
+enum class JSONRPCVersion {
+    V1_LEGACY,
+    V2
+};
+
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
 UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id);
 UniValue JSONRPCError(int code, const std::string& message);
@@ -35,6 +40,7 @@ class JSONRPCRequest
     std::string authUser;
     std::string peerAddr;
     std::any context;
+    JSONRPCVersion m_json_version = JSONRPCVersion::V1_LEGACY;
 
     void parse(const UniValue& valRequest);
 };
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index 748d83858ae7f..075dd1c2bcb0e 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -161,8 +161,10 @@ def test_batch_requests(self):
         self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
         self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "1.0"}))
 
-        self.log.info("Testing unrecognized jsonrpc version number is accepted...")
-        self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "2.1"}))
+        self.log.info("Testing unrecognized jsonrpc version number is rejected...")
+        self.test_batch_request(lambda idx: BatchOptions(
+            request_fields={"jsonrpc": "2.1"},
+            response_fields={"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}}))
 
     def test_http_status_codes(self):
         self.log.info("Testing HTTP status codes for JSON-RPC 1.1 requests...")
@@ -188,11 +190,11 @@ def test_http_status_codes(self):
         expect_http_rpc_status(500, 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, {"error": None, "id": None, "result": 0})
-        assert_equal(status, 200)
+        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "jsonrpc field must be a string"}})
+        assert_equal(status, 400)
         response, status = send_json_rpc(self.nodes[0], {"jsonrpc": "3.0", "method": "getblockcount"})
-        assert_equal(response, {"error": None, "id": None, "result": 0})
-        assert_equal(status, 200)
+        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}})
+        assert_equal(status, 400)
 
         self.log.info("Testing HTTP status codes for JSON-RPC 2.0 notifications...")
         # Not notification: id exists

From 466b90562f4785de74b548f7c4a256069e2aaf43 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Fri, 7 Jul 2023 14:41:23 -0400
Subject: [PATCH 6/9] rpc: Add "jsonrpc" field and drop null "result"/"error"
 fields

Only for JSON-RPC 2.0 requests.
---
 src/bitcoin-cli.cpp              |  8 ++++----
 src/httprpc.cpp                  | 12 ++++++------
 src/rpc/request.cpp              | 17 ++++++++++++-----
 src/rpc/request.h                |  2 +-
 src/rpc/server.cpp               |  2 +-
 test/functional/interface_rpc.py |  5 ++++-
 6 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 3591f7248bf89..0bafb4f645d27 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -302,7 +302,7 @@ class AddrinfoRequestHandler : public BaseRequestHandler
         }
         addresses.pushKV("total", total);
         result.pushKV("addresses_known", addresses);
-        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
     }
 };
 
@@ -371,7 +371,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
         }
         result.pushKV("relayfee", batch[ID_NETWORKINFO]["result"]["relayfee"]);
         result.pushKV("warnings", batch[ID_NETWORKINFO]["result"]["warnings"]);
-        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue,  /*id=*/1, JSONRPCVersion::V1_LEGACY);
     }
 };
 
@@ -623,7 +623,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
             }
         }
 
-        return JSONRPCReplyObj(UniValue{result}, NullUniValue, 1);
+        return JSONRPCReplyObj(UniValue{result}, NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
     }
 
     const std::string m_help_doc{
@@ -709,7 +709,7 @@ class GenerateToAddressRequestHandler : public BaseRequestHandler
         UniValue result(UniValue::VOBJ);
         result.pushKV("address", address_str);
         result.pushKV("blocks", reply.get_obj()["result"]);
-        return JSONRPCReplyObj(std::move(result), NullUniValue, 1);
+        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
     }
 protected:
     std::string address_str;
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 53f994efd7f0f..817c75f3b50ee 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -73,7 +73,7 @@ static std::vector<std::vector<std::string>> g_rpcauth;
 static std::map<std::string, std::set<std::string>> g_rpc_whitelist;
 static bool g_rpc_whitelist_default = false;
 
-static void JSONErrorReply(HTTPRequest* req, UniValue objError, UniValue id)
+static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCRequest& jreq)
 {
     // Send error reply from json-rpc error object
     int nStatus = HTTP_INTERNAL_SERVER_ERROR;
@@ -84,7 +84,7 @@ static void JSONErrorReply(HTTPRequest* req, UniValue objError, UniValue id)
     else if (code == RPC_METHOD_NOT_FOUND)
         nStatus = HTTP_NOT_FOUND;
 
-    std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), std::move(id)).write() + "\n";
+    std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), jreq.id, jreq.m_json_version).write() + "\n";
 
     req->WriteHeader("Content-Type", "application/json");
     req->WriteReply(nStatus, strReply);
@@ -231,9 +231,9 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                     jreq.parse(valRequest[i]);
                     reply.push_back(JSONRPCExec(jreq));
                 } catch (UniValue& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id));
+                    reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
                 } catch (const std::exception& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id));
+                    reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version));
                 }
             }
         }
@@ -243,10 +243,10 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
         req->WriteHeader("Content-Type", "application/json");
         req->WriteReply(HTTP_OK, reply.write() + "\n");
     } catch (UniValue& e) {
-        JSONErrorReply(req, std::move(e), jreq.id);
+        JSONErrorReply(req, std::move(e), jreq);
         return false;
     } catch (const std::exception& e) {
-        JSONErrorReply(req, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id);
+        JSONErrorReply(req, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq);
         return false;
     }
     return true;
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index 08e0658561bf7..b5bf1e5ffab02 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -37,14 +37,21 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params,
     return request;
 }
 
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id)
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version)
 {
     UniValue reply(UniValue::VOBJ);
-    if (!error.isNull())
-        reply.pushKV("result", NullUniValue);
-    else
+    // Add JSON-RPC version number field in v2 only.
+    if (jsonrpc_version == JSONRPCVersion::V2) reply.pushKV("jsonrpc", "2.0");
+
+    // Add both result and error fields in v1, even though one will be null.
+    // Omit the null field in v2.
+    if (error.isNull()) {
         reply.pushKV("result", std::move(result));
-    reply.pushKV("error", std::move(error));
+        if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("error", NullUniValue);
+    } else {
+        if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("result", NullUniValue);
+        reply.pushKV("error", std::move(error));
+    }
     reply.pushKV("id", std::move(id));
     return reply;
 }
diff --git a/src/rpc/request.h b/src/rpc/request.h
index 8b7217269532b..a7c9af2216b04 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -17,7 +17,7 @@ enum class JSONRPCVersion {
 };
 
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id);
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version);
 UniValue JSONRPCError(int code, const std::string& message);
 
 /** Generate a new RPC authentication cookie and write it to disk */
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index f19991409596e..8e150ef2b758e 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -364,7 +364,7 @@ UniValue JSONRPCExec(const JSONRPCRequest& jreq)
     // but inside a batch, we just include the error object and return HTTP 200
     UniValue result = tableRPC.execute(jreq);
 
-    return JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id);
+    return JSONRPCReplyObj(std::move(result), NullUniValue, jreq.id, jreq.m_json_version);
 }
 
 /**
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index 075dd1c2bcb0e..dc9fb5d40fa8d 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -48,7 +48,10 @@ def format_request(options, idx, fields):
 def format_response(options, idx, fields):
     response = {}
     response.update(id=None if options.notification else idx)
-    response.update(result=None, error=None)
+    if options.version == 2:
+        response.update(jsonrpc="2.0")
+    else:
+        response.update(result=None, error=None)
     response.update(fields)
     if options.response_fields:
         response.update(options.response_fields)

From bf1a1f1662427fbf1a43bb951364eface469bdb7 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Fri, 7 Jul 2023 14:41:23 -0400
Subject: [PATCH 7/9] 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.
---
 src/httprpc.cpp                  | 15 ++++++++++++---
 src/rpc/server.cpp               | 19 ++++++++++++++-----
 src/rpc/server.h                 |  2 +-
 test/functional/interface_rpc.py |  8 ++++----
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 817c75f3b50ee..777ad32bbe2d9 100644
--- a/src/httprpc.cpp
+++ b/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) {
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 8e150ef2b758e..a30fd3b7d0667 100644
--- a/src/rpc/server.cpp
+++ b/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);
 }
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 2761bcd9db1f7..c0e7bc04ad861 100644
--- a/src/rpc/server.h
+++ b/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();
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index dc9fb5d40fa8d..824a7200cd413 100755
--- a/test/functional/interface_rpc.py
+++ b/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())
 

From e7ee80dcf2b68684eae96070875ea13a60e3e7b0 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Fri, 7 Jul 2023 15:06:35 -0400
Subject: [PATCH 8/9] rpc: JSON-RPC 2.0 should not respond to "notifications"

For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional<UniValue> with a default value of
UniValue::VNULL.

A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
---
 src/httprpc.cpp                             | 31 ++++++++++++++++++---
 src/rpc/protocol.h                          |  1 +
 src/rpc/request.cpp                         | 10 +++++--
 src/rpc/request.h                           |  6 ++--
 test/functional/interface_rpc.py            | 27 ++++++++++++------
 test/functional/test_framework/authproxy.py |  9 ++++++
 6 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 777ad32bbe2d9..3eb34dbe6ab0a 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -211,6 +211,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             const bool catch_errors{jreq.m_json_version == JSONRPCVersion::V2};
             reply = JSONRPCExec(jreq, catch_errors);
 
+            if (jreq.IsNotification()) {
+                // Even though we do execute notifications, we do not respond to them
+                req->WriteReply(HTTP_NO_CONTENT);
+                return true;
+            }
+
         // array of requests
         } else if (valRequest.isArray()) {
             // Check authorization for each request's method
@@ -235,15 +241,32 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
             reply = UniValue::VARR;
             for (size_t i{0}; i < valRequest.size(); ++i) {
                 // Batches never throw HTTP errors, they are always just included
-                // in "HTTP OK" responses.
+                // in "HTTP OK" responses. Notifications never get any response.
+                UniValue response;
                 try {
                     jreq.parse(valRequest[i]);
-                    reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
+                    response = JSONRPCExec(jreq, /*catch_errors=*/true);
                 } catch (UniValue& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
+                    response = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version);
                 } catch (const std::exception& e) {
-                    reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version));
+                    response = JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version);
                 }
+                if (!jreq.IsNotification()) {
+                    reply.push_back(std::move(response));
+                }
+            }
+            // Return no response for an all-notification batch, but only if the
+            // batch request is non-empty. Technically according to the JSON-RPC
+            // 2.0 spec, an empty batch request should also return no response,
+            // However, if the batch request is empty, it means the request did
+            // not contain any JSON-RPC version numbers, so returning an empty
+            // response could break backwards compatibility with old RPC clients
+            // relying on previous behavior. Return an empty array instead of an
+            // empty response in this case to favor being backwards compatible
+            // over complying with the JSON-RPC 2.0 spec in this case.
+            if (reply.size() == 0 && valRequest.size() > 0) {
+                req->WriteReply(HTTP_NO_CONTENT);
+                return true;
             }
         }
         else
diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h
index 75e42e4c88713..83a9010681933 100644
--- a/src/rpc/protocol.h
+++ b/src/rpc/protocol.h
@@ -10,6 +10,7 @@
 enum HTTPStatusCode
 {
     HTTP_OK                    = 200,
+    HTTP_NO_CONTENT            = 204,
     HTTP_BAD_REQUEST           = 400,
     HTTP_UNAUTHORIZED          = 401,
     HTTP_FORBIDDEN             = 403,
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index b5bf1e5ffab02..ca424b4953a66 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -37,7 +37,7 @@ UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params,
     return request;
 }
 
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version)
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version)
 {
     UniValue reply(UniValue::VOBJ);
     // Add JSON-RPC version number field in v2 only.
@@ -52,7 +52,7 @@ UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVe
         if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("result", NullUniValue);
         reply.pushKV("error", std::move(error));
     }
-    reply.pushKV("id", std::move(id));
+    if (id.has_value()) reply.pushKV("id", std::move(id.value()));
     return reply;
 }
 
@@ -172,7 +172,11 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
     const UniValue& request = valRequest.get_obj();
 
     // Parse id now so errors from here on will have the id
-    id = request.find_value("id");
+    if (request.exists("id")) {
+        id = request.find_value("id");
+    } else {
+        id = std::nullopt;
+    }
 
     // Check for JSON-RPC 2.0 (default 1.1)
     m_json_version = JSONRPCVersion::V1_LEGACY;
diff --git a/src/rpc/request.h b/src/rpc/request.h
index a7c9af2216b04..e47f90af86b42 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -7,6 +7,7 @@
 #define BITCOIN_RPC_REQUEST_H
 
 #include <any>
+#include <optional>
 #include <string>
 
 #include <univalue.h>
@@ -17,7 +18,7 @@ enum class JSONRPCVersion {
 };
 
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id);
-UniValue JSONRPCReplyObj(UniValue result, UniValue error, UniValue id, JSONRPCVersion jsonrpc_version);
+UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONRPCVersion jsonrpc_version);
 UniValue JSONRPCError(int code, const std::string& message);
 
 /** Generate a new RPC authentication cookie and write it to disk */
@@ -32,7 +33,7 @@ std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in);
 class JSONRPCRequest
 {
 public:
-    UniValue id;
+    std::optional<UniValue> id = UniValue::VNULL;
     std::string strMethod;
     UniValue params;
     enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
@@ -43,6 +44,7 @@ class JSONRPCRequest
     JSONRPCVersion m_json_version = JSONRPCVersion::V1_LEGACY;
 
     void parse(const UniValue& valRequest);
+    [[nodiscard]] bool IsNotification() const { return !id.has_value() && m_json_version == JSONRPCVersion::V2; };
 };
 
 #endif // BITCOIN_RPC_REQUEST_H
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index 824a7200cd413..b08ca42796529 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -46,8 +46,11 @@ def format_request(options, idx, fields):
 
 
 def format_response(options, idx, fields):
+    if options.version == 2 and options.notification:
+        return None
     response = {}
-    response.update(id=None if options.notification else idx)
+    if not options.notification:
+        response.update(id=idx)
     if options.version == 2:
         response.update(jsonrpc="2.0")
     else:
@@ -129,11 +132,17 @@ def test_batch_request(self, call_options):
             if options is None:
                 continue
             request.append(format_request(options, idx, call))
-            response.append(format_response(options, idx, result))
+            r = format_response(options, idx, result)
+            if r is not None:
+                response.append(r)
 
         rpc_response, http_status = send_json_rpc(self.nodes[0], request)
-        assert_equal(http_status, 200)
-        assert_equal(rpc_response, response)
+        if len(response) == 0 and len(request) > 0:
+            assert_equal(http_status, 204)
+            assert_equal(rpc_response, None)
+        else:
+            assert_equal(http_status, 200)
+            assert_equal(rpc_response, response)
 
     def test_batch_requests(self):
         self.log.info("Testing empty batch request...")
@@ -193,10 +202,10 @@ def test_http_status_codes(self):
         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"}})
+        assert_equal(response, {"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "jsonrpc field must be a string"}})
         assert_equal(status, 400)
         response, status = send_json_rpc(self.nodes[0], {"jsonrpc": "3.0", "method": "getblockcount"})
-        assert_equal(response, {"id": None, "result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}})
+        assert_equal(response, {"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}})
         assert_equal(status, 400)
 
         self.log.info("Testing HTTP status codes for JSON-RPC 2.0 notifications...")
@@ -209,10 +218,12 @@ def test_http_status_codes(self):
         # Not notification: has "id" field
         expect_http_rpc_status(200, None,                   self.nodes[0], "getblockcount", [],  2, False)
         block_count = self.nodes[0].getblockcount()
-        expect_http_rpc_status(200, None,                   self.nodes[0], "generatetoaddress", [1, "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"],  2, True)
+        # Notification response status code: HTTP_NO_CONTENT
+        expect_http_rpc_status(204, 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(200, RPC_INVALID_ADDRESS_OR_KEY, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
+        # No error response for notifications even if they are invalid
+        expect_http_rpc_status(204, None, self.nodes[0], "generatetoaddress", [1, "invalid_address"], 2, True)
         # Sanity check: command was not executed
         assert_equal(block_count + 1, self.nodes[0].getblockcount())
 
diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
index 03042877b205a..7edf9f3679479 100644
--- a/test/functional/test_framework/authproxy.py
+++ b/test/functional/test_framework/authproxy.py
@@ -160,6 +160,15 @@ def _get_response(self):
             raise JSONRPCException({
                 'code': -342, 'message': 'missing HTTP response from server'})
 
+        # Check for no-content HTTP status code, which can be returned when an
+        # RPC client requests a JSON-RPC 2.0 "notification" with no response.
+        # Currently this is only possible if clients call the _request() method
+        # directly to send a raw request.
+        if http_response.status == HTTPStatus.NO_CONTENT:
+            if len(http_response.read()) != 0:
+                raise JSONRPCException({'code': -342, 'message': 'Content received with NO CONTENT status code'})
+            return None, http_response.status
+
         content_type = http_response.getheader('Content-Type')
         if content_type != 'application/json':
             raise JSONRPCException(

From cbc6c440e3811d342fa570713702900b3e3e75b9 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Wed, 1 Mar 2023 14:03:26 -0500
Subject: [PATCH 9/9] doc: add comments and release-notes for JSON-RPC 2.0

---
 doc/JSON-RPC-interface.md  | 16 ++++++++++++++++
 doc/release-notes-27101.md |  9 +++++++++
 src/rpc/request.cpp        | 11 +++++++++++
 src/rpc/util.cpp           |  4 ++--
 src/test/rpc_tests.cpp     |  6 +++---
 5 files changed, 41 insertions(+), 5 deletions(-)
 create mode 100644 doc/release-notes-27101.md

diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md
index ec332d23ebee5..7640102172a97 100644
--- a/doc/JSON-RPC-interface.md
+++ b/doc/JSON-RPC-interface.md
@@ -74,6 +74,22 @@ major version via the `-deprecatedrpc=` command line option. The release notes
 of a new major release come with detailed instructions on what RPC features
 were deprecated and how to re-enable them temporarily.
 
+## JSON-RPC 1.1 vs 2.0
+
+The server recognizes [JSON-RPC v2.0](https://www.jsonrpc.org/specification) requests
+and responds accordingly. A 2.0 request is identified by the presence of
+`"jsonrpc": "2.0"` in the request body. If that key + value is not present in a request,
+the legacy JSON-RPC v1.1 protocol is followed instead, which was the only available
+protocol in previous releases.
+
+|| 1.1 | 2.0 |
+|-|-|-|
+| Request marker | `"version": "1.1"` (or none) | `"jsonrpc": "2.0"` |
+| Response marker | (none) | `"jsonrpc": "2.0"` |
+| `"error"` and `"result"` fields in response | both present | only one is present |
+| HTTP codes in response | `200` unless there is any kind of RPC error (invalid parameters, method not found, etc) | Always `200` unless there is an actual HTTP server error (request parsing error, endpoint not found, etc) |
+| Notifications: requests that get no reply | (not supported) | Supported for requests that exclude the "id" field |
+
 ## Security
 
 The RPC interface allows other programs to control Bitcoin Core,
diff --git a/doc/release-notes-27101.md b/doc/release-notes-27101.md
new file mode 100644
index 0000000000000..8775b59c009f0
--- /dev/null
+++ b/doc/release-notes-27101.md
@@ -0,0 +1,9 @@
+JSON-RPC
+--------
+
+The JSON-RPC server now recognizes JSON-RPC 2.0 requests and responds with
+strict adherence to the specification (https://www.jsonrpc.org/specification):
+
+- Returning HTTP "204 No Content" responses to JSON-RPC 2.0 notifications instead of full responses.
+- Returning HTTP "200 OK" responses in all other cases, rather than 404 responses for unknown methods, 500 responses for invalid parameters, etc.
+- Returning either "result" fields or "error" fields in JSON-RPC responses, rather than returning both fields with one field set to null.
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index ca424b4953a66..d35782189e326 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -26,6 +26,17 @@
  *
  * 1.0 spec: http://json-rpc.org/wiki/specification
  * 1.2 spec: http://jsonrpc.org/historical/json-rpc-over-http.html
+ *
+ * If the server receives a request with the JSON-RPC 2.0 marker `{"jsonrpc": "2.0"}`
+ * then Bitcoin will respond with a strictly specified response.
+ * It will only return an HTTP error code if an actual HTTP error is encountered
+ * such as the endpoint is not found (404) or the request is not formatted correctly (500).
+ * Otherwise the HTTP code is always OK (200) and RPC errors will be included in the
+ * response body.
+ *
+ * 2.0 spec: https://www.jsonrpc.org/specification
+ *
+ * Also see http://www.simple-is-better.org/rpc/#differences-between-1-0-and-2-0
  */
 
 UniValue JSONRPCRequestObj(const std::string& strMethod, const UniValue& params, const UniValue& id)
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index cf48ee11e7b22..6bad1ffc55eec 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -161,7 +161,7 @@ std::string HelpExampleCliNamed(const std::string& methodname, const RPCArgList&
 
 std::string HelpExampleRpc(const std::string& methodname, const std::string& args)
 {
-    return "> curl --user myusername --data-binary '{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", "
+    return "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", "
         "\"method\": \"" + methodname + "\", \"params\": [" + args + "]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n";
 }
 
@@ -172,7 +172,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
         params.pushKV(param.first, param.second);
     }
 
-    return "> curl --user myusername --data-binary '{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", "
+    return "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", "
            "\"method\": \"" + methodname + "\", \"params\": " + params.write() + "}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n";
 }
 
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
index 0d2460c6064a3..41d52d888119e 100644
--- a/src/test/rpc_tests.cpp
+++ b/src/test/rpc_tests.cpp
@@ -551,7 +551,7 @@ BOOST_AUTO_TEST_CASE(help_example)
     // test different argument types
     const RPCArgList& args = {{"foo", "bar"}, {"b", true}, {"n", 1}};
     BOOST_CHECK_EQUAL(HelpExampleCliNamed("test", args), "> bitcoin-cli -named test foo=bar b=true n=1\n");
-    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", args), "> curl --user myusername --data-binary '{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"foo\":\"bar\",\"b\":true,\"n\":1}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
+    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", args), "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"foo\":\"bar\",\"b\":true,\"n\":1}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
 
     // test shell escape
     BOOST_CHECK_EQUAL(HelpExampleCliNamed("test", {{"foo", "b'ar"}}), "> bitcoin-cli -named test foo='b'''ar'\n");
@@ -564,7 +564,7 @@ BOOST_AUTO_TEST_CASE(help_example)
     obj_value.pushKV("b", false);
     obj_value.pushKV("n", 1);
     BOOST_CHECK_EQUAL(HelpExampleCliNamed("test", {{"name", obj_value}}), "> bitcoin-cli -named test name='{\"foo\":\"bar\",\"b\":false,\"n\":1}'\n");
-    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", {{"name", obj_value}}), "> curl --user myusername --data-binary '{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"name\":{\"foo\":\"bar\",\"b\":false,\"n\":1}}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
+    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", {{"name", obj_value}}), "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"name\":{\"foo\":\"bar\",\"b\":false,\"n\":1}}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
 
     // test array params
     UniValue arr_value(UniValue::VARR);
@@ -572,7 +572,7 @@ BOOST_AUTO_TEST_CASE(help_example)
     arr_value.push_back(false);
     arr_value.push_back(1);
     BOOST_CHECK_EQUAL(HelpExampleCliNamed("test", {{"name", arr_value}}), "> bitcoin-cli -named test name='[\"bar\",false,1]'\n");
-    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", {{"name", arr_value}}), "> curl --user myusername --data-binary '{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"name\":[\"bar\",false,1]}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
+    BOOST_CHECK_EQUAL(HelpExampleRpcNamed("test", {{"name", arr_value}}), "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", \"method\": \"test\", \"params\": {\"name\":[\"bar\",false,1]}}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n");
 
     // test types don't matter for shell
     BOOST_CHECK_EQUAL(HelpExampleCliNamed("foo", {{"arg", true}}), HelpExampleCliNamed("foo", {{"arg", "true"}}));