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

41ba061 [docs] Add release notes for wallet 'label' API. (John Newbery)
189e0ef [wallet] [rpc] introduce 'label' API for wallet (Wladimir J. van der Laan)

Pull request description:

  Add label API to wallet RPC.

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

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

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

Tree-SHA512: 45cc313c68ad529ce3a15c02181d2ab0083a7e14fe824e2cde34972713fecce512e3d4b9aa46db5355f2baa857c44b234d4fe9709225bc23c7ebbc0e03febbf5

Author: laanwj

doc/release-notes-pr12892.md

@@ -0,0 +1,32 @@
+'label' API for wallet
+----------------------
+
+A new 'label' API has been introduced for the wallet. This is intended as a
+replacement for the deprecated 'account' API.
+
+The label RPC methods mirror the account functionality, with the following functional differences:
+
+- Labels can be set on any address, not just receiving addresses. This functionality was previously only available through the GUI.
+- Labels can be deleted by reassigning all addresses using the `setlabel` RPC method.
+- There isn't support for sending transactions _from_ a label, or for determining which label a transaction was sent from.
+- Labels do not have a balance.
+
+Here are the changes to RPC methods:
+
+| Deprecated Method       | New Method            | Notes       |
+| :---------------------- | :-------------------- | :-----------|
+| `getaccount`            | `getaddressinfo`      | `getaddressinfo` returns a json object with address information instead of just the name of the account as a string. |
+| `getaccountaddress`     | `getlabeladdress`     | `getlabeladdress` throws an error by default if the label does not already exist, but provides a `force` option for compatibility with existing applications. |
+| `getaddressesbyaccount` | `getaddressesbylabel` | `getaddressesbylabel` returns a json object with the addresses as keys, instead of a list of strings. |
+| `getreceivedbyaccount`  | `getreceivedbylabel`  | _no change in behavior_ |
+| `listaccounts`          | `listlabels`          | `listlabels` does not return a balance or accept `minconf` and `watchonly` arguments. |
+| `listreceivedbyaccount` | `listreceivedbylabel` | Both methods return new `label` fields, along with `account` fields for backward compatibility. |
+| `move`                  | n/a                   | _no replacement_ |
+| `sendfrom`              | n/a                   | _no replacement_ |
+| `setaccount`            | `setlabel`            | Both methods now: <ul><li>allow assigning labels to any address, instead of raising an error if the address is not receiving address.<li>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. |
+
+| Changed Method         | Notes   |
+| :--------------------- | :------ |
+| `addmultisigaddress`   | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility. |
+| `getnewaddress`        | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility. |
+| `listunspent`          | Returns new `label` fields, along with `account` fields for backward compatibility. |

doc/release-notes.md

@@ -63,16 +63,6 @@ RPC changes
 
 - The `createrawtransaction` RPC will now accept an array or dictionary (kept for compatibility) for the `outputs` parameter. This means the order of transaction outputs can be specified by the client.
 - The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
-- Wallet `getnewaddress` and `addmultisigaddress` RPC `account` named
-  parameters have been renamed to `label` with no change in behavior.
-- Wallet `getlabeladdress`, `getreceivedbylabel`, `listreceivedbylabel`, and
-  `setlabel` RPCs have been added to replace `getaccountaddress`,
-  `getreceivedbyaccount`, `listreceivedbyaccount`, and `setaccount` RPCs,
-  which are now deprecated. There is no change in behavior between the
-  new RPCs and deprecated RPCs.
-- Wallet `listreceivedbylabel`, `listreceivedbyaccount` and `listunspent` RPCs
-  add `label` fields to returned JSON objects that previously only had
-  `account` fields.
 - `sendmany` now shuffles outputs to improve privacy, so any previously expected behavior with regards to output ordering can no longer be relied upon.
 - The new RPC `testmempoolaccept` can be used to test acceptance of a transaction to the mempool without adding it.
 

src/rpc/client.cpp

@@ -51,6 +51,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listreceivedbylabel", 0, "minconf" },
     { "listreceivedbylabel", 1, "include_empty" },
     { "listreceivedbylabel", 2, "include_watchonly" },
+    { "getlabeladdress", 1, "force" },
     { "getbalance", 1, "minconf" },
     { "getbalance", 2, "include_watchonly" },
     { "getblockhash", 0, "height" },

src/wallet/rpcwallet.cpp

@@ -3640,6 +3640,12 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
     return result;
 }
 
+void CWallet::DeleteLabel(const std::string& label)
+{
+    WalletBatch batch(*database);
+    batch.EraseAccount(label);
+}
+
 bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
 {
     if (nIndex == -1)

src/wallet/wallet.cpp

@@ -549,7 +549,7 @@ class CWalletKey
 };
 
 /**
- * Internal transfers.
+ * DEPRECATED Internal transfers.
  * Database key is acentry<account><counter>.
  */
 class CAccountingEntry
@@ -989,6 +989,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
     std::map<CTxDestination, CAmount> GetAddressBalances();
 
     std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;
+    void DeleteLabel(const std::string& label);
 
     isminetype IsMine(const CTxIn& txin) const;
     /**
@@ -1184,7 +1185,7 @@ class CReserveKey final : public CReserveScript
 
 
 /** 
- * Account information.
+ * DEPRECATED Account information.
  * Stored in wallet with key "acc"+string account name.
  */
 class CAccount

src/wallet/wallet.h

@@ -161,6 +161,11 @@ bool WalletBatch::WriteAccount(const std::string& strAccount, const CAccount& ac
     return WriteIC(std::make_pair(std::string("acc"), strAccount), account);
 }
 
+bool WalletBatch::EraseAccount(const std::string& strAccount)
+{
+    return EraseIC(std::make_pair(std::string("acc"), strAccount));
+}
+
 bool WalletBatch::WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccountingEntry& acentry)
 {
     return WriteIC(std::make_pair(std::string("acentry"), std::make_pair(acentry.strAccount, nAccEntryNum)), acentry);

src/wallet/walletdb.cpp

@@ -204,6 +204,7 @@ class WalletBatch
     bool WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccountingEntry& acentry);
     bool ReadAccount(const std::string& strAccount, CAccount& account);
     bool WriteAccount(const std::string& strAccount, const CAccount& account);
+    bool EraseAccount(const std::string& strAccount);
 
     /// Write destination data key,value tuple to database
     bool WriteDestData(const std::string &address, const std::string &key, const std::string &value);

src/wallet/walletdb.h

@@ -12,6 +12,7 @@
     - sendfrom (with account arguments)
     - move (with account arguments)
 """
+from collections import defaultdict
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
@@ -78,9 +79,12 @@ def run_test(self):
         # recognize the label/address associations.
         labels = [Label(name) for name in ("a", "b", "c", "d", "e")]
         for label in labels:
-            label.add_receive_address(node.getlabeladdress(label.name))
+            label.add_receive_address(node.getlabeladdress(label=label.name, force=True))
             label.verify(node)
 
+        # Check all labels are returned by listlabels.
+        assert_equal(node.listlabels(), [label.name for label in labels])
+
         # Send a transaction to each label, and make sure this forces
         # getlabeladdress to generate a new receiving address.
         for label in labels:
@@ -115,7 +119,7 @@ def run_test(self):
 
         # Check that setlabel can assign a label to a new unused address.
         for label in labels:
-            address = node.getlabeladdress("")
+            address = node.getlabeladdress(label="", force=True)
             node.setlabel(address, label.name)
             label.add_address(address)
             label.verify(node)
@@ -128,6 +132,7 @@ def run_test(self):
                 addresses.append(node.getnewaddress())
             multisig_address = node.addmultisigaddress(5, addresses, label.name)['address']
             label.add_address(multisig_address)
+            label.purpose[multisig_address] = "send"
             label.verify(node)
             node.sendfrom("", multisig_address, 50)
         node.generate(101)
@@ -147,9 +152,7 @@ def run_test(self):
         change_label(node, labels[2].addresses[0], labels[2], labels[2])
 
         # Check that setlabel can set the label of an address which is
-        # already the receiving address of the label. It would probably make
-        # sense for this to be a no-op, but right now it resets the receiving
-        # address, causing getlabeladdress to return a brand new address.
+        # already the receiving address of the label. This is a no-op.
         change_label(node, labels[2].receive_address, labels[2], labels[2])
 
 class Label:
@@ -160,6 +163,8 @@ def __init__(self, name):
         self.receive_address = None
         # List of all addresses assigned with this label
         self.addresses = []
+        # Map of address to address purpose
+        self.purpose = defaultdict(lambda: "receive")
 
     def add_address(self, address):
         assert_equal(address not in self.addresses, True)
@@ -175,8 +180,15 @@ def verify(self, node):
             assert_equal(node.getlabeladdress(self.name), self.receive_address)
 
         for address in self.addresses:
+            assert_equal(
+                node.getaddressinfo(address)['labels'][0],
+                {"name": self.name,
+                 "purpose": self.purpose[address]})
             assert_equal(node.getaccount(address), self.name)
 
+        assert_equal(
+            node.getaddressesbylabel(self.name),
+            {address: {"purpose": self.purpose[address]} for address in self.addresses})
         assert_equal(
             set(node.getaddressesbyaccount(self.name)), set(self.addresses))
 
@@ -192,7 +204,7 @@ def change_label(node, address, old_label, new_label):
     # address of a different label should reset the receiving address of
     # the old label, causing getlabeladdress to return a brand new
     # address.
-    if address == old_label.receive_address:
+    if old_label.name != new_label.name and address == old_label.receive_address:
         new_address = node.getlabeladdress(old_label.name)
         assert_equal(new_address not in old_label.addresses, True)
         assert_equal(new_address not in new_label.addresses, True)

test/functional/wallet_labels.py

@@ -140,7 +140,7 @@ def run_test(self):
         assert_equal(balance, balance_by_label + Decimal("0.1"))
 
         # Create a new label named "mynewlabel" that has a 0 balance
-        self.nodes[1].getlabeladdress("mynewlabel")
+        self.nodes[1].getlabeladdress(label="mynewlabel", force=True)
         received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel(0, True) if r["label"] == "mynewlabel"][0]
 
         # Test includeempty of listreceivedbylabel