Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RPC: Strict JSON-RPC 2.0 compliance (gated behind flag) #12435

Closed
wants to merge 1 commit into from
Closed

RPC: Strict JSON-RPC 2.0 compliance (gated behind flag) #12435

wants to merge 1 commit into from

Conversation

esotericnonsense
Copy link
Contributor

@esotericnonsense esotericnonsense commented Feb 14, 2018 •
edited by DrahtBot

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 bitcoin-core/univalue-subtree#12)

meshcollider
meshcollider reviewed Feb 15, 2018
View reviewed changes
src/univalue/include/univalue.h
@@ -180,7 +180,7 @@ class UniValue {
bool push_back(std::pair<std::string,UniValue> pear) {
return pushKV(pear.first, pear.second);
}
friend const UniValue& find_value( const UniValue& obj, const std::string& name);
Copy link
Contributor

Choose a reason for hiding this comment

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

univalue changes need to be submitted upstream, not here :)

Copy link
Member

Choose a reason for hiding this comment

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

He did that (see OP).

@laanwj
Copy link
Member

laanwj commented Feb 15, 2018

Concept ACK

conscott
conscott reviewed Feb 15, 2018
View reviewed changes
src/rpc/server.cpp Outdated
{
} catch (const int x) {
if (x != -1) {
throw;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, throwing an int is really weird.

jamesob
jamesob reviewed Feb 15, 2018
View reviewed changes
Copy link
Member

@jamesob jamesob 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 6fe7afa

Just a few small comments. As others have noted, should definitely figure out something to throw that isn't an int.

src/httprpc.cpp
strReply = JSONRPCReply(result, NullUniValue, jreq.id);
if (ret.isNull()) {
// JSON-RPC 2.0: return nothing at all
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Does this create different behavior for degenerate returns when not passing -strictjsonrpcspec?

src/httprpc.cpp Outdated
else
throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
std::string strReply;
strReply = ret.write() + "\n";
Copy link
Member

Choose a reason for hiding this comment

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

Could just do std::string strReply(ret.write() + "\n");

src/rpc/protocol.cpp
*/

bool fStrictJSONRPCSpec = false;
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 have some kind of g prefix given it's a global? Also, may want to use snake_case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, should be g_strict_jsonrpc_spec or such w/ the new guidelines.

@esotericnonsense esotericnonsense mentioned this pull request Feb 15, 2018
Closed
conscott
conscott reviewed Feb 15, 2018
View reviewed changes
src/univalue/include/univalue.h Outdated
@@ -304,4 +306,9 @@ extern const UniValue NullUniValue;

const UniValue& find_value(const UniValue& obj, const std::string& name, const bool throw_if_not_present = false);

class KeyNotPresentError: public std::runtime_error {
public:
KeyNotPresentError(): std::runtime_error("KeyNotPresentError") {}
Copy link
Contributor

Choose a reason for hiding this comment

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

These exception constructors are usually made explicit to prevent implicit conversion. You can see uint_error as an example.

@esotericnonsense
Copy link
Contributor Author

Tests are failing on the latest commit. Looking in to it.

@esotericnonsense
Copy link
Contributor Author

doh!

@jamesob
Copy link
Member

jamesob commented Feb 16, 2018

Squash needed at some point.

@jonasschnelli
Copy link
Contributor

Nice. Concept ACK.
Travis is failing because of the subtree change that is separately opened in bitcoin-core/univalue-subtree#12

@esotericnonsense esotericnonsense force-pushed the 2018-02-jsonrpc-2.0 branch 3 times, most recently from 43f349c to 76eb200 Compare February 19, 2018 00:05
@esotericnonsense
Copy link
Contributor Author

esotericnonsense commented Feb 19, 2018 •
edited

My attempt to squash this seems to have gone awfully wrong. Trying to fix it up...

edit: should be fine now.

@meshcollider
Copy link
Contributor

Related: #2960

laanwj
laanwj reviewed Apr 23, 2018
View reviewed changes
src/univalue/include/univalue.h
@@ -302,6 +305,6 @@ static inline bool json_isspace(int ch)

extern const UniValue NullUniValue;

const UniValue& find_value( const UniValue& obj, const std::string& name);
const UniValue& find_value(const UniValue& obj, const std::string& name, enum UniValue::keystatus *status=NULL);
Copy link
Member

@laanwj laanwj Apr 23, 2018 •
edited

Choose a reason for hiding this comment

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

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)

@laanwj
Copy link
Member

laanwj commented Apr 24, 2018

(travis failure is due to subtree check for univalue)

@DrahtBot DrahtBot closed this Jul 20, 2018
@DrahtBot
Copy link
Contributor

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.

@DrahtBot DrahtBot reopened this Jul 20, 2018
@DrahtBot DrahtBot mentioned this pull request Sep 7, 2018
Merged
@DrahtBot
Copy link
Contributor

Needs rebase

@laanwj
Copy link
Member

laanwj commented Sep 12, 2018

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…

@laanwj laanwj closed this Sep 12, 2018
@laanwj laanwj mentioned this pull request Feb 15, 2019
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jamesob jamesob jamesob left review comments

@laanwj laanwj laanwj left review comments

@meshcollider meshcollider meshcollider left review comments

@conscott conscott conscott left review comments

Assignees
No one assigned
Labels
RPC/REST/ZMQ Up for grabs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@esotericnonsense @laanwj @jamesob @jonasschnelli @meshcollider @DrahtBot @conscott @fanquake