rpc: Consistent range arguments in scantxoutset/importmulti/deriveaddresses #15497

pull sipa wants to merge 5 commits into bitcoin:master from sipa:201902_rpcconsistentrange changing 8 files +67 −42
  1. sipa commented at 10:27 pm on February 27, 2019: member

    This introduces a consistent notation for RPC arguments in scantxoutset, importmulti, and deriveaddresses, either:

    • "range" : int to just specify the end of the range
    • "range" : [int,int] to specify both the begin and the end of the range.

    For scantxoutset, this is a backward compatible new feature. For the two other RPCs, it’s an incompatible change, but neither of them has been in a release so far. Because of that non-released reason, this only makes sense in 0.18, in my opinion.

    I suggest this as an alternative to #15496, which only makes deriveaddresses compatible with importmulti, but not with the existing scantxoutset RPC. I also think [int,int] is more convenient than {"start":int,"stop":int}.

    I realize this is technically a feature added to scantxoutset after the feature freeze. If desired, I’ll drop the scantxoutset changes.

  2. fanquake added the label RPC/REST/ZMQ on Feb 27, 2019
  3. gmaxwell commented at 10:31 pm on February 27, 2019: contributor
    I like int,int. IIRC scantxoutset is experimental and we shouldn’t shy away from breaking it, if we think a change is correct.
  4. jnewbery commented at 11:08 pm on February 27, 2019: member
    Concept ACK
  5. DrahtBot commented at 11:48 pm on February 27, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15427 (Add support for descriptors to utxoupdatepsbt by sipa)

    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.

  6. in src/rpc/misc.cpp:202 in c6a3a74c16 outdated 
    198@@ -199,37 +199,39 @@ UniValue deriveaddresses(const JSONRPCRequest& request)
199             "For more information on output descriptors, see the documentation in the doc/descriptors.md file.\n"},
200             {
201                 {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
202-                {"begin", RPCArg::Type::NUM, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the beginning of the range to import."},
203-                {"end", RPCArg::Type::NUM, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end of the range to import."}
204+                {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
    promag commented at 0:46 am on February 28, 2019:
    bitcoin-cli users incoming 😄
    sipa commented at 1:28 am on February 28, 2019:
    How so?
    promag commented at 1:36 am on February 28, 2019:

    BTW should update convert params in client.cpp.

    With the shell it must be

    0bitcoin-cli ... '[0,100]'

    right?

    sipa commented at 1:43 am on February 28, 2019:
    I don’t think so.
    promag commented at 1:48 am on February 28, 2019:

    Yes, I have to test that.

    https://github.com/bitcoin/bitcoin/blob/a0d4e79b4dbbfda027054c991705313fcf04135c/src/rpc/client.cpp#L71

    This must be updated.

    sipa commented at 1:51 am on February 28, 2019:
    Right, done!
  7. promag commented at 0:56 am on February 28, 2019: member

    Looks good, concept ACK, especially for keeping it compatible.

    Is the scantxoutset change worthy a release note?

    I also think [int,int] is more convenient than {"start":int,"stop":int}.

    The object notation is more verbose, like named arguments, it increases readability.

  8. sipa commented at 1:35 am on February 28, 2019: member
    @promag Added tiny release note.
  9. sipa force-pushed on Feb 28, 2019
  10. sipa force-pushed on Feb 28, 2019
  11. in src/rpc/util.cpp:543 in 6541cc1f79 outdated 
    538+        return {0, value.get_int64()};
539+    }
540+    if (value.isArray() && value.size() == 2 && value[0].isNum() && value[1].isNum()) {
541+        return {value[0].get_int64(), value[1].get_int64()};
542+    }
543+    throw JSONRPCError(RPC_INVALID_PARAMETER, "Range must be specified as int or [int, int]");
    Sjors commented at 8:45 am on February 28, 2019:
    You could move the start <= end and >0 check here too.
    sipa commented at 9:08 pm on February 28, 2019:
    I’ve moved the start <= end check there. This function may be useful in places where negative numbers are acceptable, so I’m leaving the negative check in the callers.
  12. Sjors approved
  13. Sjors commented at 8:46 am on February 28, 2019: member
    utACK
  14. laanwj added this to the milestone 0.18.0 on Feb 28, 2019
  15. laanwj commented at 2:02 pm on February 28, 2019: member

    The object notation is more verbose, like named arguments, it increases readability

    Named arguments are useful when there is potential confusion about what the arguments mean, or when there’s need for future expansibility. Range notation is kind of self-evident and it’s entirely sure that no new fields will be introduced. begin and end adds no information then. It, for example, doesn’t tell whether it’s an open or closed range. There’s only more cognitive overload, more to type and remember.

    So I tend to disagree here. A tuple [a,b] is fine, when documented in the RPC help of course.

    Concept ACK.

  16. jonasschnelli commented at 2:05 pm on February 28, 2019: contributor
    Concept ACK (also as bugfix for 0.18 to avoid API inconsistency).
  17. flack commented at 2:10 pm on February 28, 2019: contributor
    I realize it’s needed for backward compat, but specifying a range by giving one number seems kind of counter intuitive. Esp since by looking at the param, you can’t really tell if the one number is supposed to be the start or end of the range (so there’s a 50% chance of guessing wrong). Maybe the int version should be restricted to where it’s needed for backward compatibility instead of also making it available for the new calls?
  18. Sjors commented at 3:03 pm on February 28, 2019: member
    @flack in most cases the start of the range is 0. What I think is more likely to confuse people is that ranges are 0 based, and so 1 means [0, 1] and not [0,0] (I’ve made this mistake a few times already, although ultimately it didn’t really matter).
  19. flack commented at 3:34 pm on February 28, 2019: contributor

    @Sjors interesting, the 0-based part I got right away, but I guess it depends what programming lang you usually work with :-)

    What I just find ambiguous is that range: 5 could plausibly be interpreted as either

    • range: [0, 5] (i.e. what it really does)
    • range: [5, 5] (i.e. give me just one element)
    • range: [5, inf] (i.e. start at 5, open-ended)

    ofc when you read the documentation, you know that option 1 is the correct one. But as a casual user it’s quite easy to make mistakes, since range: 5 is not really self-explanatory

  20. Sjors commented at 3:49 pm on February 28, 2019: member
    I would be OK with always requiring both start and end, so [0, 0]. It probably takes less time to type than to interpret the docs :-) Backwards compatibility is not a huge concern because we declared the scan method experimental.
  21. promag commented at 4:01 pm on February 28, 2019: member

    So I tend to disagree here. A tuple [a,b] is fine, when documented in the RPC help of course. @laanwj I didn’t say otherwise, just that object notation is more verbose.

  22. Add ParseRange function to parse args of the form int/[int,int] 7aa6a8aefb
  23. Support ranges arguments in RPC help 6b9f45e81b
  24. Add support for stop/[start,stop] ranges to scantxoutset 4566011631
  25. Use stop/[start,stop] notation in importmulti desc range 1675b7ce55
  26. sipa force-pushed on Feb 28, 2019
  27. in src/rpc/misc.cpp:215 in 0fa7e141b1 outdated 
    212+                HelpExampleCli("deriveaddresses", "\"wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#trd0mf0l\" \"[0,2]\"")
213             }}.ToString()
214         );
215     }
216 
217-    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VNUM, UniValue::VNUM});
    MarcoFalke commented at 10:23 pm on February 28, 2019:

    style-nit: Any reason to remove this. You could keep it via

    0RPCTypeCheck(request.params, {
1        UniValue::VSTR,
2        UniValueType(), // RANGE, checked later
    sipa commented at 5:40 am on March 1, 2019:
    Done.
  28. MarcoFalke commented at 10:26 pm on February 28, 2019: member
    utACK 0fa7e141b1584668e8b72d73e70520d796a7839f (ignore the style-nit)
  29. Make deriveaddresses use stop/[start,stop] notation for ranges ca253f6ebf
  30. sipa force-pushed on Mar 1, 2019
  31. Sjors commented at 12:42 pm on March 1, 2019: member

    rpc/client.cpp needs to be updated for deriveaddresses, otherwise tACK ca253f6.

    See: https://github.com/Sjors/bitcoin/commit/349c8cf2696f547cbe6d36934321d8ee3339e916

  32. MarcoFalke renamed this:
    Consistent range arguments in scantxoutset/importmulti/deriveaddresses
    rpc: Consistent range arguments in scantxoutset/importmulti/deriveaddresses
    on Mar 1, 2019
  33. MarcoFalke commented at 2:12 pm on March 1, 2019: member

    re-utACK ca253f6ebf2c9a12f5cb2b4c9f575178d058a612

    Only change since previous review is adding an RPCTypeCheck

  34. MarcoFalke merged this on Mar 1, 2019
  35. MarcoFalke closed this on Mar 1, 2019
  36. MarcoFalke referenced this in commit a6d7026a45 on Mar 1, 2019
  37. laanwj referenced this in commit 37f236acc6 on Mar 2, 2019
  38. deadalnix referenced this in commit 1d2f20a287 on Jun 17, 2020
  39. deadalnix referenced this in commit 30528c38a6 on Jun 17, 2020
  40. deadalnix referenced this in commit 671369f2bf on Jun 17, 2020
  41. deadalnix referenced this in commit 16524ee867 on Jun 17, 2020
  42. deadalnix referenced this in commit d3875009a6 on Jun 17, 2020
  43. vijaydasmp referenced this in commit dbe98c5bd9 on Sep 15, 2021
  44. vijaydasmp referenced this in commit 3d764126d8 on Sep 16, 2021
  45. vijaydasmp referenced this in commit 209fa51d9f on Sep 16, 2021
  46. vijaydasmp referenced this in commit 54d9254d6d on Sep 20, 2021
  47. dzutto referenced this in commit f5e52489c7 on Dec 1, 2021
  48. PastaPastaPasta referenced this in commit a490615a8b on Dec 11, 2021
  49. MarcoFalke locked this on Dec 16, 2021


sipa gmaxwell jnewbery DrahtBot promag Sjors laanwj jonasschnelli flack MarcoFalke

Labels
RPC/REST/ZMQ

Milestone
0.18.0

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