This patch adds a -strictjsonrpcspec flag.
If the flag is used, bitcoind enters JSON-RPC 2.0 mode, which allows it to be fully spec-compliant (and thus work with libraries like libjson-rpc-cpp without modification).
I've added a functional test for the specific bits of the spec that I've changed.
univalue changes are included in this commit for ease of review but I can pull those out (see https://github.com/bitcoin-core/univalue/pull/12)
179 | @@ -180,7 +180,7 @@ class UniValue { 180 | bool push_back(std::pair<std::string,UniValue> pear) { 181 | return pushKV(pear.first, pear.second); 182 | } 183 | - friend const UniValue& find_value( const UniValue& obj, const std::string& name);
univalue changes need to be submitted upstream, not here :)
He did that (see OP).
Concept ACK
411 | @@ -396,9 +412,15 @@ static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req) 412 | 413 | UniValue result = tableRPC.execute(jreq); 414 | rpc_result = JSONRPCReplyObj(result, NullUniValue, jreq.id); 415 | - } 416 | - catch (const UniValue& objError) 417 | - { 418 | + } catch (const int x) { 419 | + if (x != -1) { 420 | + throw;
I commented in the univalue PR that this catch should probably be for a derived class of std::exception, not int. If this is the case, there will be no need to re-throw, as the std:exception handler will catch all else below.
Yes, throwing an int is really weird.
196 | 197 | - // Send reply 198 | - strReply = JSONRPCReply(result, NullUniValue, jreq.id); 199 | + if (ret.isNull()) { 200 | + // JSON-RPC 2.0: return nothing at all 201 | + return false;
Does this create different behavior for degenerate returns when not passing -strictjsonrpcspec?
205 | - } else if (valRequest.isArray()) 206 | - strReply = JSONRPCExecBatch(jreq, valRequest.get_array()); 207 | - else 208 | - throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error"); 209 | + std::string strReply; 210 | + strReply = ret.write() + "\n";
Could just do std::string strReply(ret.write() + "\n");
20 | @@ -21,11 +21,18 @@ 21 | * 22 | * 1.0 spec: http://json-rpc.org/wiki/specification 23 | * 1.2 spec: http://jsonrpc.org/historical/json-rpc-over-http.html 24 | + * 25 | + * By passing -strictjsonrpcspec one can force Bitcoin to speak strict 2.0. 26 | + * 27 | */ 28 | 29 | +bool fStrictJSONRPCSpec = false;
Should this have some kind of g prefix given it's a global? Also, may want to use snake_case.
Yes, should be g_strict_jsonrpc_spec or such w/ the new guidelines.
Concept ACK https://github.com/bitcoin/bitcoin/pull/12435/commits/6fe7afa92d06b22b5e7447b51d64fa048c19d7a5
Just a few small comments. As others have noted, should definitely figure out something to throw that isn't an int.
305 | @@ -304,4 +306,9 @@ extern const UniValue NullUniValue; 306 | 307 | const UniValue& find_value(const UniValue& obj, const std::string& name, const bool throw_if_not_present = false); 308 | 309 | +class KeyNotPresentError: public std::runtime_error { 310 | + public: 311 | + KeyNotPresentError(): std::runtime_error("KeyNotPresentError") {}
These exception constructors are usually made explicit to prevent implicit conversion. You can see uint_error as an example.
Tests are failing on the latest commit. Looking in to it.
doh!
Squash needed at some point.
Nice. Concept ACK. Travis is failing because of the subtree change that is separately opened in https://github.com/bitcoin-core/univalue/pull/12
My attempt to squash this seems to have gone awfully wrong. Trying to fix it up...
edit: should be fine now.
304 | @@ -302,6 +305,6 @@ static inline bool json_isspace(int ch) 305 | 306 | extern const UniValue NullUniValue; 307 | 308 | -const UniValue& find_value( const UniValue& obj, const std::string& name); 309 | +const UniValue& find_value(const UniValue& obj, const std::string& name, enum UniValue::keystatus *status=NULL);
Are you sure this univalue API change is really necessary? You could find out if the key is there with an additional call to Univalue::exists? (or am I missing something)
(travis failure is due to subtree check for univalue)
<!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 151 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Closing this for now, and adding "up for grabs" tag. Let me know if you want to start work on this again and I'll reopen…
Removing "Up for grabs" as JSON 2.0 was done in #27101. cc @pinheadmz