rpc: improve getaddressinfo test coverage, help, code docs - #17283

jonatack · 2019-10-28T17:03:44Z

This PR is a continuation of the work in #12892.

Main motivations:

There is currently no test coverage for the getaddressinfo labels response. Coverage here is a prerequisite before deprecating the label response or adding multiple labels per address.
bitcoin-cli help getaddressinfo returns a few content errors, difficult-to-read formatting, and no explanation why it returns both label and labels and how they relate, which can be confusing for application developers.

Changes by order of commits:

improve/fix getaddressinfo RPCHelpman layout formatting
improve/fix getaddressinfo RPCHelpman content
clarify the label and labels fields in getaddressinfo RPCHelpman
update getaddressinfo RPCExamples addresses to bech32
add getaddressinfo code docs
add a listlabels test assertion in wallet_labels.py
add missing getaddressinfo labels test coverage and improve the existing label tests

Here are gists of the CLI help output:
bitcoin-cli help getaddressinfo before this PR
bitcoin-cli help getaddressinfo after this PR

It seems we ought to begin a deprecation process for the getaddressinfo label field? If yes, I have a follow-up ready. --> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR.

DrahtBot · 2019-10-28T20:03:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)

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.

jachiang

tACK d73709a9decdb34562de9c847eee786e6a256a82
Thanks @jonatack.
I reviewed diffs, ran passing tests.

fjahr

Code review ACK d73709a

Good cleanup. I would just recommend compressing the commits as indicated in my comments. I felt like having to review the same lines multiple times as I went through the commits and combining them will cut down on time needed for reviewing.

jonatack

Thank you @jachiang and @fjahr for your reviews. Rebased and added more detail to the label and labels help in 235417d and code docs in 6b20250.

jonatack · 2019-10-29T19:14:47Z

Updated the gist of the CLI help output after this PR (same urls as before):
bitcoin-cli help getaddressinfo before this PR
bitcoin-cli help getaddressinfo after this PR

fjahr · 2019-10-30T10:37:57Z

Code review ACK e123923e630c051f676501aab9454ecbf3bd3956

ariard

Code review and tested ACK e123923

IMO not sure it's worth it splitting getaddressinfo content and formatting change in diff commits.

jachiang · 2019-10-31T14:16:14Z

Code review ACK e123923e630c051f676501aab9454ecbf3bd3956

I find the clarification on Label vs. Labels in comments/help and the newly added test coverage of Labels helpful for anybody interfacing with getaddressinfo. Use Labels :)

promag

Overall looks good, only read the change so far.

jonatack · 2019-11-02T10:41:46Z

Thank you @jachiang and @promag for reviewing.

michaelfolkson · 2019-11-03T14:02:14Z

tACK e123923e630c051f676501aab9454ecbf3bd3956. CLI help output as described. Looking forward to reviewing the deprecation of label PR ;)

jonatack · 2019-11-05T09:25:23Z

Rebased.

jonatack · 2019-11-06T12:55:20Z

appveyor failures are unrelated:

bech32_tests.obj : error LNK2001: unresolved external symbol "bool __cdecl CaseInsensitiveEqual

ryanofsky · 2019-11-06T13:41:07Z

appveyor failures are unrelated:

Appveyor error is apparently fixed by #17384 (comment)

jonatack · 2019-11-07T09:49:03Z

Rebased and incorporated @promag's feedback.

jnewbery · 2019-11-15T17:42:13Z

Concept ACK. Thanks Jon.

I find the purpose field in the labels return object very strange, and think that we should probably remove it before encouraging people to rely on labels. The code to add the purpose field:

bitcoin/src/wallet/rpcwallet.cpp

Line 3672 in 21ee676

ret.pushKV("purpose", data.purpose);

was added in #12892 (from code originally in #7729). Prior to that, the purpose field of the CAddressBookData object wasn't exposed in the rpc.

My preference would be to:

make the label return object deprecated (only return it if bitcoind is started with a specific deprecatedrpc flag).
also make the purpose field of the labels object deprecated (only return it if bitcoind is start with the same deprecatedrpc flag) and instead return a flat array of label strings.
fully remove the label return value and purpose field in v0.21.

Some feedback on this PR:

I personally wouldn't reference twitter or mastodon in commit messages (since I expect the bitcoind git repo to last longer than those URLs). Also the justification in that commit log seems like an appeal to authority (this experienced developer found it confusing) rather than justifying the change on its own merit
in the last commit, no need to say that this is squashed. Once this is merged, then the git history will only include that commit. There's no need to say how that commit came into being.
no need to update dates in the copyright notices. That just increases the diff unnecessarily. We update them all automatically at the end of the year.
if you do agree with me and think that purpose should be removed, then there's not much point in adding the the labels_value() function in wallet_util.py.

jonatack · 2019-11-23T12:00:28Z

Thank you for the review, @jnewbery! I took your suggestions: removed mention of upcoming deprecation of the label field from the rpc help, updated the PR description and commit messages, and removed the copyright header updates.

I think this PR now does what is needed for the current situation while making minimum changes WRT existing review and is hopefully RFM.

if you do agree with me and think that purpose should be removed, then there's not much point in adding the the labels_value() function in wallet_util.py.

Since I don't know if it will be accepted, I'd like to propose the labels purpose deprecation in another PR. Idem for deprecating the label field and for adding multiple labels (tags) capability. --> EDIT: Done in #17578 and #17585.

jonatack · 2019-11-24T17:27:23Z

Deprecated getaddressinfo labels purpose in #17578 which builds on this PR.

and separate the fields with a line break for readability.

jonatack · 2019-11-25T02:00:08Z

Deprecated the getaddressinfo label field in #17585 which builds on this PR.

fjahr · 2019-11-25T15:16:18Z

Re-ACK 33f5fc3

Reviewed code changes. Build failure seems unrelated, would need a rebuild.

jnewbery

ACK 33f5fc3.

I've left some nits. please don't invalidate review now, but you could consider addressing them in your follow-up PR.

33f5fc3 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de8 rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed85 rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda34 rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in #12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07

sidhujag · 2019-11-28T18:23:28Z

This seemed to break CI / Trusty or at the very least, Trusty distribution seemed to not be able to be fetched from here on.

jonatack · 2019-11-28T20:55:04Z

This seemed to break CI / Trusty or at the very least, Trusty distribution seemed to not be able to be fetched from here on.

@sidhujag I think it's unrelated; see #17626.

… behavior 8925df8 doc: update release notes (Jon Atack) 8bb405b test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on #17283 (now merged) and is followed by #17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - #17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those. ACKs for top commit: jnewbery: reACK 8925df8 promag: Code review ACK 8925df8. meshcollider: Code review ACK 8925df8 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438

d3bc184 doc: update release notes with getaddressinfo label deprecation (Jon Atack) 72af93f test: getaddressinfo label deprecation test (Jon Atack) d48875f rpc: deprecate getaddressinfo label field (Jon Atack) dc0cabe test: remove getaddressinfo label tests (Jon Atack) c7654af doc: address pr17578 review feedback (Jon Atack) Pull request description: This PR builds on bitcoin#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 bitcoin#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. ACKs for top commit: jnewbery: ACK d3bc184 laanwj: ACK d3bc184 meshcollider: utACK d3bc184 Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d

…e docs Summary: 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539ac6d772fc646b5f184fa1efe77bf632f6a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de83900eaced906d369fe9e8887ae74b2dcf rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3330ccf70f0540cb42370796e32eff1569 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c263f8cdff7189f02040b5d02238d93da0 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed850700dfb19167d40b38f80313bd5e427ca rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda342cd20d0e0cd9f28405457544036968f2d rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in bitcoin/bitcoin#12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._ --- Depends on D7716 Backport of Core [[bitcoin/bitcoin#17283 | PR17283]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7718

…ous behavior Summary: 8925df86c4df16b1070343fef8e4d238f3cc3bd1 doc: update release notes (Jon Atack) 8bb405bbadf11391ccba7b334b4cfe66dc85b390 test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f2f11529add115d963d05599130288ae28 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14ccf2bcd1e9b2ad48e5e08881be06d9d21 rpc: incorporate review feedback from [[bitcoin/bitcoin#17283 | PR17283]] (Jon Atack) Pull request description: This PR builds on #17283 (now merged) and is followed by #17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - bitcoin/bitcoin#17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those. --- Depends on D7718 Backport of Core [[bitcoin/bitcoin#17578 | PR17578]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7724

fanquake added the RPC/REST/ZMQ label Oct 28, 2019

jachiang reviewed Oct 29, 2019

View reviewed changes

Comment thread test/functional/test_framework/wallet_util.py Outdated

jachiang approved these changes Oct 29, 2019

View reviewed changes

DrahtBot added the Needs rebase label Oct 29, 2019

fjahr approved these changes Oct 29, 2019

View reviewed changes

Comment thread src/wallet/rpcwallet.cpp Outdated

Comment thread test/functional/wallet_basic.py Outdated

Comment thread test/functional/wallet_importmulti.py Outdated

jonatack force-pushed the rpc-getaddressinfo-labels branch from d73709a to 90de8fe Compare October 29, 2019 19:04

jonatack commented Oct 29, 2019

View reviewed changes

Comment thread src/wallet/rpcwallet.cpp Outdated

Comment thread test/functional/test_framework/wallet_util.py Outdated

Comment thread test/functional/wallet_basic.py Outdated

Comment thread test/functional/wallet_importmulti.py Outdated

DrahtBot removed the Needs rebase label Oct 29, 2019

jonatack force-pushed the rpc-getaddressinfo-labels branch from 90de8fe to e123923 Compare October 30, 2019 09:55

ariard approved these changes Oct 30, 2019

View reviewed changes

Comment thread src/wallet/rpcwallet.cpp Outdated

promag reviewed Nov 1, 2019

View reviewed changes

Comment thread src/wallet/rpcwallet.cpp Outdated

Comment thread src/wallet/rpcwallet.cpp Outdated

Comment thread src/wallet/rpcwallet.cpp Outdated

DrahtBot added the Needs rebase label Nov 4, 2019

jonatack force-pushed the rpc-getaddressinfo-labels branch from e123923 to d22d7fe Compare November 5, 2019 09:18

DrahtBot removed the Needs rebase label Nov 5, 2019

DrahtBot added the Needs rebase label Nov 6, 2019

jonatack force-pushed the rpc-getaddressinfo-labels branch from d22d7fe to 8e1fbdb Compare November 7, 2019 09:42

DrahtBot removed the Needs rebase label Nov 7, 2019

DrahtBot added the Needs rebase label Nov 22, 2019

jonatack force-pushed the rpc-getaddressinfo-labels branch from 8e1fbdb to 505a4ae Compare November 23, 2019 11:25

DrahtBot removed the Needs rebase label Nov 23, 2019

rpc: improve getaddressinfo RPCHelpman formatting

70cda34

jonatack force-pushed the rpc-getaddressinfo-labels branch from 1d3a3ef to dfbd281 Compare November 24, 2019 15:20

jonatack mentioned this pull request Nov 24, 2019

rpc: simplify getaddressinfo labels, deprecate previous behavior #17578

Merged

jonatack added 6 commits November 24, 2019 23:05

rpc: improve getaddressinfo RPCHelpman content

5a0ed85

rpc: clarify label vs labels in getaddressinfo RPCHelpman

8d1ed0c

rpc: update getaddressinfo RPCExamples to bech32

2ee0cb3

rpc: add getaddressinfo code documentation

1388de8

and separate the fields with a line break for readability.

test: add listlabels test in wallet_labels.py

0f3539a

test: add rpc getaddressinfo labels test coverage

33f5fc3

jonatack force-pushed the rpc-getaddressinfo-labels branch from dfbd281 to 33f5fc3 Compare November 24, 2019 22:27

jonatack mentioned this pull request Nov 25, 2019

rpc: deprecate getaddressinfo label #17585

Merged

jnewbery reviewed Nov 26, 2019

View reviewed changes

Comment thread src/wallet/rpcwallet.cpp

Comment thread src/wallet/rpcwallet.cpp

Comment thread src/wallet/rpcwallet.cpp

laanwj merged commit 33f5fc3 into bitcoin:master Nov 26, 2019

jonatack deleted the rpc-getaddressinfo-labels branch November 28, 2019 20:53

theStack mentioned this pull request Feb 20, 2020

Change address format in RPCExamples from legacy to bech32 #18185

Closed

theStack mentioned this pull request Mar 25, 2020

lint: PR description linter #18399

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Uh oh!

Conversation

jonatack commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

jachiang left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Oct 29, 2019

Uh oh!

fjahr commented Oct 30, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jachiang commented Oct 31, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Nov 2, 2019

Uh oh!

michaelfolkson commented Nov 3, 2019

Uh oh!

jonatack commented Nov 5, 2019

Uh oh!

jonatack commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

jonatack commented Nov 7, 2019

Uh oh!

jnewbery commented Nov 15, 2019

Uh oh!

jonatack commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Nov 25, 2019

Uh oh!

fjahr commented Nov 25, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sidhujag commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Nov 28, 2019

Uh oh!

Reviewers

Assignees

jonatack commented Oct 28, 2019 •

edited

Loading

DrahtBot commented Oct 28, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack commented Nov 23, 2019 •

edited

Loading

jonatack commented Nov 24, 2019 •

edited

Loading

sidhujag commented Nov 28, 2019 •

edited

Loading