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

In commit "test: cover JSONRPC 2.0 requests, batches, and notifications" (92c6513)

This is confusing because there should be no such thing as a JSON-RPC 1.0 batch request, batches were introduced in 2.0. Also specifying req["jsonrpc"] = "1.0" is nonstandard. Would be good to clarify what behavior is standard and what is just being done for backwards compatibility.

In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (d62dcf8)

This is just stated without a reason and doesn't appear to be true.

Also, checking for the version field after the id field is bad because it means if a batch request includes an unrecognized version number, the id is lost and there is no indication which call in the batch is causing the problem.

In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (9cae84c)

This is not a great commit description because it is only describing one of the 3 changes made in this commit. In addition to making error and result multually exclusive it is also:

• Adding jsonrpc 2.0 field to responses
• Making non-batch jsonrpc 2.0 requests return 200 http status instead of error statuses when the RPC method fails

In the branch I posted #27101 (review) this commit is split into two parts and includes tests to clarify the changes.

In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (9cae84c)

Would be safest to drop default argument and make it required to specify the version. I think differences between versions are subtle and confusing enough that the version should always be explicit.

ok thanks, originally I wanted to use a default value to ensure that bitcoin-cli continued to work without any changes

Added the explicit arguments to the calls in bitcoin-cli.cpp, so far that's the only change i made to your branch

In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (9cae84c)

This declaration appears to be unused

Without it there is a compile error:

In file included from ./rpc/server.h:9:
./rpc/request.h:22:79: error: unknown type name 'JSONRPCRequest'
   22 | std::string JSONRPCReply(const UniValue& result, const UniValue& error, const JSONRPCRequest& jreq);
      |                                                                               ^
1 error generated.

In commit "rpc: use move semantics in JSONRPCReplyObj()" (51a071f)

To make sense, this commit needs to use std::move below for result, error, and id. The point of changing these arguments from const to non-const is to allow them to be moved from, and avoid the copies that would otherwise happen below.

In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (9cae84c)

re: #27101 (comment)

my understanding is that the error message will continue to be passed by reference through the end of JSONRPCReplyObj

It will be passed by value, but it will be copied because std::move is not specified, so this is less than ideal. The fix here is just to drop const from the catch declaration and then add std::move to move the value from the exception to the reply object.

In commit "rpc: JSON-RPC 2.0 should not respond to "notifications"" (d62dcf8)

I think this needs some explanation about how valRequest.size() check is here for backward compatibility since it is surprising and violates the 2.0 spec.

Here the caught UniValue& is not explicitly std::moved. But on line 272 it is. Is that intentional?

Good catch thanks. I'll need @ryanofsky to weigh in here because I keep misunderstanding c++ move semantics. But here's what I think is happening:

In JSONRPCReplyObj() in request.cpp the really import move-instead-of-copy is when we call reply.pushKV(...): https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64

That makes me think that the std::move() currently in JSONErrorReply() in httprpc.cpp is unnecessary: https://github.com/esotericnonsense/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/httprpc.cpp#L90

...and also unnecessary is the one you see on line 272.

So if I'm finally understanding, which is unlikely, then there are two places where std::move can be removed but actually this line 244 right here is OK, because the import move-instead-of-copy happens inside the function being called.

re: #27101 (comment)

I think @cbergqvist is right here and e should be replaced by std::move(e) on line 244.

The JSONRPCReplyObj signature is:

UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONVersion json_version);

So without std::move(e), the error parameter will be initialized by making a copy of e instead of moving from e. This is because e is an lvalue, while std::move(e) is an rvalue.

In JSONRPCReplyObj() in request.cpp the really import move-instead-of-copy is when we call reply.pushKV(...): https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64

That is true. The signature for pushKV is void pushKV(std::string key, UniValue val), so without std::move(error), on that line the function parameter val would be initialized by copying not moving.

That makes me think that the std::move() currently in JSONErrorReply() in httprpc.cpp is unnecessary: https://github.com/esotericnonsense/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/httprpc.cpp#L90

This is not true. std::move is needed to avoid copy-initializing the JSONRPCReplyObj error parameter there, just like on line 272 and on line

So if I'm finally understanding, which is unlikely, then there are two places where std::move can be removed but actually this line 244 right here is OK, because the import move-instead-of-copy happens inside the function being called.

Yeah this is a little off. What you are saying is true if the function signature accepts parameters by reference. For example if you have function that looks like:

void MyFunction(SomeType& param);
void MyFunction(const SomeType& param);
void MyFunction(SomeType&& param);

In this case, no matter whether you call MyFunction with std::move or without it, the parameter param is going to be a reference not a value, so no copy will be created just by calling the function. There could copies of param made inside the function, depending on what it does internally, as you say. But merely calling the function will not copy the parameter.

However, if you have a function that takes a non-reference value parameter like:

void MyFunction(SomeType param);

No matter what the function does internally, just calling the function is going to copy-construct param variable or move-construct it. And unless you use std::move or the argument is already an rvalue, the argument will be copied.

Had to check whether UniValue even had a move-constructor. It seems like one should be generated implicitly if my readings of https://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declared_move_constructor and univalue.h are correct.

@ryanofsky's explanation rings true with my long underutilized C++11 neurons.

Might be more correct & compatible to append "\r\n". See https://stackoverflow.com/questions/5757290/http-header-line-break-style

That's a reasonable comment but I kept the single character which is how it worked on master (see below). I see a few examples of \r\n in the codebase but its rare and I'm not sure how it's decided.

In commit "rpc: refactor single/batch requests" (a64a2b7)

re: #27101 (comment)

HTTP headers use \r\n, but this line is generating the JSON object in the body of the response, after the headers. And the JSON could be written with either \r\n or \n or no trailing line break at all. I think it would probably make sense not to include any trailing line break, but existing code uses \n, so in a refactoring commit it seems best not to change this.

In commit "rpc: refactor single/batch requests" (a64a2b7)

re: #27101 (comment)

HTTP headers use \r\n, but this line is generating the JSON object in the body of the response, after the headers. And the JSON could be written with either \r\n or \n or no trailing line break at all. I think it would probably make sense not to include any trailing line break, but existing code uses \n, so in a refactoring commit it seems best not to change this.

In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75)

Might be good to add a comment noting that the "jsonrpc" version field was added in the JSON-RPC 2.0 spec, so value "1.0" is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.

In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75)

Variable name valJsonRPC doesn't really follow the suggested coding style which calls for snake_case. Could rename it to "jsonrpc_version"

In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75)

Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum

Is JSON/JSONRPC really necessary for the values themselves if the enum is of the type enum class so that one always has to prepend the type? - JSONRPCVersion::JSONRPC_1_BTC vs JSONRPCVersion::1_BTC.

re: #27101 (comment)

Is JSON/JSONRPC really necessary for the values themselves if the enum is of the type enum class so that one always has to prepend the type? - JSONRPCVersion::JSONRPC_1_BTC vs JSONRPCVersion::1_BTC.

Good point. I'd probably call the constants V1_LEGACY and V2 if renaming.

In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

This is probably supposed to say 1.1 not 1.2

In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

Returning RPC_PARSE_ERROR does not seem accurate if the request was parsed successfully but there was an error executing it. This was also a preexisting problem in HTTPReq_JSONRPC for batch requests, but now the behavior will happen for all version 2 single requests as well. Would suggest cleaning this up with a change like:

--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -204,7 +204,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 return false;
             }
 
-            reply = JSONRPCExec(jreq);
+            // Legacy 1.0/1.1 behavior is for failed requests to throw
+            // exceptions which return HTTP errors and RPC errors to the client.
+            // 2.0 behavior is to catch exceptions and return HTTP success with
+            // RPC errors, as long as there is not HTTP server error.
+            const bool catch_errors{jreq.m_json_version == JSONVersion::JSON_2_0};
+            reply = JSONRPCExec(jreq, catch_errors);
 
         // array of requests
         } else if (valRequest.isArray()) {
@@ -233,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 // in "HTTP OK" responses.
                 try {
                     jreq.parse(valRequest[i]);
-                    reply.push_back(JSONRPCExec(jreq));
                 } catch (UniValue& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
                 } catch (const std::exception& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version));
                 }
+                reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
             }
         }
         else
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -358,22 +358,18 @@ bool IsDeprecatedRPCEnabled(const std::string& method)
     return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end();
 }
 
-UniValue JSONRPCExec(const JSONRPCRequest& jreq)
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors)
 {
     UniValue result;
-    if (jreq.m_json_version == JSONVersion::JSON_2_0) {
-        // JSONRPC 2.0 behavior: only throw HTTP error if the server is actually
-        // broken. Otherwise errors are sent back in "HTTP OK" responses.
+    if (catch_errors) {
         try {
             result = tableRPC.execute(jreq);
         } catch (UniValue& e) {
             return JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version);
         } catch (const std::exception& e) {
-            return JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version);
+            return JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_MISC_ERROR, e.what()), jreq.id, jreq.m_json_version);
         }
     } else {
-        // Legacy Bitcoin JSONRPC 1.0/1.2 behavior:
-        // Single requests may throw HTTP errors, handled by caller or client
         result = tableRPC.execute(jreq);
     }
 
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -181,7 +181,7 @@ extern CRPCTable tableRPC;
 void StartRPC();
 void InterruptRPC();
 void StopRPC();
-UniValue JSONRPCExec(const JSONRPCRequest& jreq);
+UniValue JSONRPCExec(const JSONRPCRequest& jreq, bool catch_errors);
 
 // Drop witness when serializing for RPC?
 bool RPCSerializationWithoutWitness();

Thanks so much for the patch. I had to move this line back up to where it was, otherwise the test fails because after pushing the error into the batch reply, we would also then push a weird extra junk reply with the current id but the result from the last successful query

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 0e3e66c515..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -238,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
                 // in "HTTP OK" responses.
                 try {
                     jreq.parse(valRequest[i]);
+                    reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
                 } catch (UniValue& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, std::move(e), jreq.id, jreq.m_json_version));
                 } catch (const std::exception& e) {
                     reply.push_back(JSONRPCReplyObj(NullUniValue, JSONRPCError(RPC_PARSE_ERROR, e.what()), jreq.id, jreq.m_json_version));
                 }
-                reply.push_back(JSONRPCExec(jreq, /*catch_errors=*/true));
             }
         }
         else

I'm trying to figure out how to write a test to catch one of these RPC_MISC_ERROR since they obviously weren't catching a RPC_PARSE_ERROR before adding your patch to the commit. If I understand correctly, that "misc" error would only throw if there was a bug in an RPC command because most of our try blocks try to catch a UniValue error first with a specific code.

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

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.

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.

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.

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.

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.

Looks like this is unused?

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