Rename account to label where appropriate

ryanofsky commented at 6:09 pm on October 20, 2017: member

This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in #7729, by getting renaming out of the way and letting that change focus on semantics.

The difference between accounts and labels is that labels apply only to addresses, while accounts apply to both addresses and transactions (transactions have “from” and “to” accounts). The code associating accounts with transactions is clumsy and unreliable so we would like get rid of it.

There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).

meshcollider commented at 6:33 pm on October 20, 2017: contributor

Concept ACK, I think this should be part of #11497?

ryanofsky commented at 6:54 pm on October 20, 2017: member

Concept ACK, I think this should be part of #11497?

I don’t mind combining prs though I don’t see what advantage it brings in this case. This PR is backwards compatible and can be merged without waiting for #7729. #11497 is also confused about which account instances need to be removed vs renamed (as you pointed out in one of your review comments), and this PR clarifies that.

in src/qt/paymentserver.cpp:638 in b105fb9634 outdated

634@@ -635,9 +635,9 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
635     payment.add_transactions(transaction.data(), transaction.size());
636 
637     // Create a new refund address, or re-use:
638-    QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant);
639-    std::string strAccount = account.toStdString();
640-    std::set<CTxDestination> refundAddresses = wallet->GetAccountAddresses(strAccount);
641+    QString label = tr("Refund from %1").arg(recipient.authenticatedMerchant);

promag commented at 11:55 pm on October 20, 2017:

Only std::string is used, so:

0std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();

ryanofsky commented at 12:02 pm on October 21, 2017:

Changed in be9665af50e41a790216ed24133bd52d1674fa4a

in src/wallet/rpcwallet.cpp:3293 in b105fb9634 outdated

3288@@ -3287,6 +3289,14 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
3289     return response;
3290 }
3291 
3292+// Deprecated account RPCs.
3293+UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); }

promag commented at 0:00 am on October 21, 2017:

Missing IsDeprecatedRPCEnabled("accounts") and throw? Same below.

meshcollider commented at 0:49 am on October 21, 2017:

This doesn’t actually use -deprecatedrpc though, that’s done in #11497?

promag commented at 0:52 am on October 21, 2017:

I made the question because in the release notes says these are deprecated.

ryanofsky commented at 12:03 pm on October 21, 2017:

Added “to be” in 52177d174d5777d09134ef67aa38a1df49454183 since after #11031, “deprecated” might now mean disabled instead of just slated for removal.

in src/wallet/wallet.h:1144 in b105fb9634 outdated

1140@@ -1141,15 +1141,15 @@ class CReserveKey final : public CReserveScript
1141 
1142 
1143 /** 
1144- * Account information.
1145- * Stored in wallet with key "acc"+string account name.
1146+ * Label information.

laanwj commented at 6:57 am on October 21, 2017:

This structure is account-specific. It’s going away when account support is dropped, as there is no state per label, it’s just an identifier. So I suggest keeping it at the current name.

ryanofsky commented at 12:03 pm on October 21, 2017:

Reverted in 603759b773db38c14dbcb02b173a5542edaefb05

in src/rpc/protocol.h:79 in b105fb9634 outdated

75@@ -76,7 +76,7 @@ enum RPCErrorCode
76     //! Wallet errors
77     RPC_WALLET_ERROR                = -4,  //!< Unspecified problem with wallet (key not found etc.)
78     RPC_WALLET_INSUFFICIENT_FUNDS   = -6,  //!< Not enough funds in wallet or account
79-    RPC_WALLET_INVALID_ACCOUNT_NAME = -11, //!< Invalid account name
80+    RPC_WALLET_INVALID_LABEL_NAME   = -11, //!< Invalid label name

laanwj commented at 6:59 am on October 21, 2017:

I think usually when renaming errors we keep the old name around as an alias - people are processing this directly to RPC wrappers, and old client code would still use the old name.

ryanofsky commented at 12:02 pm on October 21, 2017:

Added alias in acc1ee8e96b1a7a685b6e095fa92f0d4f59eea39

promag commented at 0:01 am on November 6, 2017:

Just for curiosity, why isn’t RPC_INVALID_PARAMETER used instead?

ryanofsky commented at 4:54 pm on January 2, 2018:

#11536 (review)

Just for curiosity, why isn’t RPC_INVALID_PARAMETER used instead?

I don’t know the history, but I could imagine this was added because it was useful for some client to be able to distinguish this specific error from more generic errors. Similar to how RPC_WALLET_NOT_SPECIFIED was added for bitcoin-cli in #10931.

in src/rpc/client.cpp:43 in b105fb9634 outdated

41@@ -42,12 +42,16 @@ static const CRPCConvertParam vRPCConvertParams[] =
42     { "settxfee", 0, "amount" },
43     { "getreceivedbyaddress", 1, "minconf" },
44     { "getreceivedbyaccount", 1, "minconf" },
45+    { "getreceivedbylabel", 1, "minconf" },

laanwj commented at 7:00 am on October 21, 2017:

You don’t actually introduce these methods in this PR.

Edit: oh you do, github was hiding things again.

laanwj commented at 7:02 am on October 21, 2017: member

Concept ACK - needs qa test for the new RPCs.

in src/wallet/rpcwallet.cpp:3298 in b105fb9634 outdated

3293+UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); }
3294+UniValue getaccount(const JSONRPCRequest& request) { return getlabel(request); }
3295+UniValue getaddressesbyaccount(const JSONRPCRequest& request) { return getaddressesbylabel(request); }
3296+UniValue getreceivedbyaccount(const JSONRPCRequest& request) { return getreceivedbylabel(request); }
3297+UniValue listreceivedbyaccount(const JSONRPCRequest& request) { return listreceivedbylabel(request); }
3298+UniValue setaccount(const JSONRPCRequest& request) { return setlabel(request); }

laanwj commented at 7:04 am on October 21, 2017:

This allows for no way for the method to know whether they’ve been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?

ryanofsky commented at 1:28 pm on October 21, 2017:

This allows for no way for the method to know whether they’ve been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?

The intention of this PR is to not introduce any differences between accounts and labels yet, leaving that for #7729 and any future followups. I have a rebased version of #7729 at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).

ryanofsky force-pushed on Oct 21, 2017

ryanofsky commented at 4:48 pm on October 21, 2017: member

Added 7 commits b105fb9634ad9165552464d0d0ec8f5bb77cc3b0 -> 68b48f6ddde65d4f9134ab85b82f0b48443cc84c (pr/acct.1 -> pr/acct.2, compare) Squashed 68b48f6ddde65d4f9134ab85b82f0b48443cc84c -> fec503ac115f3de2de31672bc166c8263c8b1317 (pr/acct.2 -> pr/acct.3)

needs qa test for the new RPCs.

Added wallet-labels test in fec503ac115f3de2de31672bc166c8263c8b1317

jonasschnelli commented at 1:20 am on October 23, 2017: contributor

Thanks! Concept ACK.

in src/wallet/rpcwallet.cpp:1483 in fec503ac11 outdated

1481 
1482             "\nResult:\n"
1483             "[\n"
1484             "  {\n"
1485             "    \"involvesWatchonly\" : true,   (bool) Only returned if imported addresses were involved in transaction\n"
1486             "    \"account\" : \"accountname\",  (string) The account name of the receiving account\n"

MarcoFalke commented at 9:06 am on October 23, 2017:

Should be updated, since this is an alias for “label”. (Same for other occurrences in this file)

ryanofsky commented at 2:53 pm on October 23, 2017:

Good catch, updated in 864a83780bc5f8ac3284d4327205e75ca6d0c703.

MarcoFalke commented at 9:09 am on October 23, 2017: member

Concept ACK fec503ac115f3de2de31672bc166c8263c8b1317

ryanofsky force-pushed on Oct 23, 2017

ryanofsky commented at 2:57 pm on October 23, 2017: member

Added 2 commits fec503ac115f3de2de31672bc166c8263c8b1317 -> 864a83780bc5f8ac3284d4327205e75ca6d0c703 (pr/acct.3 -> pr/acct.4, compare) Squashed 864a83780bc5f8ac3284d4327205e75ca6d0c703 -> 62df1688ef91372efec864c93cdc8968bd7493cb (pr/acct.4 -> pr/acct.5)

in doc/release-notes.md:87 in 62df1688ef outdated

81@@ -82,9 +82,17 @@ Low-level RPC changes
82   * `getnetworkinfo`
83   * `getwalletinfo`
84   * `getmininginfo`
85-
86 - `dumpwallet` no longer allows overwriting files. This is a security measure
87   as well as prevents dangerous user mistakes.
88+- `getnewaddress` and `addmultisigaddress` RPC `account` named parameters have

Sjors commented at 3:27 pm on November 9, 2017:

Do you want to keep an address alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.

ryanofsky commented at 6:05 pm on January 2, 2018:

#11536 (review)

Do you want to keep an address alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.

Thanks. Somehow I missed this comment earlier. Added in 7ae567f87eb474dafd8b3cb1351a0bdfe9e41208

MarcoFalke commented at 4:59 pm on November 10, 2017: member

Needs rebase

ryanofsky force-pushed on Nov 27, 2017

ryanofsky force-pushed on Dec 1, 2017

ryanofsky commented at 8:28 pm on December 1, 2017: member

Needs rebase

Just so this isn’t the last comment., I have been periodically rebasing this PR. The PR is just a big rename so it gets out of date pretty frequently.

laanwj added the label Wallet on Dec 12, 2017

ryanofsky commented at 6:04 pm on January 2, 2018: member

Rebased 62df1688ef91372efec864c93cdc8968bd7493cb -> a8e9ac981aeebcd482277115190374227c8be0b8 (pr/acct.5 -> pr/acct.6) due to conflicts. Rebased a8e9ac981aeebcd482277115190374227c8be0b8 -> 981ac9209629c90377d7586b33dae396017d2398 (pr/acct.6 -> pr/acct.7) due to more conflicts. Added 1 commit 981ac9209629c90377d7586b33dae396017d2398 -> 7ae567f87eb474dafd8b3cb1351a0bdfe9e41208 (pr/acct.7 -> pr/acct.8, compare) implementing Sjors suggestion. Squashed 7ae567f87eb474dafd8b3cb1351a0bdfe9e41208 -> 8ec5890f540e7d96b64824306830abc928864b46 (pr/acct.8 -> pr/acct.9)

ryanofsky force-pushed on Jan 2, 2018

ryanofsky force-pushed on Jan 4, 2018

ryanofsky commented at 6:54 pm on January 4, 2018: member

Rebased again 8ec5890f540e7d96b64824306830abc928864b46 -> 5bb0a7b79b16649a49c96b88090af051842ea7d4 (pr/acct.9 -> pr/acct.10) due to conflict with #12062.

ryanofsky force-pushed on Jan 6, 2018

ryanofsky commented at 11:21 am on January 6, 2018: member

Rebased again 5bb0a7b79b16649a49c96b88090af051842ea7d4 -> 1c0f7c7770fbae467db06f7c7e61b1d29dce7771 (pr/acct.10 -> pr/acct.11) due to conflict with #10677.

MarcoFalke added this to the milestone 0.16.0 on Jan 6, 2018

sipa commented at 12:36 pm on January 6, 2018: member

utACK 1c0f7c7770fbae467db06f7c7e61b1d29dce7771

ryanofsky force-pushed on Jan 11, 2018

ryanofsky commented at 3:58 pm on January 11, 2018: member

Rebased 1c0f7c7770fbae467db06f7c7e61b1d29dce7771 -> 96363c3f433a4b5970be036aa1cb4ebf36febeab (pr/acct.11 -> pr/acct.12) due to conflicts with #11403.

laanwj removed this from the milestone 0.16.0 on Jan 11, 2018

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

laanwj commented at 6:46 pm on January 11, 2018: member

This is not related to segwit wallet, nor a bugfix, so I’m moving the milestone to 0.17.

in doc/release-notes.md:71 in 96363c3f43 outdated

 96@@ -97,6 +97,15 @@ Low-level RPC changes
 97   * `getwalletinfo`
 98   * `getmininginfo`
 99 - The wallet RPC `getreceivedbyaddress` will return an error if called with an address not in the wallet.
100+- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
101+  parameters have been renamed to `label` with no change in behavior.
102+- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and
103+  `setlabel` RPCs have been added to replace to be deprecated
104+  `getaccountaddress`, `getreceivedbyaccount`, `listreceivedbyaccount`, and
105+  `setaccount` RPCs with no differences in behavior.

Sjors commented at 2:21 pm on January 16, 2018:

No point in adding getlabeladdress and getreceivedbylabel if #7729 gets rid of those? In other words, maybe it’s better not to rename those methods and leave their deprecation notice?

ryanofsky commented at 3:18 pm on January 16, 2018:

No point in adding getlabeladdress and getreceivedbylabel if #7729 gets rid of those?

#7729 does add getlabeladdress. It doesn’t add getreceivedbylabel, but getreceivedbylabel/account is just a way of finding incoming transactions sent to a set of addresses, equivalent to calling getaddressesbylabel and getreceivedbyaddress.

Note you can find a rebased version of 7729 here: #7729 (comment), also linked in PR description above. I think it’s significantly easier to understand than the current version.

Sjors commented at 2:31 pm on January 16, 2018: member

Concept ACK. Lightly tested: worked as expected.

Does this PR imply that #7729 must support existing accounts / labels? E.g. it can’t switch those over to a new data structure and there’s no way to tell if a wallet still needs to be upgraded?

ryanofsky force-pushed on Jan 16, 2018

ryanofsky commented at 7:14 pm on January 17, 2018: member

Rebased 96363c3f433a4b5970be036aa1cb4ebf36febeab -> 71621535c75b848cda1a17b991567e361d346050 (pr/acct.12 -> pr/acct.13) due to minor conflict with #12177.

Does this PR imply that #7729 must support existing accounts / labels? E.g. it can’t switch those over to a new data structure and there’s no way to tell if a wallet still needs to be upgraded?

No, I don’t think this PR implies that. Labels are already used in rpcdump apis and the qt interface, and aren’t a new concept. This is just a rename with no change in behavior to use terminology more consistently.

Sjors commented at 8:43 am on January 18, 2018: member

@ryanofsky:

#7729 does add getlabeladdress

It does, but the PR description says “should be removed according to discussion”. Maybe that’s out of date?

ryanofsky commented at 9:03 am on January 18, 2018: member

It does, but the PR description says “should be removed according to discussion”. Maybe that’s out of date?

Probably out of date, or could have been a typo. The discussion in that PR has been going on a while and can a little confusing to follow. If you just look at the code changes and commit messages it should make more sense, and also (sorry to keep repeating) my rebased version of #7729 on top of this PR should be even simpler to understand.

Sjors commented at 9:06 am on January 18, 2018: member

@ryanofsky I did look at that, but it confused me because it doesn’t match the description of the PR. So I suggested that that PR should probably be closed and reopened with an up to date description.

I agree that the commit descriptions themselves are quite readable, they just seem out of context.

ryanofsky commented at 9:33 am on January 18, 2018: member

I agree that the commit descriptions themselves are quite readable, they just seem out of context.

Would suggest pointing out specific problems or edits you would like to see. It could perhaps be more appropriate to edit the description than open a new PR.

ryanofsky force-pushed on Jan 18, 2018

ryanofsky commented at 4:07 pm on January 18, 2018: member

Rebased 71621535c75b848cda1a17b991567e361d346050 -> 59df27a20a633731506686a477bec10b6a5110d4 (pr/acct.13 -> pr/acct.14) due to conflict in release notes with #12210.

in src/wallet/rpcwallet.cpp:1587 in 59df27a20a outdated

1555-            "    \"account\" : \"accountname\",  (string) The account name of the receiving account\n"
1556-            "    \"amount\" : x.xxx,             (numeric) The total amount received by addresses with this account\n"
1557+            "    \"account\" : \"accountname\",  (string) DEPRECATED. Backwards compatible alias for label.\n"
1558+            "    \"amount\" : x.xxx,             (numeric) The total amount received by addresses with this label\n"
1559             "    \"confirmations\" : n,          (numeric) The number of confirmations of the most recent transaction included\n"
1560             "    \"label\" : \"label\"           (string) A comment for the address/transaction, if any\n"

jnewbery commented at 6:21 pm on January 19, 2018:

Should this also be update to:

The label of the receiving address. The default label is \"\".\n"

(as above in listreceivedbyaddress())

ryanofsky commented at 6:50 pm on February 12, 2018:

Should this also be update to:

Thanks, updated comment in c1a4f6fc5ea3b6f21413fcb3bc7aa8f67e6ff5e3

jnewbery commented at 6:31 pm on January 19, 2018: member

Tested ACK 59df27a20a633731506686a477bec10b6a5110d4. One comment inline.

Only other comments I’d make are that perhaps it’d be clearer to do all the variable renaming in the first commit and then the file renaming in the second commit. You may also consider renaming the test script to wallet_labels.py to conform with #11796.

ryanofsky force-pushed on Jan 25, 2018

ryanofsky force-pushed on Feb 1, 2018

ryanofsky force-pushed on Feb 12, 2018

ryanofsky commented at 6:50 pm on February 12, 2018: member

Rebased 59df27a20a633731506686a477bec10b6a5110d4 -> 9508f59d293b0f8c67b7b01acc3793f724ab8fe2 (pr/acct.14 -> pr/acct.15) due to conflict with test rename #11774, segwit wallet #11403, addwitness deprecate #12210 Rebased 9508f59d293b0f8c67b7b01acc3793f724ab8fe2 -> 8b9c889702db915eddfff2576cf89389090c2bf7 (pr/acct.15 -> pr/acct.16) due to conflict in release notes. Rebased 8b9c889702db915eddfff2576cf89389090c2bf7 -> 3d5d5bc03536a8c50ec4778cd599045db2abf24b (pr/acct.16 -> pr/acct.17) due to conflict with #12193. Added 1 commits 3d5d5bc03536a8c50ec4778cd599045db2abf24b -> c1a4f6fc5ea3b6f21413fcb3bc7aa8f67e6ff5e3 (pr/acct.17 -> pr/acct.18, compare) Squashed c1a4f6fc5ea3b6f21413fcb3bc7aa8f67e6ff5e3 -> 3b2be2aa88823bbb09f12935694af24cf9f292d5 (pr/acct.18 -> pr/acct.19) Rebased 3b2be2aa88823bbb09f12935694af24cf9f292d5 -> b63a7dd974df52b66582122b5a899cc44be7e520 (pr/acct.19 -> pr/acct.20) due to conflict with #12409. Rebased b63a7dd974df52b66582122b5a899cc44be7e520 -> 06c0c42960663f0ba4e6feafa60a8dc92e4ba897 (pr/acct.20 -> pr/acct.21) due to more minor conflicts. Rebased 06c0c42960663f0ba4e6feafa60a8dc92e4ba897 -> 53bafe820e20dfb8192eedfd1ce0c729536de679 (pr/acct.21 -> pr/acct.22) due to conflict with #9991.

ryanofsky force-pushed on Feb 14, 2018

ryanofsky force-pushed on Feb 23, 2018

ryanofsky force-pushed on Mar 7, 2018

laanwj added this to the "Blockers" column in a project

in src/wallet/rpcwallet.cpp:3847 in 53bafe820e outdated

3846     { "wallet",             "dumpprivkey",                      &dumpprivkey,                   {"address"}  },
3847     { "wallet",             "dumpwallet",                       &dumpwallet,                    {"filename"} },
3848     { "wallet",             "encryptwallet",                    &encryptwallet,                 {"passphrase"} },
3849-    { "wallet",             "getaccountaddress",                &getaccountaddress,             {"account"} },
3850+    { "wallet",             "getlabeladdress",                  &getlabeladdress,               {"label"} },
3851+    { "wallet",             "getaccountaddress",                &getlabeladdress,               {"account"} },

Sjors commented at 8:15 pm on March 12, 2018:

Maybe add a comment to point out that this is an alias?

ryanofsky commented at 6:48 pm on March 13, 2018:

#11536 (review)

Maybe add a comment to point out that this is an alias?

Not sure this is the appropriate place to document an alias. Also I’m a little afraid of breaking the https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/check-rpc-mappings.py script which has to parse this.

in src/wallet/rpcwallet.cpp:296 in 53bafe820e outdated

295+            "setlabel \"address\" \"label\"\n"
296+            "\nSets the label associated with the given address.\n"
297             "\nArguments:\n"
298-            "1. \"address\"         (string, required) The bitcoin address to be associated with an account.\n"
299-            "2. \"account\"         (string, required) The account to assign the address to.\n"
300+            "1. \"address\"         (string, required) The bitcoin address to be associated with an label.\n"

Sjors commented at 8:22 pm on March 12, 2018:

“a label”

ryanofsky commented at 6:48 pm on March 13, 2018:

#11536 (review)

“a label”

Good catch, fixed in 22af84bab2f5468d3fa3950e6257dacee27ae885

in doc/release-notes.md:75 in 53bafe820e outdated

68+  `setlabel` RPCs have been added to replace to be deprecated
69+  `getaccountaddress`, `getreceivedbyaccount`, `listreceivedbyaccount`, and
70+  `setaccount` RPCs with no differences in behavior.
71+- Wallet `listreceivedbylabel`, `listreceivedbyaccount` and `listunspent` RPCs
72+  add `label` fields to returned JSON objects that previously only had
73+  `account` fields.

Sjors commented at 8:25 pm on March 12, 2018:

What’s the rationale for adding a label field rather than renaming the account field? Because it wasn’t deprecated yet?

ryanofsky commented at 6:47 pm on March 13, 2018:

#11536 (review)

What’s the rationale for adding a label field rather than renaming the account field? Because it wasn’t deprecated yet?

Yes, I don’t think there’s any reason to break compatibility here. This PR is just about adding better support for labels, not breaking anything that uses accounts.

Sjors approved

Sjors commented at 8:58 pm on March 12, 2018: member

Tested ACK 53bafe82.

ryanofsky referenced this in commit 22af84bab2 on Mar 13, 2018

ryanofsky force-pushed on Mar 13, 2018

ryanofsky commented at 7:11 pm on March 13, 2018: member

Added 2 commits 53bafe820e20dfb8192eedfd1ce0c729536de679 -> f0fd2356ad17db395319080b18bc85dcc7d6f43e (pr/acct.22 -> pr/acct.23, compare) Squashed f0fd2356ad17db395319080b18bc85dcc7d6f43e -> 6a25a42d01f9688dbc7cac2e2e5c3bf31555189a (pr/acct.23 -> pr/acct.24) Rebased 6a25a42d01f9688dbc7cac2e2e5c3bf31555189a -> d596f032dcfc7beeed6ad0208ae706e5b999a584 (pr/acct.24 -> pr/acct.25) due to release notes conflict with #11872

Sjors commented at 7:13 pm on March 13, 2018: member

Not sure this is the appropriate place to document an alias.

A generic comment above the table saying that duplicate references to the same function are an alias would be fine.

ryanofsky force-pushed on Mar 13, 2018

ryanofsky commented at 7:33 pm on March 13, 2018: member

A generic comment above the table saying that duplicate references to the same function are an alias would be fine.

This isn’t strictly true, because the both the client and the server can still distinguish between the functions and treat them differently. See echo/echojson at https://github.com/bitcoin/bitcoin/blob/d42a4fe5aaae60f33a89bde78f21820abefce922/src/rpc/misc.cpp#L472-L473

I’m happy to add a comment but since I’m not sure how this relates to labels or accounts and this PR already touches ~250 lines, maybe we could improve the RPC framework documentation in a new PR.

Sjors commented at 9:14 pm on March 13, 2018: member

Sounds like documentation (in a different PR) would indeed be useful.

ryanofsky referenced this in commit 4c317d89e9 on Mar 15, 2018

ryanofsky commented at 8:42 pm on March 15, 2018: member

Sounds like documentation (in a different PR) would indeed be useful.

See #12700

PierreRochard commented at 2:36 am on March 18, 2018: contributor

Tested ACK d596f032dcfc7beeed6ad0208ae706e5b999a584

ryanofsky commented at 3:16 pm on March 19, 2018: member

@laanwj what do you think about about merging this change? All it is doing is renaming “account” to “label” so the terms are used more consistently, and then back adding a few “account” aliases to preserve backwards compatibility. Review status:

Tested ACK

jnewbery
sjors
PierreRochard

utACK

sipa

Concept ACK

MeshCollider
laanwj
jonasschnelli
MarcoFalke

Rebased d596f032dcfc7beeed6ad0208ae706e5b999a584 -> e2966ceee68f88f0b257b43f9c4091ff0c2d5d0f (pr/acct.25 -> pr/acct.26) due to conflict with #12408

Rename account to label where appropriate

This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
https://github.com/bitcoin/bitcoin/pull/7729, by getting renaming out of the
way and letting it focus on semantics.

The difference between accounts and labels is that labels apply only to
addresses, while accounts apply to both addresses and transactions
(transactions have "from" and "to" accounts). The code associating accounts
with transactions is clumsy and unreliable so we would like get rid of it.

045eeb8870

Rename wallet_accounts.py test

This is a separate commit because changing the test at the same time as
renaming it breaks git (and github) rename detection.

d2527bd54e

in doc/release-notes.md:68 in d596f032dc outdated

62@@ -63,6 +63,15 @@ RPC changes
63 
64 - The `createrawtransaction` RPC will now accept an array or dictionary (kept for compatibility) for the `outputs` parameter. This means the order of transaction outputs can be specified by the client.
65 - The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
66+- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
67+  parameters have been renamed to `label` with no change in behavior.
68+- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and

jnewbery commented at 3:34 pm on March 19, 2018:

Nit: language is perhaps a little clumsy: ...been added to replace to be deprecated.... Perhaps prefer something like:

0Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and
1`setlabel` RPCs have been added to replace `getaccountaddress`,
2`getreceivedbyaccount`, `listreceivedbyaccount`, and `setaccount` RPCs,
3which are now deprecated. There is no change in behavior between the
4new RPCs and deprecated RPCs.

ryanofsky commented at 6:07 pm on March 19, 2018:

Nit: language is perhaps a little clumsy

Updated in 843ee063efc2afe20ef0dbe076bd5989bfb5c59c

ryanofsky force-pushed on Mar 19, 2018

laanwj referenced this in commit ebdf84c960 on Mar 19, 2018

jnewbery commented at 5:40 pm on March 19, 2018: member

One comment inline.

Why does this PR not add the getlabel and getaddressesbylabel rpcs?

I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py -> wallet_labels.py

ryanofsky force-pushed on Mar 19, 2018

ryanofsky commented at 6:49 pm on March 19, 2018: member

Added 1 commits e2966ceee68f88f0b257b43f9c4091ff0c2d5d0f -> 843ee063efc2afe20ef0dbe076bd5989bfb5c59c (pr/acct.26 -> pr/acct.27, compare) Squashed 843ee063efc2afe20ef0dbe076bd5989bfb5c59c -> d2527bd54e994747d9e24043c91d46c7219d46ac (pr/acct.27 -> pr/acct.28)

Why does this PR not add the getlabel and getaddressesbylabel rpcs?

#7729 adds these with different behavior than the existing account rpcs. The idea is for this PR to just clean up stale “account” terminology and for #7729 to add new functionality for making labels work better.

I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py -> wallet_labels.py

Sure, moved rename to second commit.

in test/functional/wallet_labels.py:12 in d2527bd54e

 7+RPCs tested are:
 8+    - getlabeladdress
 9+    - getaddressesbyaccount
10+    - listaddressgroupings
11+    - setlabel
12+    - sendfrom (with account arguments)

jnewbery commented at 7:06 pm on March 19, 2018:

This (and the line below) can be updated to (with label arguments)

ryanofsky commented at 7:18 pm on March 19, 2018:

This (and the line below) can be updated to (with label arguments)

This is actually intentional. sendfrom and move are account-specific and deprecated and won’t take label arguments. Idea is that we support receiving at addresses but not really sending from addresses.

jnewbery commented at 7:44 pm on March 19, 2018:

ok, makes sense. I was confused by the code lower down in the test case:

0        # Check that sendfrom label reduces listaccounts balances.
1        for i, label in enumerate(labels):
2            to_label = labels[(i+1) % len(labels)]
3            node.sendfrom(label.name, to_label.receive_address, amount_to_send)

jnewbery commented at 7:13 pm on March 19, 2018: member

Tested ACK d2527bd54e994747d9e24043c91d46c7219d46ac. Thanks for moving the commits around.

Sorry - I’ve added one more nit in the test file.

laanwj merged this on Mar 22, 2018

laanwj closed this on Mar 22, 2018

laanwj referenced this in commit cead84b72d on Mar 22, 2018

fanquake removed this from the "Blockers" column in a project

HashUnlimited referenced this in commit bea19aea6e on Jul 31, 2018

lionello referenced this in commit 44f8c9e70c on Nov 7, 2018

jasonbcox referenced this in commit 586ece932c on Oct 11, 2019

jonspock referenced this in commit 9821a7e34d on Dec 25, 2019

jonspock referenced this in commit f84d9c2da1 on Dec 26, 2019

jonspock referenced this in commit 2342f30a14 on Dec 27, 2019

PastaPastaPasta referenced this in commit 28d8d99a2c on Jul 17, 2020

PastaPastaPasta referenced this in commit ea7575375d on Jul 17, 2020

xdustinface referenced this in commit d633b27191 on Dec 18, 2020

xdustinface referenced this in commit e86a0c093a on Dec 22, 2020

xdustinface referenced this in commit 1914effbbb on Dec 22, 2020

DrahtBot locked this on Sep 8, 2021

Rename account to label where appropriate #11536