[RPC] Various rpc argument fixes #10783

instagibbs · 2017-07-10T16:01:54Z

Audited where named args will fail to use correct default values or may fail when additional optional arguments are added.

Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur.

Included a few other small fixes while working on it.

I didn't bother fixing account-based rpc calls.

ryanofsky

utACK 0ff520dbd9dee50c56ac0e09e57c3b5c199ef336. This is a nice PR, and gets rid of most of the cases where null arguments are treated differently than missing arguments. There are few cases not handled here (getbalance, lockunspent) that are handled in master...ryanofsky:pr/multiopt, but these could be taken care of in a followup.

Note that if we wanted to, we could easily enforce that missing arguments are treated the same as null arguments by making a change like 3fc286b.

ryanofsky · 2017-07-10T18:14:09Z

src/rpc/blockchain.cpp

@@ -210,7 +210,7 @@ UniValue waitfornewblock(const JSONRPCRequest& request)
            + HelpExampleRpc("waitfornewblock", "1000")
        );
    int timeout = 0;
-    if (request.params.size() > 0)
+    if (request.params.size() > 0 && !request.params[0].isNull())


In commit "check for null values in rpc args and handle appropriately"

Instead of writing request.params.size() > X && !request.params[X].isNull() everywhere, I think you should just prefer !request.params[X].isNull(). The UniValue array implementation has bounds checking built in and already returns null for missing elements.

promag · 2017-07-11T10:05:10Z

Needs rebase, #10589 added more RPC arguments.

instagibbs · 2017-07-11T13:41:27Z

@ryanofsky since things are moving underneath me I'll leave the code simplification for a follow-up PR

now covering the getbalance and lockunspent cases

ryanofsky

Conditional utACK 03de1b66d560164187120711468e1e84c197df37 if the getbalance and lockunspent changes are reverted.

@ryanofsky since things are moving underneath me I'll leave the code simplification for a follow-up PR

I don't see a reason to go through this whole process again in another PR. You can simplify your PR right now by running:

git checkout rpcargfixes # (assuming 03de1b66d560164187120711468e1e84c197df37)
git log -p -n1 | git apply -R
git log -p -n1 | sed '/+/s/request.params.size() > \([0-9]\) && \(!request.params\[\1\]\)/\2/' | git apply
git commit -a --amend --no-edit
git push -f

ryanofsky · 2017-07-11T14:58:42Z

src/wallet/rpcwallet.cpp

@@ -753,14 +753,14 @@ UniValue getbalance(const JSONRPCRequest& request)
    if (request.params.size() == 0)
        return  ValueFromAmount(pwallet->GetBalance());

-    const std::string& account_param = request.params[0].get_str();
+    const std::string& account_param = !request.params[0].isNull() ? request.params[0].get_str() : "";


Please revert this line. This is treating a null account the same as the default account ("") instead of treating a null account the same as an unspecified account (params.size() == 0), which you would need to do by changing the params.size() check above (like I did in master...ryanofsky:pr/multiopt and verified with 3fc286b).

ryanofsky · 2017-07-11T15:04:57Z

src/wallet/rpcwallet.cpp

@@ -2309,7 +2309,7 @@ UniValue lockunspent(const JSONRPCRequest& request)
        return true;
    }

-    UniValue outputs = request.params[1].get_array();
+    UniValue outputs = !request.params[1].isNull() ? request.params[1].get_array() : NullUniValue;


Should revert this line too. This is pointless because of the type check above.

I think it's best to stick to just handling the simple cases of missing values being treated differently from null values in this PR and handle the other cases more thoughtfully in another PR. Again, I already have the changes to do this in my multiopt branch, and would be happy to post a followup that builds on your change.

instagibbs · 2017-07-11T16:19:52Z

reverted two changes as suggested and simplified PR(need to improve my sed foo)

ryanofsky

utACK 1ab6cc0e98ce43c6010358241907b96b8a2e746f if the bug in ListReceived (see below) is fixed. Change looks good and gets rid of most of the cases where code treats null arguments different from missing arguments.

ryanofsky · 2017-07-11T18:14:00Z

src/wallet/rpcwallet.cpp

@@ -1209,16 +1209,16 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
 {
    // Minimum confirmations
    int nMinDepth = 1;
-    if (params.size() > 0)
+    if (params[0].isNull())


Missing ! here

TheBlueMatt · 2017-07-11T23:02:02Z

utACK 534963d50bd6454db330432c122805b26750b257

ryanofsky

utACK 534963d50bd6454db330432c122805b26750b257. Unchanged except bugfix in ListReceived.

ryanofsky · 2017-07-17T12:50:25Z

Needs rebase. Would be nice to have more review, too because (as mentioned) I'd like to do some more cleanup on top of this.

instagibbs · 2017-07-17T12:54:09Z

rebased

promag

Minor nits, otherwise utACK.

promag · 2017-07-17T13:22:51Z

src/rpc/net.cpp

@@ -310,7 +310,7 @@ UniValue getaddednodeinfo(const JSONRPCRequest& request)

    std::vector<AddedNodeInfo> vInfo = g_connman->GetAddedNodeInfo();

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


Remove request.params.size() == 1 && ?

promag · 2017-07-17T13:26:17Z

src/rpc/mining.cpp

@@ -101,7 +101,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request)
       );

    LOCK(cs_main);
-    return GetNetworkHashPS(request.params.size() > 0 ? request.params[0].get_int() : 120, request.params.size() > 1 ? request.params[1].get_int() : -1);
+    return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);


Avoid negation?

return GetNetworkHashPS(request.params[0].isNull() ? 120 : request.params[0].get_int(), request.params[1].isNull() ? -1 : request.params[1].get_int());

ryanofsky

utACK 4dc1915. No changes since previous other than rebase to avoid conflict with recent fee estimation change on adjacent line.

Might be good to clarify PR description to say how this change affects behavior. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur.

promag · 2017-07-18T22:14:59Z

src/rpc/rawtransaction.cpp

@@ -137,7 +137,7 @@ UniValue getrawtransaction(const JSONRPCRequest& request)

    // Accept either a bool (true) or a num (>=1) to indicate verbose output.
    bool fVerbose = false;
-    if (request.params.size() > 1) {
+    if (!request.params[1].isNull()) {


This could be simplified to:

bool fVerbose = false; if (request.params[1].isNum()) { fVerbose = request.params[1].get_int() != 0; } else if (!request.params[1].isNull()) { fVerbose = request.params[1].get_bool(); }

With the advantage of removing custom error handling.

sipa · 2017-07-19T00:20:09Z

utACK 4dc1915. @promag has a number of useful suggestions that could be included.

gmaxwell

Concept ACK

sipa · 2017-07-19T03:52:19Z

I would like to see this in 0.15 still - it's effectively (at least partially) fixing the incomplete support for named arguments introduced in the previous version.

instagibbs · 2017-07-19T12:38:20Z

for the purpose of getting this merged as a 0.15 bugfix I'm going to avoid invalidating review for nits (unless told otherwise)

laanwj · 2017-07-19T17:22:23Z

utACK 4dc1915, I didn't know univalue returned NullUniValue for out-of-bounds accesses, I always wrote bound checks, but this is better.

4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders) 999ef20 importmulti options are optional (Gregory Sanders) a70d025 fixup some rpc param counting for rpc help (Gregory Sanders) Pull request description: Audited where named args will fail to use correct default values or may fail when additional optional arguments are added. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur. Included a few other small fixes while working on it. I didn't bother fixing account-based rpc calls. Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e

…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

4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders) 999ef20 importmulti options are optional (Gregory Sanders) a70d025 fixup some rpc param counting for rpc help (Gregory Sanders) Pull request description: Audited where named args will fail to use correct default values or may fail when additional optional arguments are added. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur. Included a few other small fixes while working on it. I didn't bother fixing account-based rpc calls. Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e

… 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

… 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 <UdjinM6@users.noreply.github.com>

4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders) 999ef20 importmulti options are optional (Gregory Sanders) a70d025 fixup some rpc param counting for rpc help (Gregory Sanders) Pull request description: Audited where named args will fail to use correct default values or may fail when additional optional arguments are added. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur. Included a few other small fixes while working on it. I didn't bother fixing account-based rpc calls. Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e

… 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 <UdjinM6@users.noreply.github.com>

instagibbs mentioned this pull request Jul 10, 2017

[WIP] sendtoaddress named arguments work with asset args ElementsProject/elements#204

Closed

ryanofsky reviewed Jul 10, 2017

View reviewed changes

laanwj added the RPC/REST/ZMQ label Jul 11, 2017

instagibbs force-pushed the rpcargfixes branch from 0ff520d to 03de1b6 Compare July 11, 2017 13:38

ryanofsky reviewed Jul 11, 2017

View reviewed changes

instagibbs force-pushed the rpcargfixes branch 3 times, most recently from 9962647 to 1ab6cc0 Compare July 11, 2017 16:12

ryanofsky reviewed Jul 11, 2017

View reviewed changes

instagibbs force-pushed the rpcargfixes branch from 1ab6cc0 to 534963d Compare July 11, 2017 18:21

ryanofsky reviewed Jul 11, 2017

View reviewed changes

instagibbs added 3 commits July 17, 2017 08:53

fixup some rpc param counting for rpc help

a70d025

importmulti options are optional

999ef20

check for null values in rpc args and handle appropriately

4dc1915

instagibbs force-pushed the rpcargfixes branch from 534963d to 4dc1915 Compare July 17, 2017 12:53

promag reviewed Jul 17, 2017

View reviewed changes

ryanofsky reviewed Jul 17, 2017

View reviewed changes

instagibbs mentioned this pull request Jul 17, 2017

Simple, backwards compatible RPC multiwallet support. #10829

Closed

promag reviewed Jul 18, 2017

View reviewed changes

gmaxwell reviewed Jul 19, 2017

View reviewed changes

laanwj added this to the 0.15.0 milestone Jul 19, 2017

laanwj merged commit 4dc1915 into bitcoin:master Jul 20, 2017

ryanofsky mentioned this pull request Jul 25, 2017

Add simple light-client mode (RPC only) #10794

Closed

ryanofsky mentioned this pull request Aug 14, 2017

Avoid treating null RPC arguments different from missing arguments #11050

Merged

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RPC] Various rpc argument fixes #10783

[RPC] Various rpc argument fixes #10783

instagibbs commented Jul 10, 2017 •

edited

ryanofsky left a comment •

edited

ryanofsky Jul 10, 2017

promag commented Jul 11, 2017

instagibbs commented Jul 11, 2017

ryanofsky left a comment

ryanofsky Jul 11, 2017

ryanofsky Jul 11, 2017

instagibbs commented Jul 11, 2017

ryanofsky left a comment

ryanofsky Jul 11, 2017

TheBlueMatt commented Jul 11, 2017

ryanofsky left a comment

ryanofsky commented Jul 17, 2017

instagibbs commented Jul 17, 2017

promag left a comment

promag Jul 17, 2017

promag Jul 17, 2017

ryanofsky left a comment •

edited

promag Jul 18, 2017

sipa commented Jul 19, 2017

gmaxwell left a comment

sipa commented Jul 19, 2017

instagibbs commented Jul 19, 2017

laanwj commented Jul 19, 2017

[RPC] Various rpc argument fixes #10783

[RPC] Various rpc argument fixes #10783

Conversation

instagibbs commented Jul 10, 2017 • edited

ryanofsky left a comment • edited

Choose a reason for hiding this comment

ryanofsky Jul 10, 2017

Choose a reason for hiding this comment

promag commented Jul 11, 2017

instagibbs commented Jul 11, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jul 11, 2017

Choose a reason for hiding this comment

ryanofsky Jul 11, 2017

Choose a reason for hiding this comment

instagibbs commented Jul 11, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jul 11, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Jul 11, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented Jul 17, 2017

instagibbs commented Jul 17, 2017

promag left a comment

Choose a reason for hiding this comment

promag Jul 17, 2017

Choose a reason for hiding this comment

promag Jul 17, 2017

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

promag Jul 18, 2017

Choose a reason for hiding this comment

sipa commented Jul 19, 2017

gmaxwell left a comment

Choose a reason for hiding this comment

sipa commented Jul 19, 2017

instagibbs commented Jul 19, 2017

laanwj commented Jul 19, 2017

instagibbs commented Jul 10, 2017 •

edited

ryanofsky left a comment •

edited

ryanofsky left a comment •

edited