rpc: introduce 'label' API for wallet

laanwj commented at 4:02 pm on March 21, 2016: 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 (that would be a follow-up pull).

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. (in contrast to accounts.)
Labels have no balance
- No getreceivedbylabel
- No balances in listlabels
- listreceivedbylabel can show received transactions to addresses with a label, not use the account tally (currently it is removed, but according to discussion that goes too far as this doesn’t inherently have to do with balance)
- listlabels has no minconf or watchonly argument
- Remove move
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.
These calls stay the same, with account argument renamed to label:
- importaddress
- importprivkey
- importpubkey

API

Short description of every RPC call: for detailed information check RPC help. The general idea is to offer the same functionality as the GUI label system. Labels are simply a name for an address, or a group of addresses.

Do not use the deprecated account system and the label system with the same wallet at the same time. These APIs use the same underlying data in the database for (slightly) different purposes, using them interchangeably will give unexpected results. (Just like using the GUI labels and account system at the same time. Using the GUI labels and the label API at the same time, however, is no problem)

getlabel: returns the label (and other address book data) associated with an address
- This exposes the fields in the CAddressBookData structure, which is currently the ‘purpose’ (sending address, receiving address) and ‘destdata’ (used for storing payment requests IIRC)
getaddressesbylabel: get addresses labelled with one label
listlabels: list all labels (or labels with a certain purpose, such as receive/send)
setlabel: assign a label to an address
getlabeladdress: get the ’label address’ for the specified label. This gets an unused address with the label, creating one if necessary should be removed according to discussion

These calls have a deprecated account parameter, which can be turned into a label-parameter as is:

listtransactions

Open questions

Should there be such a thing as a ’label address’? My initial feeling about this was ’no’, labels are just a name for one or more addresses, intuitively there is no “default address”, and it also isn’t a GUI feature (->no)

laanwj added the label Wallet on Mar 21, 2016

laanwj added the label RPC on Mar 21, 2016

jonasschnelli commented at 4:14 pm on March 21, 2016: contributor

Concept ACK. I just wonder if this makes the wallet code more complex (add another layer).

My idea was it to duplicate the current wallet implementation (cp src/wallet src/newwallet-approach) and add such things there (after removing the accounts-related code). Also the Bip32 and @CodeShark multi-wallet PR could be added there.

The second wallet could come without API stableness (for the first two releases or so) and could be marked as experimental.

laanwj commented at 4:17 pm on March 21, 2016: member

I just wonder if this makes the wallet code more complex (add another layer).

I disagree:

All of this functionality is required anyway to support the GUI.
Yes, there is some intentional duplication, but only until the account calls are ripped out, which should be one of the next steps.
There are only added RPC calls in rpcwallet.cpp. The CWallet class is not complicated by this.

The point here is to give a non-deprecated equivalent to the ’label’ system as used in the GUI, so the subset of the ‘account system’ that people are still allowed to use. This is a required, but up to now missing part of deprecating the account system.

I’m not trying to rule out any other work that is being done such as multi-wallet support. I think this is pretty much orthogonal. As for alternative wallets, they’ve been proposed since at least 2012 - but none have materialized yet. And none of this change rules them out.

jonasschnelli commented at 4:33 pm on March 21, 2016: contributor

As for alternative wallets, they’ve been proposed since at least 2012 - but none have materialized yet. And none of this change rules them out.

Yes. I agree. This PR has a clear value.

luke-jr commented at 7:42 pm on March 21, 2016: member

Note getaccountaddress does not presently get a “default” address, it gets an unused address with the label, creating one if necessary. This seems useful only for mining, since no other context can guarantee an address hasn’t been “used” but not sent to yet. I can’t think of a good way to deprecate this, however.

laanwj commented at 8:53 am on March 22, 2016: member

it gets an unused address with the label, creating one if necessary.

I wonder if we can find a better (or at least simpler, the GetAccountAddress function is pretty terrible) way to do this, now that we’re creating a new API anyway. Need to think about this a bit.

laanwj commented at 7:10 am on March 23, 2016: member

@luke-jr

I wonder if we can find a better (or at least simpler, the GetAccountAddress function is pretty terrible) way to do this, now that we’re creating a new API anyway. Need to think about this a bit.

I thought of the following: you could use two labels, one for the ‘active’ address, one for the ’normal’. Say mining_active and mining.

When the miner needs an address it will:

addr = getaddressesbylabel mining_active (if no address returned, go to “if so” directly below and skip the first)
getreceivedbyaddress - check with wallet that addr has been used before (or maybe add a more convenient RPC call for checking a single address)
If so:
- setlabel <addr> mining - move current address to normal label
- addr = getnewaddress mining_active - generate new address in mining_active label
- Use addr for mining to
If not:
- Use addr for mining to

This is a little bit more involved at the user side, but it avoids special administration (needing to keep around CAccount structure per label) at the server side.

luke-jr commented at 7:25 am on April 7, 2016: member

That looks like a lot of overhead, and this is a rather time-sensitive call, as the miner is working on stale work until it’s done.

Also, why are there no getreceivedbylabel/listreceivedbylabel? These don’t have anything to do with balances.

laanwj commented at 2:19 pm on April 7, 2016: member

That looks like a lot of overhead, and this is a rather time-sensitive call, as the miner is working on stale work until it’s done.

I’d suggest to try it. It shouldn’t be much slower.

Also, why are there no getreceivedbylabel/listreceivedbylabel? These don’t have anything to do with balances.

Looks like you’re right. getreceivedbyaccount doesn’t actually return the account balance, but the total number of coins sent to the addresses that make up the account?

listreceivedbyaccount on the other hand goes over the account tally. But I agree if it only were to show actual transactions sent to addresses belonging to a label it’d be ok.

sipa commented at 2:24 pm on April 7, 2016: member

@luke-jr Wouldn’t it be feasible to instead generate a sequence of deterministic addresses for mining, for example using BIP32 derivation with the block height as index?

luke-jr commented at 2:37 pm on April 7, 2016: member

I’d suggest to try it. It shouldn’t be much slower. @laanwj getreceivedbyaddress at least would loop over all the wtx… and then there’s the additional latency from the back and forth of multiple calls. I haven’t tried it yet, though.

Wouldn’t it be feasible to instead generate a sequence of deterministic addresses for mining, for example using BIP32 derivation with the block height as index? @sipa Perhaps, if the wallet had a way to do this. Using the height seems incompatible with gap limits, though?

sipa commented at 2:48 pm on April 7, 2016: member

@luke-jr I mean the mining software can do derivation, and import the keys into the wallet when a block is found.

MarcoFalke commented at 12:02 pm on May 22, 2016: member

The functional test coverage for accounts is minimal or not existent, I think we should move forward with this pull.

Needs rebase.

jonasschnelli commented at 12:54 pm on May 22, 2016: contributor

Re-Concept ACK. I think this solution makes more sense than the closed #7830. Needs tests, rebase and release-note mentioning.

wallclockbuilder commented at 4:04 pm on May 22, 2016: none

Needs tests.

laanwj commented at 1:13 pm on June 2, 2016: member

@wallclockbuilder No shit, have you seen the TODOs at the bottom of the opening post?

laanwj commented at 1:15 pm on June 2, 2016: member

To be clear I posted this to get comments on the API, is there anything left to be done there? I’m going to write tests when it is clear that this is what we want at all.

laanwj added this to the milestone Future on Jun 13, 2016

MarcoFalke commented at 8:05 pm on July 14, 2016: member

Do not use the deprecated account system and the label system with the same wallet at the same time […] optional: a flag in the wallet to prevent use of both the account and label API

I would not consider this optional. User will always do what you not want them to do.

sipa commented at 12:46 pm on August 25, 2016: member

Concept ACK

andrewbaine commented at 2:38 am on September 24, 2016: none

listtransactions has an “account” argument where you now you would pass “*” if you need to supply non-default args for count, from, and includeWatchOnly. Will there be a way to query for transactions affecting any address with a given label? Could we tack on a “label” argument to listtransactions?

laanwj commented at 5:25 am on September 24, 2016: member

I think the account argument of listtransactions could simply be re-used as a label argument. As listing transactions to a label has nothing to do with per-label balances there is no need to drop that particular functionality,

andrewbaine commented at 3:00 pm on October 3, 2016: none

repurposing the “account” argument to be “label” makes sense to me

jtimon commented at 10:38 pm on November 17, 2016: contributor

Fast review ACK (besides needing rebase and the promised tests). The API is actually more than I expected but still simple enough, I think.

optional: a flag in the wallet to prevent use of both the account and label API

I’m not sure it’s worth to bother. I think we should go ahead and completely remove account functionality within 0.14 instead. But whatever we do, it can be done in a later PR.

instagibbs commented at 2:06 pm on November 18, 2016: member

labels are associated with addresses, instead of addresses associated with labels

Description nit: not sure I catch the distinction here.

Should there be such a thing as a ’label address'?

I’d say no unless there is a compelling use-case that can’t be replicated another way.

in src/wallet/rpcwallet.cpp: in d036fa4411 outdated

2502@@ -2497,6 +2503,216 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2503     return result;
2504 }
2505 
2506+UniValue getlabeladdress(const UniValue& params, bool fHelp)

instagibbs commented at 2:27 pm on November 18, 2016:

would getnewlabeladdress be wrong? As-is gave me the impression that this was some static address.

in src/wallet/rpcwallet.cpp: in d036fa4411 outdated

2513+            "getlabeladdress \"label\"\n"
2514+            "\nReturns the current 'label address' for this label.\n"
2515+            "\nArguments:\n"
2516+            "1. \"label\"       (string, required) The label for the address. It can also be set to the empty string \"\" to represent the default label.\n"
2517+            "\nResult:\n"
2518+            "\"bitcoinaddress\"   (string) The 'label address' for the label\n"

instagibbs commented at 2:28 pm on November 18, 2016:

perhaps mention latest unused or something similar.

laanwj commented at 12:47 pm on November 21, 2016:

I still think we should get rid of this.

in src/wallet/rpcwallet.cpp: in d036fa4411 outdated

2630+    if (fHelp || params.size() > 1)
2631+        throw runtime_error(
2632+            "listlabels ( \"purpose\" )\n"
2633+            "\nReturns the list of all labels, or labels that are assigned to addresses with a specific purpose.\n"
2634+            "\nArguments:\n"
2635+            "1. \"purpose\"  (string, optional) Address purpose to list labels for ('send','receive'). An empty string is the same as not providing this argument.\n"

instagibbs commented at 2:42 pm on November 18, 2016:

I’m not as familiar with accounts as others, but is this type of parameter actually going to be used? When would you want to list labels that go certain directions? I have a feeling labels people choose will reflect this already.

laanwj commented at 12:47 pm on November 21, 2016:

The goal of this API is to expose exactly the same functionality that the GUI uses. The GUI distinguishes between different kind of labels (to show sending/receiving addresses), so it should be offered here as well.

in src/wallet/rpcwallet.cpp: in d036fa4411 outdated

2673+        return NullUniValue;
2674+
2675+    if (fHelp || params.size() < 1 || params.size() > 2)
2676+        throw runtime_error(
2677+            "setlabel \"bitcoinaddress\" \"label\"\n"
2678+            "\nSets the label associated with the given address.\n"

instagibbs commented at 2:46 pm on November 18, 2016:

it also adds it to the address book if previously unknown, correct?

laanwj commented at 12:51 pm on November 21, 2016:

Indeed, that is what setting a label does. If there is no labeling information associated with an address it will create that. We don’t want to use the term “address book” here I think.

instagibbs commented at 2:50 pm on November 18, 2016: member

concept ACK, with some nits/questions.

I am not an expert on how accounts exactly work as-is.

It also may be helpful to give a brief motivation in the OP. What’s wrong with accounts, and what label fixes about that. Currently it’s a list of differences without clear motivation.

laanwj commented at 12:48 pm on November 21, 2016: member

It also may be helpful to give a brief motivation in the OP. What’s wrong with accounts, and what label fixes about that. Currently it’s a list of differences without clear motivation.

See #3816

jonasschnelli commented at 12:53 pm on November 21, 2016: contributor

utACK a2557ffb80543af5e92c3db03d257fe594b0d910 needs (trivial) rebase for the new JSONRPCRequest object passing.

instagibbs commented at 1:09 pm on November 21, 2016: member

@laanwj yes I read the issue, but there are some disagreements in that thread about what the actual issue is(malleability being the first issue noted?). I assume it’s along the lines of “people want to watermark addresses, but bitcoind wallet shouldn’t try to be an accounting system for those labels”.

motatoes commented at 10:52 pm on November 22, 2016: none

Hi. When is this new feature expected to be rolled out?

sipa commented at 10:54 pm on November 22, 2016: member

@motatoes Like everything, when it’s ready. That may be in 0.14.0 or later.

in src/wallet/rpcwallet.cpp: in d036fa4411 outdated

2700+        // If so, delete the account record for it. Labels, unlike addresses can be deleted,
2701+        // and we wouldn't do this, the record would stick around forever.
2702+        if (pwalletMain->mapAddressBook.count(address.Get()))
2703+        {
2704+            string strOldLabel = pwalletMain->mapAddressBook[address.Get()].name;
2705+            if (address == GetAccountAddress(strOldLabel))

ryanofsky commented at 11:17 pm on December 12, 2016:

This condition should be changed to if (strOldLabel != strLabel && address == GetAccountAddress(strOldLabel)), so calling setlabel on an address which already has the same label will just be a no-op, instead of creating an unexpected side effect where the label’s default label address gets discarded.

ryanofsky commented at 0:00 am on December 13, 2016: member

@laanwj , I created an RPC test for this change here: https://github.com/ryanofsky/bitcoin/commit/2dac8eb0d915b4c04764eff100d81718b2cd9a90

Feel free to incorporate it in this PR, or I could create a separate one.

jtimon commented at 3:52 pm on December 16, 2016: contributor

Needs rebase

Tintress15 approved

Tintress15 commented at 1:37 am on February 18, 2017: none

Get a balance

jtimon commented at 10:42 pm on March 6, 2017: contributor

Needs rebase again, sorry for not reviewing after the last rebase.

sipa commented at 7:39 pm on March 16, 2017: member

Concept reACK

laanwj added this to the milestone 0.15.0 on Mar 16, 2017

laanwj removed this from the milestone Future on Mar 16, 2017

TheBlueMatt commented at 0:30 am on March 21, 2017: member

Grr, posted on wrong PR…(and cant delete reviews?)

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

TheBlueMatt commented at 9:13 pm on March 24, 2017: member

Needs rebase. Concept ACK

in src/wallet/rpcwallet.cpp:3122 in d036fa4411 outdated

2676+        throw runtime_error(
2677+            "setlabel \"bitcoinaddress\" \"label\"\n"
2678+            "\nSets the label associated with the given address.\n"
2679+            "\nArguments:\n"
2680+            "1. \"bitcoinaddress\"  (string, required) The bitcoin address to be associated with an label.\n"
2681+            "2. \"label\"           (string, required) The label to assign to the address.\n"

JeremyRubin commented at 10:26 pm on March 28, 2017:

Is the label optional or required?

If it’s required update L2675 to if (fHelp || params.size() != 2) and L2693-2695 to std::string strLabel = AccountFromValue(params[1]);, if optional update the docstring.

in src/wallet/rpcwallet.cpp:2711 in d036fa4411 outdated

2706+                DeleteAccount(strOldLabel);
2707+        }
2708+        pwalletMain->SetAddressBook(address.Get(), strLabel, "receive");
2709+    }
2710+    else
2711+        pwalletMain->SetAddressBook(address.Get(), strLabel, "send");

JeremyRubin commented at 10:31 pm on March 28, 2017:

Braces please!

in src/wallet/rpcwallet.cpp:2543 in e21f890df6 outdated

2536@@ -2537,13 +2537,13 @@ UniValue getlabeladdress(const UniValue& params, bool fHelp)
2537 /** Convert CAddressBookData to JSON record.
2538  * The verbosity of the output is configurable based on the command.
2539  */
2540-static UniValue AddressBookDataToJSON(const CAddressBookData& data, bool includeName, bool includeDestData)
2541+static UniValue AddressBookDataToJSON(const CAddressBookData& data, bool verbose)
2542 {
2543     UniValue ret(UniValue::VOBJ);
2544-    if (includeName)
2545+    if (verbose)

JeremyRubin commented at 10:34 pm on March 28, 2017:

Does the order of push_back matter here? Might be cleaner to lump all if (verbose) under one branch…

in src/wallet/rpcwallet.cpp:3816 in d036fa4411 outdated

2529+    string strLabel = AccountFromValue(params[0]);
2530+
2531+    UniValue ret(UniValue::VSTR);
2532+
2533+    ret = GetAccountAddress(strLabel).ToString();
2534+    return ret;

JeremyRubin commented at 10:41 pm on March 28, 2017:

It seems like L2529-2534 could be a one or two liner, rather than 4 (and maybe get rid of the whitespace).

in src/wallet/rpcwallet.cpp:2548 in d036fa4411 outdated

2543+    if (includeName)
2544+        ret.push_back(Pair("name", data.name));
2545+    ret.push_back(Pair("purpose", data.purpose));
2546+    if (includeDestData) {
2547+        UniValue ddata(UniValue::VOBJ);
2548+        BOOST_FOREACH(const PAIRTYPE(std::string, std::string) &item, data.destdata)

JeremyRubin commented at 10:42 pm on March 28, 2017:

does a builtin for range loop work here?

in src/wallet/rpcwallet.cpp:2617 in d036fa4411 outdated

2612+
2613+    string strLabel = AccountFromValue(params[0]);
2614+
2615+    // Find all addresses that have the given label
2616+    UniValue ret(UniValue::VOBJ);
2617+    BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, CAddressBookData)& item, pwalletMain->mapAddressBook)

JeremyRubin commented at 10:43 pm on March 28, 2017:

Can this not be a for range loop?

in src/wallet/rpcwallet.cpp:2659 in d036fa4411 outdated

2654+    std::string purpose;
2655+    if (params.size() > 0)
2656+        purpose = params[0].get_str();
2657+
2658+    std::set<std::string> setLabels;
2659+    BOOST_FOREACH(const PAIRTYPE(CTxDestination, CAddressBookData)& entry, pwalletMain->mapAddressBook) {

JeremyRubin commented at 10:45 pm on March 28, 2017:

can use for range?

in src/wallet/rpcwallet.cpp:2664 in d036fa4411 outdated

2659+    BOOST_FOREACH(const PAIRTYPE(CTxDestination, CAddressBookData)& entry, pwalletMain->mapAddressBook) {
2660+        if (purpose.empty() || entry.second.purpose == purpose)
2661+            setLabels.insert(entry.second.name);
2662+    }
2663+    UniValue ret(UniValue::VARR);
2664+    BOOST_FOREACH(const std::string &name, setLabels)

JeremyRubin commented at 10:46 pm on March 28, 2017:

can use for range?

in src/wallet/rpcwallet.cpp:2656 in d036fa4411 outdated

2651+
2652+    LOCK2(cs_main, pwalletMain->cs_wallet);
2653+
2654+    std::string purpose;
2655+    if (params.size() > 0)
2656+        purpose = params[0].get_str();

JeremyRubin commented at 10:47 pm on March 28, 2017:

validate the purpose string?

in src/wallet/rpcwallet.cpp:3142 in a2557ffb80 outdated

2694+    if (params.size() > 1)
2695+        strLabel = AccountFromValue(params[1]);
2696+
2697+    if (IsMine(*pwalletMain, address.Get()))
2698+    {
2699+        // Detect when changing the label of an address that is the 'label address' of another label:

JeremyRubin commented at 10:51 pm on March 28, 2017:

This behavior is probably just to mirror prior behavior, but perhaps a better alternative would be to create a fresh address for the account & allow deleting of account via another mechanism.

in src/wallet/rpcwallet.cpp:2697 in a2557ffb80 outdated

2692+
2693+    string strLabel;
2694+    if (params.size() > 1)
2695+        strLabel = AccountFromValue(params[1]);
2696+
2697+    if (IsMine(*pwalletMain, address.Get()))

JeremyRubin commented at 10:54 pm on March 28, 2017:

What’s the correct behavior when address is watchonly?

JeremyRubin approved

JeremyRubin commented at 10:55 pm on March 28, 2017: contributor

utack!

Overall, looks good. Just nits from me really. Only thing I was concerned about is the proper behavior for watchonly addresses, perhaps some added documentation for that would be good.

ryanofsky commented at 12:25 pm on April 7, 2017: member

@laanwj, do you want to cherry pick my unit test into this PR? (https://github.com/ryanofsky/bitcoin/commit/2dac8eb0d915b4c04764eff100d81718b2cd9a90)

In IRC recently, you requested that people “please review the API, not the code,” and I think the unit test can help with this, because it demonstrates the API in action, covers various subtleties, and is also well commented.

laanwj commented at 4:03 pm on April 7, 2017: member

@laanwj, do you want to cherry pick my unit test into this PR? (ryanofsky@2dac8eb)

Yes, I will, thank you!

TheBlueMatt commented at 4:26 pm on April 23, 2017: member

API ACK. Still needs rebase :(

gmaxwell commented at 9:58 pm on May 19, 2017: contributor

I expected to be able to attach multiple labels to a single address.

laanwj commented at 6:43 am on May 20, 2017: member

I expected to be able to attach multiple labels to a single address.

That is not possible in the GUI either, in the OP I’ve defined the scope as:

The general idea is to offer the same functionality as the GUI label system

Such functionality could be added in the future.

jtimon commented at 9:26 am on May 20, 2017: contributor

Nobody seems to have big problems with the API for a while. Perhaps it’s time to rebase and start reviewing the code itself

jnewbery commented at 3:25 pm on June 12, 2017: member

I wanted to test this so I rebased on master and cherry-picked Russ’s test. I’ve pushed it here: https://github.com/jnewbery/bitcoin/tree/pr7729 . Feel free to reset onto that commit. Note that there were quite a few refactors (multiwallet, namespaces, database wrapper, JSONRPC request, rpc names args), so someone else should probably review the rebase to make sure I haven’t missed anything.

Note that the final test in Russ’s test script currently fails. I added some trace to the script and see that the address returned by getlabeladdress isn’t included in the list of addresses returned by getaddressesbylabel:

0-146-> getaddressesbylabel ["c"]
1<-146- [0.000135] {"n1HsWg6ANV5Z8SLojKNyjqCRf5saqQKQed": {"purpose": "receive"}, "mfYMvaXrHp2RUxe1q1PwRgS8AokYjgUSmf": {"purpose": "receive"}, "mhVHLa9KCHGMXHBNpyfAHNZXmwKaaTg6Fj": {"purpose": "receive"}}
2-147-> getlabeladdress ["c"]
3<-147- [0.001559] "mnvNe2eG3hmderC3y5E4SYLNnwWsYrSg1c"
4-148-> getlabel ["mnvNe2eG3hmderC3y5E4SYLNnwWsYrSg1c"]
5<-148- [0.000136] {"name": "c", "destdata": {}, "purpose": "receive"}

I haven’t yet dug into why that’s the case.

jnewbery commented at 6:40 pm on June 12, 2017: member

I’ve made a few more changes to rpcwallet.cpp to reflect more recent style guidelines (no BOOST_FOREACH and braces for if statements. I’ve also committed a fixup commit for the failing test case. @laanwj feel free to squash that if you’re happy with it.

Once you’ve taken the changes I’ll add my review comments.

laanwj force-pushed on Jun 14, 2017

laanwj commented at 9:19 am on June 14, 2017: member

@jnewbery Thanks so much! Updated this pull to that branch.

jnewbery commented at 2:45 pm on June 14, 2017: member

needs rebase again! (although this one should be easier - just fixing the BASE_SCRIPTS list in test_runner.py)

in src/wallet/walletdb.h:207 in 21a49c206f outdated

203@@ -204,6 +204,7 @@ class CWalletDB
204     bool WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccountingEntry& acentry);
205     bool ReadAccount(const std::string& strAccount, CAccount& account);
206     bool WriteAccount(const std::string& strAccount, const CAccount& account);
207+    bool EraseAccount(const std::string& strAccount);

jnewbery commented at 3:08 pm on June 14, 2017:

I think the comment above may need updating to:

remove comments about accounting entries (or say they’re deprecated)
comment that ‘acc’ database entries are for labels.

in src/wallet/rpcwallet.cpp:3855 in 21a49c206f outdated

2995+            "\nArguments:\n"
2996+            "1. \"bitcoinaddress\"  (string, required) The bitcoin address for label lookup.\n"
2997+            "\nResult:\n"
2998+            "  { (json object with information about address)\n"
2999+            "    \"name\": \"labelname\" (string) The label\n"
3000+            "    \"purpose\": \"string\" (string) Purpose of address (\"send\" for sending address, \"receive\" for receiving address)\n"

jnewbery commented at 3:47 pm on June 14, 2017:

If I’m understanding AddressBookDataToJSON() correctly, then the result will also include an array of destdata.

jnewbery commented at 2:29 pm on March 28, 2018:

Comment: https://github.com/bitcoin/bitcoin/pull/7729/files#r121986789 not addressed. Result also includes an array of destdata. Please update help text.

EDIT: I think we should just drop the destdata from the response. It wasn’t available in the old account RPCs, and it appears to me that the only place we add to destdata is in saveReceiveRequest().

We can always add destdata to the response in a later PR if required.

in src/wallet/rpcwallet.cpp:2955 in 21a49c206f outdated

2951+        );
2952+
2953+    LOCK2(cs_main, pwallet->cs_wallet);
2954+
2955+    // Parse the label first so we don't generate a key if there's an error
2956+    std::string strLabel = AccountFromValue(request.params[0]);

jnewbery commented at 4:29 pm on June 14, 2017:

It’d be nice to eventually rename this function LabelFromValue(). That can be done in a follow-up PR.

in src/wallet/rpcwallet.cpp:3927 in 21a49c206f outdated

3067+    if (request.fHelp || request.params.size() > 1)
3068+        throw std::runtime_error(
3069+            "listlabels ( \"purpose\" )\n"
3070+            "\nReturns the list of all labels, or labels that are assigned to addresses with a specific purpose.\n"
3071+            "\nArguments:\n"
3072+            "1. \"purpose\"  (string, optional) Address purpose to list labels for ('send','receive'). An empty string is the same as not providing this argument.\n"

jnewbery commented at 4:31 pm on June 14, 2017:

nit: I don’t think An empty string is the same as not providing this argument. is required

promag commented at 2:49 pm on July 7, 2017:

Agree with @jnewbery, empty value should be an error then.

promag commented at 2:58 pm on July 7, 2017:

BTW, why is this argument needed? I mean, the client can filter the result. IMO pagination is more interesting.

in src/wallet/rpcwallet.cpp:3142 in 21a49c206f outdated

3138+        strLabel = AccountFromValue(request.params[1]);
3139+    }
3140+
3141+    if (IsMine(*pwallet, address.Get()))
3142+    {
3143+        // Detect when changing the label of an address that is the 'label address' of another label:

jnewbery commented at 4:37 pm on June 14, 2017:

I think we should remove the concept of ’label address’ and change this section to delete the label when removing the final address from that label.

in src/wallet/rpcwallet.cpp:3136 in 21a49c206f outdated

3132+    if (!address.IsValid()) {
3133+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
3134+    }
3135+
3136+    std::string strLabel;
3137+    if (request.params.size() > 1){

jnewbery commented at 4:48 pm on June 14, 2017:

What happens if request.params.size() == 1? Are you supposed to be able to remove a label using this rpc without a second argument? If so, I think you want to call pwallet->DelAddressBook.

meshcollider commented at 6:19 am on October 20, 2017:

Is there any difference between setting an empty label vs deleting the label?

laanwj commented at 10:17 am on October 20, 2017:

Well if you set all addresses with a certain label to the empty label, that label is effectively deleted. Deleting the label is a shortcut to remove it from all addresses.

jnewbery commented at 5:03 pm on June 14, 2017: member

A few comments inline. More general feedback:

I really don’t like the concept of a ’label address’. It seems to go against the philosophy of ’labels are associated with addresses, instead of addresses associated with labels’, and makes the distinction between labels and accounts more confusing. You also mentioned in the OP this would greatly simplify the code though - it would be impossible to get rid of the CAccount structure due to this.. So it sounds to me like removing label addresses makes the interface clearer for users and makes the implementation simpler, so let’s just do it. #7729 (comment) seems to be a sensible solution for miners using account addresses.
I agree that users should be able to add multiple labels to an address. That can be done as a follow-up PR.
You say in the OP Do not use the deprecated account system and the label system with the same wallet at the same time.. There doesn’t appear to be any discussion yet on how to achieve this, or how to handle migrating from accounts to labels. One suggestion: hide these new RPCs with old wallet versions, add a new enablelabels RPC which upgrades the wallet to a new version, and on the new version, hide the account RPCs. The enablelabels RPC should describe the implications of moving from accounts to labels. I think this requires a bit more thought and discussion.

laanwj force-pushed on Jun 14, 2017

in src/wallet/rpcwallet.cpp:3148 in 54a9b2d94a outdated

3145@@ -3146,7 +3146,7 @@ UniValue setlabel(const JSONRPCRequest& request)
3146         if (pwallet->mapAddressBook.count(address.Get()))
3147         {
3148             std::string strOldLabel = pwallet->mapAddressBook[address.Get()].name;
3149-            if (address == GetAccountAddress(pwallet, strOldLabel)) {
3150+            if (strOldLabel != strLabel && address == GetAccountAddress(pwallet, strOldLabel)) {

ryanofsky commented at 5:12 pm on June 15, 2017:

Note: This change makes calling setlabel on an address which already has the same label a no-op, see #7729 (review).

laanwj commented at 11:18 am on June 25, 2017: member

So to summarize above discussion, the following changes should be made:

Add listreceivedbylabel, listing transactions to addresses that have a certain label
Remove the default label address. Discussion confirms my intuition that it’s better to get rid of this.
- ‘acc’ database entries, as well as CAccount, would not be necessary for labels. After this change, labels have no metadata, so a lot of code can be simplified.
setlabel on an address that already has the same label should be a no-op
Documentation changes / nits

RPC which upgrades the wallet to a new version, and on the new version, hide the account RPCs. The enablelabels RPC should describe the implications of moving from accounts to labels. I think this requires a bit more thought and discussion.

Agree re: versioning. We already have a command line argument to upgrade the wallet version, -upgradewallet. Wouldn’t that be enough? Either label or account RPCs could be usable based on the wallet version. I don’t see why a RPC to do the upgrade would be preferable, it’s something that will be done at most once.

laanwj force-pushed on Jun 29, 2017

in src/wallet/rpcwallet.cpp:3830 in 0e3ef241d1 outdated

2969+    if (verbose) {
2970+        ret.push_back(Pair("name", data.name));
2971+    }
2972+    ret.push_back(Pair("purpose", data.purpose));
2973+    if (verbose) {
2974+        UniValue ddata(UniValue::VOBJ);

promag commented at 2:42 pm on July 7, 2017:

Nit, rename ddata to dest_data.

in src/wallet/rpcwallet.cpp:155 in 0e3ef241d1 outdated

151@@ -152,6 +152,11 @@ UniValue getnewaddress(const JSONRPCRequest& request)
152     return CBitcoinAddress(keyID).ToString();
153 }
154 
155+void DeleteAccount(CWallet * const pwallet, std::string strAccount)

promag commented at 2:45 pm on July 7, 2017:

Nit, shouldn’t we follow the convention in new code in favor of consistency? In that case rename strAccount to account_name for instance?

promag commented at 3:01 pm on July 7, 2017: member

Partial review. Is this going forward?

jnewbery commented at 3:52 pm on July 7, 2017: member

We already have a command line argument to upgrade the wallet version, -upgradewallet. Wouldn’t that be enough

ACK - yes this is enough.

The wallet startup command line arguments always seemed strange to me, and if we were building this from scratch we might implement upgradewallet as an RPC or an external tool. However, it makes sense to use the existing infrastructure.

Either label or account RPCs could be usable based on the wallet version.

✅ sounds good to me.

Two additional points:

Obviously documentation is key here. The wallet upgrade will effectively remove the accounts from the wallet, so we need to make sure users understand the implication before upgrading.
When will we be able to remove the account code entirely? At that point will bitcoind no longer support wallets older than the new version? Is there precedence for how we deprecate wallet features like this?

Is this going forward?

It feels like we’ve missed the boat for 0.15. @laanwj - it’d be great to get this in early in the next cycle. I’ll have some spare time in the next few weeks. Let me know if there’s anything I can do to help here. I’m happy to help write and test the upgrade code.

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

laanwj added this to the milestone 0.16.0 on Jul 11, 2017

laanwj removed this from the milestone 0.15.0 on Jul 11, 2017

mmgen commented at 11:27 am on July 23, 2017: none

@laanwj importaddress, importpubkey and importprivkey have “label” arguments that currently set the account for an address. (To delete the old account for an address and create a new one, it’s enough to just re-import the address.) Won’t these calls need to be updated as well? I don’t see them in your patch.

mmgen commented at 9:39 am on July 29, 2017: none

@jnewbery Since I got no reponse from @laanwj, I’ll refer you to my above comment. The behavior I mentioned can be easily demonstrated with the following:

$ bitcoin-cli importaddress n2FgXPKwuFkCXF946EnoxWJDWF2VwQ6q8J 'Label 1'
$ bitcoin-cli listaccounts 0 true | grep 'Label 1'
 "Label 1": 0.00000000,
$ bitcoin-cli importaddress n2FgXPKwuFkCXF946EnoxWJDWF2VwQ6q8J 'Label 2'
$ bitcoin-cli listaccounts 0 true | grep 'Label 1'
$ bitcoin-cli listaccounts 0 true | grep 'Label 2'
 "Label 2": 0.00000000,

jnewbery commented at 2:39 am on July 30, 2017: member

@mmgen : @laanwj is very busy with the 0.15 release right now, so I’m not surprised he hasn’t updated this PR. I’m planning on picking this up early in 0.16 and I’ll review your comments then. Any help with review/testing then will be very much appreciated!

mmgen commented at 10:59 am on July 30, 2017: none

@jnewbery Thanks! Will be ready to help when needed.

achow101 commented at 7:12 pm on October 14, 2017: member

Concept ACK, needs rebase.

Maybe this and #11497 should be done together?

Earlier in this thread, it was suggested that the version number be bumped to distinguish between whether to use accounts or wallets. Because of the HD wallets thing with the optional features in the version number, would we want to bump the version number (which would then mean only HD wallets can use labels) or do something else?

ryanofsky commented at 2:51 pm on October 16, 2017: member

Should probably just drop the version number idea now that we have the -deprecatedrpc mechanism. Seems like adding versioning to the decision of which rpcs to enable would make the transition more complicated without providing a clear benefit.

jamesscookk approved

meshcollider commented at 9:12 am on October 20, 2017: contributor

I’ve rebased this on current master here, as well as fixed a few of the nits above: https://github.com/MeshCollider/bitcoin/tree/201710_labels_rebased Feel free to use that if it helps, and squash my commits into yours if needed :)

Still to-do summary from comments:

Remove getlabeladdress RPC
Add listreceivedbylabel RPC
Incorporate #11497 to disable accounts at the same time (including renaming of account params to label)

mmgen commented at 10:07 am on October 20, 2017: none

importaddress, importpubkey and importprivkey have ’label’ arguments (see my note above) that currently set the account for the address. After the label API is introduced, will these now set the label, or the account? Since both APIs will continue to exist side-by-side for a time, this could be a source of potential confusion for users. My wallet uses importaddress to set the account, which I use as an alias for a given address.

laanwj commented at 10:19 am on October 20, 2017: member

@mmgen

importaddress, importpubkey and importprivkey have ’label’ arguments (see my note above) that currently set the account for the address. After the label API is introduced, will these now set the label, or the account?

The same arguments will be used for label (where it makes sense) to ease the transition and avoid unused holes in the argument list. This is mentioned in the OP. For 0.16 it will be possible to switch between label and account API with a command line argument, but will default to labels. 0.17 will likely drop suppport for accounts completely. @MeshCollider

I’ve rebased this on current master here, as well as fixed a few of the nits above:

Thanks! Will take that over.

ryanofsky referenced this in commit b105fb9634 on Oct 20, 2017

ryanofsky referenced this in commit cb7ef6fd59 on Oct 21, 2017

ryanofsky commented at 5:04 pm on October 21, 2017: member

Rebased this on top of #11536 in https://github.com/ryanofsky/bitcoin/commits/pr/label. Has 2 commits:

6a0d27412d9c1b16e4bdfc406ee1e7b0ee6a2a51 rpc: introduce ’label’ API for wallet
fef41789e8d7293ccfd859bc90fa0a3b2484752d [wallet] Make setlabel idempotent

ryanofsky referenced this in commit 62df1688ef on Oct 23, 2017

ryanofsky referenced this in commit d7e0e284a1 on Oct 23, 2017

ryanofsky referenced this in commit a8e9ac981a on Nov 27, 2017

ryanofsky referenced this in commit d31cc7fcb2 on Dec 1, 2017

ryanofsky referenced this in commit 93b303cf55 on Dec 1, 2017

ryanofsky referenced this in commit b6753cc2fe on Dec 1, 2017

ryanofsky referenced this in commit d15bc6857c on Dec 1, 2017

ryanofsky referenced this in commit 8f3f7764e0 on Dec 1, 2017

ryanofsky referenced this in commit 52dec73c24 on Jan 2, 2018

ryanofsky referenced this in commit d10545ef08 on Jan 4, 2018

sipa commented at 10:51 am on January 6, 2018: member

Can we haz this for 0.16?

ryanofsky referenced this in commit 709f61a277 on Jan 6, 2018

ryanofsky referenced this in commit ca202db9f5 on Jan 6, 2018

ryanofsky referenced this in commit 7faf506341 on Jan 6, 2018

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

Can we haz this for 0.16?

I think the best way to move this forward would be to review and merge #11536, which gets account -> label renaming out of the way so this PR can be small and simple and doesn’t need to be constantly rebased.

laanwj commented at 9:36 pm on January 6, 2018: member

0.17, not 0.16. 0.16 is supposed to be the intermediate segwit wallet release, I’d strongly prefer to not add any other features for it that potentially hold it up longer.

On Jan 6, 2018 11:51, “Pieter Wuille” notifications@github.com wrote:

Can we haz this for 0.16?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/7729#issuecomment-355739097, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutpkOslEiyC81Ta-E4I1OTix7NPwBks5tH1A8gaJpZM4H1S2Y .

MarcoFalke removed this from the milestone 0.16.0 on Jan 7, 2018

MarcoFalke added this to the milestone 0.17.0 on Jan 7, 2018

ryanofsky referenced this in commit b387136f8b on Jan 11, 2018

ryanofsky referenced this in commit 85aa8a7d8a on Jan 11, 2018

ryanofsky referenced this in commit 0e2537eda0 on Jan 11, 2018

ryanofsky referenced this in commit 4f91d7d756 on Jan 16, 2018

ryanofsky referenced this in commit 3cafe5db76 on Jan 17, 2018

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

The rebased version seems quite different from this (or at least from the description), yet is relevant in order to understand #11536. Maybe this PR should be closed and replaced?

ryanofsky commented at 8:54 am on January 18, 2018: member

The rebased version seems quite different from this (or at least from the description), yet is relevant in order to understand #11536. Maybe this PR should be closed and replaced?

This should not be true. The rebased version Iinked in my comment above #7729 (comment), has the exact same code changes as the current version of this PR, plus some additional “account” -> “label” renames for some “account” occurrences missed here. #11536 + #7729 should be a pure superset of #7729.

If there are any actual differences that you see, please point them out in #11536 or here so I can fix my branch.

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

Maybe it’s only the description of this PR that’s out of date. In particular this list suggests that label RPC commands are a subset of the original account commands:

Whereas the actual code and commit messages don’t remove any methods.

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

The description at the top of this PR is slightly out of date. It should be edited to match the description in the actual commit message, which should be fully up to date.

Whereas the actual code and commit messages don’t remove any methods.

I think you are taking “remove” in the description too literally. Perhaps it could be rephrased. This PR is adding label functions, not removing existing account functions (that meant to be done later in order to preserve compatibility). When the description says “remove” it is just referring to not adding a new label function corresponding to an existing account function.

ryanofsky referenced this in commit a4291c5695 on Jan 18, 2018

ryanofsky referenced this in commit 59da7c2201 on Jan 18, 2018

ryanofsky referenced this in commit a87969582e on Jan 18, 2018

ryanofsky referenced this in commit e001e17292 on Jan 25, 2018

ryanofsky referenced this in commit 450d39ec20 on Jan 25, 2018

ryanofsky referenced this in commit 44eb4b4122 on Feb 1, 2018

ryanofsky referenced this in commit 52d6da74fd on Feb 12, 2018

ryanofsky referenced this in commit 63d296598b on Feb 12, 2018

ryanofsky referenced this in commit 435f535222 on Feb 14, 2018

ryanofsky referenced this in commit 4c8e3265b2 on Feb 23, 2018

ryanofsky referenced this in commit 5217956f1d on Mar 7, 2018

ryanofsky referenced this in commit 8e7396f8dd on Mar 7, 2018

Sjors commented at 10:09 pm on March 8, 2018: member

Can you add a remark at the top that this (maybe) depends on #11536?

ryanofsky referenced this in commit df650a2c56 on Mar 13, 2018

ryanofsky referenced this in commit 10c3a14e9e on Mar 13, 2018

ryanofsky referenced this in commit 947e86d171 on Mar 15, 2018

ryanofsky referenced this in commit dea1bab4e4 on Mar 19, 2018

ryanofsky referenced this in commit 045eeb8870 on Mar 19, 2018

ryanofsky referenced this in commit 39f5cf82ad on Mar 19, 2018

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.

6a0d27412d

[wallet] Make setlabel idempotent

Prevent setlabel calls on an address that already has the same label, and which
is the already the default "label address" of that label, from generating a
new default address.

This way calling setlabel on an address that already has the specified label is
a no-op.

Change suggested by Russell Yanofsky <russ@yanofsky.org> in
https://github.com/bitcoin/bitcoin/pull/7729#discussion_r92064042

fef41789e8

laanwj referenced this in commit cead84b72d on Mar 22, 2018

ryanofsky referenced this in commit fd249e199f on Mar 23, 2018

ryanofsky commented at 0:55 am on March 23, 2018: member

Rebased PR at https://github.com/ryanofsky/bitcoin/commits/pr/label:

6a0d27412d9c1b16e4bdfc406ee1e7b0ee6a2a51 rpc: introduce ’label’ API for wallet
fef41789e8d7293ccfd859bc90fa0a3b2484752d [wallet] Make setlabel idempotent

laanwj force-pushed on Mar 23, 2018

laanwj commented at 11:43 am on March 23, 2018: member

@ryanofsky Thank you! Replaced the branch with that one.

in src/wallet/rpcwallet.cpp:304 in fef41789e8

304+            "1. \"bitcoinaddress\"  (string, required) The bitcoin address to be associated with a label.\n"
305+            "2. \"label\"           (string, required) The label to assign to the address.\n"
306             "\nExamples:\n"
307-            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\" \"tabby\"")
308-            + HelpExampleRpc("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\", \"tabby\"")
309+            + HelpExampleCli("setlabel", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ\" \"tabby\"")

MarcoFalke commented at 11:06 pm on March 23, 2018:

Unrelated change?

If you really wanted to change that DummyAddress(Params()) would be a better choice.

in src/wallet/rpcwallet.cpp:213 in 6a0d27412d outdated

212@@ -208,11 +213,11 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
213     if (request.fHelp || request.params.size() != 1)
214         throw std::runtime_error(
215             "getlabeladdress \"label\"\n"
216-            "\nReturns the current Bitcoin address for receiving payments to this label.\n"
217+            "\nReturns the current 'label address' for this label.\n"
218             "\nArguments:\n"
219-            "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"

Sjors commented at 10:41 am on March 28, 2018:

I’m fine with removing this remark, but note that the behavior of creating a new address still exists (though without an address type param). It might make sense to deprecate that behavior in a followup PR, and require getnewaddress if getlabeladdress doesn’t return anything. That also makes the choice of potentially reusing an address more explicit.

Sjors approved

Sjors commented at 10:47 am on March 28, 2018: member

tACK fef41789e8.

I just noticed that changing or removing a label in the console live updates the Receiving Addresses UI, nice! For a future PR we should figure out what to do with the Receive and Transaction tab “labels”, as these seem to use an independent mechanism.

laanwj commented at 10:53 am on March 28, 2018: member

For a future PR we should figure out what to do with the Receive and Transaction tab “labels”, as these seem to use an independent mechanism.

Transaction tab labels use the same mechanism so it would be a matter of listening to a notification and repainting. Receive requests are stored separately, because they have extra metadata besides a label.

Sjors commented at 11:03 am on March 28, 2018: member

Ah yes, I see it. Restarting QT doesn’t change or remove a label from Requested payments history for me, as you point out. Switching tabs is enough to update labels in the transactions view.

in src/wallet/rpcwallet.cpp:4031 in fef41789e8

4023@@ -3882,6 +4024,26 @@ static const CRPCCommand commands[] =
4024     { "wallet",             "removeprunedfunds",                &removeprunedfunds,             {"txid"} },
4025     { "wallet",             "rescanblockchain",                 &rescanblockchain,              {"start_height", "stop_height"} },
4026 
4027+    /** Account functions (deprecated) */
4028+    { "wallet",             "getaccountaddress",                &getlabeladdress,               {"account"} },
4029+    { "wallet",             "getaccount",                       &getaccount,                    {"address"} },
4030+    { "wallet",             "getaddressesbyaccount",            &getaddressesbyaccount,         {"account"} },
4031+    { "wallet",             "getaddressinfo",                   &getaddressinfo,                {"address"} },

jnewbery commented at 2:24 pm on March 28, 2018:

I don’t think getaddressinfo should be deprecated. It’s not an account rpc

ryanofsky commented at 3:39 pm on March 28, 2018:

I don’t think getaddressinfo should be deprecated. It’s not an account rpc

Good catch. getaddressinfo should remain where it was above “getbalance”. It’s my fault for accidentally moving it into this section during a rebase.

in src/wallet/rpcwallet.cpp:298 in fef41789e8

294@@ -290,14 +295,14 @@ UniValue setlabel(const JSONRPCRequest& request)
295 
296     if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
297         throw std::runtime_error(
298-            "setlabel \"address\" \"label\"\n"
299+            "setlabel \"bitcoinaddress\" \"label\"\n"

jnewbery commented at 2:27 pm on March 28, 2018:

Why rename the arguments/return values to bitcoinaddress everywhere? Seems like a gratuitous API break.

address is used in many RPCs for a bitcoin address. Why not continue that convention? (and if you really must change this, current style guidelines call for snake_case for args)

ryanofsky commented at 3:24 pm on March 28, 2018:

Why rename the arguments/return values to bitcoinaddress everywhere?

Agree it would be better to stick to address, but just as a historical note, this wasn’t a “gratuitious” API break when it was originally written in 8571fee617aa22a43d73efa2560cd2ddf55c2478, because it preceded #11536, so the setlabel RPC was brand new.

in src/wallet/rpcwallet.cpp:3853 in fef41789e8

3848+            "getlabel \"bitcoinaddress\"\n"
3849+            "\nReturns the label associated with the given address.\n"
3850+            "\nArguments:\n"
3851+            "1. \"bitcoinaddress\"  (string, required) The bitcoin address for label lookup.\n"
3852+            "\nResult:\n"
3853+            "  { (json object with information about address)\n"

jnewbery commented at 2:31 pm on March 28, 2018:

Can we make this RPC return an array? We may want to be able to attach multiple labels to addresses in the future (https://github.com/bitcoin/bitcoin/pull/7729#issuecomment-302854998), and making this RPC return an array will allow us to do that without a breaking API change

ryanofsky commented at 3:31 pm on March 28, 2018:

Can we make this RPC return an array?

It might be better to not add a getlabel RPC at all but instead just return this information in getaddressinfo (recently added in #10583).

in src/wallet/rpcwallet.cpp:3958 in fef41789e8

3953+        if (purpose.empty() || entry.second.purpose == purpose){
3954+            setLabels.insert(entry.second.name);
3955+        }
3956+    }
3957+    UniValue ret(UniValue::VARR);
3958+    for (const std::string &name : setLabels) {

jnewbery commented at 2:34 pm on March 28, 2018:

Should we sort the label names before returning?

in src/wallet/rpcwallet.cpp:3911 in fef41789e8

3906+    UniValue ret(UniValue::VOBJ);
3907+    for (const std::pair<CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
3908+        if (item.second.name == strLabel) {
3909+            ret.push_back(Pair(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false)));
3910+        }
3911+    }

jnewbery commented at 2:46 pm on March 28, 2018:

Should this throw an error if the label doesn’t exist? Currently it returns an empty object.

in src/wallet/rpcwallet.cpp:216 in fef41789e8

212@@ -208,11 +213,11 @@ UniValue getlabeladdress(const JSONRPCRequest& request)
213     if (request.fHelp || request.params.size() != 1)
214         throw std::runtime_error(
215             "getlabeladdress \"label\"\n"
216-            "\nReturns the current Bitcoin address for receiving payments to this label.\n"
217+            "\nReturns the current 'label address' for this label.\n"

jnewbery commented at 3:00 pm on March 28, 2018:

The behaviour for this RPC is weird. If called for a label that doesn’t exist, it creates a new label, and then adds a new address as the ’label address’ for that label. That’s not very intuitive, and I think it’s a bad experience (for example if a user typos an existing label name). Can we change this so that the rpc returns an error if called with a non-existent label name?

in test/functional/wallet_labels.py:18 in fef41789e8

14@@ -15,6 +15,7 @@
15 
16 from test_framework.test_framework import BitcoinTestFramework
17 from test_framework.util import assert_equal
18+from collections import defaultdict

jnewbery commented at 3:03 pm on March 28, 2018:

nit: standard library imports before project imports please!

jnewbery commented at 3:05 pm on March 28, 2018: member

Comments inline.

Feel free to squash my second commit into the first.

jnewbery commented at 9:02 pm on April 4, 2018: member

@laanwj - it seems like you’re struggling to find time to maintain this PR.

I’d like to push this forwards and make sure it gets in for v0.17. Would you object if I took ownership?

GSPP commented at 10:06 am on April 5, 2018: none

Maybe there should be a call returning all addresses and all labels (a bulk call). If you want to dump all address-label pairs this would involve many RPC calls right now. This bulk call should even (optionally?) include internal addresses such as pool addresses. Simply all data. I have had the need many times to reflect over absolutely all wallet contents and such an API call would have been very welcome.

jnewbery commented at 5:39 pm on April 5, 2018: member

I’ve opened #12892 to supersede this PR. Thanks for all your work @laanwj !

MarcoFalke commented at 5:46 pm on April 5, 2018: member

Indeed thanks to @laanwj for initiating it and coming up with the original version of the code. And thx to @ryanofsky and @jnewbery for picking it up.

MarcoFalke closed this on Apr 5, 2018

HashUnlimited referenced this in commit 981db7d8c1 on Jul 31, 2018

lionello referenced this in commit 77d59004b1 on Nov 7, 2018

xdustinface referenced this in commit 685dec4421 on Dec 18, 2020

xdustinface referenced this in commit 32610f0613 on Dec 18, 2020

xdustinface referenced this in commit 2392f03ffa on Dec 18, 2020

xdustinface referenced this in commit 409e9ac8da on Dec 18, 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

gades referenced this in commit b9049d5089 on Mar 12, 2022

DrahtBot locked this on Aug 16, 2022

rpc: introduce ’label’ API for wallet #7729

API

Open questions