Skip to content

Commit

Permalink
Rename account to label where appropriate
Browse files Browse the repository at this point in the history
This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
bitcoin#7729, by getting renaming out of the
way and letting it focus on semantics.

The difference between accounts and labels is that labels apply only to
addresses, while accounts apply to both addresses and transactions
(transactions have "from" and "to" accounts). The code associating accounts
with transactions is clumsy and unreliable so we would like get rid of it.
  • Loading branch information
ryanofsky authored and xdustinface committed Dec 18, 2020
1 parent 122078b commit 32610f0
Show file tree
Hide file tree
Showing 8 changed files with 225 additions and 213 deletions.
4 changes: 4 additions & 0 deletions src/rpc/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "getreceivedbyaddress", 2, "addlocked" },
{ "getreceivedbyaccount", 1, "minconf" },
{ "getreceivedbyaccount", 2, "addlocked" },
{ "getreceivedbylabel", 2, "addlocked" },
{ "listaddressbalances", 0, "minamount" },
{ "listreceivedbyaddress", 0, "minconf" },
{ "listreceivedbyaddress", 1, "addlocked" },
Expand All @@ -56,6 +57,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "listreceivedbyaccount", 1, "addlocked" },
{ "listreceivedbyaccount", 2, "include_empty" },
{ "listreceivedbyaccount", 3, "include_watchonly" },
{ "listreceivedbylabel", 1, "addlocked" },
{ "listreceivedbylabel", 2, "include_empty" },
{ "listreceivedbylabel", 3, "include_watchonly" },
{ "getbalance", 1, "minconf" },
{ "getbalance", 2, "addlocked" },
{ "getbalance", 3, "include_watchonly" },
Expand Down
4 changes: 3 additions & 1 deletion src/rpc/protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ enum RPCErrorCode
//! Wallet errors
RPC_WALLET_ERROR = -4, //!< Unspecified problem with wallet (key not found etc.)
RPC_WALLET_INSUFFICIENT_FUNDS = -6, //!< Not enough funds in wallet or account
RPC_WALLET_INVALID_ACCOUNT_NAME = -11, //!< Invalid account name
RPC_WALLET_INVALID_LABEL_NAME = -11, //!< Invalid label name
RPC_WALLET_KEYPOOL_RAN_OUT = -12, //!< Keypool ran out, call keypoolrefill first
RPC_WALLET_UNLOCK_NEEDED = -13, //!< Enter the wallet passphrase with walletpassphrase first
RPC_WALLET_PASSPHRASE_INCORRECT = -14, //!< The wallet passphrase entered was incorrect
Expand All @@ -87,6 +87,8 @@ enum RPCErrorCode
RPC_WALLET_NOT_SPECIFIED = -19, //!< No wallet specified (error when there are multiple wallets loaded)


//! Backwards compatible aliases
RPC_WALLET_INVALID_ACCOUNT_NAME = RPC_WALLET_INVALID_LABEL_NAME,
//! Unused reserved codes, kept around for backwards compatibility. Do not reuse.
RPC_FORBIDDEN_BY_SAFE_MODE = -2, //!< Server is in safe mode, and command is not allowed in safe mode
};
Expand Down
204 changes: 105 additions & 99 deletions src/wallet/rpcwallet.cpp

Large diffs are not rendered by default.

22 changes: 11 additions & 11 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1049,12 +1049,12 @@ bool CWallet::AccountMove(std::string strFrom, std::string strTo, CAmount nAmoun
return true;
}

bool CWallet::GetAccountDestination(CTxDestination &dest, std::string strAccount, bool bForceNew)
bool CWallet::GetLabelDestination(CTxDestination &dest, const std::string& label, bool bForceNew)
{
WalletBatch batch(*database);

CAccount account;
batch.ReadAccount(strAccount, account);
batch.ReadAccount(label, account);

if (!bForceNew) {
if (!account.vchPubKey.IsValid())
Expand All @@ -1079,8 +1079,8 @@ bool CWallet::GetAccountDestination(CTxDestination &dest, std::string strAccount
return false;

dest = account.vchPubKey.GetID();
SetAddressBook(dest, strAccount, "receive");
batch.WriteAccount(strAccount, account);
SetAddressBook(dest, label, "receive");
batch.WriteAccount(label, account);
} else {
dest = account.vchPubKey.GetID();
}
Expand Down Expand Up @@ -2763,7 +2763,7 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons
for (const CTxOut& out : wtx.tx->vout) {
if (outgoing && IsChange(out)) {
debit -= out.nValue;
} else if (IsMine(out) & filter && (depth >= minDepth || (fAddLocked && wtx.IsLockedByInstantSend())) && (!account || *account == GetAccountName(out.scriptPubKey))) {
} else if (IsMine(out) & filter && (depth >= minDepth || (fAddLocked && wtx.IsLockedByInstantSend())) && (!account || *account == GetLabelName(out.scriptPubKey))) {
balance += out.nValue;
}
}
Expand Down Expand Up @@ -4347,7 +4347,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
return WalletBatch(*database).EraseName(EncodeDestination(address));
}

const std::string& CWallet::GetAccountName(const CScript& scriptPubKey) const
const std::string& CWallet::GetLabelName(const CScript& scriptPubKey) const
{
CTxDestination address;
if (ExtractDestination(scriptPubKey, address) && !scriptPubKey.IsUnspendable()) {
Expand All @@ -4357,9 +4357,9 @@ const std::string& CWallet::GetAccountName(const CScript& scriptPubKey) const
}
}
// A scriptPubKey that doesn't have an entry in the address book is
// associated with the default account ("").
const static std::string DEFAULT_ACCOUNT_NAME;
return DEFAULT_ACCOUNT_NAME;
// associated with the default label ("").
const static std::string DEFAULT_LABEL_NAME;
return DEFAULT_LABEL_NAME;
}

/**
Expand Down Expand Up @@ -4739,15 +4739,15 @@ std::set< std::set<CTxDestination> > CWallet::GetAddressGroupings()
return ret;
}

std::set<CTxDestination> CWallet::GetAccountAddresses(const std::string& strAccount) const
std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) const
{
LOCK(cs_wallet);
std::set<CTxDestination> result;
for (const std::pair<const CTxDestination, CAddressBookData>& item : mapAddressBook)
{
const CTxDestination& address = item.first;
const std::string& strName = item.second.name;
if (strName == strAccount)
if (strName == label)
result.insert(address);
}
return result;
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -1026,7 +1026,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
int64_t IncOrderPosNext(WalletBatch *batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
DBErrors ReorderTransactions();
bool AccountMove(std::string strFrom, std::string strTo, CAmount nAmount, std::string strComment = "") EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool GetAccountDestination(CTxDestination &dest, std::string strAccount, bool bForceNew = false);
bool GetLabelDestination(CTxDestination &dest, const std::string& label, bool bForceNew = false);

void MarkDirty();
bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true);
Expand Down Expand Up @@ -1103,7 +1103,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
std::set<std::set<CTxDestination>> GetAddressGroupings() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
std::map<CTxDestination, CAmount> GetAddressBalances();

std::set<CTxDestination> GetAccountAddresses(const std::string& strAccount) const;
std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;

isminetype IsMine(const CTxIn& txin) const;
/**
Expand Down Expand Up @@ -1134,7 +1134,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

bool DelAddressBook(const CTxDestination& address);

const std::string& GetAccountName(const CScript& scriptPubKey) const;
const std::string& GetLabelName(const CScript& scriptPubKey) const;

void GetScriptForMining(std::shared_ptr<CReserveScript> &script);

Expand Down
136 changes: 68 additions & 68 deletions test/functional/wallet_accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@
# Copyright (c) 2016 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test account RPCs.
"""Test label RPCs.
RPCs tested are:
- getaccountaddress
- getlabeladdress
- getaddressesbyaccount
- listaddressgroupings
- setaccount
- setlabel
- sendfrom (with account arguments)
- move (with account arguments)
"""

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal

class WalletAccountsTest(BitcoinTestFramework):
class WalletLabelsTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 1
Expand Down Expand Up @@ -70,29 +70,29 @@ def run_test(self):

node.generate(1)

# we want to reset so that the "" account has what's expected.
# we want to reset so that the "" label has what's expected.
# otherwise we're off by exactly the fee amount as that's mined
# and matures in the next 100 blocks
node.sendfrom("", common_address, fee)
amount_to_send = 1.0

# Create accounts and make sure subsequent account API calls
# recognize the account/address associations.
accounts = [Account(name) for name in ("a", "b", "c", "d", "e")]
for account in accounts:
account.add_receive_address(node.getaccountaddress(account.name))
account.verify(node)
# Create labels and make sure subsequent label API calls
# 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.verify(node)

# Send a transaction to each account, and make sure this forces
# getaccountaddress to generate a new receiving address.
for account in accounts:
node.sendtoaddress(account.receive_address, amount_to_send)
account.add_receive_address(node.getaccountaddress(account.name))
account.verify(node)
# Send a transaction to each label, and make sure this forces
# getlabeladdress to generate a new receiving address.
for label in labels:
node.sendtoaddress(label.receive_address, amount_to_send)
label.add_receive_address(node.getlabeladdress(label.name))
label.verify(node)

# Check the amounts received.
node.generate(1)
for account in accounts:
for label in labels:
assert_equal(
node.getreceivedbyaddress(account.addresses[0]), amount_to_send)
assert_equal(node.getreceivedbyaccount(account.name), amount_to_send)
Expand All @@ -102,65 +102,65 @@ def run_test(self):
to_account = accounts[(i+1) % len(accounts)]
node.sendfrom(account.name, to_account.receive_address, amount_to_send)
node.generate(1)
for account in accounts:
account.add_receive_address(node.getaccountaddress(account.name))
account.verify(node)
assert_equal(node.getreceivedbyaccount(account.name), 2)
node.move(account.name, "", node.getbalance(account.name))
account.verify(node)
for label in labels:
label.add_receive_address(node.getlabeladdress(label.name))
label.verify(node)
assert_equal(node.getreceivedbylabel(label.name), 2)
node.move(label.name, "", node.getbalance(label.name))
label.verify(node)
node.generate(101)
expected_account_balances = {"": 52000}
for account in accounts:
expected_account_balances[account.name] = 0
for label in labels:
expected_account_balances[label.name] = 0
assert_equal(node.listaccounts(), expected_account_balances)
assert_equal(node.getbalance(""), 52000)

# Check that setaccount can assign an account to a new unused address.
for account in accounts:
for label in labels:
address = node.getaccountaddress("")
node.setaccount(address, account.name)
node.setlabel(address, label.name)
account.add_address(address)
account.verify(node)
assert(address not in node.getaddressesbyaccount(""))

# Check that addmultisigaddress can assign accounts.
for account in accounts:
for label in labels:
addresses = []
for x in range(10):
addresses.append(node.getnewaddress())
multisig_address = node.addmultisigaddress(5, addresses, account.name)['address']
account.add_address(multisig_address)
account.verify(node)
multisig_address = node.addmultisigaddress(5, addresses, label.name)['address']
label.add_address(multisig_address)
label.verify(node)
node.sendfrom("", multisig_address, 50)
node.generate(101)
for account in accounts:
assert_equal(node.getbalance(account.name), 50)
for label in labels:
assert_equal(node.getbalance(label.name), 50)

# Check that setaccount can change the account of an address from a
# different account.
change_account(node, accounts[0].addresses[0], accounts[0], accounts[1])
# Check that setlabel can change the label of an address from a
# different label.
change_label(node, labels[0].addresses[0], labels[0], labels[1])

# Check that setaccount can change the account of an address which
# is the receiving address of a different account.
change_account(node, accounts[0].receive_address, accounts[0], accounts[1])
# Check that setlabel can change the label of an address which
# is the receiving address of a different label.
change_label(node, labels[0].receive_address, labels[0], labels[1])

# Check that setaccount can set the account of an address already
# in the account. This is a no-op.
change_account(node, accounts[2].addresses[0], accounts[2], accounts[2])
# Check that setlabel can set the label of an address already
# in the label. This is a no-op.
change_label(node, labels[2].addresses[0], labels[2], labels[2])

# Check that setaccount can set the account of an address which is
# already the receiving address of the account. It would probably make
# 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 getaccountaddress to return a brand new address.
change_account(node, accounts[2].receive_address, accounts[2], accounts[2])
# address, causing getlabeladdress to return a brand new address.
change_label(node, labels[2].receive_address, labels[2], labels[2])

class Account:
class Label:
def __init__(self, name):
# Account name
# Label name
self.name = name
# Current receiving address associated with this account.
# Current receiving address associated with this label.
self.receive_address = None
# List of all addresses assigned with this account
# List of all addresses assigned with this label
self.addresses = []

def add_address(self, address):
Expand All @@ -174,7 +174,7 @@ def add_receive_address(self, address):
def verify(self, node):
if self.receive_address is not None:
assert self.receive_address in self.addresses
assert_equal(node.getaccountaddress(self.name), self.receive_address)
assert_equal(node.getlabeladdress(self.name), self.receive_address)

for address in self.addresses:
assert_equal(node.getaccount(address), self.name)
Expand All @@ -183,26 +183,26 @@ def verify(self, node):
set(node.getaddressesbyaccount(self.name)), set(self.addresses))


def change_account(node, address, old_account, new_account):
assert_equal(address in old_account.addresses, True)
node.setaccount(address, new_account.name)
def change_label(node, address, old_label, new_label):
assert_equal(address in old_label.addresses, True)
node.setlabel(address, new_label.name)

old_account.addresses.remove(address)
new_account.add_address(address)
old_label.addresses.remove(address)
new_label.add_address(address)

# Calling setaccount on an address which was previously the receiving
# address of a different account should reset the receiving address of
# the old account, causing getaccountaddress to return a brand new
# Calling setlabel on an address which was previously the receiving
# 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_account.receive_address:
new_address = node.getaccountaddress(old_account.name)
assert_equal(new_address not in old_account.addresses, True)
assert_equal(new_address not in new_account.addresses, True)
old_account.add_receive_address(new_address)
if 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)
old_label.add_receive_address(new_address)

old_account.verify(node)
new_account.verify(node)
old_label.verify(node)
new_label.verify(node)


if __name__ == '__main__':
WalletAccountsTest().main()
WalletLabelsTest().main()
2 changes: 1 addition & 1 deletion test/functional/wallet_import_rescan.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def check(self, txid=None, amount=None, confirmations=None):

if txid is not None:
tx, = [tx for tx in txs if tx["txid"] == txid]
assert_equal(tx["account"], self.label)
assert_equal(tx["label"], self.label)
assert_equal(tx["address"], self.address["address"])
assert_equal(tx["amount"], amount)
assert_equal(tx["category"], "receive")
Expand Down
Loading

0 comments on commit 32610f0

Please sign in to comment.