Support JSON-RPC 2.0 when requested by client #27101

pinheadmz · 2023-02-14T19:13:42Z

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 · 2023-02-14T19:13:45Z

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.

ajtowns

Concept ACK

src/rpc/request.h

fanquake · 2023-02-20T17:41:13Z

Concept ACK

sipa · 2023-02-27T21:33:38Z

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 · 2023-02-28T12:31:16Z

Concept ACK.

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

This comment: https://github.com/bitcoin/bitcoin/pull/27101/files#diff-188b88992c9e08a108e852e7f69a2c2c6569d4b1212c9d05c46fdfd7fcb778d7R21-R27
HelpExampleRPC() to json 2.0
Could warrant a release note to document new behaviours when using json 2.0

pinheadmz · 2023-03-01T19:08:45Z

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

stickies-v

Concept ACK

pinheadmz · 2023-03-20T15:04:55Z

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

willcl-ark

ACK 2da823199

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.

willcl-ark · 2023-03-20T21:22:53Z

test/functional/interface_rpc.py

Should this not be a status: 200, error: -32602 under json 2.0?

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

test/functional/interface_rpc.py

doc/release-notes-27101.md

test/functional/interface_rpc.py

pinheadmz · 2023-03-28T18:17:06Z

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)

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.

pinheadmz · 2024-05-14T17:28:00Z

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

cbergqvist · 2024-05-15T12:42:16Z

src/httprpc.cpp

 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)


nit: JSONErrorReply -> JSONRPCErrorReply, although it could be argued that it actually does write a JSON object in the response.

cbergqvist

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.

cbergqvist · 2024-05-15T12:46:27Z

src/rpc/server.cpp

-    {
-        rpc_result = JSONRPCReplyObj(NullUniValue,
-                                     JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors)


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.

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

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.

ryanofsky · 2024-05-15T14:14:44Z

src/rpc/request.cpp

+        }
+        // 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.


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.

ryanofsky · 2024-05-15T14:19:18Z

src/rpc/server.cpp

-    {
-        rpc_result = JSONRPCReplyObj(NullUniValue,
-                                     JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors)


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 · 2024-05-15T14:31:02Z

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

tdb3 · 2024-05-15T14:48:51Z

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

tdb3

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

maflcko · 2024-05-20T12:35:08Z

doc/JSON-RPC-interface.md

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


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.

maflcko · 2024-05-20T12:36:31Z

doc/release-notes-27101.md

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


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.

maflcko · 2024-05-20T13:45:30Z

test/functional/interface_rpc.py

-    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


Looks like this is unused?

@pinheadmz did you end up following up to these comments?

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

cbergqvist · 2024-06-17T22:44:50Z

src/httprpc.cpp

+                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);
+                }


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

pinheadmz mentioned this pull request Feb 14, 2023

Support JSON-RPC 2.0 #2960

Closed

pinheadmz force-pushed the jsonrpc-2.0 branch from d21b25b to 440d6a2 Compare February 14, 2023 19:21

ajtowns reviewed Feb 16, 2023

View reviewed changes

src/rpc/request.h Outdated Show resolved Hide resolved

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

pinheadmz force-pushed the jsonrpc-2.0 branch from b139110 to 814c3c1 Compare March 1, 2023 18:48

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

stickies-v reviewed Mar 2, 2023

View reviewed changes

pinheadmz force-pushed the jsonrpc-2.0 branch from d7a87a1 to 2da8231 Compare March 2, 2023 15:29

pinheadmz marked this pull request as ready for review March 2, 2023 16:38

pinheadmz mentioned this pull request Mar 8, 2023

doc: document json rpc endpoints #27225

Merged

willcl-ark approved these changes Mar 21, 2023

View reviewed changes

pinheadmz force-pushed the jsonrpc-2.0 branch 2 times, most recently from 5388efb to c9a3862 Compare March 22, 2023 18:43

This was referenced Mar 24, 2023

Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet #27279

Merged

Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated #27322

Closed

pinheadmz mentioned this pull request Mar 27, 2023

Inconsistent RPC error handling #19087

Closed

pinheadmz force-pushed the jsonrpc-2.0 branch from c9a3862 to a754467 Compare March 28, 2023 18:16

pinheadmz force-pushed the jsonrpc-2.0 branch from a754467 to b583b39 Compare March 28, 2023 18:23

Matthew Zipkin added 4 commits May 14, 2024 10:39

rpc: Add "jsonrpc" field and drop null "result"/"error" fields

466b905

Only for JSON-RPC 2.0 requests.

rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests

bf1a1f1

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.

doc: add comments and release-notes for JSON-RPC 2.0

cbc6c44

pinheadmz force-pushed the jsonrpc-2.0 branch from 8583c2f to cbc6c44 Compare May 14, 2024 17:22

cbergqvist reviewed May 15, 2024

View reviewed changes

cbergqvist approved these changes May 15, 2024

View reviewed changes

DrahtBot requested a review from ryanofsky May 15, 2024 13:04

ryanofsky approved these changes May 15, 2024

View reviewed changes

tdb3 approved these changes May 16, 2024

View reviewed changes

ryanofsky merged commit 75118a6 into bitcoin:master May 16, 2024

maflcko reviewed May 20, 2024

View reviewed changes

pinheadmz mentioned this pull request Jun 6, 2024

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

Merged

cbergqvist reviewed Jun 17, 2024

View reviewed changes

edilmedeiros mentioned this pull request Aug 5, 2024

Bitcoin OpenRPC Specification #29477

Closed

storopoli mentioned this pull request Aug 29, 2024

EXP-189: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types alpenlabs/alpen#251

Merged

11 tasks

sipa mentioned this pull request Oct 6, 2024

RPC breakage with v28.0 #31039

Closed

Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Nov 18, 2024

Merge bitcoin#27101: Support JSON-RPC 2.0 when requested by client

f6c51ea

Akira45-0 mentioned this pull request Feb 13, 2025

Unable to use Bisq as a full DAO node with Bitcoin Core 28.1 - Might be related to BuyerVerifiesPreparedDelayedPayoutTx Exception error bisq-network/bisq#7389

Open

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

bitcoin locked and limited conversation to collaborators Jun 17, 2025

Support JSON-RPC 2.0 when requested by client #27101

Support JSON-RPC 2.0 when requested by client #27101

Uh oh!

Conversation

pinheadmz commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Feb 20, 2023

Uh oh!

sipa commented Feb 27, 2023

Uh oh!

willcl-ark commented Feb 28, 2023

Uh oh!

pinheadmz commented Mar 1, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Mar 20, 2023

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pinheadmz commented Mar 28, 2023

Uh oh!

pinheadmz commented May 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented May 15, 2024

Uh oh!

tdb3 commented May 15, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pinheadmz commented Feb 14, 2023 •

edited

Loading

DrahtBot commented Feb 14, 2023 •

edited

Loading