bitcoin-cli: Eliminate "Error parsing JSON" errors #15448

ryanofsky · 2019-02-20T15:17:23Z

Just pass arguments that can't be parsed as JSON as string arguments. If the
RPC method isn't expecting a string argument, this will result in more specific
errors from the RPC. And if it allows an argument to be a string as one
possible type (for example the hash_or_height argument to getblockstats),
this avoids the need for bitcoin-cli users to use double JSON quoting+shell
quoting to pass the string.

Just pass arguments that can't be parsed as JSON as string arguments. If the RPC method isn't expecting a string argument, this will result in more specific errors from the RPC. And if it allows an argument to be a string as one possible type (for example the `hash_or_height` argument to `getblockstats`), this avoids the need for bitcoin-cli users to use double JSON quoting+shell quoting to pass the string.

promag · 2019-02-20T15:49:09Z

I think this approach is fine, unless there is an use case where the end result would be really bad - which I don't believe there is.

Concept ACK from me.

maflcko · 2019-02-20T16:03:19Z

If this is conceptually ACKed, the now unused ParseNonRFCJSONValue should be removed

maflcko

Tested ACK 45c743

This is effectively moving the type check from the client to the server (unparsable json will get send as string). I guess this is fine

# type check is moved to server
$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest getrawtransaction ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff True ffff  ; done
error: Error parsing JSON:True

error code: -1
error message:
JSON value is not a boolean as expected


# test conversion
$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echojson true 1 '"str"' '[1,2]' '{"a":1}'  ; done
[
  true,
  1,
  "str",
  [
    1,
    2
  ],
  {
    "a": 1
  }
]
[
  true,
  1,
  "str",
  [
    1,
    2
  ],
  {
    "a": 1
  }
]

# test no conversion
$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echo true 1 '"str"' '[1,2]' '{"a":1}'  ; done
[
  "true",
  "1",
  "\"str\"",
  "[1,2]",
  "{\"a\":1}"
]
[
  "true",
  "1",
  "\"str\"",
  "[1,2]",
  "{\"a\":1}"
]

# test conversion with invalid json
$ for i in {master,45c743}; do ./src/bitcoin-cli-$i -regtest echojson True  ; done
error: Error parsing JSON:True
[
  "True"
]

maflcko · 2019-02-20T16:39:25Z

src/rpc/client.cpp


-        if (!rpcCvtTable.convert(strMethod, idx)) {
+        UniValue json_value;
+        if (!rpcCvtTable.convert(strMethod, idx) || !json_value.read(strVal)) {


Seems odd to have two inversions in a condition. Would be easier to write

if convert() and read(): # push back parsed value else: # push back str

Keep diff small? 👼 But agree.

jgarzik · 2019-02-20T17:25:40Z

This is effectively moving the type check from the client to the server (unparsable json will get send as string)

Agree. This matches a common frontend validation pattern:

The server must validate and reject garbage. (1) Thus one option is to lean on the remote server to perform that validation.

However, it is often better UX -- and potentially saves users $$ in bandwidth, and saves server CPU cycles -- to (2) avoid sending data the client can prove, via its own validation, is known garbage data.

Yes, this is double validation; but the 90+% common case is rejected on the client side, remaining efficient by sending no garbage over the network.

ryanofsky · 2019-02-20T18:22:21Z

Will wait for concept acks & nacks before working more on this. To summarize comments so far:

Advantages of this change

More usable bitcoin-cli getblockstats command, no longer requiring double json+shell quoting. Other rpc methods could benefit similarly since nothing about this change is specific to getblockstats.
Potentially improved error reporting. Server side error reporting can be more customized and provide context.
Simplified bitcoin-cli logic. Gets rid of thrown exception.

Disadvantages of this change

Increased latency and bandwidth (in error case only) since errors now handled server side.
Potentially worse error reporting in the future. bitcoin-cli error reporting is bad now, but if it's removed entirely, there's loss of opportunity to improve it in the future.

Potential followups

bitcoin-cli: Eliminate "Error parsing JSON" errors #15448 (comment): logic should be inverted for clarity
bitcoin-cli: Eliminate "Error parsing JSON" errors #15448 (comment): unused ParseNonRFCJSONValue function should be removed
Should have python test for new bitcoin-cli behavior.

maflcko · 2019-02-20T18:38:26Z

I don't think we should be optimizing for bandwidth in the error case, which can only happen when a developer debugs locally.

practicalswift · 2019-02-20T23:49:47Z

Concept ACK

In this case I think we should optimise for user-friendliness.

jonasschnelli · 2019-02-21T05:48:05Z

Concept ACK

laanwj · 2019-02-21T07:58:39Z

I'm kind of skeptical about this, to be honest. I prefer strong validation both client and server side, from my experience, lazy "anything goes" validation (in this case "let the server sort them out") has resulted in quite some unexpected disasters. At the least it makes it harder to reason about what things will do, in advance.

Empact · 2019-03-07T15:31:18Z

Looks like this removed all remaining calls to ParseNonRFCJSONValue, so it can be removed.

promag

ACK 45c7439, above comment #15448 (comment) looks good.

Should have python test for new bitcoin-cli behavior.

👍

has resulted in quite some unexpected disasters.

@laanwj like what? Preventing disasters and client validation sound like 2 different things and pretty much unrelated. But better safe than sorry? No strong opinion.

promag · 2019-06-25T14:28:23Z

src/rpc/client.cpp


-        if (!rpcCvtTable.convert(strMethod, idx)) {
+        UniValue json_value;
+        if (!rpcCvtTable.convert(strMethod, idx) || !json_value.read(strVal)) {


Keep diff small? 👼 But agree.

promag · 2019-06-25T14:31:20Z

src/rpc/client.cpp


-        if (!rpcCvtTable.convert(strMethod, name)) {
+        UniValue json_value;
+        if (!rpcCvtTable.convert(strMethod, name) || !json_value.read(value)) {


Same as above.

laanwj · 2019-06-25T15:24:58Z

@laanwj like what? Preventing disasters and client validation sound like 2 different things and pretty much unrelated. But better safe than sorry? No strong opinion.

Defense in depth, belt and suspenders, etc.

I mean the underlying issue is that there is an ambiguity, and resolving this ambiguity is here moved from the client to the server, which isn't always better. It's better if both the client and server know what the interpretation of some value is. SQL injection attacks are a well-known problem that is caused by trusting that the server will the right thing when you just pass the value as-is.

7099984 rpc: doc: Fix and extend getblockstats examples (Adam Soltys) Pull request description: This pull fixes the example curl command for `getblockstats` which doesn't work as is because it's missing a comma between the params and has single quotes around the second parameter. It also adds an additional example of getting block stats by hash by using a known workaround (#15412) to get bitcoin-cli to treat the hash parameter as JSON instead of a string since there is ongoing deliberation about how or whether to fix the root issue (#15448). ACKs for top commit: theStack: ACK 7099984 Tree-SHA512: 84a5b7f449f06fff785bc0afbc1a7dfd55454bc76c52a8945e91556f87f3edfdc5a1780faab8fcfd6c415b734295b7c67d2e04ba7b6cfa91a77758af5dda53ae

Just pass arguments that can't be parsed as JSON as string arguments. If the RPC method isn't expecting a string argument, this will result in more specific errors from the RPC. And if it allows an argument to be a string as one possible type (for example the `hash_or_height` argument to `getblockstats`), this avoids the need for bitcoin-cli users to use double JSON quoting+shell quoting to pass the string. This is a port of bitcoin/bitcoin#15448 Includes several adaptations by Andrea Suisani: - Remove unused ParseNonRFCJSONValue and related unit tests - Avoid double inversion in the same if condition - Functional test adaptation Merge #15488: bitcoin-cli: Eliminate "Error parsing JSON" errors Test plan: - On a version without this patch (!446), run `bitcoin-cli getblockstats 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f` (passing the hash value without quotes ), and observe you get the following error message from the command line program: `error: Error parsing JSON:000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f` - On a version with this patch, run the above command and you should get block statistics JSON output instead of the error message. - Run `test/functional/test_runner.py interface_bitcoin_cli_getblockstats` - it should pass.

promag · 2020-09-21T08:53:10Z

@laanwj this just pretends the client typed "" - it fallbacks the argument to string which seems natural considering this is a command line tool.

@ryanofsky just wrote 4c19cf5 but then I've realised it's pretty much the same as this.

DrahtBot · 2020-09-22T04:33:55Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19949 (cli: Parse and allow hash value by fjahr)

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.

laanwj · 2020-11-23T12:20:15Z

@laanwj this just pretends the client typed "" - it fallbacks the argument to string which seems natural considering this is a command line tool.

That's my problem with it "pretends the client typed". If it is purely a user-facing tool like a GUI it might be different (still, it's often better not to guess at intent). The problem is that tools like this are also used in scripts and such, and they might deal with (partially) untrusted input. I think determinism and predictability is very important in input processing.

7099984 rpc: doc: Fix and extend getblockstats examples (Adam Soltys) Pull request description: This pull fixes the example curl command for `getblockstats` which doesn't work as is because it's missing a comma between the params and has single quotes around the second parameter. It also adds an additional example of getting block stats by hash by using a known workaround (bitcoin#15412) to get bitcoin-cli to treat the hash parameter as JSON instead of a string since there is ongoing deliberation about how or whether to fix the root issue (bitcoin#15448). ACKs for top commit: theStack: ACK bitcoin@7099984 Tree-SHA512: 84a5b7f449f06fff785bc0afbc1a7dfd55454bc76c52a8945e91556f87f3edfdc5a1780faab8fcfd6c415b734295b7c67d2e04ba7b6cfa91a77758af5dda53ae

kallewoof · 2022-01-31T22:52:32Z

Concept ACK

achow101 · 2022-10-12T17:58:27Z

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

ryanofsky mentioned this pull request Feb 20, 2019

RPC getblockstats first argument inconsistency #15412

Closed

maflcko approved these changes Feb 20, 2019

View reviewed changes

fanquake added the RPC/REST/ZMQ label Feb 20, 2019

promag reviewed Jun 25, 2019

View reviewed changes

ajtowns mentioned this pull request Jul 23, 2019

cli/gui: support "@height" in place of blockhash for getblock on client side #16439

Closed

asoltys mentioned this pull request Dec 30, 2019

rpc: doc: Fix and extend getblockstats examples #17831

Merged

DrahtBot closed this Mar 9, 2020

DrahtBot reopened this Mar 9, 2020

This was referenced Sep 12, 2020

cli: Parse and allow hash value #19949

Closed

getblockstats RPC command has issues #19885

Closed

Sjors mentioned this pull request Oct 9, 2020

Coinstats Index #19521

Merged

DrahtBot mentioned this pull request Jan 24, 2021

rpc: faster JSON generation #20999

Closed

Sjors mentioned this pull request Jan 31, 2022

rpc: add require_checksum flag to deriveaddresses #24162

Closed

achow101 closed this Oct 12, 2022

bitcoin deleted a comment from DrahtBot Oct 13, 2022

ryanofsky mentioned this pull request May 31, 2023

RPC: Accept options as named-only parameters #26485

Merged

bitcoin locked and limited conversation to collaborators Oct 13, 2023

bitcoin-cli: Eliminate "Error parsing JSON" errors #15448

bitcoin-cli: Eliminate "Error parsing JSON" errors #15448

Uh oh!

Conversation

ryanofsky commented Feb 20, 2019

Uh oh!

promag commented Feb 20, 2019

Uh oh!

maflcko commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

promag Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

jgarzik commented Feb 20, 2019

Uh oh!

ryanofsky commented Feb 20, 2019

Uh oh!

maflcko commented Feb 20, 2019

Uh oh!

practicalswift commented Feb 20, 2019

Uh oh!

jonasschnelli commented Feb 21, 2019

Uh oh!

laanwj commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Mar 7, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

promag Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 25, 2019

Uh oh!

promag commented Sep 21, 2020

Uh oh!

DrahtBot commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jan 31, 2022

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

maflcko commented Feb 20, 2019 •

edited

Loading

maflcko left a comment •

edited

Loading

laanwj commented Feb 21, 2019 •

edited

Loading

DrahtBot commented Sep 22, 2020 •

edited

Loading

laanwj commented Nov 23, 2020 •

edited

Loading