Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from Mar 1, 2019

Conversation

Projects
None yet
@sipa
Copy link
Member

commented Feb 27, 2019

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.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I like int,int. IIRC scantxoutset is experimental and we shouldn't shy away from breaking it, if we think a change is correct.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

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.

@promag
Copy link
Member

left a comment

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.

Show resolved Hide resolved src/rpc/misc.cpp
@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@promag Added tiny release note.

@sipa sipa force-pushed the sipa:201902_rpcconsistentrange branch 2 times, most recently from dbe2ea0 to 6541cc1 Feb 28, 2019

@Sjors

Sjors approved these changes Feb 28, 2019

Copy link
Member

left a comment

utACK

Show resolved Hide resolved src/rpc/util.cpp Outdated

@laanwj laanwj added this to the 0.18.0 milestone Feb 28, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Concept ACK (also as bugfix for 0.18 to avoid API inconsistency).

@flack

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

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?

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

@flack

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@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

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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.

@sipa sipa force-pushed the sipa:201902_rpcconsistentrange branch from 6541cc1 to 0fa7e14 Feb 28, 2019

@MarcoFalke
Copy link
Member

left a comment

utACK 0fa7e14 (ignore the style-nit)

Show resolved Hide resolved src/rpc/misc.cpp

@sipa sipa force-pushed the sipa:201902_rpcconsistentrange branch from 0fa7e14 to ca253f6 Mar 1, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

See: Sjors@349c8cf

@MarcoFalke MarcoFalke changed the title Consistent range arguments in scantxoutset/importmulti/deriveaddresses rpc: Consistent range arguments in scantxoutset/importmulti/deriveaddresses Mar 1, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

re-utACK ca253f6

Only change since previous review is adding an RPCTypeCheck

@MarcoFalke MarcoFalke merged commit ca253f6 into bitcoin:master Mar 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Mar 1, 2019

Merge #15497: rpc: Consistent range arguments in scantxoutset/importm…
…ulti/deriveaddresses

ca253f6 Make deriveaddresses use stop/[start,stop] notation for ranges (Pieter Wuille)
1675b7c Use stop/[start,stop] notation in importmulti desc range (Pieter Wuille)
4566011 Add support for stop/[start,stop] ranges to scantxoutset (Pieter Wuille)
6b9f45e Support ranges arguments in RPC help (Pieter Wuille)
7aa6a8a Add ParseRange function to parse args of the form int/[int,int] (Pieter Wuille)

Pull request description:

  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.

Tree-SHA512: 1cbebb90cf34f106786dbcec7afbf3f43fb8b7e46cc7e6763faf1bc1babf12375a1b3c3cf86ee83c21ed2171d99b5a2f60331850bc613db25538c38b6a056676

laanwj added a commit that referenced this pull request Mar 2, 2019

Merge #15510: [rpc] deriveaddresses: add range to CRPCConvertParam
9586157 [rpc] deriveaddresses: add range to CRPCConvertParam (Sjors Provoost)

Pull request description:

  Missing from #15497

Tree-SHA512: 469de3f896bcd3435a480685e5257c51ba895df0311329d5e5a3cb2e1894e5358324473d998ea45221776aefe8836a7af6c4f12198a36d2d10bf6761991cfd60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.