rpc: deprecate getaddressinfo label #17585

pull jonatack wants to merge 5 commits into bitcoin:master from jonatack:deprecate-getaddressinfo-label changing 10 files +75 −33
  1. jonatack commented at 1:56 am on November 25, 2019: member

    This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo label field. The deprecated behavior can be re-enabled by starting bitcoind with -deprecatedrpc=label.

    See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and #17283 (comment) for more context.

    Reviewers: This PR may be tested manually by building, then running bitcoind with and without the -deprecatedrpc=label flag while verifying the rpc getaddressinfo output and help text.

    Next step: add support for multiple labels.

  2. fanquake added the label RPC/REST/ZMQ on Nov 25, 2019
  3. jonatack force-pushed on Nov 25, 2019
  4. DrahtBot commented at 4:16 am on November 25, 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:

    • #17958 (rpc: query general daemon information via RPC by brakmic)

    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.

  5. laanwj commented at 3:48 pm on November 25, 2019: member
    I’m not sure about this. It wasn’t too long ago that we deprecated “account” here. Now we’re deprecating “label” too. For most purposes (like identifying invoices), assigning one label per address seems to be enough. If someone uses multiple labels per address then yes, a single “label” won’t do. But I’m not sure it’s worth the hassle for all current clients.
  6. jonatack commented at 4:17 pm on November 25, 2019: member

    Thanks. It seems to me that the current state of both label and labels being returned might be confusing. Based on the previous two wallet meeting discussions, at the moment I imagine one of these options going forward:

    1. deprecate label, and add multiple label capability to labels (seemed to be the most popular option)

    2. keep label, and deprecate labels

    3. keep label, and change labels to tags as a separate feature from the label

  7. laanwj commented at 11:44 am on November 26, 2019: member
    Maybe add ’label’ only if there’s a single label. Clients that don’t use multiple-label-per-address functionality will be unaffected, while there’s no need to guess what to return if there’s multiple when you are using this.
  8. jnewbery commented at 3:11 pm on November 26, 2019: member

    Concept ACK. Returning the same information in two places in the same RPC call is redundant and confusing (especially when we add support for multiple labels per address, as I believe has always been the plan).

    change labels to tags as a separate feature from the label

    Please no!

    Maybe add ’label’ only if there’s a single label.

    I think it adds unnecessary complexity to clients to have the structure of the return object change based on values.

    A few comments on the approach (same comments as for #17578):

    • squash the first two commits (RPC changes and test changes) to avoid breaking git bisect
    • add more text to the deprecated RPC help text (what the user should do to retain the old behaviour and when the old behaviour will be removed entirely)
    • move the deprecation test to rpc_deprecated.py
  9. laanwj referenced this in commit d8a66626d6 on Nov 26, 2019
  10. laanwj commented at 12:11 pm on November 27, 2019: member

    I think it adds unnecessary complexity to clients to have the structure of the return object change based on values.

    Could return it as "" then. That already happens if there’s no label so clients can cope with it.

    I would be the last to complain about breaking backwards compatibility for good reason, but I really think we’re going overboard here with RPC deprecation changes every release, but okay, I seem to be the only one with that opinion.

  11. laanwj commented at 12:28 pm on November 27, 2019: member

    whoops :blush: I think I’m confused here. joinmarket, for example, already switched to using labels in 2018

    0commit b52bc06acfead65c2b88ee4f64af44bc223656e8
1Author: chris-belcher <chris-belcher@users.noreply.github.com>
2Date:   Thu Sep 6 18:03:17 2018 +0100
3
4    Switch over to using labels instead of accounts
5    
6    Bitcoin Core 0.17 deprecates the accounts feature and replaces it with
7    labels. The RPC functions using accounts still work for use with older
8    version of Core.

    I thought it was something recently introduced, but it was added in 189e0ef33ec66f03abf85cfd4d0ede1a0c5c02d3 in the initial introduction of the labels API in 2016. I’m not exactly sure why a single “label” ever got added.

    • 0.16: did not have getaddressinfo at all, but validateaddress, with a account field (not label)
    • 189e0ef33ec66f03abf85cfd4d0ede1a0c5c02d3: labels was added
    • 109e05dcd163b8ddb7f4b3550db6b9ab833b2c04 : label was added
    • 0.17: getaddressinfo was introduced, with both label and labels already there

    My confusion came from that I thought there was an intermediate release with only label (but it’s even the other way around, labels was first!). Anyhow, no problem here. Concept ACK.

  12. sidhujag referenced this in commit 44df8dbe56 on Nov 27, 2019
  13. DrahtBot added the label Needs rebase on Dec 13, 2019
  14. jonatack force-pushed on Dec 13, 2019
  15. DrahtBot removed the label Needs rebase on Dec 13, 2019
  16. meshcollider referenced this in commit 7ea3b85ecf on Jan 7, 2020
  17. DrahtBot added the label Needs rebase on Jan 7, 2020
  18. sidhujag referenced this in commit 5c7fa94863 on Jan 8, 2020
  19. doc: address pr17578 review feedback 
    - https://github.com/bitcoin/bitcoin/pull/17578#discussion_r363975411
- https://github.com/bitcoin/bitcoin/pull/17578#discussion_r363969721
- https://github.com/bitcoin/bitcoin/pull/17578#discussion_r362703553
    c7654af6f8
  20. test: remove getaddressinfo label tests dc0cabeda4
  21. rpc: deprecate getaddressinfo label field d48875fa20
  22. test: getaddressinfo label deprecation test 72af93f364
  23. jonatack force-pushed on Jan 9, 2020
  24. jonatack commented at 5:18 pm on January 9, 2020: member
    Rebased and addressed the remaining nits in #17578.
  25. DrahtBot removed the label Needs rebase on Jan 9, 2020
  26. in doc/release-notes-17585.md:1 in 77b340e77a outdated 
    0@@ -0,0 +1,6 @@
1+Deprecated or removed RPCs
    jnewbery commented at 4:46 pm on January 10, 2020:

    This release note could be added to release-notes-17578.md and restructured to something like:

    0- The `getaddressinfo` return object has been changed:
1  - the `label` field has been deprecated and will be removed...
2  - the `labels` field now returns an array...

    I think that will be a bit easier to parse in the final release notes.

    The only reason we have separate release note files for different PRs is to avoid merge conflicts. That’s not a problem here because 17578 has already been merged, so it’s fine to just add to that file.

    jonatack commented at 12:38 pm on January 11, 2020:
    Thanks! Good idea; done.
  27. jnewbery commented at 5:00 pm on January 10, 2020: member

    Code review ACK 77b340e77aa6faa03feedb04f18bb9a7f7e9e8a3

    One minor doc nit, which you can ignore.

  28. jonatack force-pushed on Jan 11, 2020
  29. doc: update release notes with getaddressinfo label deprecation d3bc184081
  30. jonatack force-pushed on Jan 11, 2020
  31. jonatack commented at 12:47 pm on January 11, 2020: member
    Thanks for reviewing and the info/suggestion, @jnewbery! Updated the last release note commit.
  32. jnewbery commented at 10:14 pm on January 13, 2020: member
    ACK d3bc18408146e91b3836f72360ff6fa2420b6887
  33. laanwj commented at 12:29 pm on January 29, 2020: member
    ACK d3bc18408146e91b3836f72360ff6fa2420b6887
  34. meshcollider commented at 8:32 am on February 2, 2020: contributor
    utACK d3bc18408146e91b3836f72360ff6fa2420b6887
  35. pull[bot] referenced this in commit 6d0e532ae0 on Feb 2, 2020
  36. meshcollider merged this on Feb 2, 2020
  37. meshcollider closed this on Feb 2, 2020
  38. jonatack deleted the branch on Feb 2, 2020
  39. sidhujag referenced this in commit 97c86196db on Feb 3, 2020
  40. meshcollider referenced this in commit 02b26ba1c1 on Jun 21, 2020
  41. sidhujag referenced this in commit 0039aa5c7a on Jul 7, 2020
  42. sidhujag referenced this in commit c81cfe7f9e on Nov 10, 2020
  43. sidhujag referenced this in commit ae30b1a10d on Nov 10, 2020
  44. sidhujag referenced this in commit 86863f0148 on Nov 10, 2020
  45. deadalnix referenced this in commit 0eed7c672c on Dec 18, 2020
  46. Fabcien referenced this in commit 9623f4e186 on Dec 18, 2020
  47. Fabcien referenced this in commit 5dadedebd9 on Dec 19, 2020
  48. deadalnix referenced this in commit 58c2ace496 on Dec 19, 2020
  49. deadalnix referenced this in commit 43a25ae628 on Dec 19, 2020
  50. DrahtBot locked this on Feb 15, 2022


jonatack DrahtBot laanwj jnewbery meshcollider

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