json-rpc 2.0 followups: docs, tests, cli

pinheadmz commented at 3:52 pm on June 6, 2024: member

This is a follow-up to #27101.

Addresses post-merge comments
bitcoin-cli uses JSON-RPC 2.0
functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)

DrahtBot commented at 3:52 pm on June 6, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, cbergqvist

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

pinheadmz force-pushed on Jun 6, 2024

DrahtBot added the label CI failed on Jun 6, 2024

DrahtBot commented at 4:28 pm on June 6, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25901938167

in doc/release-notes-27101.md:6 in f9a62fc1df outdated

 6-
 7-- Returning HTTP "204 No Content" responses to JSON-RPC 2.0 notifications instead of full responses.
 8-- Returning HTTP "200 OK" responses in all other cases, rather than 404 responses for unknown methods, 500 responses for invalid parameters, etc.
 9-- Returning either "result" fields or "error" fields in JSON-RPC responses, rather than returning both fields with one field set to null.
10+strict adherence to the [specification](https://www.jsonrpc.org/specification).
11+See [JSON-RPC-interface.md](JSON-RPC-interface.md#json-rpc-11-vs-20) for details.

maflcko commented at 4:34 pm on June 6, 2024:

I’d say absolute links make sense in this case, because the file will be moved without modification later on into a different folder (./doc/release-notes/)

pinheadmz commented at 6:35 pm on June 6, 2024:

hmmm I’m struggling to get a link to survive the lint test

pinheadmz commented at 7:12 pm on June 6, 2024:

ok passed ci with /doc/...

pinheadmz force-pushed on Jun 6, 2024

DrahtBot removed the label CI failed on Jun 6, 2024

in doc/JSON-RPC-interface.md:36 in 19676e8feb outdated

32@@ -33,10 +33,10 @@ requests when multiple wallets are in use.
33 
34 ```sh
35 # Get block count from the / endpoint when rpcuser=alice and rpcport=38332
36-$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/
37+$ curl --user alice --data-binary '{"jsonrpc": "2.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/

tdb3 commented at 11:33 pm on June 6, 2024:

Since this line is being changed, maybe can also remove the extraneous semicolon after application/json.

See #30215#pullrequestreview-2092102442

pinheadmz commented at 1:29 pm on June 7, 2024:

hmmm I think I’m going to pass on this, there’s a few other files with the ; and if it doesn’t break anything I’ll just leave them all the same… but if @fanquake agrees I’ll add a commit to this PR.

fanquake commented at 2:34 pm on June 7, 2024:

No objections from me.

pinheadmz commented at 2:49 pm on June 7, 2024:

okie doke, added

in doc/JSON-RPC-interface.md:39 in 19676e8feb outdated

32@@ -33,10 +33,10 @@ requests when multiple wallets are in use.
33 
34 ```sh
35 # Get block count from the / endpoint when rpcuser=alice and rpcport=38332
36-$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/
37+$ curl --user alice --data-binary '{"jsonrpc": "2.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/
38 
39 # Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet
40-$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: application/json;' localhost:38332/wallet/desc-wallet
41+$ curl --user alice --data-binary '{"jsonrpc": "2.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: application/json;' localhost:38332/wallet/desc-wallet

tdb3 commented at 11:33 pm on June 6, 2024:

Same here

in doc/release-notes-27101.md:6 in 19676e8feb outdated

 6-
 7-- Returning HTTP "204 No Content" responses to JSON-RPC 2.0 notifications instead of full responses.
 8-- Returning HTTP "200 OK" responses in all other cases, rather than 404 responses for unknown methods, 500 responses for invalid parameters, etc.
 9-- Returning either "result" fields or "error" fields in JSON-RPC responses, rather than returning both fields with one field set to null.
10+strict adherence to the [specification](https://www.jsonrpc.org/specification).
11+See [JSON-RPC-interface.md](/doc/JSON-RPC-interface.md#json-rpc-11-vs-20) for details.

tdb3 commented at 0:55 am on June 7, 2024:

It’s great to keep the specifics in a unified location (i.e. doc/JSON-RPC-interface.md). Looks like the JSON-RPC 1.1 vs 2.0 table (https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#json-rpc-11-vs-20) covers the differences in behavior for 200/404/500, and error/result both vs one being present. Didn’t see 204 mentioned. How about something like:

 0diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md
 1index 2a97aa351d0..a6abb7e569b 100644
 2--- a/doc/JSON-RPC-interface.md
 3+++ b/doc/JSON-RPC-interface.md
 4@@ -88,7 +88,7 @@ protocol in previous releases.
 5 | Response marker | (none) | `"jsonrpc": "2.0"` |
 6 | `"error"` and `"result"` fields in response | both present | only one is present |
 7 | 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) |
 8-| Notifications: requests that get no reply | (not supported) | Supported for requests that exclude the "id" field |
 9+| Notifications: requests that get no reply | (not supported) | Supported for requests that exclude the "id" field.  Response of `204` (No Content) is issued  |
10 
11 ## Security

pinheadmz commented at 1:27 pm on June 7, 2024:

Yeah good idea here thanks, updating

tdb3 commented at 1:49 pm on June 7, 2024:

As a follow-up, checked that bitcoind does actually send the exact reason phrase “No Content”, so the update made is correct (technically the status code is what matters to a client and reason phrase is flexible, but if we’re saying we send “No Content” we should (and do)).

in src/bitcoin-cli.cpp:301 in 19676e8feb outdated

297@@ -298,7 +298,7 @@ class AddrinfoRequestHandler : public BaseRequestHandler
298         }
299         addresses.pushKV("total", total);
300         result.pushKV("addresses_known", std::move(addresses));
301-        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
302+        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V2);

tdb3 commented at 1:31 am on June 7, 2024:

Verified that bitcoin-cli -addrinfo’s request to getnodeaddresses and the resultant response now specify jsonrpc 2.0 successfully.

pinheadmz commented at 1:31 pm on June 7, 2024:

Great work thanks! I monitored with Wireshark as well. The other local test I did is to build bitcoin with legacy support removed. Only one test should fail, the interface_rpc test from #27101 which explicitly makes a legacy request.

 0diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
 1index 87b9f18b33..e72e349bf6 100644
 2--- a/src/rpc/request.cpp
 3+++ b/src/rpc/request.cpp
 4@@ -200,9 +200,7 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
 5         // The "jsonrpc" key was added in the 2.0 spec, but some older documentation
 6         // incorrectly included {"jsonrpc":"1.0"} in a request object, so we
 7         // maintain that for backwards compatibility.
 8-        if (jsonrpc_version.get_str() == "1.0") {
 9-            m_json_version = JSONRPCVersion::V1_LEGACY;
10-        } else if (jsonrpc_version.get_str() == "2.0") {
11+        if (jsonrpc_version.get_str() == "2.0") {
12             m_json_version = JSONRPCVersion::V2;
13         } else {
14             throw JSONRPCError(RPC_INVALID_REQUEST, "JSON-RPC version not supported");

in src/bitcoin-cli.cpp:370 in 19676e8feb outdated

366@@ -367,7 +367,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
367         }
368         result.pushKV("relayfee", batch[ID_NETWORKINFO]["result"]["relayfee"]);
369         result.pushKV("warnings", batch[ID_NETWORKINFO]["result"]["warnings"]);
370-        return JSONRPCReplyObj(std::move(result), NullUniValue,  /*id=*/1, JSONRPCVersion::V1_LEGACY);
371+        return JSONRPCReplyObj(std::move(result), NullUniValue,  /*id=*/1, JSONRPCVersion::V2);

tdb3 commented at 1:43 am on June 7, 2024:

Verified that bitcoin-cli -getinfo’s requests to getnetworkinfo, getblockchaininfo, getwalletinfo, getbalances, and listwallets specify jsonrpc 2.0 successfully (as well as the resultant responses).

in src/bitcoin-cli.cpp:625 in 19676e8feb outdated

621@@ -622,7 +622,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
622             }
623         }
624 
625-        return JSONRPCReplyObj(UniValue{result}, NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
626+        return JSONRPCReplyObj(UniValue{result}, NullUniValue, /*id=*/1, JSONRPCVersion::V2);

tdb3 commented at 1:45 am on June 7, 2024:

Verified that bitcoin-cli -netinfo’s requests to getpeerinfo, and getnetworkinfo specify jsonrpc 2.0 successfully (as well as the resultant responses).

in src/bitcoin-cli.cpp:712 in 19676e8feb outdated

708@@ -709,7 +709,7 @@ class GenerateToAddressRequestHandler : public BaseRequestHandler
709         UniValue result(UniValue::VOBJ);
710         result.pushKV("address", address_str);
711         result.pushKV("blocks", reply.get_obj()["result"]);
712-        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
713+        return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V2);

tdb3 commented at 1:49 am on June 7, 2024:

Verified that generatetoaddress (and response) specify jsonrpc 2.0 successfully. Also verified the same for -generate’s requests to getnewaddress and generatetoaddress (successful).

in test/functional/test_framework/authproxy.py:142 in 19676e8feb outdated

146+                    'code': -342, 'message': 'non-200 HTTP status code but no JSON-RPC error'}, status)
147+            else:
148+                return response['result']
149         else:
150+            assert response['jsonrpc'] == '2.0'
151+            if status != HTTPStatus.OK:

tdb3 commented at 2:10 am on June 7, 2024:

This would also throw if 204 was received. This seems ok because none of our tests should send a “notification” (i.e. without id param), so 204 should not be received.

nit: If you end up touching this file again, might want include a comment like # Throws for non-200 2xx status (such as 204) as well as non-2xx such as 3xx/4xx/5xx. This could help increase clarity that throwing on 204 is the desired behavior (and maybe prevent a PR from being opened mistakenly).

pinheadmz commented at 1:35 pm on June 7, 2024:

There also would be no error or result fields in the 204 case! I think it’s self explanatory that the authproxy call function expects a very specific type of response.

tdb3 commented at 2:11 am on June 7, 2024: contributor

Approach ACK. Solid follow up from the JSON RPC 2.0 work. Left a few comments and notes documenting what was checked.

Also ran all unit/functional tests (all passed). During functional test execution, did a spot check in Wireshark (e.g. for a getblockchaininfo call by a test) to ensure "jsonrpc": "2.0" was included (it was).

doc: update and link for JSON-RPC 2.0 0ead71df8c

test: remove unused variable in interface_rpc.py d39bdf3397

bitcoin-cli: use json-rpc 2.0 391843b029

test: use json-rpc 2.0 in all functional tests by default b225295298

pinheadmz force-pushed on Jun 7, 2024

minor: remove unnecessary semicolons from RPC content type examples 1f6ab1215b

tdb3 approved

tdb3 commented at 5:07 pm on June 7, 2024: contributor

ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df Great work and thanks for following up with the comments.

cbergqvist approved

cbergqvist commented at 9:33 pm on June 7, 2024: contributor

ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df

Tests

1.a

PR version bitcoin-cli (19676e8feb6cbef4b2ebb284a85667124f038b20) <-> v27.1rc1 bitcoind (fccd32efe6e2950b2c74fdec2ade54040ca32a2c)

Inspected getblock traffic using Wireshark and observed how a jsonrpc="2.0" request was responded to with an object containing version=1 but hash and confirmations were at the same level as it’s referring to the block version, not the JSONRPC version.

New bitcoin-cli was able to parse & output the result object to stdout without any errors.

1.b

Requesting a non-existent block hash and got HTTP status code 500 with expected error message specifying “Block not found”.

2.a

Tested the reverse of the above (PR version bitcoind vs v27.1rc1 bitcoin-cli).

Client was observed to not send any version field of any kind, and the new server responded without jsonrpc/version. The response had both result and error fields at the same time, which is not allowed by 2.0, but matches the old behavior.

Old bitcoin-cli parsed and printed expected result to stdout.

2.b

Tried requesting a non-existent block hash, and got the old behavior of HTTP status code 500 + response object with expected error.

Conclusion

v27.1 and prior bitcoinds simply don’t respect clients asking for jsonrpc="2.0", they don’t even error out, but simply give you the 1.1 version. This means that JSONRPC 2.0 clients, unless they are very tolerant like bitcoin-cli, should require newer bitcoind versions, and probably do a version check as a first call.

The bitcoind behavior of not supporting 2.0 already changed in #27101, this PR is simply changing bitcoin-cli to always request 2.0, making the above tests easier. In hindsight my tests maybe weren’t surgically appropriate to this PR, but further validates #27101 and at least didn’t uncover anything surprising.

fanquake merged this on Jun 8, 2024

fanquake closed this on Jun 8, 2024

json-rpc 2.0 followups: docs, tests, cli #30238

Code Coverage

Reviews

Tests

1.a

1.b

2.a

2.b

Conclusion