Merge bitcoin/bitcoin#27101: Support JSON-RPC 2.0 when requested by client

cbc6c440e3811d342fa570713702900b3e3e75b9 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2b68684eae96070875ea13a60e3e7b0 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662427fbf1a43bb951364eface469bdb7 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f4785de74b548f7c4a256069e2aaf43 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3a7217eaa8c5972515bf622bedadfce rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6feaf1856e7886820b70874209fd90b rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec445e4d6bb277400758083b0b4e8b174 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da37a3203e05a9f39f303d7df19b6d81 test: refactor interface_rpc.py (Matthew Zipkin)

Pull request description:

  Closes https://github.com/bitcoin/bitcoin/issues/2960

  Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found:
  - returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error)
  - returning both `"error"` and `"result"` fields together in a response object.
  - different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors)

  https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility.

  https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned.

  The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially:

  - always return HTTP 200 "OK" unless there really is a server error or malformed request
  - either return `"error"` OR `"result"` but never both
  - same behavior for single and batch requests

  If this is merged next steps can be:

  - Refactor bitcoin-cli to always use strict 2.0
  - Refactor the python test framework to always use strict 2.0 for everything
  - Begin deprecation process for 1.0/1.1 behavior (?)

  If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.

ACKs for top commit:
  cbergqvist:
    re ACK cbc6c440e3811d342fa570713702900b3e3e75b9
  ryanofsky:
    Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.
  tdb3:
    re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9

Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36

2 files changed, 191 insertions, 35 deletions

diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
index e873e2da0b..b08ca42796 100755
--- a/test/functional/interface_rpc.py
+++ b/test/functional/interface_rpc.py
@@ -4,22 +4,80 @@
 # 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_ADDRESS_OR_KEY = -5
+RPC_INVALID_PARAMETER      = -8
+RPC_METHOD_NOT_FOUND       = -32601
+RPC_INVALID_REQUEST        = -32600
+RPC_PARSE_ERROR            = -32700
+
+
+@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):
+    if options.version == 2 and options.notification:
+        return None
+    response = {}
+    if not options.notification:
+        response.update(id=idx)
+    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)
+    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):
@@ -48,37 +106,126 @@ class RPCInterfaceTest(BitcoinTestFramework):
         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...")
-
-        results = self.nodes[0].batch([
+    def test_batch_request(self, call_options):
+        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]},
+            # Invalid request format
+            {"pizza": "sausage"}
+        ]
+        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 = call_options(idx)
+            if options is None:
+                continue
+            request.append(format_request(options, idx, call))
+            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)
+        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...")
+        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 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 requests...")
-
-        expect_http_status(404, -32601, self.nodes[0].invalidmethod)
-        expect_http_status(500, -8, self.nodes[0].getblockhash, 42)
+        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 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, {"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, {"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
+        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()
+        # 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())
+        # 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())
 
     def test_work_queue_exceeded(self):
         self.log.info("Testing work queue exceeded...")
@@ -94,7 +241,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
 
     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()
 
diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
index 03042877b2..7edf9f3679 100644
--- a/test/functional/test_framework/authproxy.py
+++ b/test/functional/test_framework/authproxy.py
@@ -160,6 +160,15 @@ class AuthServiceProxy():
             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(