rpc: simplify getaddressinfo labels, deprecate previous behavior

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

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

0  "labels": [
1    {
2      "name": "DOUBLE SPEND",
3      "purpose": "receive"
4    }

after

0  "labels": [
1    "DOUBLE SPEND"
2  ]

The deprecated behavior can be re-enabled by starting bitcoind with -deprecatedrpc=labelspurpose.

For context, see:

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.

DrahtBot added the label Docs on Nov 24, 2019

DrahtBot added the label RPC/REST/ZMQ on Nov 24, 2019

DrahtBot added the label Tests on Nov 24, 2019

DrahtBot added the label Wallet on Nov 24, 2019

fanquake removed the label Docs on Nov 24, 2019

fanquake removed the label Tests on Nov 24, 2019

fanquake removed the label Wallet on Nov 24, 2019

DrahtBot commented at 11:54 pm on November 24, 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:

#17809 (rpc: Auto-format RPCResult by MarcoFalke)
#17804 (doc: Misc RPC help fixes by MarcoFalke)
#17585 (rpc: deprecate getaddressinfo label by jonatack)
#17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)

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.

jonatack force-pushed on Nov 24, 2019

jonatack force-pushed on Nov 25, 2019

in src/wallet/rpcwallet.cpp:3755 in b2e5ca1759 outdated

3778+            "  \"labels\"                            (object) An array of labels associated with the address. Currently limited to one label but returned\n"
3779+            "                                               as an array to keep the API stable if multiple labels are enabled in the future.\n"
3780             "    [\n"
3781+            "      \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n"
3782+            "      ,...\n"
3783+            "    DEPRECATED:\n"

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

Since you’ve named v0.21 explicitly in the release notes, you could also do so here, and also advise users how to re-enabled the deprecated behaviour (by using -deprecatedrpc=labelspurpose)

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

Done.

in src/wallet/rpcwallet.cpp:3844 in b2e5ca1759 outdated

3843+        if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
3844+            labels.push_back(AddressBookDataToJSON(mi->second, true));
3845+        } else {
3846+            labels.push_back(mi->second.name);
3847+        }
3848+        ret.pushKV("labels", std::move(labels));

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

Moving this inside the if block is a behaviour change (previously an empty array would be added to the return object if the address had no labels)

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

I didn’t see an actual change in behavior by putting this line inside the conditional, but I’ll go with being prudent as you suggest. Done.

in test/functional/test_runner.py:215 in b2e5ca1759 outdated

210@@ -211,6 +211,7 @@
211     'p2p_permissions.py',
212     'feature_blocksdir.py',
213     'feature_config_args.py',
214+    'rpc_getaddressinfo_labels_purpose_deprecation.py',

jnewbery commented at 3:00 pm on November 26, 2019:

I think it’s better to just add the test to the rpc_deprecated.py test file.

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

I think it’s better to just add the test to the rpc_deprecated.py test file.

I started to, then quickly saw that this would completely change that file (here we are not testing a removed RPC but a change in a returned value), and restoring this test file after 0.20 is released would be more complicated than deleting a test file.

jnewbery commented at 3:00 pm on November 26, 2019: member

Concept ACK. A few comments inline.

jnewbery commented at 3:04 pm on November 26, 2019: member

I also think you need to squash pc: simplify getaddressinfo labels, deprecate previous behavior and test: update getaddressinfo labels tests commits. I think the intermediate commit with the RPC changes but not the test changes breaks git bisect.

laanwj referenced this in commit d8a66626d6 on Nov 26, 2019

jnewbery commented at 4:34 pm on November 26, 2019: member

rebase please!

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

jonatack force-pushed on Dec 2, 2019

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

I also think you need to squash pc: simplify getaddressinfo labels, deprecate previous behavior and test: update getaddressinfo labels tests commits. I think the intermediate commit with the RPC changes but not the test changes breaks git bisect.

Done.

Thanks @jnewbery for reviewing! I took all your all suggestions except merging the deprecation test into rpc_deprecated.py (per my comment above), added a commit incorporating your review feedback from #17283, and rebased. I also beefed up the deprecation test a bit and moved the release note to its own file rather than modifying release-notes.md directly.

in test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py:49 in 583be1e156 outdated

44+            self.test_labels(node_num=1, label_name=label, expected_value=expected)
45+
46+    def run_test(self):
47+        """Test getaddressinfo labels with and without -deprecatedrpc flag."""
48+        self.test_labels_with_deprecatedrpc_flag()
49+        self.test_labels_without_deprecatedrpc_flag()

jnewbery commented at 5:46 pm on December 2, 2019:

Nit: I think this would be clearer as just a single run_test() function, since these helper functions are only used in one place and they’re going to be deleted in v0.21 anyway.

jonatack commented at 12:32 pm on December 6, 2019:

Done.

jnewbery commented at 5:51 pm on December 2, 2019: member

utACK 583be1e156343f1e7c734fcb9d04db03e96f4cac

One style nit in the test. Feel free to take or ignore.

weerayut3 approved

jonatack force-pushed on Dec 6, 2019

jonatack commented at 12:33 pm on December 6, 2019: member

Thanks for reviewing @jnewbery. Addressed the style nit and rebased.

jnewbery commented at 8:16 pm on December 11, 2019: member

ACK 2e06da5d62e5b3563001d73b62e90a5a61b47993

DrahtBot added the label Needs rebase on Dec 13, 2019

jonatack force-pushed on Dec 13, 2019

jonatack commented at 1:25 pm on December 13, 2019: member

Rebased.

jnewbery commented at 2:08 pm on December 13, 2019: member

ACK 55dfbc80c37296d6cab736ab267d887840b22367

Only change is rebasing on master.

DrahtBot removed the label Needs rebase on Dec 13, 2019

instagibbs commented at 3:38 pm on December 13, 2019: member

What’s the best argument in favor of the “purpose” field? Aside from it being “strange” I didn’t see any discussion really on the links? I’d like to know why it was there before removing it.

jnewbery commented at 4:06 pm on December 13, 2019: member

“purpose” was only exposed in this RPC in a recent release, merged in #12892 (originally from the commit here: https://github.com/bitcoin/bitcoin/pull/7729/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3828).

I expect no-one is using it.

instagibbs commented at 4:07 pm on December 13, 2019: member

@jnewbery Yes I read that particular fact in the OP. Maybe just saying “why it’s weird” is enough, I just have no preconception here.

fjahr commented at 1:26 am on December 17, 2019: member

ACK 55dfbc80c37296d6cab736ab267d887840b22367

Reviewed code, ran tests and tested behavior manually.

in src/wallet/rpcwallet.cpp:3711 in 7f79778dcd outdated

3707@@ -3708,6 +3708,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3708         return NullUniValue;
3709     }
3710 
3711+    const std::string example_address = "\"bc1qz0wjhhvtlzqec26ftsrwpkwyevjdm7apwctfvx\"";

MarcoFalke commented at 9:59 pm on December 27, 2019:

I liked that the address was invalid, so it is harder to accidentally send funds to it

jonatack commented at 10:28 pm on December 27, 2019:

I’ll change it back and add a mention in the developer notes rpc section then :smile:

jonatack commented at 10:33 pm on December 27, 2019:

Though I agree with @jnewbery that it’s nicer when the rpc examples actually work when copy-pasted. It’s a trade-off, to be sure.

MarcoFalke commented at 0:36 am on December 28, 2019:

It does work. It is an example for an invalid address ;)

jonatack commented at 8:02 pm on December 28, 2019:

Done!

jnewbery commented at 5:48 pm on January 2, 2020:

so it is harder to accidentally send funds to it

Huh? This is the help text for getaddressinfo(). It doesn’t send funds.

This help text originally showed a valid address, then was changed to invalid in 17283 (https://github.com/bitcoin/bitcoin/pull/17283#discussion_r350784833), and has now been changed to valid again and invalid again. I don’t think it matters that much, but can we just pick one and stick to it?

Also agree that the developer notes should be updated to stop bikeshedding around this.

rpc: incorporate review feedback from PR 17283

- (reverted after follow-on review by maintainers: provide a valid address in getaddressinfo RPCExample)
- remove unneeded code comments

7851f14ccf

jonatack force-pushed on Dec 28, 2019

jonatack commented at 8:04 pm on December 28, 2019: member

Changed back to using an invalid address for the RPCExamples and rebased.

jnewbery commented at 5:58 pm on January 2, 2020: member

Code review ACK 4752a2a37f63063656e5e0ef02bb861c605c2931

in doc/release-notes-17578.md:5 in 4752a2a37f outdated

0@@ -0,0 +1,8 @@
1+Deprecated or removed RPCs
2+--------------------------
3+
4+- The `getaddressinfo` RPC `labels` field now returns an array of label name
5+  strings. Previously, it returned an array of JSON hashes containing `name` and

luke-jr commented at 4:25 am on January 3, 2020:

JSON has Objects, not hashes.

jonatack commented at 6:52 pm on January 3, 2020:

done

rpc: simplify getaddressinfo labels, deprecate previous behavior

- change the value returned in the RPC getaddressinfo `labels` field to an array
  of label name strings

- deprecate the previous behavior of returning a JSON hash structure containing
  label `name` and address `purpose` key/value pairs

- update the relevant tests

60aba1f2f1

test: getaddressinfo labels purpose deprecation test 8bb405bbad

doc: update release notes

Update the release notes regarding the change in rpc getaddressinfo `labels`.

8925df86c4

jonatack force-pushed on Jan 3, 2020

jonatack commented at 6:58 pm on January 3, 2020: member

Doc-only changes after @luke-jr’s review:

0doc/release-notes-17578.md
1-  strings. Previously, it returned an array of JSON hashes containing `name` and
2+  strings. Previously, it returned an array of JSON objects containing `name` and

0src/wallet/rpcwallet.cpp
1-    // value of the name key/value pair in the labels hash array below.
2+    // value of the name key/value pair in the labels array below.

@luke-jr, @fjahr, @jnewbery, @MarcoFalke, @weerayut3, @instagibbs, would you mind ACKing (or re-ACKing)?

jnewbery commented at 7:03 pm on January 3, 2020: member

reACK 8925df8

promag commented at 7:18 pm on January 3, 2020: member

I thought the only change in tests should be to enable labelspurpose? Then these test changes would be done when the deprecated behavior is removed.

jonatack commented at 7:21 pm on January 3, 2020: member

@promag The labelspurpose deprecation itself is tested in 8bb405bbadf11391ccba7b334b4cfe66dc85b390. That test can be deleted after the 0.21 release. I think it’s good to test both sides of the change in this PR, but particularly the new default state.

in doc/release-notes-17578.md:7 in 8925df86c4

0@@ -0,0 +1,8 @@
1+Deprecated or removed RPCs
2+--------------------------
3+
4+- The `getaddressinfo` RPC `labels` field now returns an array of label name
5+  strings. Previously, it returned an array of JSON objects containing `name` and
6+  `purpose` key/value pairs, which is now deprecated and will be removed in
7+  0.21. To re-enable the previous behavior, launch bitcoind with

promag commented at 7:49 pm on January 3, 2020:

nit, just launch with -deprecatedrpc....

jonatack commented at 7:37 am on January 5, 2020:

Thanks; will remove “bitcoind” here if need to retouch this PR, otherwise will change it in one of the follow-on PRs like #17585 or in the wiki.

promag commented at 6:59 pm on January 7, 2020:

Sure, just to clarify, I’d drop bitcoind because the argument also applies to bitcoin-qt.

jonatack commented at 10:08 pm on January 7, 2020:

Yes, agreed.

jonatack commented at 5:17 pm on January 9, 2020:

Done in #17585

promag commented at 7:50 pm on January 3, 2020: member

Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1.

in src/wallet/rpcwallet.cpp:3829 in 60aba1f2f1 outdated

3829+    // equivalent to the `label` field above. Currently only one label can be
3830+    // associated with an address, but we return an array so the API remains
3831+    // stable if we allow multiple labels to be associated with an address in
3832+    // the future.
3833+    //
3834+    // DEPRECATED: The previous behavior of returning an array containing a JSON

meshcollider commented at 9:54 pm on January 7, 2020:

minor nit: I would move this comment down to the line below with the actual deprecation check

jonatack commented at 5:17 pm on January 9, 2020:

thanks; done in #17585

meshcollider commented at 10:22 pm on January 7, 2020: contributor

Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1

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

meshcollider merged this on Jan 7, 2020

meshcollider closed this on Jan 7, 2020

jonatack deleted the branch on Jan 8, 2020

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

jonatack referenced this in commit c7654af6f8 on Jan 9, 2020

MarcoFalke referenced this in commit 218274de7d on Jan 16, 2020

sidhujag referenced this in commit e55ad7796e on Jan 16, 2020

meshcollider referenced this in commit 6d0e532ae0 on Feb 2, 2020

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

HashUnlimited referenced this in commit bf9aaa706e on Apr 17, 2020

meshcollider referenced this in commit 02b26ba1c1 on Jun 21, 2020

sidhujag referenced this in commit 0039aa5c7a on Jul 7, 2020

jasonbcox referenced this in commit b615bbcd0a on Oct 1, 2020

van-orton referenced this in commit 0daa05e4dc on Oct 30, 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 4479086628 on Nov 10, 2020

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

deadalnix referenced this in commit 0eed7c672c on Dec 18, 2020

backpacker69 referenced this in commit da6921fe0d on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

rpc: simplify getaddressinfo labels, deprecate previous behavior #17578

Conflicts