rpc: introduce 'label' API for wallet

[laanwj]

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.
  • but at least @luke-jr is relying on this behavior in his miner.
  • this would greatly simplify the code though - it would be impossible to get rid of the CAccount structure due to this.

TODO

• Needs RPC tests (thanks ryanofsky)
• Needs mention in release notes
• optional: a flag in the wallet to prevent use of both the account and label API

[jonasschnelli]

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]

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]

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]

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]

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]

@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]

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]

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]

@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]

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]

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

[MarcoFalke]

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

Needs rebase.

[jonasschnelli]

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

[wallclockbuilder]

Needs tests.

[laanwj]

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

[laanwj]

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.

[MarcoFalke]

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]

Concept ACK

[andrewbaine]

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]

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]

repurposing the "account" argument to be "label" makes sense to me

[jtimon]

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]

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.

[instagibbs]

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.

[instagibbs]

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

[instagibbs]

perhaps mention latest unused or something similar.

[laanwj]

I still think we should get rid of this.

[instagibbs]

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]

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.

[instagibbs]

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

[laanwj]

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.

[laanwj]

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]

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

[instagibbs]

@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]

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

[sipa]

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

[ryanofsky]

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]

@laanwj , I created an RPC test for this change here: ryanofsky/bitcoin@2dac8eb

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

[jtimon]

Needs rebase

[Tintress15]

Get a balance

[jtimon]

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

[sipa]

Concept reACK

[TheBlueMatt]

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

[TheBlueMatt]

Needs rebase. Concept ACK

[JeremyRubin]

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.

[JeremyRubin]

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.

[JeremyRubin]

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

[JeremyRubin]

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

[JeremyRubin]

does a builtin for range loop work here?

[JeremyRubin]

Can this not be a for range loop?

[JeremyRubin]

validate the purpose string?

[JeremyRubin]

can use for range?

[JeremyRubin]

can use for range?

[JeremyRubin]

What's the correct behavior when address is watchonly?

[JeremyRubin]

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.

[JeremyRubin]

Braces please!

[ryanofsky]

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

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]

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

Yes, I will, thank you!

[TheBlueMatt]

API ACK. Still needs rebase :(

[gmaxwell]

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

[laanwj]

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]

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]

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:

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

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

[jnewbery]

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]

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

[jnewbery]

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

[jnewbery]

A few comments inline. More general feedback:

1. 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.

2. I agree that users should be able to add multiple labels to an address. That can be done as a follow-up PR.

3. 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.

[jnewbery]

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.

[jnewbery]

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

[jnewbery]

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

[jnewbery]

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

[promag]

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

[promag]

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

[jnewbery]

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.

[jnewbery]

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.

[ryanofsky]

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

[laanwj]

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.

[promag]

Partial review. Is this going forward?

[promag]

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]

Nit, rename ddata to dest_data.

[promag]

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

[promag]

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

[jnewbery]

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:

1. 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.
2. 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.

[mmgen]

@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]

@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]

@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!