-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Support JSON-RPC 2.0 when requested by client #27101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
d21b25b
to
440d6a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Concept ACK |
97af16b
to
b139110
Compare
Concept ACK. Migrating to strict JSON-RPC 2.0 for |
Concept ACK. Might be worth updating a few other things at the same time if you continue to move ahead:
|
@willcl-ark thanks, I added comments and release notes. I also wrote a tiny testing package using libjson-rpc-cpp to check against an "outside" JSON-RPC 2.0 implementation. The package is https://github.com/pinheadmz/jsonrpc-bitcoin and seems to like the 2.0 implementation so far. |
ec2b51b
to
d7a87a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
This PR is ready for code review if any of you fine handsome concept-ACKers have the time ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2da8231
Left a few comments which could be addressed here, in a followup or not at all. None of them materially affect the current implementation of this feature which works well.
Although, I would be curious to know why we are still responding with http 500
errors for invalid json after the move to json 2.0.
test/functional/interface_rpc.py
Outdated
AuthServiceProxy.jsonrpc20 = True | ||
# All 200! | ||
expect_http_status(200, -32601, self.nodes[0].invalidmethod) # RPC_METHOD_NOT_FOUND | ||
expect_http_status(200, -8, self.nodes[0].getblockhash, 42) # RPC_INVALID_PARAMETER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be a status: 200, error: -32602
under json 2.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah good point, a few error codes like this are specified. One problem with this is that our application error RPC_INVALID_PARAMETER
(-8
) is coded ~186 times in RPC functions everywhere. I suppose we could re-map those error codes back to the jsonrpc 2.0 spec in JSONRPCExecOne()
? Only when the request indicates jsonrpc 2.0
5388efb
to
c9a3862
Compare
Added a scripted-diff to completely replace all occurrences of the application-defined |
Only for JSON-RPC 2.0 requests.
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the RPC method failed but the HTTP request was otherwise valid. Batch requests already did not return HTTP errors previously.
For JSON-RPC 2.0 requests we need to distinguish between a missing "id" field and "id":null. This is accomplished by making the JSONRPCRequest id property a std::optional<UniValue> with a default value of UniValue::VNULL. A side-effect of this change for non-2.0 requests is that request which do not specify an "id" field will no longer return "id": null in the response.
force push to cbc6c44:
thanks again for the reviews @cbergqvist @ryanofsky |
@@ -73,8 +73,11 @@ static std::vector<std::vector<std::string>> g_rpcauth; | |||
static std::map<std::string, std::set<std::string>> g_rpc_whitelist; | |||
static bool g_rpc_whitelist_default = false; | |||
|
|||
static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id) | |||
static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCRequest& jreq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: JSONErrorReply
-> JSONRPCErrorReply
, although it could be argued that it actually does write a JSON object in the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re ACK cbc6c44
All functional tests passed (with a few automatic skips), except feature_dbcrash
- slow, unrelated => excluded, and feature_index_prune
=> timed out because rebase with bumped timeout has been held-off.
{ | ||
rpc_result = JSONRPCReplyObj(NullUniValue, | ||
JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id); | ||
UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would have gone the opposite way and called it throw_errors
since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #27101 (comment)
Would have gone the opposite way and called it
throw_errors
since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.
I think either way is fine, but catch_errors does seem more literally correct since the argument is just controlling whether the exceptions will be caught. Errors will be still be thrown regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "rpc: identify JSON-RPC 2.0 requests" (2ca1460)
I think it would be a little clearer to say "continue to accept that" instead of "maintain that." Otherwise it sounds like we are trying to maintain incorrectly including the field, not just allowing it if it is specified.
{ | ||
rpc_result = JSONRPCReplyObj(NullUniValue, | ||
JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id); | ||
UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #27101 (comment)
Would have gone the opposite way and called it
throw_errors
since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.
I think either way is fine, but catch_errors does seem more literally correct since the argument is just controlling whether the exceptions will be caught. Errors will be still be thrown regardless.
@willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack |
Definitely. I'll plan to take a look tonight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re ACK for cbc6c44
Great work. Performed brief code review. Re-ran the tests described in #27101 (comment) again (actions 1 through 7, with the caveat that v27.0 was used as the baseline for comparison rather v26.0, and regtest was used). Everything worked as expected. Also ran all functional tests (passed).
and responds accordingly. A 2.0 request is identified by the presence of | ||
`"jsonrpc": "2.0"` in the request body. If that key + value is not present in a request, | ||
the legacy JSON-RPC v1.1 protocol is followed instead, which was the only available | ||
protocol in previous releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Now that this is merged, it could say "in 27.0 and prior releases." Otherwise, on 29.x it will read as-if 28.0 had it missing.
|
||
- 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit instead of duplicating the section from doc/JSON-RPC-interface.md
here, it seems better to link/refer to it. Otherwise, if the section is updated, this may go stale or must be updated at the same time.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinheadmz did you end up following up to these comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there...
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 #27101. - Addresses [post-merge comments ](#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 #27101) ACKs for top commit: tdb3: ACK 1f6ab12 cbergqvist: ACK 1f6ab12 Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
try { | ||
jreq.parse(valRequest[i]); | ||
response = JSONRPCExec(jreq, /*catch_errors=*/true); | ||
} catch (UniValue& e) { | ||
response = JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version); | ||
} catch (const std::exception& e) { | ||
response = JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Came across this block again while working on something else. It struck me that the exception handling looked redundant as we catch the same exact exception types inside JSONRPCExec(jreq, /*catch_errors=*/true
). But the jreq.parse()
call will throw on missing methods etc.
One could move out response = JSONRPCExec(jreq, /*catch_errors=*/true);
to after the block, but then one would need to guard against using a jreq
that failed to parse. So the current version of the block is probably preferable both in readability and efficiency).
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
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
Closes #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:
"error"
and"result"
fields together in a response object.#15495 added regression tests after a discussion in #15381 to kinda lock in our RPC behavior to preserve backwards compatibility.
#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 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, essentially:"error"
OR"result"
but never bothIf this is merged next steps can be:
If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.