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.

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

Fix null id in RPC response during startup #6362

Merged
merged 1 commit into from Jul 2, 2015

Conversation

@forrestv
Copy link
Contributor

@forrestv forrestv commented Jul 2, 2015

When processing RPC commands during warmup phase, parse the
request object before returning an error so that id value can
be used in the response.

Prior to this commit, RPC commands sent during Bitcoin's
warmup/startup phase were responded to with a JSON-RPC error
with an id of null, which violated the JSON-RPC 2.0 spec:

id: This member is REQUIRED. It MUST be the same as the value
of the id member in the Request Object. If there was an error
in detecting the id in the Request object (e.g. Parse
error/Invalid Request), it MUST be Null.

request object before returning an error so that id value can
be used in the response.

Prior to this commit, RPC commands sent during Bitcoin's
warmup/startup phase were responded to with a JSON-RPC error
with an id of null, which violated the JSON-RPC 2.0 spec:

id: This member is REQUIRED. It MUST be the same as the value
of the id member in the Request Object. If there was an error
in detecting the id in the Request object (e.g. Parse
error/Invalid Request), it MUST be Null.
@laanwj
Copy link
Member

@laanwj laanwj commented Jul 2, 2015

Good catch. The new place is better in any case because this also catches other invocations of CRPCTable::execute, such as from the GUI.

Tested ACK.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jul 2, 2015

utACK.
If I'm right, the REST interface does also check if the we are in warmup phase. Because moving the warmup check will also affect REST.

@laanwj
Copy link
Member

@laanwj laanwj commented Jul 2, 2015

@jonasschnelli the code is moved from HTTPReq_JSONRPC to CRPCTable::execute, both is downstream from REST/HTTP selection, so RESTS cannot be affected. REST should do its own check and raise its own error, which has nothing to do with JSONRPCError.

Here's the same check for REST: https://github.com/bitcoin/bitcoin/blob/master/src/rest.cpp#L556

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jul 2, 2015

Sorry. Right, there are no changes for REST calls.

@laanwj laanwj merged commit 72b9452 into bitcoin:master Jul 2, 2015
1 check passed
laanwj added a commit that referenced this issue Jul 2, 2015
72b9452 When processing RPC commands during warmup phase, parse the request object before returning an error so that id value can be used in the response. (Forrest Voight)
luke-jr added a commit to luke-jr/bitcoin that referenced this issue Jan 10, 2016
request object before returning an error so that id value can
be used in the response.

Prior to this commit, RPC commands sent during Bitcoin's
warmup/startup phase were responded to with a JSON-RPC error
with an id of null, which violated the JSON-RPC 2.0 spec:

id: This member is REQUIRED. It MUST be the same as the value
of the id member in the Request Object. If there was an error
in detecting the id in the Request object (e.g. Parse
error/Invalid Request), it MUST be Null.

Github-Pull: bitcoin#6362
Rebased-From: 72b9452
luke-jr added a commit to luke-jr/bitcoin that referenced this issue Jan 10, 2016
request object before returning an error so that id value can
be used in the response.

Prior to this commit, RPC commands sent during Bitcoin's
warmup/startup phase were responded to with a JSON-RPC error
with an id of null, which violated the JSON-RPC 2.0 spec:

id: This member is REQUIRED. It MUST be the same as the value
of the id member in the Request Object. If there was an error
in detecting the id in the Request object (e.g. Parse
error/Invalid Request), it MUST be Null.

Github-Pull: bitcoin#6362
Rebased-From: 72b9452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants