rpc: improve getaddressinfo test coverage, help, code docs

jonatack commented at 5:03 pm on October 28, 2019: member

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.

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

DrahtBot commented at 8:03 pm on October 28, 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:

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

in test/functional/test_framework/wallet_util.py:91 in d73709a9de outdated

87@@ -88,6 +88,11 @@ def get_multisig(node):
88                     p2sh_p2wsh_script=CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),
89                     p2sh_p2wsh_addr=script_to_p2sh_p2wsh(script_code))
90 
91+def labels_value(name="", purpose="receive"):

jachiang commented at 8:28 am on October 29, 2019:

Question: Would it make sense for the labels_value method to indicate that it is only for the single member labels case (since getaddressinfo may support multiple labels in the future)?

jonatack commented at 8:33 am on October 29, 2019:

Thanks @jachiang. I thought I’d update it if/when adding support for multiple labels, though happy to take a good suggestion now for a better name or docstring.

jachiang commented at 8:41 am on October 29, 2019:

You’re right, a docstring or inline comment would have probably sufficient for sb who is just looking at the functional test, and not the rpc code (where labels vector is actually documented). Happy to wait and see what other reviewers think :)

jonatack commented at 10:19 am on October 29, 2019:

The rpc code doc seems good, but explaining it in the help might be a good idea. Thanks, might update that. Thanks for the review!

jonatack commented at 6:44 pm on October 29, 2019:

Updated the code doc and help with more details.

jachiang approved

jachiang commented at 8:36 am on October 29, 2019: contributor

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

DrahtBot added the label Needs rebase on Oct 29, 2019

in src/wallet/rpcwallet.cpp:3719 in 11176290b3 outdated

3665                 {
3666                     {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to get the information of."},
3667                 },
3668                 RPCResult{
3669             "{\n"
3670-            "  \"address\" : \"address\",        (string) The bitcoin address validated\n"

fjahr commented at 2:11 pm on October 29, 2019:

There seems to be just some white space formatting in the commit on the following lines. Since they are changed in the next commit again I would combine these commits.

jonatack commented at 6:41 pm on October 29, 2019:

Yes, the first commit is limited to formatting fixes only, to keep it separate from the content fixes, and the specific label/labels content changes. It was more work to keep them separate but hopefully clearer for reviewing, and in case of requested edits or dropping any unwanted commits. I named the commits accordingly.

in test/functional/wallet_basic.py:50 in 2c41df31b7 outdated

49@@ -47,9 +50,6 @@ def check_fee_amount(self, curr_balance, balance_with_fee, fee_per_byte, tx_size
50     def get_vsize(self, txn):
51         return self.nodes[0].decoderawtransaction(txn)['vsize']
52 
53-    def labels_value(self, name="", purpose="receive"):

fjahr commented at 4:38 pm on October 29, 2019:

Would combine this commit with bb87d8aac71dc87d1f11c26bd97fa6dc7b650a75, as you are mostly touching LoC that you touched in the that one as well.

jonatack commented at 6:48 pm on October 29, 2019:

I made a conscious effort to keep the logical changes separate, in case any changes were not wanted, for example if extracting this function to wallet_util.py was seen as unneeded abstraction.

in test/functional/wallet_importmulti.py:122 in d73709a9de outdated

118@@ -119,10 +119,12 @@ def run_test(self):
119         # ScriptPubKey + internal + label
120         self.log.info("Should not allow a label to be specified when internal is true")
121         key = get_key(self.nodes[0])
122+        label = "Unsuccessful labelling for internal addresses"

fjahr commented at 4:40 pm on October 29, 2019:

I think this one is touching the same lines as 3dcf1481cefae6ff2d585181578004045c3e3a1c, so I would combine these commits as well.

jonatack commented at 6:54 pm on October 29, 2019:

The main change of this commit is extending testing of the getaddressinfo labels response to the test_importmulti functions. I can try to move the renamings to the previous commit.

jonatack commented at 9:59 am on October 30, 2019:

@fjahr I squashed the 4 commits adding RPC getaddressinfo “labels” test coverage and improving the existing “label” tests down to a single commit at e123923e630c051f676501aab9454ecbf3bd3956. Can you please re-ACK?

fjahr approved

fjahr commented at 4:46 pm on October 29, 2019: member

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 force-pushed on Oct 29, 2019

jonatack commented at 7:06 pm on October 29, 2019: member

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 commented at 7:14 pm on October 29, 2019: member

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

DrahtBot removed the label Needs rebase on Oct 29, 2019

jonatack force-pushed on Oct 30, 2019

fjahr commented at 10:37 am on October 30, 2019: member

Code review ACK e123923e630c051f676501aab9454ecbf3bd3956

in src/wallet/rpcwallet.cpp:3746 in 235417d61f outdated

3708@@ -3709,15 +3709,17 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3709             "                                                         getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath,\n"
3710             "                                                         hdseedid) and relation to the wallet (ismine, iswatchonly).\n"
3711             "  \"iscompressed\" : true|false,        (boolean, optional) If the pubkey is compressed.\n"
3712-            "  \"label\" :  \"label\"                  (string) The label associated with the address. Defaults to \"\".\n"
3713+            "  \"label\" :  \"label\"                  (string) The label associated with the address. Defaults to \"\". Equivalent to the name field in the labels array\n"
3714+            "                                               below. This label field may be deprecated in the future; prefer consuming the labels array below instead.\n"

ariard commented at 5:25 pm on October 30, 2019:

Not sure about this, but deprecation shouldn’t be spread among multiples version (instead of multiple PRs inside same version) ?

jonatack commented at 5:49 pm on October 30, 2019:

Yes. This is merely a recommandation in the help as a helpful guide for application developers confused whether to use label and labels.

luke-jr commented at 7:24 pm on November 8, 2019:

It seems unclear if multiple labels makes sense, so I don’t think we’re ready to deprecate this yet.

(Multiple tags does make sense, but “label” has historically been a description for the specific tx)

jonatack commented at 11:35 am on November 23, 2019:

Removed the line about deprecation.

0-"\"label\" :  \"label\"   (string) The label associated with the address. Defaults to \"\". Equivalent to the name field in the labels array\n"
1-"                              below. This label field may be deprecated in the future; prefer consuming the labels array below instead.\n"
2+"\"label\" :  \"label\"   (string) The label associated with the address. Defaults to \"\". Equivalent to the name field in the labels array.\n"

ariard approved

ariard commented at 5:34 pm on October 30, 2019: member

Code review and tested ACK e123923

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

jachiang commented at 2:16 pm on October 31, 2019: contributor

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

in src/wallet/rpcwallet.cpp:3739 in 1d6884d4d4 outdated

3735@@ -3736,32 +3736,56 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3736     UniValue ret(UniValue::VOBJ);
3737     CTxDestination dest = DecodeDestination(request.params[0].get_str());
3738 
3739-    // Make sure the destination is valid
3740+    // Ensure the destination is valid.

promag commented at 11:41 pm on November 1, 2019:

1d6884d4d4b541d0c04ef10da208ef657122aa1f

The code below is clear and simple, why add the obvious comment?

jonatack commented at 9:46 am on November 7, 2019:

Removed.

in src/wallet/rpcwallet.cpp:3744 in 1d6884d4d4 outdated

3740+    // Ensure the destination is valid.
3741     if (!IsValidDestination(dest)) {
3742         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
3743     }
3744 
3745+    // Return address field.

promag commented at 11:41 pm on November 1, 2019:

1d6884d4d4b541d0c04ef10da208ef657122aa1f

IMO these “return foo field” comments are useless.

jonatack commented at 10:40 am on November 2, 2019:

Thank you for reviewing, @promag. Some of the comments add info that isn’t obvious and the others are for consistency and separation. I propose to keep the 3 ACKs here and remove the too-obvious comments in the follow-up (to add multiple labels / deprecate the label field).

jonatack commented at 9:46 am on November 7, 2019:

Had to rebase, so removed the obvious comments.

in src/wallet/rpcwallet.cpp:3825 in 1d6884d4d4 outdated

3823     if (mi != pwallet->mapAddressBook.end()) {
3824         labels.push_back(AddressBookDataToJSON(mi->second, true));
3825     }
3826     ret.pushKV("labels", std::move(labels));
3827 
3828+    // Return final response.

promag commented at 11:42 pm on November 1, 2019:

1d6884d4d4b541d0c04ef10da208ef657122aa1f

Pretty obvious too?

jonatack commented at 9:46 am on November 7, 2019:

Removed

promag commented at 11:44 pm on November 1, 2019: member

Overall looks good, only read the change so far.

jonatack commented at 10:41 am on November 2, 2019: member

Thank you @jachiang and @promag for reviewing.

michaelfolkson commented at 2:02 pm on November 3, 2019: contributor

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

DrahtBot added the label Needs rebase on Nov 4, 2019

jonatack force-pushed on Nov 5, 2019

jonatack commented at 9:25 am on November 5, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Nov 5, 2019

jonatack commented at 12:55 pm on November 6, 2019: member

appveyor failures are unrelated:

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

ryanofsky commented at 1:41 pm on November 6, 2019: member

appveyor failures are unrelated:

Appveyor error is apparently fixed by #17384 (comment)

DrahtBot added the label Needs rebase on Nov 6, 2019

jonatack force-pushed on Nov 7, 2019

jonatack commented at 9:49 am on November 7, 2019: member

Rebased and incorporated @promag’s feedback.

DrahtBot removed the label Needs rebase on Nov 7, 2019

jnewbery commented at 5:42 pm on November 15, 2019: member

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:

https://github.com/bitcoin/bitcoin/blob/21ee676dd6a7d9704367b6412bf8e1e443ec2b5b/src/wallet/rpcwallet.cpp#L3672

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.

DrahtBot added the label Needs rebase on Nov 22, 2019

jonatack force-pushed on Nov 23, 2019

jonatack commented at 12:00 pm on November 23, 2019: member

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.

DrahtBot removed the label Needs rebase on Nov 23, 2019

rpc: improve getaddressinfo RPCHelpman formatting 70cda342cd

jonatack force-pushed on Nov 24, 2019

jonatack commented at 5:27 pm on November 24, 2019: member

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

rpc: improve getaddressinfo RPCHelpman content 5a0ed85070

rpc: clarify label vs labels in getaddressinfo RPCHelpman 8d1ed0c263

rpc: update getaddressinfo RPCExamples to bech32 2ee0cb3330

rpc: add getaddressinfo code documentation

and separate the fields with a line break for readability.

1388de8390

test: add listlabels test in wallet_labels.py 0f3539ac6d

test: add rpc getaddressinfo labels test coverage 33f5fc32e5

jonatack force-pushed on Nov 24, 2019

jonatack commented at 2:00 am on November 25, 2019: member

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

fjahr commented at 3:16 pm on November 25, 2019: member

Re-ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151

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

in src/wallet/rpcwallet.cpp:3797 in 33f5fc32e5

3792     }
3793+
3794     ret.pushKV("iswatchonly", bool(mine & ISMINE_WATCH_ONLY));
3795+
3796+    // Return DescribeWalletAddress fields.
3797+    // Always returned: isscript, ischange, iswitness.

jnewbery commented at 2:40 pm on November 26, 2019:

nit (please don’t invalidate review ACKs to change this): this commenting would be better placed next to the DescribeWalletAddress function or DescribeWalletAddressVisitor class, since it’s very possible that the key-values returned will change in future, and keeping comments close to the code it’s describing reduce the chances of them going stale.

jonatack commented at 3:03 pm on November 26, 2019:

Good idea – will update #17578 with this.

jonatack commented at 9:23 am on December 2, 2019:

Removed.

in src/wallet/rpcwallet.cpp:3812 in 33f5fc32e5

3807         ret.pushKV("label", pwallet->mapAddressBook[dest].name);
3808     }
3809+
3810     ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
3811 
3812+    // Fetch KeyMetadata, if present, for the timestamp, hdkeypath, hdseedid,

jnewbery commented at 2:42 pm on November 26, 2019:

nit: Seems redundant. The code below is very easy to read.

jonatack commented at 3:04 pm on November 26, 2019:

ACK, will update #17578 with this.

jonatack commented at 9:23 am on December 2, 2019:

Removed in #17578.

in src/wallet/rpcwallet.cpp:3761 in 33f5fc32e5

3788             "}\n"
3789                 },
3790                 RPCExamples{
3791-                    HelpExampleCli("getaddressinfo", "\"1PSSGeFHDnKNxiEyFrD1wcEaHr9hrQDDWc\"")
3792-            + HelpExampleRpc("getaddressinfo", "\"1PSSGeFHDnKNxiEyFrD1wcEaHr9hrQDDWc\"")
3793+                    HelpExampleCli("getaddressinfo", "\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\"") +

jnewbery commented at 2:48 pm on November 26, 2019:

Is this intentionally an invalid address? The previous example was a valid address, which I think is useful since users can test the RPC and see a valid return object by using the example

jonatack commented at 3:06 pm on November 26, 2019:

Yes, I noticed addresses with a missing character in other RPC examples and thought it might be the norm. Will update #17578.

jonatack commented at 9:24 am on December 2, 2019:

Added a valid address in #17578.

jnewbery commented at 2:51 pm on November 26, 2019: member

ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151.

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

laanwj referenced this in commit d8a66626d6 on Nov 26, 2019

laanwj merged this on Nov 26, 2019

laanwj closed this on Nov 26, 2019

sidhujag referenced this in commit 44df8dbe56 on Nov 27, 2019

sidhujag commented at 6:23 pm on November 28, 2019: none

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 deleted the branch on Nov 28, 2019

jonatack commented at 8:55 pm on November 28, 2019: member

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.

meshcollider referenced this in commit 7ea3b85ecf on Jan 7, 2020

sidhujag referenced this in commit 5c7fa94863 on Jan 8, 2020

pull[bot] referenced this in commit 6d0e532ae0 on Feb 2, 2020

sidhujag referenced this in commit 97c86196db on Feb 3, 2020

jasonbcox referenced this in commit cc5951bf2a on Oct 1, 2020

jasonbcox referenced this in commit b615bbcd0a on Oct 1, 2020

sidhujag referenced this in commit c81cfe7f9e on Nov 10, 2020

sidhujag referenced this in commit ae30b1a10d on Nov 10, 2020

sidhujag referenced this in commit 86863f0148 on Nov 10, 2020

DrahtBot locked this on Dec 16, 2021

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

Conflicts