[RPC] Various rpc argument fixes #10783

pull instagibbs wants to merge 3 commits into bitcoin:master from instagibbs:rpcargfixes changing 6 files +51 −51
  1. instagibbs commented at 4:01 PM on July 10, 2017: member

    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.

  2. in src/rpc/blockchain.cpp:213 in 0ff520dbd9 outdated 
     209 | @@ -210,7 +210,7 @@ UniValue waitfornewblock(const JSONRPCRequest& request)
 210 |              + HelpExampleRpc("waitfornewblock", "1000")
 211 |          );
 212 |      int timeout = 0;
 213 | -    if (request.params.size() > 0)
 214 | +    if (request.params.size() > 0 && !request.params[0].isNull())
    ryanofsky commented at 6:14 PM on July 10, 2017:

    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.

  3. ryanofsky commented at 6:39 PM on July 10, 2017: member

    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 https://github.com/bitcoin/bitcoin/compare/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 3fc286bbf21df9bfebcb6a9f685f22365d98b15f.

  4. laanwj added the label RPC/REST/ZMQ on Jul 11, 2017
  5. promag commented at 10:05 AM on July 11, 2017: member

    Needs rebase, #10589 added more RPC arguments.

  6. instagibbs force-pushed on Jul 11, 2017
  7. instagibbs commented at 1:41 PM on July 11, 2017: member

    @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

  8. in src/wallet/rpcwallet.cpp:756 in 03de1b66d5 outdated 
     752 | @@ -753,14 +753,14 @@ UniValue getbalance(const JSONRPCRequest& request)
 753 |      if (request.params.size() == 0)
 754 |          return  ValueFromAmount(pwallet->GetBalance());
 755 |  
 756 | -    const std::string& account_param = request.params[0].get_str();
 757 | +    const std::string& account_param = !request.params[0].isNull() ? request.params[0].get_str() : "";
    ryanofsky commented at 2:58 PM on July 11, 2017:

    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 https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiopt and verified with 3fc286bbf21df9bfebcb6a9f685f22365d98b15f).

  9. in src/wallet/rpcwallet.cpp:2312 in 03de1b66d5 outdated 
    2308 | @@ -2309,7 +2309,7 @@ UniValue lockunspent(const JSONRPCRequest& request)
2309 |          return true;
2310 |      }
2311 |  
2312 | -    UniValue outputs = request.params[1].get_array();
2313 | +    UniValue outputs = !request.params[1].isNull() ? request.params[1].get_array() : NullUniValue;
    ryanofsky commented at 3:04 PM on July 11, 2017:

    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.

  10. ryanofsky commented at 3:17 PM on July 11, 2017: member

    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
  11. instagibbs force-pushed on Jul 11, 2017
  12. instagibbs force-pushed on Jul 11, 2017
  13. instagibbs force-pushed on Jul 11, 2017
  14. instagibbs commented at 4:19 PM on July 11, 2017: member

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

  15. in src/wallet/rpcwallet.cpp:1212 in 1ab6cc0e98 outdated 
    1208 | @@ -1209,16 +1209,16 @@ UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByA
1209 |  {
1210 |      // Minimum confirmations
1211 |      int nMinDepth = 1;
1212 | -    if (params.size() > 0)
1213 | +    if (params[0].isNull())
    ryanofsky commented at 6:14 PM on July 11, 2017:

    Missing ! here

  16. ryanofsky commented at 6:16 PM on July 11, 2017: member

    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.

  17. instagibbs force-pushed on Jul 11, 2017
  18. TheBlueMatt commented at 11:02 PM on July 11, 2017: member

    utACK 534963d50bd6454db330432c122805b26750b257

  19. ryanofsky commented at 11:13 PM on July 11, 2017: member

    utACK 534963d50bd6454db330432c122805b26750b257. Unchanged except bugfix in ListReceived.

  20. ryanofsky commented at 12:50 PM on July 17, 2017: member

    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.

  21. fixup some rpc param counting for rpc help a70d025366
  22. importmulti options are optional 999ef2073a
  23. check for null values in rpc args and handle appropriately 4dc1915bce
  24. instagibbs force-pushed on Jul 17, 2017
  25. instagibbs commented at 12:54 PM on July 17, 2017: member

    rebased

  26. in src/rpc/net.cpp:313 in 4dc1915bce 
     309 | @@ -310,7 +310,7 @@ UniValue getaddednodeinfo(const JSONRPCRequest& request)
 310 |  
 311 |      std::vector<AddedNodeInfo> vInfo = g_connman->GetAddedNodeInfo();
 312 |  
 313 | -    if (request.params.size() == 1) {
 314 | +    if (request.params.size() == 1 && !request.params[0].isNull()) {
    promag commented at 1:22 PM on July 17, 2017:

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

  27. in src/rpc/mining.cpp:104 in 4dc1915bce 
     100 | @@ -101,7 +101,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request)
 101 |         );
 102 |  
 103 |      LOCK(cs_main);
 104 | -    return GetNetworkHashPS(request.params.size() > 0 ? request.params[0].get_int() : 120, request.params.size() > 1 ? request.params[1].get_int() : -1);
 105 | +    return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);
    promag commented at 1:26 PM on July 17, 2017:

    Avoid negation?

    return GetNetworkHashPS(request.params[0].isNull() ? 120 : request.params[0].get_int(), request.params[1].isNull() ? -1 : request.params[1].get_int());
  28. promag commented at 1:29 PM on July 17, 2017: member

    Minor nits, otherwise utACK.

  29. ryanofsky commented at 2:21 PM on July 17, 2017: member

    utACK 4dc1915bceacb94446454428da676772290b8c48. 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.

  30. in src/rpc/rawtransaction.cpp:140 in 4dc1915bce 
     136 | @@ -137,7 +137,7 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
 137 |  
 138 |      // Accept either a bool (true) or a num (>=1) to indicate verbose output.
 139 |      bool fVerbose = false;
 140 | -    if (request.params.size() > 1) {
 141 | +    if (!request.params[1].isNull()) {
    promag commented at 10:14 PM on July 18, 2017:

    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.

  31. sipa commented at 12:20 AM on July 19, 2017: member

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

  32. gmaxwell commented at 1:46 AM on July 19, 2017: contributor

    Concept ACK

  33. sipa commented at 3:52 AM on July 19, 2017: member

    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.

  34. instagibbs commented at 12:38 PM on July 19, 2017: member

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

  35. laanwj commented at 5:22 PM on July 19, 2017: member

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

  36. laanwj added this to the milestone 0.15.0 on Jul 19, 2017
  37. laanwj merged this on Jul 20, 2017
  38. laanwj closed this on Jul 20, 2017
  39. laanwj referenced this in commit 041dad94b0 on Jul 20, 2017
  40. laanwj referenced this in commit 7ed57d3d7c on Aug 22, 2017
  41. PastaPastaPasta referenced this in commit 8453bc4686 on Aug 8, 2019
  42. PastaPastaPasta referenced this in commit 66f9ef56ac on Aug 8, 2019
  43. PastaPastaPasta referenced this in commit 9d206814cb on Aug 12, 2019
  44. PastaPastaPasta referenced this in commit 539a78e092 on Sep 19, 2019
  45. PastaPastaPasta referenced this in commit ed28a5c08f on Sep 23, 2019
  46. PastaPastaPasta referenced this in commit d4aa263fee on Sep 24, 2019
  47. PastaPastaPasta referenced this in commit 654e6f8b30 on Nov 19, 2019
  48. PastaPastaPasta referenced this in commit 793c968bc4 on Nov 21, 2019
  49. PastaPastaPasta referenced this in commit 0f2bae362a on Dec 9, 2019
  50. PastaPastaPasta referenced this in commit 2539bf1313 on Jan 1, 2020
  51. PastaPastaPasta referenced this in commit 343f6c85b3 on Jan 2, 2020
  52. PastaPastaPasta referenced this in commit 77b48b2e44 on Jan 2, 2020
  53. PastaPastaPasta referenced this in commit 0cb0c3c39d on Jan 2, 2020
  54. PastaPastaPasta referenced this in commit a0565cba25 on Jan 2, 2020
  55. PastaPastaPasta referenced this in commit f48c315abc on Jan 2, 2020
  56. PastaPastaPasta referenced this in commit 7f1f623787 on Jan 3, 2020
  57. barrystyle referenced this in commit 5bdc47f9ce on Jan 22, 2020
  58. ckti referenced this in commit d236b9a357 on Mar 28, 2021
  59. gades referenced this in commit 9184462af7 on Jun 26, 2021
  60. DrahtBot locked this on Sep 8, 2021
Contributors
instagibbsryanofskypromagTheBlueMattsipagmaxwelllaanwj
Labels
RPC/REST/ZMQ

Milestone
0.15.0

Linked (view graph)
#11050 Avoid treating null RPC arguments different from missing arguments
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-27 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me