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

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:7729_jnewbery changing 10 files +241 −46
  1. jnewbery commented at 5:28 pm on April 5, 2018: member

    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.
  2. jnewbery commented at 5:34 pm on April 5, 2018: member

    This is #7729 with the following changes:

  3. jnewbery commented at 5:36 pm on April 5, 2018: member
    For full history of this PR, see #7729. Requesting review from reviewers of that PR: @Sjors , @MarcoFalke , @ryanofsky , @jonasschnelli , @instagibbs , @sipa , @luke-jr .
  4. in src/wallet/rpcwallet.cpp:320 in a8d607dda8 outdated 
    318+            "2. \"label\"           (string, required) The label to assign to the address.\n"
319             "\nExamples:\n"
320-            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\" \"tabby\"")
321-            + HelpExampleRpc("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\", \"tabby\"")
322+            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ\" \"tabby\"")
323+            + HelpExampleRpc("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ\", \"tabby\"")
    MarcoFalke commented at 5:39 pm on April 5, 2018:
    Unrelated change.
    jnewbery commented at 5:41 pm on April 5, 2018:
    reverted
  5. jnewbery force-pushed on Apr 5, 2018
  6. jnewbery added the label Wallet on Apr 5, 2018
  7. jnewbery added this to the milestone 0.17.0 on Apr 5, 2018
  8. in src/wallet/rpcwallet.cpp:212 in c733a983bc outdated 
    209@@ -205,14 +210,15 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
210         return NullUniValue;
211     }
212 
213-    if (request.fHelp || request.params.size() != 1)
214+    if (request.fHelp || request.params.size() > 2)
215         throw std::runtime_error(
216             "getlabeladdress \"label\"\n"
217-            "\nReturns the current Bitcoin address for receiving payments to this label.\n"
218+            "\nReturns the current 'label address' for this label.\n"
    jonasschnelli commented at 6:00 pm on April 5, 2018:
    Since this call gets even weirder, shouldn’t we deprecate it in this PR?
    jnewbery commented at 8:09 pm on April 5, 2018:

    Aparently some miners require this functionality (https://github.com/bitcoin/bitcoin/pull/7729#issuecomment-199444036).

    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.

  9. in src/wallet/rpcwallet.cpp:3890 in c733a983bc outdated 
    3885+            "\nExamples:\n"
3886+            + HelpExampleCli("getaddressesbylabel", "\"tabby\"")
3887+            + HelpExampleRpc("getaddressesbylabel", "\"tabby\"")
3888+        );
3889+
3890+    LOCK2(cs_main, pwallet->cs_wallet);
    promag commented at 8:45 pm on April 5, 2018:
    Remove cs_main?
    jnewbery commented at 4:00 pm on April 6, 2018:
    done
  10. in src/wallet/rpcwallet.cpp:3891 in c733a983bc outdated 
    3891+
3892+    std::string label = LabelFromValue(request.params[0]);
3893+
3894+    // Find all addresses that have the given label
3895+    std::vector<std::pair<std::string, UniValue>> addresses;
3896+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
    promag commented at 8:49 pm on April 5, 2018:

    Future improvements:

    • move this to wallet function?
    • index address by label with a multi map?
    jnewbery commented at 4:52 pm on April 6, 2018:
    I’ll leave this functionality here for now. Can be refactored later if required.
  11. in src/wallet/rpcwallet.cpp:3898 in c733a983bc outdated 
    3893+
3894+    // Find all addresses that have the given label
3895+    std::vector<std::pair<std::string, UniValue>> addresses;
3896+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
3897+        if (item.second.name == label) {
3898+            addresses.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));
    promag commented at 8:50 pm on April 5, 2018:
    Could push to ret and avoid addresses?
    jnewbery commented at 4:25 pm on April 6, 2018:
    yes, done.
  12. in src/wallet/rpcwallet.cpp:3942 in c733a983bc outdated 
    3937+            + HelpExampleCli("listlabels", "send") +
3938+            "\nAs json rpc call\n"
3939+            + HelpExampleRpc("listlabels", "receive")
3940+        );
3941+
3942+    LOCK2(cs_main, pwallet->cs_wallet);
    promag commented at 8:51 pm on April 5, 2018:
    Remove cs_main?
    jnewbery commented at 4:00 pm on April 6, 2018:
    done
  13. in src/wallet/rpcwallet.cpp:3945 in c733a983bc outdated 
    3940+        );
3941+
3942+    LOCK2(cs_main, pwallet->cs_wallet);
3943+
3944+    std::string purpose;
3945+    if (request.params.size() > 0) {
    promag commented at 8:51 pm on April 5, 2018:
    !params[0].isNull()
    jnewbery commented at 4:27 pm on April 6, 2018:
    done
  14. in src/wallet/rpcwallet.cpp:3951 in c733a983bc outdated 
    3946+        purpose = request.params[0].get_str();
3947+    }
3948+
3949+    std::set<std::string> label_set;
3950+    for (const std::pair<CTxDestination, CAddressBookData>& entry : pwallet->mapAddressBook) {
3951+        if (purpose.empty() || entry.second.purpose == purpose){
    promag commented at 8:52 pm on April 5, 2018:
    Nit, space before {.
    jnewbery commented at 4:28 pm on April 6, 2018:
    fixed
  15. in src/wallet/rpcwallet.cpp:3958 in c733a983bc outdated 
    3953+        }
3954+    }
3955+
3956+    // Move label names to a list and sort before returning
3957+    std::list<std::string> label_list(label_set.begin(), label_set.end());
3958+    label_list.sort();
    promag commented at 8:56 pm on April 5, 2018:
    Already sorted?
    jnewbery commented at 4:31 pm on April 6, 2018:
    Yes, you’re right. I’ve removed label_list
  16. in src/wallet/rpcwallet.cpp:3960 in c733a983bc outdated 
    3955+
3956+    // Move label names to a list and sort before returning
3957+    std::list<std::string> label_list(label_set.begin(), label_set.end());
3958+    label_list.sort();
3959+    UniValue ret(UniValue::VARR);
3960+    for (const std::string &name : label_list) {
    promag commented at 8:58 pm on April 5, 2018:
    Nit space after &.
    jnewbery commented at 4:32 pm on April 6, 2018:
    fixed
  17. in src/wallet/rpcwallet.cpp:190 in c733a983bc outdated 
    186@@ -187,6 +187,11 @@ UniValue getnewaddress(const JSONRPCRequest& request)
187     return EncodeDestination(dest);
188 }
189 
190+void DeleteLabel(CWallet& wallet, std::string label)
    promag commented at 9:27 pm on April 5, 2018:
    Nit, move to CWallet::DeleteLabel(const std::string& label)? Or make it static for now?
    PierreRochard commented at 11:39 pm on April 5, 2018:
    Agree, if left unchanged this would be the only direct CWalletDB call from rpcwallet. Seems as though it should go through CWallet
    jnewbery commented at 4:39 pm on April 6, 2018:
    Moved into CWallet
  18. in src/wallet/rpcwallet.cpp:215 in c733a983bc outdated 
    209@@ -205,14 +210,15 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
210         return NullUniValue;
211     }
212 
213-    if (request.fHelp || request.params.size() != 1)
214+    if (request.fHelp || request.params.size() > 2)
215         throw std::runtime_error(
216             "getlabeladdress \"label\"\n"
    promag commented at 9:28 pm on April 5, 2018:
    Missing ( force ).
    jnewbery commented at 4:40 pm on April 6, 2018:
    fixed
  19. in src/wallet/rpcwallet.cpp:213 in c733a983bc outdated 
    209@@ -205,14 +210,15 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
210         return NullUniValue;
211     }
212 
213-    if (request.fHelp || request.params.size() != 1)
214+    if (request.fHelp || request.params.size() > 2)
    promag commented at 9:28 pm on April 5, 2018:
    Missing < 1.
    jnewbery commented at 4:51 pm on April 6, 2018:
    fixed
  20. in src/wallet/rpcwallet.cpp:234 in c733a983bc outdated 
    229@@ -224,6 +230,20 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
230 
231     // Parse the label first so we don't generate a key if there's an error
232     std::string label = LabelFromValue(request.params[0]);
233+    bool force = false;
234+    if (!request.params[1].isNull())
    promag commented at 9:29 pm on April 5, 2018:
    Nit, add {}.
    jnewbery commented at 4:49 pm on April 6, 2018:
    added
  21. jnewbery commented at 9:36 pm on April 5, 2018: member
    Thanks for the review @promag . Will address comments tomorrow.
  22. in src/wallet/rpcwallet.cpp:331 in c733a983bc outdated 
    327@@ -308,22 +328,24 @@ UniValue setlabel(const JSONRPCRequest& request)
328     }
329 
330     std::string label;
331-    if (!request.params[1].isNull())
332+    if (!request.params[1].isNull()) {
    PierreRochard commented at 3:36 pm on April 6, 2018:
    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.
    jnewbery commented at 4:46 pm on April 6, 2018:
    I’ve updated the behaviour to match the help text: the rpc call will fail if no label is provided.
  23. jnewbery force-pushed on Apr 6, 2018
  24. jnewbery commented at 4:54 pm on April 6, 2018: member
    Addressed review comments from @promag and @PierreRochard . Are you able to re-review?
  25. PierreRochard commented at 6:33 pm on April 6, 2018: contributor
    Yes, re-reviewing now
  26. PierreRochard commented at 9:47 pm on April 6, 2018: contributor

    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.

  27. in src/wallet/wallet.cpp:3643 in cec0de8723 outdated 
    3642@@ -3643,6 +3643,12 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
3643     return result;
3644 }
3645 
3646+void CWallet::DeleteLabel(std::string label)
    promag commented at 11:16 pm on April 6, 2018:
    Nit, const std::string& label.
    jnewbery commented at 2:17 pm on April 10, 2018:
    Agree that’s better. Fixed.
  28. PierreRochard commented at 9:21 am on April 7, 2018: contributor

    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)

  29. jnewbery force-pushed on Apr 7, 2018
  30. jnewbery commented at 4:43 pm on April 7, 2018: member
    Thanks for the fixes @PierreRochard! I’ve updated the tests as you suggested and force pushed.
  31. PierreRochard commented at 7:06 pm on April 7, 2018: contributor
    Tested ACK 1f7f1dfd1c16cf0c9a700e2b262ffa8f6c6559cd
  32. laanwj commented at 2:40 am on April 9, 2018: member
    I’m still not convinced we really need getlabeladdress in the first place, but anyhow. utACK https://github.com/bitcoin/bitcoin/pull/12892/commits/1f7f1dfd1c16cf0c9a700e2b262ffa8f6c6559cd
  33. MarcoFalke commented at 6:24 pm on April 9, 2018: member
    Needs rebase
  34. jnewbery force-pushed on Apr 9, 2018
  35. jnewbery commented at 7:22 pm on April 9, 2018: member

    Rebased.

    I’m still not convinced we really need getlabeladdress

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

    I kept this call because there was a comment in the previous PR (https://github.com/bitcoin/bitcoin/pull/7729#issuecomment-199444036) 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?

  36. PierreRochard commented at 7:58 pm on April 9, 2018: contributor
    @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))
  37. ryanofsky commented at 8:02 pm on April 9, 2018: member

    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.

  38. jnewbery commented at 9:25 pm on April 9, 2018: member

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

  39. jnewbery force-pushed on Apr 9, 2018
  40. in src/wallet/rpcwallet.cpp:3742 in 86d3394de8 outdated 
    3733@@ -3720,6 +3734,17 @@ UniValue DescribeWalletAddress(CWallet* pwallet, const CTxDestination& dest)
3734     return ret;
3735 }
3736 
3737+/** Convert CAddressBookData to JSON record.  */
3738+static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool verbose)
3739+{
3740+    UniValue ret(UniValue::VOBJ);
3741+    if (verbose) {
3742+        ret.push_back(Pair("name", data.name));
    promag commented at 10:54 pm on April 9, 2018:
    pushKV.
    jnewbery commented at 2:17 pm on April 10, 2018:
    fixed
  41. in src/wallet/rpcwallet.cpp:3744 in 86d3394de8 outdated 
    3739+{
3740+    UniValue ret(UniValue::VOBJ);
3741+    if (verbose) {
3742+        ret.push_back(Pair("name", data.name));
3743+    }
3744+    ret.push_back(Pair("purpose", data.purpose));
    promag commented at 10:54 pm on April 9, 2018:
    pushKV.
    jnewbery commented at 2:18 pm on April 10, 2018:
    fixed
  42. laanwj added this to the "Blockers" column in a project
  43. laanwj commented at 8:46 am on April 10, 2018: member

    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

  44. in src/wallet/rpcwallet.cpp:3892 in 86d3394de8 outdated 
    3887+
3888+    // Find all addresses that have the given label
3889+    UniValue ret(UniValue::VOBJ);
3890+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
3891+        if (item.second.name == label) {
3892+            ret.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));
    promag commented at 2:12 pm on April 10, 2018:
    pushKV.
    jnewbery commented at 2:18 pm on April 10, 2018:
    fixed
  45. promag commented at 2:13 pm on April 10, 2018: member
    Will test. Some nits thou.
  46. in doc/release-notes-pr12892.md:28 in 86d3394de8 outdated 
    23+- Labels can be deleted by reassigning all addresses using the `setlabel` RPC method.
24+- The returned object from `getaddressesbylabel` is a json object with the addresses
25+  as keys instead of an array of strings.
26+
27+Conceptually, labels are associated with addresses, instead of addresses
28+associated with labels. (unlike with accounts.)
    promag commented at 2:14 pm on April 10, 2018:

    Nit,

    0associated with labels (unlike with accounts).
  47. jnewbery force-pushed on Apr 10, 2018
  48. jnewbery commented at 2:20 pm on April 10, 2018: member

    Thanks @promag - I’ve addressed your comments.

    86d3394 -> 74a9e81 . Only change is addressing promag comments:

     0diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
 1index aeea7af..50495d0 100644
 2--- a/src/wallet/rpcwallet.cpp
 3+++ b/src/wallet/rpcwallet.cpp
 4@@ -3739,9 +3739,9 @@ static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool v
 5 {
 6     UniValue ret(UniValue::VOBJ);
 7     if (verbose) {
 8-        ret.push_back(Pair("name", data.name));
 9+        ret.pushKV("name", data.name);
10     }
11-    ret.push_back(Pair("purpose", data.purpose));
12+    ret.pushKV("purpose", data.purpose);
13     return ret;
14 }
15 
16@@ -3889,7 +3889,7 @@ UniValue getaddressesbylabel(const JSONRPCRequest& request)
17     UniValue ret(UniValue::VOBJ);
18     for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
19         if (item.second.name == label) {
20-            ret.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));
21+            ret.pushKV(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false));
22         }
23     }
24 
25diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
26index 7e0ce54..673d91c 100644
27--- a/src/wallet/wallet.cpp
28+++ b/src/wallet/wallet.cpp
29@@ -3640,7 +3640,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
30     return result;
31 }
32 
33-void CWallet::DeleteLabel(std::string label)
34+void CWallet::DeleteLabel(const std::string& label)
35 {
36     WalletBatch batch(*database);
37     batch.EraseAccount(label);
38diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
39index 4d59059..99b09ec 100644
40--- a/src/wallet/wallet.h
41+++ b/src/wallet/wallet.h
42@@ -989,7 +989,7 @@ public:
43     std::map<CTxDestination, CAmount> GetAddressBalances();
44 
45     std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;
46-    void DeleteLabel(std::string label);
47+    void DeleteLabel(const std::string& label);
48 
49     isminetype IsMine(const CTxIn& txin) const;
50     /**
  49. in src/wallet/rpcwallet.cpp:3918 in 74a9e812da outdated 
    3913+            "\nReturns the list of all labels, or labels that are assigned to addresses with a specific purpose.\n"
3914+            "\nArguments:\n"
3915+            "1. \"purpose\"  (string, optional) Address purpose to list labels for ('send','receive'). An empty string is the same as not providing this argument.\n"
3916+            "\nResult:\n"
3917+            "[                      (json array of string)\n"
3918+            "  \"label\",  (string) Label name\n"
    promag commented at 2:25 pm on April 10, 2018:

    Nit, could have better alignment:

    0Result:
1[                      (json array of string)
2  "label",  (string) Label name
3  ...
4]
    jnewbery commented at 3:06 pm on April 10, 2018:
    fixed
  50. in src/wallet/rpcwallet.cpp:3944 in 74a9e812da outdated 
    3937+    }
3938+
3939+    std::set<std::string> label_set;
3940+    for (const std::pair<CTxDestination, CAddressBookData>& entry : pwallet->mapAddressBook) {
3941+        if (purpose.empty() || entry.second.purpose == purpose) {
3942+            label_set.insert(entry.second.name);
    promag commented at 2:27 pm on April 10, 2018:

    Build ret here:

    0if (label_set.insert(entry.second.name).second) {
1    ret.push_back(name);
2}
    jnewbery commented at 3:05 pm on April 10, 2018:
    Placing into a set sorts the labels by name (mapAddressBook will be sorted by key, which is the CTxDestination)
    jnewbery commented at 3:41 pm on April 10, 2018:
    Added a comment to explain that this is to sort the labels.
  51. in src/wallet/rpcwallet.cpp:218 in cd62be8583 outdated 
    217-            "1. \"label\"         (string, required) The label name for the address. It can also be set to the empty string \"\" to represent the default label. The label does not need to exist, it will be created and a new address created  if there is no label by the given name.\n"
218+            "1. \"label\"         (string, required) The label for the address. It can also be set to the empty string \"\" to represent the default label.\n"
219+            "2. \"force\"         (bool, optional) Whether the label should be created if it does not yet exist. If False, the RPC will return an error if called with a label that doesn't exist. Defaults to False.\n"
220             "\nResult:\n"
221-            "\"address\"          (string) The label bitcoin address\n"
222+            "\"address\"          (string) The 'label address' for the label\n"
    ryanofsky commented at 2:37 pm on April 10, 2018:
    Would suggest changing this to “(string) The current receiving address for the label.” I think ’label address’ is pretty opaque and introducing it as a formal term is less helpful than just saying “current receiving address” or “default receiving address” when there’s a need to refer to it. Receiving address / receive_address is also how it’s referred to in wallet_labels.py.
    jnewbery commented at 3:23 pm on April 10, 2018:
    yes, agree that’s better.
  52. in src/wallet/rpcwallet.cpp:215 in 74a9e812da outdated 
    214+            "getlabeladdress \"label\" ( force ) \n"
215+            "\nReturns the default receiving address for this label. This will reset to a fresh address once there's a transaction that spends to it.\n"
216             "\nArguments:\n"
217-            "1. \"label\"         (string, required) The label name for the address. It can also be set to the empty string \"\" to represent the default label. The label does not need to exist, it will be created and a new address created  if there is no label by the given name.\n"
218+            "1. \"label\"         (string, required) The label for the address. It can also be set to the empty string \"\" to represent the default label.\n"
219+            "2. \"force\"         (bool, optional) Whether the label should be created if it does not yet exist. If False, the RPC will return an error if called with a label that doesn't exist. Defaults to False.\n"
    promag commented at 2:41 pm on April 10, 2018:
    Defaults to depends on the method.
    ryanofsky commented at 3:01 pm on April 10, 2018:

    Defaults to depends on the method.

    I guess simplest way to fix this would be to say “Defaults to false, unless the getaccountaddress method alias is being called, in which case the default is true for backwards compatibility.”

    If you wanted to be more clever I guess you could adapt the help text.

    All of this is making me less a fan of the original suggestion to have a force argument, though: https://github.com/bitcoin/bitcoin/pull/7729/files/fef41789e8d7293ccfd859bc90fa0a3b2484752d#r177781021

    I definitely wouldn’t object if you wanted to drop it.

    jnewbery commented at 3:16 pm on April 10, 2018:
    thanks! fixed
    jnewbery commented at 3:26 pm on April 10, 2018:

    I guess simplest way to fix this would be to say…

    Yes, that’s clear. Updated.

    All of this is making me less a fan of the original suggestion to have a force argument, though.

    Please - can we not hold this PR up on the details of the getlabeladdress RPC? I would much rather we merged something that made progress towards removing the accounts system, and then worked out what to do about getlabeladdress later.

  53. promag commented at 2:56 pm on April 10, 2018: member

    Looks good, more comments 😄

    In some calls, if the response is empty, RPC_WALLET_INVALID_LABEL_NAME is raised instead of just returning []. However calling listlabels with an unknown purpose will return []. Consistency would be nice (I prefer the empty array, I don’t see it as an error).

    The format of some help messages could be fixed, but I guess it’s not relevant at this point.

    BTW, I also don’t see the value of getlabeladdress. Maybe just keep the old getaccountaddress under a deprecation flag?

  54. in src/wallet/rpcwallet.cpp:331 in 74a9e812da outdated 
    330+    std::string label = LabelFromValue(request.params[1]);
331 
332-    // Only add the label if the address is yours.
333     if (IsMine(*pwallet, dest)) {
334-        // Detect when changing the label of an address that is the 'unused current key' of another label:
335+        // Detect when changing the label of an address that is the 'label address' of another label:
    ryanofsky commented at 3:14 pm on April 10, 2018:
    Maybe say receiving address instead of ’label address’ here too.
    jnewbery commented at 3:24 pm on April 10, 2018:
    fixed
  55. jnewbery force-pushed on Apr 10, 2018
  56. jnewbery commented at 3:32 pm on April 10, 2018: member

    Thanks @promag and @ryanofsky - I’ve answered/addressed your comments.

    In some calls, if the response is empty, RPC_WALLET_INVALID_LABEL_NAME is raised instead of just returning []. However calling listlabels with an unknown purpose will return [].

    I don’t think that’s inconsistent - calling eg getaddressesbylabel with a non-existent label throws an error. Calling listlabels with a purpose that no addresses are using returns the list of all addresses that have that purpose (ie []).

    I also don’t see the value of getlabeladdress

    You and me both pal, but let’s defer that discussion for a future PR.

  57. jnewbery force-pushed on Apr 10, 2018
  58. jnewbery force-pushed on Apr 10, 2018
  59. in doc/release-notes-pr12892.md:4 in 883605dc4c outdated 
    0@@ -0,0 +1,28 @@
1+'label' API for wallet
2+----------------------
3+
4+A new 'label' API has been introduced for the wallet. This is intended as a
    ryanofsky commented at 4:29 pm on April 10, 2018:

    This is great! There are a lot of changes I would suggest but I thought it would be easier to just make them in a wiki page:

    https://github.com/bitcoin-core/bitcoin-devwiki/wiki/12892-Label-API-release-notes

    The main change is trying to organize the RPC information in a table.

    Feel free to delete the wiki page and move anything here.

  60. ryanofsky commented at 4:40 pm on April 10, 2018: member
    utACK 883605dc4c0d28f32b724a2cf46fee87711ab6e9. Just for the record, the reason I hadn’t posted an ack earlier was because I wanted to go through the release notes, not because of #12892 (review). But I get that #7729 was open for 2 years, so you might be trying to make up lost time. :stuck_out_tongue:
  61. jnewbery force-pushed on Apr 10, 2018
  62. jnewbery commented at 4:57 pm on April 10, 2018: member

    Thanks for the release notes suggestions Russ. I’ve taken them almost exactly verbatim from your wiki, with the following change:

    delete the previous label associated with an address, instead of making an implicit getaccountaddress call to ensure the previous label still has a receiving address

    to

    delete the previous label associated with an address when the final address using that label is reassigned to a different label, instead of making an implicit getaccountaddress call to ensure the previous label still has a receiving address

  63. ryanofsky commented at 5:16 pm on April 10, 2018: member
    utACK cc505e493ac06901776332a538cde5624e83fa84. I made a few very minor tweaks to punctuation and wording in the wiki, but feel free to ignore these unless perhaps you’re updating the PR anyway.
  64. jnewbery commented at 5:35 pm on April 10, 2018: member

    feel free to ignore these unless perhaps you’re updating the PR anyway.

    Thanks, let’s leave these for now. Minor punctuation/wording can be fixed when release notes are merged at the end of the release cycle.

  65. jnewbery force-pushed on Apr 10, 2018
  66. jnewbery commented at 5:39 pm on April 10, 2018: member

    feel free to ignore these unless perhaps you’re updating the PR anyway.

    Bleurgh. lint-whitespace didn’t like my release notes. Fixed them and pulled in Russ’s changes.

  67. jnewbery force-pushed on Apr 10, 2018
  68. jamesob commented at 8:56 pm on April 10, 2018: member

    utACK https://github.com/bitcoin/bitcoin/pull/12892/commits/b00faff9ed653ea28d02ff6d821183389fcd8b29

    Looks good. The release notes are very helpful.

  69. in src/wallet/rpcwallet.cpp:3940 in b00faff9ed outdated 
    3935+    std::string purpose;
3936+    if (!request.params[0].isNull()) {
3937+        purpose = request.params[0].get_str();
3938+    }
3939+
3940+    // Add to a list to sort by label name, then insert into Univalue array
    promag commented at 11:00 pm on April 10, 2018:

    Add to a set ...?

    Anyway, I mean there is no point in sorting the response, could very well be an unsorted list. We just want to make sure there are no repeated labels.

    jnewbery commented at 11:28 pm on April 10, 2018:
    Thanks. Fixed to Add to a set
  70. promag commented at 11:00 pm on April 10, 2018: member
    utACK b00faff.
  71. [wallet] [rpc] introduce 'label' API for wallet 
    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.

Thanks to Pierre Rochard for test fixes.
    189e0ef33e
  72. [docs] Add release notes for wallet 'label' API. 41ba061804
  73. jnewbery force-pushed on Apr 10, 2018
  74. jnewbery commented at 11:28 pm on April 10, 2018: member
    Fixed comment. b00faff -> 41ba061
  75. promag commented at 8:18 am on April 11, 2018: member
    utACK 41ba061.
  76. in src/wallet/rpcwallet.cpp:230 in 189e0ef33e outdated 
    226@@ -226,6 +227,21 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
227 
228     // Parse the label first so we don't generate a key if there's an error
229     std::string label = LabelFromValue(request.params[0]);
230+    bool force = request.strMethod == "getaccountaddress" ? true : false;
    kallewoof commented at 8:54 am on April 11, 2018:
    Nit: ? true : false is redundant.
    jnewbery commented at 1:06 pm on April 11, 2018:
    duh. Will tidy up in follow-up PR.
    promag commented at 1:17 pm on April 11, 2018:
    Let it go, will go away when accounts are removed.
    kallewoof commented at 1:37 pm on April 11, 2018:
    Yeah, not worth a PR for just that.
    jnewbery commented at 7:10 pm on April 11, 2018:
    Fixed in #12953
  77. kallewoof commented at 9:06 am on April 11, 2018: member
    utACK 41ba0618048b7b2d7e257bb52cf8f138d29dabd3
  78. laanwj merged this on Apr 11, 2018
  79. laanwj closed this on Apr 11, 2018
  80. laanwj referenced this in commit 9b3370d1c6 on Apr 11, 2018
  81. fanquake removed this from the "Blockers" column in a project
  82. jasonbcox referenced this in commit 71da2efe0b on Nov 8, 2019
  83. laanwj referenced this in commit d8a66626d6 on Nov 26, 2019
  84. sidhujag referenced this in commit 44df8dbe56 on Nov 27, 2019
  85. jasonbcox referenced this in commit cc5951bf2a on Oct 1, 2020
  86. sidhujag referenced this in commit c81cfe7f9e on Nov 10, 2020
  87. xdustinface referenced this in commit 71d4486006 on Dec 18, 2020
  88. xdustinface referenced this in commit 557862de0b on Dec 18, 2020
  89. xdustinface referenced this in commit d6be289a08 on Dec 22, 2020
  90. xdustinface referenced this in commit 5de3ab5d6a on Dec 22, 2020
  91. gades referenced this in commit f9771013dd on Mar 12, 2022
  92. DrahtBot locked this on Aug 16, 2022


jnewbery MarcoFalke jonasschnelli promag PierreRochard laanwj ryanofsky jamesob kallewoof

Labels
Wallet

Milestone
0.17.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me