json-rpc 2.0 followups: docs, tests, cli #30238
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
89a13a8 to
19676e8
Compare
There was a problem hiding this comment.
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/JSON-RPC-interface.md
Outdated
| @@ -33,10 +33,10 @@ requests when multiple wallets are in use. | |||
|
|
|||
| ```sh | |||
| # Get block count from the / endpoint when rpcuser=alice and rpcport=38332 | |||
| $ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/ | |||
| $ curl --user alice --data-binary '{"jsonrpc": "2.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: application/json;' localhost:38332/ | |||
There was a problem hiding this comment.
Since this line is being changed, maybe can also remove the extraneous semicolon after application/json.
See #30215 (review)
There was a problem hiding this comment.
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.
doc/JSON-RPC-interface.md
Outdated
|
|
||
| # Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet | ||
| $ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: application/json;' localhost:38332/wallet/desc-wallet | ||
| $ curl --user alice --data-binary '{"jsonrpc": "2.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: application/json;' localhost:38332/wallet/desc-wallet |
| strict adherence to the [specification](https://www.jsonrpc.org/specification). | ||
| See [JSON-RPC-interface.md](/doc/JSON-RPC-interface.md#json-rpc-11-vs-20) for details. |
There was a problem hiding this comment.
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:
diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md
index 2a97aa351d0..a6abb7e569b 100644
--- a/doc/JSON-RPC-interface.md
+++ b/doc/JSON-RPC-interface.md
@@ -88,7 +88,7 @@ protocol in previous releases.
| 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 |
+| Notifications: requests that get no reply | (not supported) | Supported for requests that exclude the "id" field. Response of `204` (No Content) is issued |
## SecurityThere was a problem hiding this comment.
Yeah good idea here thanks, updating
There was a problem hiding this comment.
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)).
| @@ -298,7 +298,7 @@ class AddrinfoRequestHandler : public BaseRequestHandler | |||
| } | |||
| addresses.pushKV("total", total); | |||
| result.pushKV("addresses_known", std::move(addresses)); | |||
| return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY); | |||
| return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V2); | |||
There was a problem hiding this comment.
Verified that bitcoin-cli -addrinfo's request to getnodeaddresses and the resultant response now specify jsonrpc 2.0 successfully.
There was a problem hiding this comment.
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.
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index 87b9f18b33..e72e349bf6 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -200,9 +200,7 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
// 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") {
+ if (jsonrpc_version.get_str() == "2.0") {
m_json_version = JSONRPCVersion::V2;
} else {
throw JSONRPCError(RPC_INVALID_REQUEST, "JSON-RPC version not supported");
| @@ -367,7 +367,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, /*id=*/1, JSONRPCVersion::V1_LEGACY); | |||
| return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V2); | |||
There was a problem hiding this comment.
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).
| @@ -622,7 +622,7 @@ class NetinfoRequestHandler : public BaseRequestHandler | |||
| } | |||
| } | |||
|
|
|||
| return JSONRPCReplyObj(UniValue{result}, NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY); | |||
| return JSONRPCReplyObj(UniValue{result}, NullUniValue, /*id=*/1, JSONRPCVersion::V2); | |||
There was a problem hiding this comment.
Verified that bitcoin-cli -netinfo's requests to getpeerinfo, and getnetworkinfo specify jsonrpc 2.0 successfully (as well as the resultant responses).
| @@ -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, /*id=*/1, JSONRPCVersion::V1_LEGACY); | |||
| return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V2); | |||
There was a problem hiding this comment.
Verified that generatetoaddress (and response) specify jsonrpc 2.0 successfully. Also verified the same for -generate's requests to getnewaddress and generatetoaddress (successful).
| else: | ||
| assert response['jsonrpc'] == '2.0' | ||
| if status != HTTPStatus.OK: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ACK 1f6ab12
Tests
1.a
PR version bitcoin-cli (19676e8) <-> v27.1rc1 bitcoind (fccd32e)
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.
1f6ab12 minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin) b225295 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin) 391843b bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin) d39bdf3 test: remove unused variable in interface_rpc.py (Matthew Zipkin) 0ead71d doc: update and link for JSON-RPC 2.0 (Matthew Zipkin) Pull request description: This is a follow-up to bitcoin#27101. - Addresses [post-merge comments ](bitcoin#27101 (comment)) - bitcoin-cli uses JSON-RPC 2.0 - functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by bitcoin#27101) ACKs for top commit: tdb3: ACK 1f6ab12 cbergqvist: ACK 1f6ab12 Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
cbc6c44 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin) e7ee80d rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin) bf1a1f1 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin) 466b905 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin) 2ca1460 rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin) a64a2b7 rpc: refactor single/batch requests (Matthew Zipkin) df6e375 rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin) 09416f9 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin) 4202c17 test: refactor interface_rpc.py (Matthew Zipkin) Pull request description: Closes bitcoin#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) bitcoin#15495 added regression tests after a discussion in bitcoin#15381 to kinda lock in our RPC behavior to preserve backwards compatibility. bitcoin#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 cbc6c44 ryanofsky: Code review ACK cbc6c44. 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 cbc6c44 Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36 Merge bitcoin#30238: json-rpc 2.0 followups: docs, tests, cli 1f6ab12 minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin) b225295 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin) 391843b bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin) d39bdf3 test: remove unused variable in interface_rpc.py (Matthew Zipkin) 0ead71d doc: update and link for JSON-RPC 2.0 (Matthew Zipkin) Pull request description: This is a follow-up to bitcoin#27101. - Addresses [post-merge comments ](bitcoin#27101 (comment)) - bitcoin-cli uses JSON-RPC 2.0 - functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by bitcoin#27101) ACKs for top commit: tdb3: ACK 1f6ab12 cbergqvist: ACK 1f6ab12 Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c Add missing include and enum
This is a follow-up to #27101.