Avoid treating null RPC arguments different from missing arguments #11050

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/narg changing 7 files +71 −61
  1. ryanofsky commented at 11:59 pm on August 14, 2017: member

    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 (review)
    • Add braces around if statements. #11050 (review)
    • Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. #11050 (review)
  2. Get rid of redundant RPC params.size() checks 
    No change in behavior.
    e666efcdba
  3. Avoid treating null RPC arguments different from missing arguments 
    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.
    e067673f4e
  4. fanquake added the label RPC/REST/ZMQ on Aug 15, 2017
  5. ryanofsky force-pushed on Aug 15, 2017
  6. in src/wallet/rpcwallet.cpp:2387 in 4195613ea4 outdated 
    2374         if (fUnlock)
2375             pwallet->UnlockAllCoins();
2376         return true;
2377     }
2378 
2379+    RPCTypeCheckArgument(request.params[1], UniValue::VARR);
    promag commented at 0:55 am on August 15, 2017:
    get_array already throws and above you also replace RPCTypeCheck validation with get_bool. Remove?
    ryanofsky commented at 1:17 am on August 15, 2017:

    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.

    promag commented at 1:37 am on August 15, 2017:
    So add RPCTypeCheckArgument for 1st param?
    promag commented at 1:37 am on August 15, 2017:
    Sorry, you did!
    jnewbery commented at 7:16 pm on August 17, 2017:

    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 commented at 7:44 pm on August 17, 2017:

    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 commented at 9:07 pm on August 17, 2017:

    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

    ryanofsky commented at 0:16 am on August 18, 2017:
    Added as followup in #11050#issue-250186439
  7. in src/wallet/rpcwallet.cpp:2381 in 4195613ea4 outdated 
    2368+    RPCTypeCheckArgument(request.params[0], UniValue::VBOOL);
2369 
2370     bool fUnlock = request.params[0].get_bool();
2371 
2372-    if (request.params.size() == 1) {
2373+    if (request.params[1].isNull()) {
    promag commented at 0:59 am on August 15, 2017:
    IMO size() sounds better in this case.
    ryanofsky commented at 1:28 am on August 15, 2017:

    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.

    promag commented at 1:40 am on August 15, 2017:
    If another argument is added then this would be if (request.params[1].isNull() && request.params[2].isNull()) right?
    ryanofsky commented at 3:14 am on August 15, 2017:

    Thread #11050 (review)

    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).

    laanwj commented at 7:49 am on August 15, 2017:
    Agree with ryanofsky. isNull is correct here.
  8. promag commented at 1:03 am on August 15, 2017: member
    utACK 4195613 modulo nits.
  9. Update developer notes after params.size() cleanup fd5d71ec4b
  10. ryanofsky commented at 3:37 am on August 15, 2017: member
    Added a documentation commit 4195613ea411c164e8b30c5803d35ea90a863dee -> f4c29d6e9d11885f522ea05499261ba0da56af36 (pr/narg.2 -> pr/narg.3, compare)
  11. promag commented at 1:14 pm on August 15, 2017: member
    utACK f4c29d6.
  12. Clean up getbalance RPC parameter handling 
    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.
    745d2e315f
  13. ryanofsky commented at 8:21 pm on August 15, 2017: member

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

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

  14. ryanofsky force-pushed on Aug 15, 2017
  16. in src/rpc/rawtransaction.cpp:925 in 745d2e315f 
    921@@ -922,7 +922,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
922     const uint256& hashTx = tx->GetHash();
923 
924     CAmount nMaxRawTxFee = maxTxFee;
925-    if (request.params.size() > 1 && request.params[1].get_bool())
926+    if (!request.params[1].isNull() && request.params[1].get_bool())
    TheBlueMatt commented at 6:40 pm on August 17, 2017:
    Couldnt this just be request.params[1].isTrue() like many other checks?
    ryanofsky commented at 6:59 pm on August 17, 2017:

    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.

    jnewbery commented at 7:04 pm on August 17, 2017:
    This is safer. It throws an error when the user passes in “True” as a string (rather than silently evaluating to false)
    laanwj commented at 7:25 am on August 22, 2017:

    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.

  17. in src/wallet/rpcwallet.cpp:854 in 745d2e315f 
    850@@ -838,11 +851,11 @@ UniValue movecmd(const JSONRPCRequest& request)
851     CAmount nAmount = AmountFromValue(request.params[2]);
852     if (nAmount <= 0)
853         throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
854-    if (request.params.size() > 3)
855+    if (!request.params[3].isNull())
    TheBlueMatt commented at 6:43 pm on August 17, 2017:
    How about we just stop type-checking it instead?
    ryanofsky commented at 7:10 pm on August 17, 2017:

    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.

  18. jnewbery approved
  19. jnewbery commented at 7:22 pm on August 17, 2017: member

    utACK 745d2e315f39d7591c0ea9e772a19e3cd9b51b09

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

  20. ryanofsky commented at 7:49 pm on August 17, 2017: member

    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.)

  21. jnewbery approved
  22. jnewbery commented at 9:08 pm on August 17, 2017: member
    ACK
  23. in src/rpc/rawtransaction.cpp:349 in 745d2e315f 
    346             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range");
347         rawTx.nLockTime = nLockTime;
348     }
349 
350-    bool rbfOptIn = request.params.size() > 3 ? request.params[3].isTrue() : false;
351+    bool rbfOptIn = request.params[3].isTrue();
    promag commented at 11:09 pm on August 17, 2017:
    This param should be type checked?
    ryanofsky commented at 0:07 am on August 18, 2017:

    This param should be type checked?

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

    promag commented at 0:36 am on August 18, 2017:

    This commit is not changing behavior

    Right!

  24. in src/rpc/net.cpp:196 in 745d2e315f 
    192@@ -193,7 +193,7 @@ UniValue getpeerinfo(const JSONRPCRequest& request)
193 UniValue addnode(const JSONRPCRequest& request)
194 {
195     std::string strCommand;
196-    if (request.params.size() == 2)
197+    if (!request.params[1].isNull())
    promag commented at 11:14 pm on August 17, 2017:
    Add {} where appropriate.
    ryanofsky commented at 0:03 am on August 18, 2017:

    Add {} where appropriate.

    Added as followup in #11050#issue-250186439. 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 commented at 0:35 am on August 18, 2017:
    Sure.
  25. promag commented at 0:37 am on August 18, 2017: member
    utACK 745d2e3.
  26. TheBlueMatt commented at 9:07 pm on August 20, 2017: member
    utACK 745d2e315f39d7591c0ea9e772a19e3cd9b51b09
  27. laanwj commented at 7:26 am on August 22, 2017: member
    utACK 745d2e3
  28. laanwj merged this on Aug 22, 2017
  29. laanwj closed this on Aug 22, 2017
  30. laanwj referenced this in commit 7ed57d3d7c on Aug 22, 2017
  31. mempko referenced this in commit 190eff756e on Sep 28, 2017
  32. PastaPastaPasta referenced this in commit 539a78e092 on Sep 19, 2019
  33. PastaPastaPasta referenced this in commit ed28a5c08f on Sep 23, 2019
  34. PastaPastaPasta referenced this in commit d4aa263fee on Sep 24, 2019
  35. jasonbcox referenced this in commit 5be9de3ada on Oct 25, 2019
  36. PastaPastaPasta referenced this in commit 654e6f8b30 on Nov 19, 2019
  37. PastaPastaPasta referenced this in commit 793c968bc4 on Nov 21, 2019
  38. PastaPastaPasta referenced this in commit 0f2bae362a on Dec 9, 2019
  39. codablock referenced this in commit 0f468409bd on Dec 31, 2019
  40. codablock referenced this in commit 80214e6cd0 on Dec 31, 2019
  41. PastaPastaPasta referenced this in commit 3563c13e0c on Jan 1, 2020
  42. PastaPastaPasta referenced this in commit 9524e37eb3 on Jan 1, 2020
  43. PastaPastaPasta referenced this in commit 2539bf1313 on Jan 1, 2020
  44. PastaPastaPasta referenced this in commit 1a229f9465 on Jan 1, 2020
  45. PastaPastaPasta referenced this in commit 7cec724543 on Jan 1, 2020
  46. PastaPastaPasta referenced this in commit 343f6c85b3 on Jan 2, 2020
  47. PastaPastaPasta referenced this in commit 993f61799b on Jan 2, 2020
  48. PastaPastaPasta referenced this in commit 1310c32e68 on Jan 2, 2020
  49. PastaPastaPasta referenced this in commit 77b48b2e44 on Jan 2, 2020
  50. PastaPastaPasta referenced this in commit 0cb0c3c39d on Jan 2, 2020
  51. PastaPastaPasta referenced this in commit a0565cba25 on Jan 2, 2020
  52. PastaPastaPasta referenced this in commit f48c315abc on Jan 2, 2020
  53. PastaPastaPasta referenced this in commit 7f1f623787 on Jan 3, 2020
  54. jonspock referenced this in commit a8b09f09d0 on Oct 1, 2020
  55. jonspock referenced this in commit 61450889e5 on Oct 1, 2020
  56. jonspock referenced this in commit 17ee0208e9 on Oct 5, 2020
  57. jonspock referenced this in commit 11b3649b62 on Oct 10, 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


ryanofsky promag jnewbery laanwj meshcollider TheBlueMatt

Labels
RPC/REST/ZMQ

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: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me