Avoid treating null RPC arguments different from missing arguments #11050

ryanofsky · 2017-08-14T23:59:22Z

This is a followup to #10783.

The first commit doesn't change behavior at all, just simplifies code.
The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors.
The third commit updates developer notes after the cleanup.
The forth commit does some additional code cleanup in getbalance.

Followup changes that should happen in future PRs:

Replace uses of .isTrue() with calls to .get_bool() so numbers, objects, and strings cause type errors instead of being interpreted as false. Avoid treating null RPC arguments different from missing arguments #11050 (comment)
Add braces around if statements. Avoid treating null RPC arguments different from missing arguments #11050 (comment)
Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. Avoid treating null RPC arguments different from missing arguments #11050 (comment)

No change in behavior.

This changes RPC methods to treat null arguments the same as missing arguments, instead of throwing type errors. Specifically: - `getbalance` method now returns the wallet balance when the `account` param is null instead of throwing a type error (same as when parameter is missing). It is still an error to supply `minconf` or `watchonly` options when the account is null. - `addnode` and `setban` methods now return help text instead of type errors if `command` params are null (same as when params are missing). - `sendrawtransaction`, `setaccount`, `movecmd`, `sendfrom`, `addmultisigaddress`, `listaccounts`, `lockunspent` methods accept null default values where missing values were previously allowed, and treat them the same.

promag

utACK 4195613 modulo nits.

promag · 2017-08-15T00:55:22Z

src/wallet/rpcwallet.cpp

        return true;
    }

+    RPCTypeCheckArgument(request.params[1], UniValue::VARR);


get_array already throws and above you also replace RPCTypeCheck validation with get_bool. Remove?

get_array already throws and above you also replace RPCTypeCheck validation with get_bool. Remove?

It throws a different error that doesn't specify passed type name.

So add RPCTypeCheckArgument for 1st param?

Sorry, you did!

It throws a different error that doesn't specify passed type name.

So we should update is_<type>() error message to output the passed type.

So we should update is_() error message to output the passed type.

If you mean get_<type>() error message, we can do this but it would involve making a change upstream in https://github.com/jgarzik/univalue. Anyway this commit is trying to do exactly one thing: "treat null arguments the same as missing arguments, instead of throwing type errors". If people don't like RPCTypeCheck functions and would prefer to rely on UniValue exceptions, it really seems like a followup for a separate PR.

yes, I meant get_<type>().

Agree that this should be done in a follow-up PR. I've tested that this PR maintains existing behaviour for the error checking in lockunspent

Added as followup in #11050 (comment)

promag · 2017-08-15T00:59:09Z

src/wallet/rpcwallet.cpp

    bool fUnlock = request.params[0].get_bool();

-    if (request.params.size() == 1) {
+    if (request.params[1].isNull()) {


IMO size() sounds better in this case.

The problem with checking size == 1 is that it's fragile and will break if you want to add new arguments to the RPC.

Checking params.size() can also create confusion when using named arguments, because the current named argument implementation creates variable length null-padded arrays, so adding or removing one named argument can cause a completely different argument to switch from missing to null or vice versa.

The point of this PR is to avoid uses of params.size() to prevent missing arguments from being treated differently than null arguments.

If another argument is added then this would be if (request.params[1].isNull() && request.params[2].isNull()) right?

Thread #11050 (comment)

No, params[1] is the transactions argument to the lockunspent RPC. The transactions argument may be an array of transaction outputs, or it may be null, or it may be missing.

This code is just saying that if the transactions argument is null or missing, then all transactions should be unlocked. The previous code only worked if transactions argument was missing, but not if the transactions argument was null (because it would trigger a type error).

Agree with ryanofsky. isNull is correct here.

ryanofsky · 2017-08-15T03:37:18Z

Added a documentation commit 4195613 -> f4c29d6 (pr/narg.2 -> pr/narg.3, compare)

promag · 2017-08-15T13:14:52Z

utACK f4c29d6.

Only change in behavior is that unsupported combinations of parameters now trigger more specific error messages instead of the vague "JSON value is not a string as expected" error.

ryanofsky · 2017-08-15T20:21:53Z

Added another commit to clean up getbalance code a little and make it return nicer errors: f4c29d6 -> a53cb75 (pr/narg.3 -> pr/narg.4, compare).

Updated commit messages (no code changes): a53cb75 -> 745d2e3 (pr/narg.4 -> pr/narg.5)

meshcollider · 2017-08-16T12:09:33Z

utACK 745d2e3

TheBlueMatt · 2017-08-17T18:40:51Z

src/rpc/rawtransaction.cpp


    CAmount nMaxRawTxFee = maxTxFee;
-    if (request.params.size() > 1 && request.params[1].get_bool())
+    if (!request.params[1].isNull() && request.params[1].get_bool())


Couldnt this just be request.params[1].isTrue() like many other checks?

Couldnt this just be request.params[1].isTrue() like many other checks?

This would be a different (and I think worse) behavior. Calling get_bool will throw an exception if the type is not boolean, while calling isTrue will just return false. I think it's better to throw an exception if a value other than true, false, or null is passed, instead taking all strings, objects and numbers to be false.

This is safer. It throws an error when the user passes in "True" as a string (rather than silently evaluating to false)

rather than silently evaluating to false

Yes, please be careful to throw exceptions on wrong types instead of silently ignoring errors. Than can lead to hazardous bugs in user code.

TheBlueMatt · 2017-08-17T18:43:52Z

src/wallet/rpcwallet.cpp

    if (nAmount <= 0)
        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
-    if (request.params.size() > 3)
+    if (!request.params[3].isNull())


How about we just stop type-checking it instead?

How about we just stop type-checking it instead?

I think there's some benefit to keeping the type checking here, and it doesn't really cost anything. Maybe somebody forgets the dummy parameter and is calling this RPC with a comment. Slightly better to throw an error in get_int instead of silently dropping the comment.

jnewbery

utACK 745d2e3

I especially like the changes to getbalance. Returning an error is preferable to silently ignoring arguments passed in.

jnewbery · 2017-08-17T19:16:28Z

src/wallet/rpcwallet.cpp

        return true;
    }

+    RPCTypeCheckArgument(request.params[1], UniValue::VARR);


It throws a different error that doesn't specify passed type name.

So we should update is_<type>() error message to output the passed type.

ryanofsky

Thanks for the review.

I especially like the changes to getbalance. Returning an error is preferable to silently ignoring arguments passed in.

Note that the arguments weren't silently ignored before. They would just trigger more opaque "JSON value is not a string as expected" errors if they were non-null. (This is mentioned in the commit message.)

ryanofsky · 2017-08-17T19:44:03Z

src/wallet/rpcwallet.cpp

        return true;
    }

+    RPCTypeCheckArgument(request.params[1], UniValue::VARR);


So we should update is_() error message to output the passed type.

If you mean get_<type>() error message, we can do this but it would involve making a change upstream in https://github.com/jgarzik/univalue. Anyway this commit is trying to do exactly one thing: "treat null arguments the same as missing arguments, instead of throwing type errors". If people don't like RPCTypeCheck functions and would prefer to rely on UniValue exceptions, it really seems like a followup for a separate PR.

jnewbery

ACK

jnewbery · 2017-08-17T21:07:58Z

src/wallet/rpcwallet.cpp

        return true;
    }

+    RPCTypeCheckArgument(request.params[1], UniValue::VARR);


yes, I meant get_<type>().

Agree that this should be done in a follow-up PR. I've tested that this PR maintains existing behaviour for the error checking in lockunspent

promag · 2017-08-17T23:09:51Z

src/rpc/rawtransaction.cpp

    }

-    bool rbfOptIn = request.params.size() > 3 ? request.params[3].isTrue() : false;
+    bool rbfOptIn = request.params[3].isTrue();


This param should be type checked?

This param should be type checked?

Added as followup in #11050 (comment). This commit is not changing behavior in any way, and the PR is not adding new type checking in other places.

This commit is not changing behavior

Right!

promag · 2017-08-17T23:14:17Z

src/rpc/net.cpp

 {
    std::string strCommand;
-    if (request.params.size() == 2)
+    if (!request.params[1].isNull())


Add {} where appropriate.

Add {} where appropriate.

Added as followup in #11050 (comment). This PR is already doing a bunch of different things and has been reviewed by 4 people. I don't want to add noise, delay, and complexity sticking braces everywhere as well.

promag · 2017-08-18T00:37:10Z

utACK 745d2e3.

TheBlueMatt · 2017-08-20T21:07:33Z

utACK 745d2e3

laanwj · 2017-08-22T07:26:34Z

utACK 745d2e3

…g arguments 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to #10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. #11050 (comment) - [ ] Add braces around if statements. #11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. #11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9

…g arguments 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to #10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin/bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin/bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin/bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9

…g arguments Summary: 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to #10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin/bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin/bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin/bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9 Backport of Core PR11050 bitcoin/bitcoin#11050 Depends on D3464 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3997

… missing arguments 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to bitcoin#10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9

… missing arguments 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to bitcoin#10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9 More of 11050 Fixes for bitcoin#11050 backport Fixes for bitcoin#11050 Fix masternode_winners RPC help check and revert few params[x].isNull() changes RPC: resolve index and '!' problems Co-Authored-By: UdjinM6 <[email protected]>

…g arguments Summary: 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to #10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin/bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin/bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin/bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9 Backport of Core PR11050 bitcoin/bitcoin#11050 Depends on D3464 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3997

… missing arguments 745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky) fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky) e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky) e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky) Pull request description: This is a followup to bitcoin#10783. - The first commit doesn't change behavior at all, just simplifies code. - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors. - The third commit updates developer notes after the cleanup. - The forth commit does some additional code cleanup in `getbalance`. Followup changes that should happen in future PRs: - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin#11050 (comment) - [ ] Add braces around if statements. bitcoin#11050 (comment) - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin#11050 (comment) Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9 More of 11050 Fixes for bitcoin#11050 backport Fixes for bitcoin#11050 Fix masternode_winners RPC help check and revert few params[x].isNull() changes RPC: resolve index and '!' problems Co-Authored-By: UdjinM6 <[email protected]>

ryanofsky added 2 commits August 14, 2017 19:38

Get rid of redundant RPC params.size() checks

e666efc

No change in behavior.

fanquake added the RPC/REST/ZMQ label Aug 15, 2017

ryanofsky force-pushed the pr/narg branch from 20b27af to 4195613 Compare August 15, 2017 00:03

promag reviewed Aug 15, 2017

View reviewed changes

Update developer notes after params.size() cleanup

fd5d71e

Clean up getbalance RPC parameter handling

745d2e3

Only change in behavior is that unsupported combinations of parameters now trigger more specific error messages instead of the vague "JSON value is not a string as expected" error.

ryanofsky force-pushed the pr/narg branch from a53cb75 to 745d2e3 Compare August 15, 2017 20:29

TheBlueMatt reviewed Aug 17, 2017

View reviewed changes

jnewbery approved these changes Aug 17, 2017

View reviewed changes

ryanofsky commented Aug 17, 2017

View reviewed changes

jnewbery approved these changes Aug 17, 2017

View reviewed changes

promag reviewed Aug 17, 2017

View reviewed changes

laanwj merged commit 745d2e3 into bitcoin:master Aug 22, 2017

ryanofsky mentioned this pull request Nov 16, 2017

RPC: Internal named params #11660

Closed

codablock added a commit to codablock/dash that referenced this pull request Dec 31, 2019

Fixes for bitcoin#11050 backport

0f46840

codablock added a commit to codablock/dash that referenced this pull request Dec 31, 2019

Fixes for bitcoin#11050

80214e6

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Avoid treating null RPC arguments different from missing arguments #11050

Avoid treating null RPC arguments different from missing arguments #11050

Uh oh!

Conversation

ryanofsky commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 15, 2017 • 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!

ryanofsky commented Aug 15, 2017

Uh oh!

promag commented Aug 15, 2017

Uh oh!

ryanofsky commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky 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!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ryanofsky commented Aug 14, 2017 •

edited

Loading

laanwj Aug 15, 2017 •

edited

Loading

ryanofsky commented Aug 15, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading