[wallet] [rpc] introduce 'label' API for wallet #12892

jnewbery · 2018-04-05T17:28:35Z

Add label API to wallet RPC.

This is one step towards #3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

These functions aren't DEPRECATED in the help
Help mentions 'label' instead of accounts. In the language used, labels are
associated with addresses, instead of addresses associated with labels. (unlike
with accounts.)
Labels have no balance
- No balances in listlabels
- listlabels has no minconf or watchonly argument
Like in the GUI, labels can be set on any address, not just receiving addreses
Unlike accounts, labels can be deleted.
Being unable to delete them is a common annoyance (see XMLRPC: setaccount moves label to the other, unused address #1231).
Currently only by reassigning all addresses using setlabel, but an explicit
call deletelabel which assigns all address to the default label may make
sense.

jnewbery · 2018-04-05T17:34:57Z

This is #7729 with the following changes:

rebased on master
don't mark getaddressinfo as deprecated (rpc: introduce 'label' API for wallet #7729 (comment))
used address instead of bitcoinaddress as the argument for several RPC methods (rpc: introduce 'label' API for wallet #7729 (comment))
remove destdata from RPC return objects (rpc: introduce 'label' API for wallet #7729 (comment))
remove the getlabel RPC method, now that we have getaddressinfo (rpc: introduce 'label' API for wallet #7729 (comment))
sort labels before returning in listlabels RPC (rpc: introduce 'label' API for wallet #7729 (comment))
add an optional force bool argument to getlabeladdress, defaulting to False. When set to false, getlabeladdress will fail with an error if called on a label that doesn't exist. When set to true getlabeladdress will create a new label and new label address if called on a label that doesn't exist. I originally wanted to always throw an error if the label didn't exist (https://github.com/bitcoin/bitcoin/pull/7729/files/fef41789e8d7293ccfd859bc90fa0a3b2484752d#diff-df7d84ff2f53fcb2a0dc15a3a51e55ce), but apparently some miners depend on being able to create labels/accounts this way (rpc: introduce 'label' API for wallet #7729 (comment))
getaddressesbylabel throws an error if called for a label that doesn't exist (rpc: introduce 'label' API for wallet #7729 (comment))
fixup python tests to work with slightly modified RPCs

@laanwj doesn't have time to maintain #7729. I'd like to try to get this merged before v0.17.

jnewbery · 2018-04-05T17:36:45Z

For full history of this PR, see #7729. Requesting review from reviewers of that PR: @Sjors , @MarcoFalke , @ryanofsky , @jonasschnelli , @instagibbs , @sipa , @luke-jr .

maflcko · 2018-04-05T17:39:55Z

src/wallet/rpcwallet.cpp

-            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\" \"tabby\"")
-            + HelpExampleRpc("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\", \"tabby\"")
+            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ\" \"tabby\"")
+            + HelpExampleRpc("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ\", \"tabby\"")


Unrelated change.

jonasschnelli · 2018-04-05T18:00:10Z

src/wallet/rpcwallet.cpp

        throw std::runtime_error(
            "getlabeladdress \"label\"\n"
-            "\nReturns the current Bitcoin address for receiving payments to this label.\n"
+            "\nReturns the current 'label address' for this label.\n"


Since this call gets even weirder, shouldn't we deprecate it in this PR?

Aparently some miners require this functionality (#7729 (comment)).

I'd personally prefer to deprecate/remove this RPC method, but in the interest of making progress on this, let's maintain the current functionality and reconsider deprecation later.

promag · 2018-04-05T20:45:36Z

src/wallet/rpcwallet.cpp

+            + HelpExampleRpc("getaddressesbylabel", "\"tabby\"")
+        );
+
+    LOCK2(cs_main, pwallet->cs_wallet);


Remove cs_main?

promag · 2018-04-05T20:49:18Z

src/wallet/rpcwallet.cpp

+
+    // Find all addresses that have the given label
+    std::vector<std::pair<std::string, UniValue>> addresses;
+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {


Future improvements:

move this to wallet function?

index address by label with a multi map?

I'll leave this functionality here for now. Can be refactored later if required.

promag · 2018-04-05T20:50:29Z

src/wallet/rpcwallet.cpp

+    std::vector<std::pair<std::string, UniValue>> addresses;
+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
+        if (item.second.name == label) {
+            addresses.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));


Could push to ret and avoid addresses?

promag · 2018-04-05T20:51:05Z

src/wallet/rpcwallet.cpp

+            + HelpExampleRpc("listlabels", "receive")
+        );
+
+    LOCK2(cs_main, pwallet->cs_wallet);


Remove cs_main?

promag · 2018-04-05T20:51:45Z

src/wallet/rpcwallet.cpp

+    LOCK2(cs_main, pwallet->cs_wallet);
+
+    std::string purpose;
+    if (request.params.size() > 0) {


!params[0].isNull()

promag · 2018-04-05T20:58:41Z

src/wallet/rpcwallet.cpp

+    std::list<std::string> label_list(label_set.begin(), label_set.end());
+    label_list.sort();
+    UniValue ret(UniValue::VARR);
+    for (const std::string &name : label_list) {


Nit space after &.

promag · 2018-04-05T21:27:09Z

src/wallet/rpcwallet.cpp

@@ -187,6 +187,11 @@ UniValue getnewaddress(const JSONRPCRequest& request)
    return EncodeDestination(dest);
 }

+void DeleteLabel(CWallet& wallet, std::string label)


Nit, move to CWallet::DeleteLabel(const std::string& label)? Or make it static for now?

Agree, if left unchanged this would be the only direct CWalletDB call from rpcwallet. Seems as though it should go through CWallet

Moved into CWallet

promag · 2018-04-05T21:28:06Z

src/wallet/rpcwallet.cpp

@@ -205,14 +210,15 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
        return NullUniValue;
    }

-    if (request.fHelp || request.params.size() != 1)
+    if (request.fHelp || request.params.size() > 2)
        throw std::runtime_error(
            "getlabeladdress \"label\"\n"


Missing ( force ).

promag · 2018-04-05T21:28:32Z

src/wallet/rpcwallet.cpp

@@ -205,14 +210,15 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
        return NullUniValue;
    }

-    if (request.fHelp || request.params.size() != 1)
+    if (request.fHelp || request.params.size() > 2)


Missing < 1.

promag · 2018-04-05T21:29:13Z

src/wallet/rpcwallet.cpp

@@ -224,6 +230,20 @@ UniValue getlabeladdress(const JSONRPCRequest& request)

    // Parse the label first so we don't generate a key if there's an error
    std::string label = LabelFromValue(request.params[0]);
+    bool force = false;
+    if (!request.params[1].isNull())


Nit, add {}.

jnewbery · 2018-04-05T21:36:31Z

Thanks for the review @promag . Will address comments tomorrow.

PierreRochard · 2018-04-06T15:36:58Z

src/wallet/rpcwallet.cpp

@@ -308,22 +328,24 @@ UniValue setlabel(const JSONRPCRequest& request)
    }

    std::string label;
-    if (!request.params[1].isNull())
+    if (!request.params[1].isNull()) {


It seems odd to me that label is marked as a required argument for setlabel but if it's not provided then we default to an empty string instead of throwing an error. If we want to keep the behavior then label should be marked as optional.

I've updated the behaviour to match the help text: the rpc call will fail if no label is provided.

jnewbery · 2018-04-06T16:54:22Z

Addressed review comments from @promag and @PierreRochard . Are you able to re-review?

PierreRochard · 2018-04-06T18:33:56Z

Yes, re-reviewing now

PierreRochard · 2018-04-06T21:47:21Z

A behavior difference between the old getaccountaddress and the new aliased getlabeladdress is causing the wallet_basic.py to fail. I was able to fix it by adding force to the test, but I think it's the endpoint that should be changed if the goal is to not break the API with the aliasing.

rpc_listtransactions.py and wallet_listreceivedby.py are also failing, it may be due to the same root cause but I won't be able to confirm until tomorrow.

PierreRochard · 2018-04-07T09:21:51Z

Fixing wallet_basic.py and rpc_listtransactions.py: If we want to continue supporting the behavior of the aliased getaccountaddress, force should default to true when that call is made, so something like bool force = request.strMethod == "getaccountaddress" ? true : false;

Fixing wallet_listreceivedby.py: line 143 should read self.nodes[1].getlabeladdress(label="mynewlabel", force=True)

jnewbery · 2018-04-07T16:43:04Z

Thanks for the fixes @PierreRochard! I've updated the tests as you suggested and force pushed.

PierreRochard · 2018-04-07T19:06:40Z

Tested ACK 1f7f1dfd1c16cf0c9a700e2b262ffa8f6c6559cd

laanwj · 2018-04-09T02:40:10Z

I'm still not convinced we really need getlabeladdress in the first place, but anyhow.
utACK 1f7f1df

maflcko · 2018-04-09T18:24:44Z

Needs rebase

jnewbery · 2018-04-09T19:22:16Z

Rebased.

I'm still not convinced we really need getlabeladdress

ok, that's three concept NACKs for getlabeladdress (me, @jonasschnelli #12892 (comment) and @laanwj #12892 (comment)).

I kept this call because there was a comment in the previous PR (#7729 (comment)) that this functionality was important for some miners and I wanted to avoid controversy to get this PR merged. However, if there's overwhelming opposition to getlabeladdress, it makes sense to remove it. getlabeladdress doesn't make much sense to me, and blurs the distinction that "labels are associated with addresses, instead of addresses associated with labels".

Other reviewers of this and the previous PR (@promag, @PierreRochard, @Sjors, @MarcoFalke , @ryanofsky , @instagibbs , @sipa , @luke-jr) - please can you provide concept input into whether we should just remove getlabeladdress?

PierreRochard · 2018-04-09T19:58:03Z

@jnewbery I agree that getlabeladdress should be removed, it has too much overlap with getnewaddress and miner-specific use cases are likely best implemented in the mining software (this suggestion seems reasonable: #7729 (comment))

ryanofsky · 2018-04-09T20:02:41Z

Other reviewers of this and the previous PR (@promag, @PierreRochard, @Sjors, @MarcoFalke , @ryanofsky , @instagibbs , @sipa , @luke-jr) - please can you provide concept input into whether we should just remove getlabeladdress?

I think it would be better to keep this PR as focused as possible. The goal should just be to fill in the last missing bits of the label implementation, so it is easy to switch from accounts to labels and we can get rid of broken "account balance" code and code that tries to track what account a transaction is sent from. I don't think it makes sense to introduce additional differences between accounts and labels that will make migration more difficult than necessary.

The main problem I see with getlabeladdress is just the documentation which is completely self-referential:

Returns the current 'label address' for this label.

Instead of explanatory like:

Returns the default receiving address for this label. This will reset to a fresh address once there's a transaction that spends to it.

Maybe there are other problems with getlabeladdress, but I can't see where they have been mentioned.

Also, I think this PR needs release notes. Good documentation is probably the most important part of this change.

jnewbery · 2018-04-09T21:25:15Z

I don't think it makes sense to introduce additional differences between accounts and labels that will make migration more difficult than necessary.

ok, I can see the merit in that. Let's keep this PR focused and we can squabble over removing getlabeladdress later.

I've updated the help text for getlabeladdress and added release notes.

Re-review would be very much appreciated at this point 😃

laanwj · 2018-04-10T08:46:51Z

My intent was not for the getlabeladdress discussion to block this PR. We need to make a decision about it before the 0.17 release, not necessarily now.

getlabeladdress doesn't make much sense to me, and blurs the distinction that "labels are associated with addresses, instead of addresses associated with labels".

Another problem with it is that it requires keeping this whole CAccount administration in place. Part of the reason for this account to label refactor is simplify the wallet mess and to get rid of that.

Will re-review.

re-utACK 86d3394de8e053c863b1e55b1c8c935d36884802

promag

Will test. Some nits thou.

promag · 2018-04-06T23:16:50Z

src/wallet/wallet.cpp

@@ -3643,6 +3643,12 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
    return result;
 }

+void CWallet::DeleteLabel(std::string label)


Nit, const std::string& label.

Agree that's better. Fixed.

promag · 2018-04-09T22:54:28Z

src/wallet/rpcwallet.cpp

+{
+    UniValue ret(UniValue::VOBJ);
+    if (verbose) {
+        ret.push_back(Pair("name", data.name));


promag · 2018-04-09T22:54:32Z

src/wallet/rpcwallet.cpp

+    if (verbose) {
+        ret.push_back(Pair("name", data.name));
+    }
+    ret.push_back(Pair("purpose", data.purpose));


promag · 2018-04-10T14:12:02Z

src/wallet/rpcwallet.cpp

+    UniValue ret(UniValue::VOBJ);
+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
+        if (item.second.name == label) {
+            ret.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));


Add label API to wallet RPC. This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't actually remove anything yet. These initially mirror the account functions, with the following differences: - These functions aren't DEPRECATED in the help - Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) - Labels have no balance - No balances in `listlabels` - `listlabels` has no minconf or watchonly argument - Like in the GUI, labels can be set on any address, not just receiving addreses - Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance (see bitcoin#1231). Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense. Thanks to Pierre Rochard for test fixes.

jnewbery · 2018-04-10T23:28:28Z

Fixed comment. b00faff -> 41ba061

promag · 2018-04-11T08:18:27Z

utACK 41ba061.

kallewoof

utACK 41ba061

kallewoof · 2018-04-11T08:54:37Z

src/wallet/rpcwallet.cpp

@@ -226,6 +227,21 @@ UniValue getlabeladdress(const JSONRPCRequest& request)

    // Parse the label first so we don't generate a key if there's an error
    std::string label = LabelFromValue(request.params[0]);
+    bool force = request.strMethod == "getaccountaddress" ? true : false;


Nit: ? true : false is redundant.

duh. Will tidy up in follow-up PR.

Let it go, will go away when accounts are removed.

Yeah, not worth a PR for just that.

Fixed in #12953

41ba061 [docs] Add release notes for wallet 'label' API. (John Newbery) 189e0ef [wallet] [rpc] introduce 'label' API for wallet (Wladimir J. van der Laan) Pull request description: Add label API to wallet RPC. This is one step towards #3816 ("Remove bolt-on account system") although it doesn't actually remove anything yet. These initially mirror the account functions, with the following differences: - These functions aren't DEPRECATED in the help - Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) - Labels have no balance - No balances in `listlabels` - `listlabels` has no minconf or watchonly argument - Like in the GUI, labels can be set on any address, not just receiving addreses - Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance (see #1231). Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense. Tree-SHA512: 45cc313c68ad529ce3a15c02181d2ab0083a7e14fe824e2cde34972713fecce512e3d4b9aa46db5355f2baa857c44b234d4fe9709225bc23c7ebbc0e03febbf5

Summary: 41ba061 [docs] Add release notes for wallet 'label' API. (John Newbery) 189e0ef [wallet] [rpc] introduce 'label' API for wallet (Wladimir J. van der Laan) Pull request description: Add label API to wallet RPC. This is one step towards #3816 ("Remove bolt-on account system") although it doesn't actually remove anything yet. These initially mirror the account functions, with the following differences: - These functions aren't DEPRECATED in the help - Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) - Labels have no balance - No balances in `listlabels` - `listlabels` has no minconf or watchonly argument - Like in the GUI, labels can be set on any address, not just receiving addreses - Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance (see #1231). Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense. Tree-SHA512: 45cc313c68ad529ce3a15c02181d2ab0083a7e14fe824e2cde34972713fecce512e3d4b9aa46db5355f2baa857c44b234d4fe9709225bc23c7ebbc0e03febbf5 Backport of Core PR12892 bitcoin/bitcoin#12892 Depends on D3308 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, markblundeberg Reviewed By: Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3949

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

… code docs 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 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 bitcoin#17578 and bitcoin#17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07

…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

… code docs 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 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 bitcoin#17578 and bitcoin#17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07

…llet Add label API to wallet RPC. This is one step towards dashpay#3816 ("Remove bolt-on account system") although it doesn't actually remove anything yet. These initially mirror the account functions, with the following differences: - These functions aren't DEPRECATED in the help - Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) - Labels have no balance - No balances in `listlabels` - `listlabels` has no minconf or watchonly argument - Like in the GUI, labels can be set on any address, not just receiving addreses - Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance (see dashpay#1231). Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense. Thanks to Pierre Rochard for test fixes.

Add label API to wallet RPC. This is one step towards dashpay#3816 ("Remove bolt-on account system") although it doesn't actually remove anything yet. These initially mirror the account functions, with the following differences: - These functions aren't DEPRECATED in the help - Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) - Labels have no balance - No balances in `listlabels` - `listlabels` has no minconf or watchonly argument - Like in the GUI, labels can be set on any address, not just receiving addreses - Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance (see dashpay#1231). Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense. Thanks to Pierre Rochard for test fixes.

jnewbery mentioned this pull request Apr 5, 2018

rpc: introduce 'label' API for wallet #7729

Closed

maflcko reviewed Apr 5, 2018

View reviewed changes

jnewbery force-pushed the 7729_jnewbery branch from a8d607d to c733a98 Compare April 5, 2018 17:41

jnewbery added the Wallet label Apr 5, 2018

jnewbery added this to the 0.17.0 milestone Apr 5, 2018

jonasschnelli reviewed Apr 5, 2018

View reviewed changes

promag reviewed Apr 5, 2018

View reviewed changes

PierreRochard reviewed Apr 6, 2018

View reviewed changes

jnewbery force-pushed the 7729_jnewbery branch from c733a98 to cec0de8 Compare April 6, 2018 16:53

jnewbery force-pushed the 7729_jnewbery branch from cec0de8 to 1f7f1df Compare April 7, 2018 16:42

jnewbery force-pushed the 7729_jnewbery branch from 1f7f1df to 0277f2d Compare April 9, 2018 19:21

jnewbery force-pushed the 7729_jnewbery branch from 0277f2d to 86d3394 Compare April 9, 2018 21:25

promag reviewed Apr 10, 2018

View reviewed changes

jnewbery force-pushed the 7729_jnewbery branch from 86d3394 to 74a9e81 Compare April 10, 2018 14:19

laanwj and others added 2 commits April 10, 2018 19:27

[docs] Add release notes for wallet 'label' API.

41ba061

jnewbery force-pushed the 7729_jnewbery branch from b00faff to 41ba061 Compare April 10, 2018 23:27

kallewoof reviewed Apr 11, 2018

View reviewed changes

laanwj merged commit 41ba061 into bitcoin:master Apr 11, 2018

ryanofsky mentioned this pull request Apr 11, 2018

Hide accounts system behind deprecation switch #11497

Closed

jnewbery mentioned this pull request Apr 11, 2018

Remove wallet 'account' API #12952

Closed

6 tasks

drafie mentioned this pull request Apr 20, 2018

Accounts depreciated jfm-so/piWallet#64

Open

jnewbery mentioned this pull request Apr 23, 2018

[wallet] [rpc] Remove getlabeladdress RPC #13060

Merged

Sjors mentioned this pull request May 15, 2018

getaddressesbyaccount is deprecated chris-belcher/electrum-personal-server#22

Closed

promag mentioned this pull request Jul 27, 2018

Add RPC to set transaction comment #12594

Closed

jonatack mentioned this pull request Oct 30, 2019

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

Merged

7 tasks

jnewbery mentioned this pull request Dec 13, 2019

rpc: simplify getaddressinfo labels, deprecate previous behavior #17578

Merged

Fuzzbawls mentioned this pull request Jan 8, 2020

Timeline: Removal of outdated internal accounting system PIVX-Project/PIVX#1248

Closed

Fuzzbawls mentioned this pull request Jun 25, 2020

[Tracking] Remove wallet 'account' API PIVX-Project/PIVX#1706

Closed

11 tasks

UdjinM6 mentioned this pull request Dec 18, 2020

backport: Deprecate accounts dashpay/dash#3886

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

[wallet] [rpc] introduce 'label' API for wallet #12892

[wallet] [rpc] introduce 'label' API for wallet #12892

Uh oh!

Conversation

jnewbery commented Apr 5, 2018

Uh oh!

jnewbery commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreRochard Apr 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 6, 2018

Uh oh!

PierreRochard commented Apr 6, 2018

Uh oh!

PierreRochard commented Apr 6, 2018

Uh oh!

PierreRochard commented Apr 7, 2018

Uh oh!

jnewbery commented Apr 7, 2018

Uh oh!

PierreRochard commented Apr 7, 2018

jnewbery commented Apr 5, 2018 •

edited

Loading

PierreRochard Apr 5, 2018 •

edited

Loading

ryanofsky commented Apr 9, 2018 •

edited

Loading

laanwj commented Apr 10, 2018 •

edited

Loading