Support JSON-RPC 2.0 when requested by client #27101

Were we not using the out-of-spec "jsonrpc": "1.0" here previously?

edit: It seems it doesn’t make any difference as was not being validated? 🤷🏼‍♂️

Note that we do actually test json-rpc error code -32602 in a different test, which might be worth documenting here?

https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/p2p_disconnect_ban.py#L111

Similarly -32603 is tested here:

https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/feature_fee_estimation.py#L335

This error is caused by a failure to parse JSON in ReadBody() so theoretically we also can’t read the "jsonrpc": "2.0" pair either. I agree with you though, the HTTP transport is fine and should be 200, because the error is coming from the json parser. I guess the options are:

1. always send 200 here and kinda break backwards compat
2. attempt to parse the "jsonrpc" string and only send 200 if we can detect the “2.0” value
3. leave as-is, meaning invalid requests will get 500 even if they otherwise intended to be “2.0”

Just thinking out loud here; we use a code comment to indicate what the error code represents but in many of the other tests it appears we don’t (admittedly here it is the point of the test, and in the other locations it’s not). I wonder whether it might be clearer to have these enumerated in a single location, e.g. util.py, and then they could be used by name here and elsewhere? (this might be a change someone else will just make in the future otherwise, too).

I think it’s probably better left for a followup though, if at all.

In d11fb70b3fea7c1be5f48e32837d2c81dc7cbd10 “test: cover more HTTP error codes in interface_rpc.py”

This seems unnecessary?

Reverting this change fails the test because we are divorcing HTTP status from RPC error, so you can have an HTTP OK with RPC error and some test cases now also ensure both are OK.

 02023-07-07T15:44:27.147000Z TestFramework (ERROR): Assertion failed
 1Traceback (most recent call last):
 2  File "/Users/matthewzipkin/Desktop/work/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
 3    self.run_test()
 4  File "/Users/matthewzipkin/Desktop/work/bitcoin/test/functional/interface_rpc.py", line 157, in run_test
 5    self.test_http_status_codes()
 6  File "/Users/matthewzipkin/Desktop/work/bitcoin/test/functional/interface_rpc.py", line 125, in test_http_status_codes
 7    expect_http_status(200, None,   self.nodes[0].getblockhash, 0)
 8  File "/Users/matthewzipkin/Desktop/work/bitcoin/test/functional/interface_rpc.py", line 19, in expect_http_status
 9    raise AssertionError(f"Expected RPC error {expected_rpc_code}, got none")
10AssertionError: Expected RPC error None, got none

In commit “test: cover more HTTP error codes in interface_rpc.py” (b41f09cbc09a6f5d75636eb2f7ba8bc6d8802942)

I also don’t see why this should be necessary, and the change doesn’t make sense logically because now an http status code integer is passed to the expect_http_status function but the value is ignored in this case. Would suggest reverting this change and, if necessary, just writing a new expect_ function instead of trying to extend expect_http_status in a way where it appears to be checking things it’s not.

In 94d02376c853aa4835de413d3f75b8b06bcb5193 “rpc: JSON-RPC 2.0 should not respond to “notifications””

This results in 500 and a response body of “Unhandled request”. I think it should actually be a 200 with no response body.

0            if (jreq.IsNotification()) {
1                req->WriteReply(HTTP_OK);
2                return true;
3            }

In commit “rpc: refactor single/batch requests” (125ef7a61885d2ffc0d32b66cdfb3dced2a4417b)

This change doesn’t seem right because jreq.parse was just called above on line 196, and now the request is going to be parsed for a second time inside JSONRPCExecOne for no reason. I think this could be fixed by just dropping the jreq.parse call inside JSONRPCExecOne. It looks like later commit 4f22286c4aa65a113c44153847c857075a4225d6 does this anyway, so would be better to drop it up front and not change semantics multiple times. It would also be good to make the ExecOne jreq parameter const at that point since it should no longer be modifying the request. I would also rename ExecOne and ExecBatch functions to avoid giving the misleading impression that they are alternatives to each other, when actually they are operating at different levels and do not do the same things. ExecBatch handles errors, ExecOne does not. ExecBatch parses and executes, ExecOne should only execute requests without parsing them. Actually, I would probably just drop the ExecBatch function entirely since it is short and only called in one place. It would be clearer to just inline it, IMO.

Great review, thank you! I cleaned up the commit history in a rebase. The diff is minimal but in the range diff you can see the rebasing removed a commit:

git range-diff fbcf1029a7ba47d07a4566cf1f9d2e7b4870304a 576ec3e2debdb63e142eee2c2fa00af995542f42 5d5e8cbca785db851747c5143dc1d651a1ea11e6

Regarding naming and in-lining, I could see the two functions being named JSONRPCParseBatch() and JSONRPCExec()? I still like both of those definitions in server.cpp but if you think the “parse batch” logic can live happily ever after in HTTPReq_JSONRPC() I won’t protest. Either way would these 1 or 2 changes make more sense as a nice easy commit on top? Or should be rebased into the branch.

In commit “rpc: refactor single/batch requests” (fcc8cd08dbed9a44bab277ad267306dd2a9f5199)

re: #27101 (review)

I’d specifically suggest deleting the JSONRPCExecBatch function in this commit and moving the code it contains to httprpc.cpp (the only place it’s called). Also would rename JSONRPCExecOne to JSONRPCExec. Reasons:

• It would make division of responsibilities of httprpc and rpc/server.cpp clear. Instead of both of these files iterating over the batch object (former for method whitelisting, latter for method execution), the batch iterating code would live in one place and the server.cpp would not need to be concerned with batching.
• It would avoid giving false impression in httprpc.cpp that JSONRPCExecBatch and JSONRPCExecOne functions are alternatives to each other, when they aren’t because they don’t handle errors the same way, and one modifies the request while the other doesn’t.
• It would make error handling more straightforward. The fact that ExecBatch function catches exceptions internally, while ExecOne throws them up to the caller is a footgun.
• It would reduce the size of the code and as well as the size of the commit diff.

In commit “rpc: refactor single/batch requests” (fcc8cd08dbed9a44bab277ad267306dd2a9f5199)

Would be clearer if jreq were a const reference, since it is not modified

In commit “MOVEONLY: define class before functions in request.h” (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)

It seems ok to move these functions, but for future reference, there should be no need to do this because you can forward declare the class with class JSONRPCRequest; at the top of the file. I do think it would be slightly preferable to do that instead of moving code, but it’s not important.

In commit “MOVEONLY: define class before functions in request.h” (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)

Not important, but I think the developer guide suggests braces around multiline if statements because it can prevent subtle bugs from sneaking in. Generally only older code in bitcoin core omits the braces on multiline if statements.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (55dae852d88cdc6a4ad7e4a5cd3ba1988af9872c)

This comment is confusing because result and error values are mutually exclusive in both cases.

You could clarify the comment saying “error and result keys are mutually exclusive, to clarify that the comment is referring to keys not values. But I think the way this is written with duplicated logic is unnecessarily confusing anyway. Would suggest just writing the logic once and explaining what it is doing:

 0// Add JSON-RPC version number field in v2 only.
 1if (json_version == JSONVersion::JSON_2_0) reply.pushKV("jsonrpc", "2.0");
 2
 3// Add both result and error fields in v1, even though one will be null.
 4// Omit the null field in v2.
 5if (error.isNull()) {
 6    reply.pushKV("result", result);
 7    if (json_version == JSONVersion::JSON_1_BTC) reply.pushKV("error", NullUniValue);
 8} else {
 9    if (json_version == JSONVersion::JSON_1_BTC) reply.pushKV("result", NullUniValue);
10    reply.pushKV("error", error);
11}

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (28e4d9e7dd1604f1504f191d258a32f495c2fb4f)

Would suggest reply = std::move(*maybe_result); to avoid copying the result object

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (28e4d9e7dd1604f1504f191d258a32f495c2fb4f)

Would suggest std::move(*maybe_result) here as well to avoid copying

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (28e4d9e7dd1604f1504f191d258a32f495c2fb4f)

The implementation of this notification feature seems unnecessarily complicated. If you take earlier suggestion to delete the JSONRPCExecBatch function, support for notifications could be implemented entirely in the HTTPReq_JSONRPC function without sprawl across multiple files and extra complexity in rpc/server.cpp. rpc/server.cpp could just generate replies, not worry about what happens to replies after they are returned, and not have to deal with notifications or batching.

In commit “test: cover more HTTP error codes in interface_rpc.py” (b41f09cbc09a6f5d75636eb2f7ba8bc6d8802942)

Is this new line just expanding test coverage and not testing new behavior? Not critical, but in general it helps reviewing if test coverage is expanded first, before commits making code changes. Then it is a lot easier to review the code changes, because you can see how they affect tests. And it is easier to identify gaps in test coverage. An initial commit expanding test coverage before making any code changes is an ideal way to clarify current behavior before changing it.

In commit “test: cover more HTTP error codes in interface_rpc.py” (b41f09cbc09a6f5d75636eb2f7ba8bc6d8802942)

As noted above, 200 status code appears to be ignored here and not checked. Would suggest just dropping this line or finding a different way to test this code path, if it’s important.

In commit “test: handle 204 NO_CONTENT correctly in authproxy” (c85c9998dc98d15a7e42a0d2eb142cb516126243)

The logic in this function seems too permissive:

• If an empty “application/json” response is returned, now it returns None instead of treating the missing JSON as an error
• It does not treat responses that have NO_CONTENT status code but do have content as errors. It will even parse json and return from NO_CONTENT responses and ignore the content_type value.

The code is also more confusing now that these cases are mixed together instead of being handled separately. Would suggest just checking for the NO_CONTENT status up front, actually verifying that no content is present, and not mixing up the no content code with the json parsing code.

In commit “test: enable JSON-RPC 2.0 calls in tests and verify expected behavior” (65c2f4448dc556e7a922ebac4abed7ddfc8d7745)

This test code is broken because the requests do not have unique ids. So when the responses come back, the later responses override earlier ones in the result_by_id array, the existing test cases without a version fields will not be checked, and any result they return will be treated as valid.

I also think copying and pasting existing test cases for version 1.1 and version 2.0 makes this code harder to understand and less maintainable. The differences between versions should be small so IMO it would be better to just add a version argument (or version options like #27101 (review)) to the test_batch_request method and call it 3 times. The other advantage of making the version variable is that it will make it more obvious what the differences between versions actually are. For example it is easy to miss the fact that 1.1 and 2.0 test cases are actually passing the version number in two different fields.

In commit “test: enable JSON-RPC 2.0 calls in tests and verify expected behavior” (65c2f4448dc556e7a922ebac4abed7ddfc8d7745)

I think instead of hardcoding this logic in this AuthServiceProxy method and not allowing it to be reused for batch requests, it would be better to define a simple utility function that could be used both here and in the test_batch_request test case, and by any other code that needs to send batch requests. Something like:

0def format_rpc_request(req, v1=False, v2=False):
1    if v1: req["version"] = "1.1"
2    if v2: req["jsonrpc"] = "2.0"
3    return req

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (5d5e8cbca785db851747c5143dc1d651a1ea11e6)

This is vague and doesn’t say what’s changing. Would suggest something more like “The server now recognizes JSON-RPC 2.0 requests and treats them differently. Previously the JSON-RPC version was ignored, now JSON-RPC 2.0 requests follow the specification by:

• 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.”

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (5d5e8cbca785db851747c5143dc1d651a1ea11e6)

Review note: The only thing actually changing here is the number 1 to the number 2. The new quotes and line breaks have no affect on the program.

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (5d5e8cbca785db851747c5143dc1d651a1ea11e6)

Maybe say “invalidly formatted” rather than just “invalid” because otherwise it’s not obvious what types of requests would or wouldn’t cause http errors.

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (5d5e8cbca785db851747c5143dc1d651a1ea11e6)

Instead of including these examples here where it’s less they will be seen after next release, would suggest adding a JSON-RPC 2.0 section to the https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md doc and incorporating these examples in that documentation

In commit “rpc: identify JSON-RPC 2.0 requests” (8c619f70ba0021265041493b25a98f77afa96c55)

I might have missed it, but I don’t think I noticed a test case for this condition. Could consider adding one

In commit “rpc: refactor single/batch requests” (27afe01560b1d930c3b072a180cab95c857eff24)

This ret variable seems unnecessary. Can this just say reply = UniValue{UniValue::VARR} and use the reply variable instead of ret below?

The name is also confusing because ret normally refers to a return value, which this is not what this is.

In commit “rpc: refactor single/batch requests” (27afe01560b1d930c3b072a180cab95c857eff24)

Not new behavior but the result object, which is potentially large, is copied for no reason here. It would be nice in a separate commit to change JSONRPCReplyObj to accept a plain UniValue parameter instead of const UniValue& so std::move(result) could be passed here, and the result wouldn’t be copied unnecessarily.

In commit “rpc: identify JSON-RPC 2.0 requests” (9e1c6fa84eef606d5504ebcbbd2a2ca7a4f748dc)

The spec says this field ‘MUST be exactly “2.0”’ (https://www.jsonrpc.org/specification#request_object), so maybe it should be an error if the field is set and contains any other value. This way if there is ever a JSON-RPC 2.1 or 3.0 protocol, clients can easily check for support without getting back a strange JSON 1.1 response. FWIW, chatgpt suggestd the following response:

0{
1    "jsonrpc": "2.0",
2    "error": {
3        "code": -32600,
4        "message": "Invalid Request",
5        "data": "JSON-RPC version not supported."
6    },
7    "id": null
8}

And setting the id field to the actual request ID if possible to determine

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (c9196a2e5b747753e0d5ee59e1bcb987943eedbf)

I think these two lines should be switched to always return the result field before the error field. This would make responses look more consistent and avoid changing previous behavior.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (c9196a2e5b747753e0d5ee59e1bcb987943eedbf)

Again suggestion for an separate earlier commit, but it would be good if JSONRPCReplyObj took a UniValue error parameter instead of const UniValue& error so the error object could be moved instead of copied.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (9cae84c18706ce3b23b6f31466553161112deb70)

re: #27101 (review)

my understanding is that the error message will continue to be passed by reference through the end of JSONRPCReplyObj

It will be passed by value, but it will be copied because std::move is not specified, so this is less than ideal. The fix here is just to drop const from the catch declaration and then add std::move to move the value from the exception to the reply object.

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (bc2d55898b315d1ee80a70377d65f79c829246ed)

Would be good to std::move(response) here to avoid a copy

In commit “test: cover JSONRPC 2.0 requests, batches, and notifications” (d61e469179dae4e94fd00f47c2eeb8533d19e13c)

It’s not great to make this a static method and use a global variable, because now tests are no longer independent of each other and can potentially interfere with each other. Also using deep copy is unnecessary here because only the top level dictionary is modified. Also the name of the class “Format” is not very descriptive. Would suggest cleaning all this up:

 0--- a/test/functional/interface_rpc.py
 1+++ b/test/functional/interface_rpc.py
 2@@ -6,7 +6,6 @@
 3 
 4 import json
 5 import os
 6-from copy import deepcopy
 7 from test_framework.test_framework import BitcoinTestFramework
 8 from test_framework.util import assert_equal, assert_greater_than_or_equal
 9 from threading import Thread
10@@ -19,19 +18,19 @@ RPC_INVALID_REQUEST        = -32600
11 RPC_PARSE_ERROR            = -32700
12 
13 
14-class Format:
15-    ID_COUNT = 0
16+class RequestBuilder:
17+    def __init__(self):
18+        self.id_count = 0
19 
20- [@staticmethod](/bitcoin-bitcoin/contributor/staticmethod/)
21-    def rpc_request(body, version=1, notification=False):
22-        req = deepcopy(body)
23+    def rpc_request(self, fields, version=1, notification=False):
24+        req = dict(**fields)
25         if version == 1:
26             req["version"] = "1.1"
27         if version == 2:
28             req["jsonrpc"] = "2.0"
29         if not notification:
30-            req["id"] = Format.ID_COUNT
31-            Format.ID_COUNT += 1
32+            req["id"] = self.id_count
33+            self.id_count += 1
34         return req
35 
36 
37@@ -45,7 +44,7 @@ def send_json_rpc(node, body: object) -> tuple[object, int]:
38 
39 
40 def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params=None, version=1, notification=False):
41-    req = Format.rpc_request({"method": method, "params": params}, version, notification)
42+    req = RequestBuilder().rpc_request({"method": method, "params": params}, version, notification)
43     response, status = send_json_rpc(node, req)
44 
45     if expected_rpc_error_code is not None:
46@@ -91,11 +90,12 @@ class RPCInterfaceTest(BitcoinTestFramework):
47             # Invalid request format
48             {"pizza": "sausage"}
49         ]
50+        builder = RequestBuilder()
51 
52         self.log.info("Testing basic JSON-RPC 1.1 batch request...")
53         body = []
54         for cmd in commands:
55-            body.append(Format.rpc_request(cmd))
56+            body.append(builder.rpc_request(cmd))
57         response, status = send_json_rpc(self.nodes[0], body)
58         assert_equal(status, 200)
59         # JSON 1.1: Every response has a "result" and an "error"
60@@ -112,7 +112,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
61         self.log.info("Testing basic JSON-RPC 2.0 batch request...")
62         body = []
63         for cmd in commands:
64-            body.append(Format.rpc_request(cmd, version=2))
65+            body.append(builder.rpc_request(cmd, version=2))
66         response, status = send_json_rpc(self.nodes[0], body)
67         assert_equal(status, 200)
68         # JSON 2.0: Each response has either a "result" or an "error"
69@@ -130,7 +130,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
70         body = []
71         version = 1
72         for cmd in commands:
73-            body.append(Format.rpc_request(cmd, version=version))
74+            body.append(builder.rpc_request(cmd, version=version))
75             version = 2 if version == 1 else 1
76         response, status = send_json_rpc(self.nodes[0], body)
77         assert_equal(status, 200)
78@@ -149,7 +149,7 @@ class RPCInterfaceTest(BitcoinTestFramework):
79         body = []
80         notification = False
81         for cmd in commands:
82-            body.append(Format.rpc_request(cmd, version=2, notification=notification))
83+            body.append(builder.rpc_request(cmd, version=2, notification=notification))
84             notification = True
85         response, status = send_json_rpc(self.nodes[0], body)
86         assert_equal(status, 200)

In commit “test: cover more HTTP error codes in interface_rpc.py” (29ebb0d7774ee2d10aa274086e8ff4b2395f7047)

Setting params to None seems like a bug because it means a "params": null JSON field will be sent, when previously authproxy would send an empty params object. Would suggest changing this to params={} to avoid a change in behavior.

Also, I’m not sure if it is a bug for our server to accept null params. The spec says “If present, parameters for the rpc call MUST be provided as a Structured value” and “params MUST be an Array” or “params MUST be an Object”, so it seems like null is not supposed to be allowed, and probably should trigger a server error.

I actually had this as params=[] a few updates ago but that fails lint (see below) but I’ll clean it up with a different work-around:

 0A mutable list or dict seems to be used as default parameter value:
 1
 2test/functional/interface_rpc.py:41:def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params={}):
 3
 4This is how mutable list and dict default parameter values behave:
 5
 6>>> def f(i, j=[], k={}):
 7...     j.append(i)
 8...     k[i] = True
 9...     return j, k
10...
11>>> f(1)
12([1], {1: True})
13>>> f(1)
14([1, 1], {1: True})
15>>> f(2)
16([1, 1, 2], {1: True, 2: True})
17
18The intended behaviour was likely:
19
20>>> def f(i, j=None, k=None):
21...     if j is None:
22...         j = []
23...     if k is None:
24...         k = {}
25...     j.append(i)
26...     k[i] = True
27...     return j, k
28...
29>>> f(1)
30([1], {1: True})
31>>> f(1)
32([1], {1: True})
33>>> f(2)
34([2], {2: True})
35^---- failure generated from lint-python-mutable-default-parameters.py

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)

Would drop “Protocols can be mixed in a batch request.” It seems ok, but not a good thing to encourage, and unless it’s required by the specs, I don’t think we should commit to this behavior by adding it to our documentation.

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)

Think this should also mention that responses contain “jsonrpc”: “2.0” field. Or this could be an additional row in the table.

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (bc2d55898b315d1ee80a70377d65f79c829246ed)

This does not seem correct. According to https://www.jsonrpc.org/specification#notification a notification is a ‘Request object without an “id” member’, not a request object with a null id member. It also explicitly says requests that have “Null as a value for the id member” are treated as “Responses with an unknown id” not notifications.

Probably a reasonable way to fix this would be to change the JSONRPCRequest::id type from UniValue to std::optional<UniValue> to distinction between a null id and a missing id.

In commit “doc: add comments and release-notes for JSON-RPC 2.0” (756da6d1e67fba65a992dc0090ee8c0cfa26abe3)

Maybe replace “errors like if…” with “errors in cases like the endpoint not being found (404) or the RPC request not being formatted correctly (500).” Grammar sounds a little off the way it is written now.

In commit “test: handle 204 NO_CONTENT correctly in authproxy” (174a995edf594dccf745d2593c281d3e4a1ff7f7)

It would be good to have a comment that explains why this block of code is needed, because it seems like an impossible case to hit through normal usage of authproxy. Maybe: “# 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.”

nit: Author’s choice on whether this is addressed or not.

Minor non-compliance to the JSON RPC 2.0 spec (rpc call with an empty Array in https://www.jsonrpc.org/specification#examples). Providing an empty array results in an empty array provided in the response, but should result in a single error response (e.g. "jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": null}).

A check for valRequest.size() == 0 before entering the for loop in 236 could enable error response (rather than empty response array). Perhaps this is best handled when future updates attempt more strict compliance to 2.0?

This is a good catch but unfortunately I think we need to stick with this behavior for backwards compatibility. If we get a request that does not have the exact key-value pair "jsonrpc":"2.0" we have to resort to legacy behavior. In that case, the user has submitted an empty batch and what they get back is an empty vector.

The closest 2.0 example I can think of is this:

0--> [{"jsonrpc":"2.0", "id":""}]
1<-- [{'jsonrpc': '2.0', 'error': {'code': -32600, 'message': 'Missing method'}, 'id': ''}]

if "jsonrpc" is removed, we revert to legacy behavior and if "id" is removed we get no response at all since that is a notification:

0--> [{"jsonrpc":"2.0"}]
1<-- []

This last example actually does look non-compliant according to the last example with an all-notification batch. So I think I will address that

Thanks!!!

Fixed in force push to 771d1e1d206efe687b8661ab966cc1a62cc7ba39

 0diff --git a/src/httprpc.cpp b/src/httprpc.cpp
 1index 54354a8625..f678668fd3 100644
 2--- a/src/httprpc.cpp
 3+++ b/src/httprpc.cpp
 4@@ -247,6 +247,11 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
 5                 }
 6                 if (!jreq.IsNotification()) reply.push_back(std::move(response));
 7             }
 8+            if (reply.empty()) {
 9+                // All-notification batch expects no response
10+                req->WriteReply(HTTP_NO_CONTENT);
11+                return true;
12+            }
13         }
14         else
15             throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
16diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
17index 309345731c..dcb0b66921 100755
18--- a/test/functional/interface_rpc.py
19+++ b/test/functional/interface_rpc.py
20@@ -161,6 +161,14 @@ class RPCInterfaceTest(BitcoinTestFramework):
21             ]
22         )
23 
24+        self.log.info("Testing JSON-RPC 2.0 batch of ALL notifications...")
25+        body = []
26+        for cmd in commands:
27+            body.append(builder.rpc_request(cmd, version=2, notification=True))
28+        response, status = send_json_rpc(self.nodes[0], body)
29+        assert_equal(status, 204) # HTTP_NO_CONTENT
30+        assert_equal(response, None) # not even an empty array
31+
32     def test_http_status_codes(self):
33         self.log.info("Testing HTTP status codes for JSON-RPC 1.1 requests...")
34         # OK

Pulled changes (771d1e1d206efe687b8661ab966cc1a62cc7ba39), built, ran all functional tests (all passed). Re-ran the following on 771d1e1d206efe687b8661ab966cc1a62cc7ba39 and on release v26.0.

Action (1): Unexpected difference

curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Unexpected difference

v26.0 behavior: []

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: (no content) (in alignment with 2.0 spec, but not matching legacy behavior) Applicable line(s): httprpc.cpp:250 if (reply.empty()) { seems to be treating 2.0 and 1.x requests identically. At first glance, seems like a check for 2.0 here might rememdy this (e.g., would need to be both 2.0 and reply.empty()).

Action (2): Minor Unexpected difference

curl --user test --data-binary '' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Unexpected difference

v26.0 behavior: {"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: {"result":null,"error":{"code":-32700,"message":"Parse error"}} Diff: legacy includes "id":null while new code does not.

Action (3): Expected difference

curl --user test --data-binary '[{"jsonrpc":"2.0", "id":""}]' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Expected difference

v26.0 behavior: [{"result":null,"error":{"code":-32600,"message":"Missing method"},"id":""}]

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: [{"jsonrpc":"2.0","error":{"code":-32600,"message":"Missing method"},"id":""}] Seems conformant to 2.0 spec.

Action (4): Expected difference

curl --user test --data-binary '[{"jsonrpc":"2.0"}]' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Expected difference

v26.0 behavior: [{"result":null,"error":{"code":-32600,"message":"Missing method"},"id":null}]

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: (no content) Seems compliant to 2.0 spec (batch with only a notification, albeit with a missing method, so no error response).

re: #27101 (review)

Action (1): Unexpected difference

curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Unexpected difference

v26.0 behavior: []

PR 27101 (771d1e1) behavior: (no content) (in alignment with 2.0 spec, but not matching legacy behavior) Applicable line(s): httprpc.cpp:250 if (reply.empty()) { seems to be treating 2.0 and 1.x requests identically. At first glance, seems like a check for 2.0 here might rememdy this (e.g., would need to be both 2.0 and reply.empty()).

I think this is unavoidable if we want to comply with the 2.0 spec which says “If there are no Response objects contained within the Response array as it is to be sent to the client, the server MUST NOT return an empty Array and should return nothing at all.”

Although from discussion thread above it sounds like maybe we are willing to not fully comply with 2.0 spec if we are prioritizing backwards compatibility over spec compatibility in cases where 2.0 spec is mandating a certain response even when the request does not contain a "jsonrpc":"2.0" field.

Either approach seems fine but we should probably note this corner case in the RPC doc if favoring compatibility over compliance with the spec, or note it in the release notes if we are going to favor compliance with the spec over compatibility in a case where jsonrpc 2.0 is not actually specified.

Action (2): Minor Unexpected difference

curl --user test --data-binary '' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Unexpected difference

v26.0 behavior: {"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}

PR 27101 (771d1e1) behavior: {"result":null,"error":{"code":-32700,"message":"Parse error"}} Diff: legacy includes "id":null while new code does not.

This is a little surprising. Seems better to keep previous behavior if this isn’t actually specifying jsonrpc 2.0?

Action (3): Expected difference

@tdb3, are actions 3-7 you listed bringing up potential bugs or they just recording what you tested? I didn’t look closely all of these but on the surface they seem like expected behavior.

Although from discussion thread above it sounds like maybe we are willing to not fully comply with 2.0 spec if we are prioritizing backwards compatibility over spec compatibility in cases where 2.0 spec is mandating a certain response even when the request does not contain a “jsonrpc”:“2.0” field.

Yeah, exactly this. If/when support for jsonrpc 1.0 and 1.1 is removed, then the implementation could be updated to achieve full/closer compliance to the 2.0 spec. If it’s decided that jsonrpc 1.0/1.1 support is never to be removed, then we’d have to accept imperfect conformance to 2.0.

we should probably note this corner case in the RPC doc if favoring compatibility over compliance with the spec, or note it in the release notes if we are going to favor compliance with the spec over compatibility in a case where jsonrpc 2.0 is not actually specified.

Agreed. This would inform users (of 2.0) and increase awareness for any 1.0/1.1 removal PR. @pinheadmz, how about something like this for doc/JSON-RPC-interface.md:

Existing text:

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.

Addition:

0Note: Since backwards compatibility is being prioritized at this time, 
1and legacy JSON-RPC protocol is followed for responses to requests 
2without presense of "jsonrpc": "2.0", some non-conformance to the 
3JSON-RPC 2.0 specification will occur.  For example, a request 
4containing an empty array ("[]", i.e. body containing an empty array) 
5would receive a response of "[]" (empty array, matching legacy response 
6behavior) while the JSON-RPC specification prohibits an empty array as 
7a response to an all-notification batch.

This is a little surprising. Seems better to keep previous behavior if this isn’t actually specifying jsonrpc 2.0?

Agreed. 58cb22c83c1b72af7f3c580dc6261cf50e7ed2cb was observed to fix this, so now the response maches the legacy response.

are actions 3-7 you listed bringing up potential bugs or they just recording what you tested? I didn’t look closely all of these but on the surface they seem like expected behavior.

Yeah, thank you. I should have been clearer. Actions 3 through 7 are just recording what was tested. All expected behavior (no bugs observed) for 3 through 7.

Issuing an example command from https://developer.bitcoin.org/reference/rpc/getblockchaininfo.html resulted in error response {"result":null,"error":{"code":-32600,"message":"JSON-RPC version not supported"}} due to the example’s use of "jsonrpc":"1.0". It looks like JSON RPC spec for 1.0 had no version field in the spec, 1.1 (WD/ALT) had the “version” field to specify version, and 2.0 spec has “jsonrpc” (must be set to “2.0”).

If a client uses a “jsonrpc” field other than “2.0” it will receive the error (from exception thrown in line 202), whereas in the previous implementation the request would be processed successfully. This does seem to affect backwards compatibility with 1.x implementations, although I’m unsure how widespread the usage of the jsonrpc field is in existing client implementations. The original issue #2960 claims 61/70 implementions work only with 2.0 (so likely to use 2.0 as the value in the jsonrpc field and not encounter the error response). At the very least, the developer.bitcoin.org examples seem to be non-comformant to the JSON-RPC specs (1.0 doesn’t define the “jsonrpc” field, 1.1 requires “version”, and 2.0 requires “jsonrpc”).

Increased backwards compatibility at the expense of strict compliance to 2.0 could be achieved by adding else if clauses to fallback to 1.x handling if the jsonrpc field is “1.0” (example below). Author’s choice. I’d support either way (not continuing to enable non-conformant requests, or enabling non-comformant requests with future notification of 1.x deprecation).

0} else if(valJsonRPC.get_str() == "1.0") {
1    /* m_json_version is JSONVersion::JSON_1_BTC */
2}

Hm this is a bit of a mess, you’re right. The behavior in this PR makes sense in response to @ryanofsky comment: #27101 (review)

The help text has also been modified in src/rpc/util.cpp (98031401c744ea3c2b62927fabf27c22b639c3cf) to always recommend "jsonrpc":"2.0"

I think you are illustrating an important issue though – anyone who has used bitcoin-cli help (for ANY RPC command!) would have seen instructions to include "jsonrpc":"1.0" in their raw curl request. That was not only unnecessary, but also wrong.

So even though I’m trying to maintain complete backwards compatibility, there is a likelihood that users have copied this old help text verbatim and will encounter errors when upgrading.

I’m not sure how to proceed on this, might need input from other reviewers. "jsonrpc":"1.0" is not a standard anywhere, not even in Bitcoin… but it has been in the RPC help text for a looooong time 😭

I’m not sure how to proceed on this, might need input from other reviewers. "jsonrpc":"1.0" is not a standard anywhere, not even in Bitcoin… but it has been in the RPC help text for a looooong time 😭

This is a good find, and I haven’t looked closely, but it seems like there is not any conflict here. We can continue to accept "jsonrpc":"1.0" for compatibility, continue to require `“jsonrpc”:“2.0” for JSON-RPC 2.0 behavior to comply with the 2.0 spec, and reject any other values so if there is a JSON-RPC 2.1 or 3.0 spec in the future, future versions of bitcoin can support it, and clients can determine that they are not supported right now.

Updated with some tests. ACK. Looks fixed!

Action (5): Successfully executed response, legacy behavior retained

curl --user test --data-binary '{"jsonrpc":"1.0","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Successfully executed response

v26.0 behavior: {"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":1706747596,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de2f42db","size_on_disk":44084938,"pruned":false,"warnings":""},"error":null,"id":"tester"}

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: {"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":1706747596,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de2f42db","size_on_disk":44084938,"pruned":false,"warnings":"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"},"error":null,"id":"tester"}

Action (6): Successfully executed response, legacy behavior retained

curl --user test --data-binary '{"version":"1.1","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Successfully executed response

v26.0 behavior: {"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":1706747596,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de2f42db","size_on_disk":44084938,"pruned":false,"warnings":""},"error":null,"id":"tester"}

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: {"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":1706747596,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de2f42db","size_on_disk":44084938,"pruned":false,"warnings":"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"},"error":null,"id":"tester"}

Action (7): Successfully executed response, legacy behavior retained

curl --user test --data-binary '{"jsonrpc":"2.0","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/

Result: Successfully executed response

v26.0 behavior: {"result":{"chain":"signet","blocks":4321,"headers":4321,"bestblockhash":"00000356f9dc5bec54dc61f44bd0302859342d2b881a6b5febe984616f465abc","difficulty":0.001127075738664131,"time":1706751196,"mediantime":1706748196,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de792056","size_on_disk":44085350,"pruned":false,"warnings":""},"error":null,"id":"tester"}

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: {"jsonrpc":"2.0","result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":1706747596,"verificationprogress":1,"initialblockdownload":false,"chainwork":"00000000000000000000000000000000000000000000000000000004de2f42db","size_on_disk":44084938,"pruned":false,"warnings":"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"},"id":"tester"}

In commit “test: cover JSONRPC 2.0 requests, batches, and notifications” (92c651394bcb1c476cc62989936edd621b5d4aa5)

This is confusing because there should be no such thing as a JSON-RPC 1.0 batch request, batches were introduced in 2.0. Also specifying req[“jsonrpc”] = “1.0” is nonstandard. Would be good to clarify what behavior is standard and what is just being done for backwards compatibility.

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (d62dcf81e152d03aa3cf02d0eb0e7e9c44807464)

This is just stated without a reason and doesn’t appear to be true.

Also, checking for the version field after the id field is bad because it means if a batch request includes an unrecognized version number, the id is lost and there is no indication which call in the batch is causing the problem.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (9cae84c18706ce3b23b6f31466553161112deb70)

This is not a great commit description because it is only describing one of the 3 changes made in this commit. In addition to making error and result multually exclusive it is also:

• Adding jsonrpc 2.0 field to responses
• Making non-batch jsonrpc 2.0 requests return 200 http status instead of error statuses when the RPC method fails

In the branch I posted #27101#pullrequestreview-1922496475 this commit is split into two parts and includes tests to clarify the changes.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (9cae84c18706ce3b23b6f31466553161112deb70)

Would be safest to drop default argument and make it required to specify the version. I think differences between versions are subtle and confusing enough that the version should always be explicit.

In commit “rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies” (9cae84c18706ce3b23b6f31466553161112deb70)

This declaration appears to be unused

Without it there is a compile error:

0In file included from ./rpc/server.h:9:
1./rpc/request.h:22:79: error: unknown type name 'JSONRPCRequest'
2   22 | std::string JSONRPCReply(const UniValue& result, const UniValue& error, const JSONRPCRequest& jreq);
3      |                                                                               ^
41 error generated.

In commit “rpc: use move semantics in JSONRPCReplyObj()” (51a071f24caa89b3dab2e9a7b46d5b3a75ef4913)

To make sense, this commit needs to use std::move below for result, error, and id. The point of changing these arguments from const to non-const is to allow them to be moved from, and avoid the copies that would otherwise happen below.

In commit “rpc: JSON-RPC 2.0 should not respond to “notifications”” (d62dcf81e152d03aa3cf02d0eb0e7e9c44807464)

I think this needs some explanation about how valRequest.size() check is here for backward compatibility since it is surprising and violates the 2.0 spec.

Good catch thanks. I’ll need @ryanofsky to weigh in here because I keep misunderstanding c++ move semantics. But here’s what I think is happening:

In JSONRPCReplyObj() in request.cpp the really import move-instead-of-copy is when we call reply.pushKV(...): https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64

That makes me think that the std::move() currently in JSONErrorReply() in httprpc.cpp is unnecessary: https://github.com/esotericnonsense/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/httprpc.cpp#L90

…and also unnecessary is the one you see on line 272.

So if I’m finally understanding, which is unlikely, then there are two places where std::move can be removed but actually this line 244 right here is OK, because the import move-instead-of-copy happens inside the function being called.

re: #27101 (review)

I think @cbergqvist is right here and e should be replaced by std::move(e) on line 244.

The JSONRPCReplyObj signature is:

0UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONVersion json_version);

So without std::move(e), the error parameter will be initialized by making a copy of e instead of moving from e. This is because e is an lvalue, while std::move(e) is an rvalue.

In JSONRPCReplyObj() in request.cpp the really import move-instead-of-copy is when we call reply.pushKV(...): https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64

That is true. The signature for pushKV is void pushKV(std::string key, UniValue val), so without std::move(error), on that line the function parameter val would be initialized by copying not moving.

That makes me think that the std::move() currently in JSONErrorReply() in httprpc.cpp is unnecessary: https://github.com/esotericnonsense/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/httprpc.cpp#L90

This is not true. std::move is needed to avoid copy-initializing the JSONRPCReplyObj error parameter there, just like on line 272 and on line

So if I’m finally understanding, which is unlikely, then there are two places where std::move can be removed but actually this line 244 right here is OK, because the import move-instead-of-copy happens inside the function being called.

Yeah this is a little off. What you are saying is true if the function signature accepts parameters by reference. For example if you have function that looks like:

0void MyFunction(SomeType& param);
1void MyFunction(const SomeType& param);
2void MyFunction(SomeType&& param);

In this case, no matter whether you call MyFunction with std::move or without it, the parameter param is going to be a reference not a value, so no copy will be created just by calling the function. There could copies of param made inside the function, depending on what it does internally, as you say. But merely calling the function will not copy the parameter.

However, if you have a function that takes a non-reference value parameter like:

0void MyFunction(SomeType param);

No matter what the function does internally, just calling the function is going to copy-construct param variable or move-construct it. And unless you use std::move or the argument is already an rvalue, the argument will be copied.

That’s a reasonable comment but I kept the single character which is how it worked on master (see below). I see a few examples of \r\n in the codebase but its rare and I’m not sure how it’s decided.

https://github.com/bitcoin/bitcoin/blob/63d0b930f821132badd804822a46232a5f98bbef/src/rpc/request.cpp#L52-L56

In commit “rpc: refactor single/batch requests” (a64a2b77e09bff784a2635ba19ff4aa6582bb5a5)

re: #27101 (review)

HTTP headers use \r\n, but this line is generating the JSON object in the body of the response, after the headers. And the JSON could be written with either \r\n or \n or no trailing line break at all. I think it would probably make sense not to include any trailing line break, but existing code uses \n, so in a refactoring commit it seems best not to change this.

In commit “rpc: identify JSON-RPC 2.0 requests” (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Might be good to add a comment noting that the “jsonrpc” version field was added in the JSON-RPC 2.0 spec, so value “1.0” is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.

In commit “rpc: identify JSON-RPC 2.0 requests” (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Variable name valJsonRPC doesn’t really follow the suggested coding style which calls for snake_case. Could rename it to “jsonrpc_version”

In commit “rpc: identify JSON-RPC 2.0 requests” (6d0be75f3040c3190759c3ba6b5740897f2d3d29)

Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum

re: #27101 (review)

Is JSON/JSONRPC really necessary for the values themselves if the enum is of the type enum class so that one always has to prepend the type? - JSONRPCVersion::JSONRPC_1_BTC vs JSONRPCVersion::1_BTC.

Good point. I’d probably call the constants V1_LEGACY and V2 if renaming.

In commit “rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests” (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

I think the commit message has a typo. It says “Non-batch requests already did not return HTTP errors previously.” but I think it is supposed to say “Batch requests already did not return HTTP errors previously.”

In commit “rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests” (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

This is probably supposed to say 1.1 not 1.2