Skip to content

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Feb 14, 2023

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:

  • 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)

#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:

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 14, 2023

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 cbergqvist, ryanofsky, tdb3
Concept ACK ajtowns, fanquake, sipa, stickies-v, vincenzopalazzo
Stale ACK willcl-ark

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29946 (JSON-RPC request Content-Type is application/json by luke-jr)

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.

Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@fanquake
Copy link
Member

Concept ACK

@pinheadmz pinheadmz force-pushed the jsonrpc-2.0 branch 3 times, most recently from 97af16b to b139110 Compare February 27, 2023 21:24
@sipa
Copy link
Member

sipa commented Feb 27, 2023

Concept ACK. Migrating to strict JSON-RPC 2.0 for bitcoin-cli and whatever opts into 2.0 is cool, even if it means keeping support for kinda-sorta-JSON-RPC 1.0 around on the server side.

@willcl-ark
Copy link
Member

Concept ACK.

Might be worth updating a few other things at the same time if you continue to move ahead:

@pinheadmz
Copy link
Member Author

@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.

@pinheadmz pinheadmz force-pushed the jsonrpc-2.0 branch 4 times, most recently from ec2b51b to d7a87a1 Compare March 1, 2023 21:25
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@pinheadmz pinheadmz marked this pull request as ready for review March 2, 2023 16:38
@pinheadmz
Copy link
Member Author

This PR is ready for code review if any of you fine handsome concept-ACKers have the time ❤️

Copy link
Member

@willcl-ark willcl-ark left a 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.

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
Copy link
Member

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?

Copy link
Member Author

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

@pinheadmz
Copy link
Member Author

Added a scripted-diff to completely replace all occurrences of the application-defined RPC_INVALID_PARAMETER (-8) with the JSONRPC 2.0 spec-defined RPC_INVALID_PARAMS (-32602)

pinheadmz added 4 commits May 14, 2024 10:39
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.
@pinheadmz
Copy link
Member Author

force push to cbc6c44:

  • rename enum to JSONRPCVersion::{V1_LEGACY, V2}
  • use std::move() in httprpc
  • pass catch_errors argument to JSONRPCExec()
  • replace RPC_PARSE_ERROR with RPC_MISC_ERROR in JSONRPCExec()

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)
Copy link
Contributor

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.

Copy link
Contributor

@cbergqvist cbergqvist left a 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)
Copy link
Contributor

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.

Copy link
Contributor

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.

@DrahtBot DrahtBot requested a review from ryanofsky May 15, 2024 13:04
Copy link
Contributor

@ryanofsky ryanofsky left a 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.
Copy link
Contributor

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)
Copy link
Contributor

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.

@ryanofsky
Copy link
Contributor

@willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack

@tdb3
Copy link
Contributor

tdb3 commented May 15, 2024

@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.

Copy link
Contributor

@tdb3 tdb3 left a 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).

@ryanofsky ryanofsky merged commit 75118a6 into bitcoin:master May 16, 2024
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.
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there...

fanquake added a commit that referenced this pull request Jun 8, 2024
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
Comment on lines +246 to +253
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);
}
Copy link
Contributor

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).

Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Sep 13, 2024
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
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Sep 13, 2024
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
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Sep 13, 2024
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
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Sep 13, 2024
@sipa sipa mentioned this pull request Oct 6, 2024
Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Nov 18, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Jun 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support JSON-RPC 2.0