New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename account to label where appropriate #11536
Conversation
Concept ACK, I think this should be part of #11497? |
I don't mind combining prs though I don't see what advantage it brings in this case. This PR is backwards compatible and can be merged without waiting for #7729. #11497 is also confused about which account instances need to be removed vs renamed (as you pointed out in one of your review comments), and this PR clarifies that. |
src/qt/paymentserver.cpp
Outdated
QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant); | ||
std::string strAccount = account.toStdString(); | ||
std::set<CTxDestination> refundAddresses = wallet->GetAccountAddresses(strAccount); | ||
QString label = tr("Refund from %1").arg(recipient.authenticatedMerchant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only std::string
is used, so:
std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in be9665a
src/wallet/rpcwallet.cpp
Outdated
@@ -3287,6 +3289,14 @@ UniValue rescanblockchain(const JSONRPCRequest& request) | |||
return response; | |||
} | |||
|
|||
// Deprecated account RPCs. | |||
UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing IsDeprecatedRPCEnabled("accounts")
and throw? Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually use -deprecatedrpc though, that's done in #11497?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the question because in the release notes says these are deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/wallet/wallet.h
Outdated
@@ -1141,15 +1141,15 @@ class CReserveKey final : public CReserveScript | |||
|
|||
|
|||
/** | |||
* Account information. | |||
* Stored in wallet with key "acc"+string account name. | |||
* Label information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This structure is account-specific. It's going away when account support is dropped, as there is no state per label, it's just an identifier. So I suggest keeping it at the current name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in 603759b
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think usually when renaming errors we keep the old name around as an alias - people are processing this directly to RPC wrappers, and old client code would still use the old name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added alias in acc1ee8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, why isn't RPC_INVALID_PARAMETER
used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, why isn't RPC_INVALID_PARAMETER used instead?
I don't know the history, but I could imagine this was added because it was useful for some client to be able to distinguish this specific error from more generic errors. Similar to how RPC_WALLET_NOT_SPECIFIED was added for bitcoin-cli in #10931.
@@ -42,12 +42,16 @@ static const CRPCConvertParam vRPCConvertParams[] = | |||
{ "settxfee", 0, "amount" }, | |||
{ "getreceivedbyaddress", 1, "minconf" }, | |||
{ "getreceivedbyaccount", 1, "minconf" }, | |||
{ "getreceivedbylabel", 1, "minconf" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't actually introduce these methods in this PR.
Edit: oh you do, github was hiding things again.
Concept ACK - needs qa test for the new RPCs. |
src/wallet/rpcwallet.cpp
Outdated
UniValue getaddressesbyaccount(const JSONRPCRequest& request) { return getaddressesbylabel(request); } | ||
UniValue getreceivedbyaccount(const JSONRPCRequest& request) { return getreceivedbylabel(request); } | ||
UniValue listreceivedbyaccount(const JSONRPCRequest& request) { return listreceivedbylabel(request); } | ||
UniValue setaccount(const JSONRPCRequest& request) { return setlabel(request); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?
The intention of this PR is to not introduce any differences between accounts and labels yet, leaving that for #7729 and any future followups. I have a rebased version of #7729 at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/qt/paymentserver.cpp
Outdated
QString account = tr("Refund from %1").arg(recipient.authenticatedMerchant); | ||
std::string strAccount = account.toStdString(); | ||
std::set<CTxDestination> refundAddresses = wallet->GetAccountAddresses(strAccount); | ||
QString label = tr("Refund from %1").arg(recipient.authenticatedMerchant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in be9665a
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added alias in acc1ee8
src/wallet/rpcwallet.cpp
Outdated
@@ -3287,6 +3289,14 @@ UniValue rescanblockchain(const JSONRPCRequest& request) | |||
return response; | |||
} | |||
|
|||
// Deprecated account RPCs. | |||
UniValue getaccountaddress(const JSONRPCRequest& request) { return getlabeladdress(request); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/wallet/wallet.h
Outdated
@@ -1141,15 +1141,15 @@ class CReserveKey final : public CReserveScript | |||
|
|||
|
|||
/** | |||
* Account information. | |||
* Stored in wallet with key "acc"+string account name. | |||
* Label information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in 603759b
src/wallet/rpcwallet.cpp
Outdated
UniValue getaddressesbyaccount(const JSONRPCRequest& request) { return getaddressesbylabel(request); } | ||
UniValue getreceivedbyaccount(const JSONRPCRequest& request) { return getreceivedbylabel(request); } | ||
UniValue listreceivedbyaccount(const JSONRPCRequest& request) { return listreceivedbylabel(request); } | ||
UniValue setaccount(const JSONRPCRequest& request) { return setlabel(request); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for no way for the method to know whether they've been called as labelXX or accountXX, as we might want to implement slightly different behavior for both, that seems important information?
The intention of this PR is to not introduce any differences between accounts and labels yet, leaving that for #7729 and any future followups. I have a rebased version of #7729 at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK fec503a
src/wallet/rpcwallet.cpp
Outdated
"\nArguments:\n" | ||
"1. minconf (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n" | ||
"2. include_empty (bool, optional, default=false) Whether to include accounts that haven't received any payments.\n" | ||
"2. include_empty (bool, optional, default=false) Whether to include labels that haven't received any payments.\n" | ||
"3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n" | ||
|
||
"\nResult:\n" | ||
"[\n" | ||
" {\n" | ||
" \"involvesWatchonly\" : true, (bool) Only returned if imported addresses were involved in transaction\n" | ||
" \"account\" : \"accountname\", (string) The account name of the receiving account\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be updated, since this is an alias for "label". (Same for other occurrences in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, updated in 864a837.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/wallet/rpcwallet.cpp
Outdated
"\nArguments:\n" | ||
"1. minconf (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n" | ||
"2. include_empty (bool, optional, default=false) Whether to include accounts that haven't received any payments.\n" | ||
"2. include_empty (bool, optional, default=false) Whether to include labels that haven't received any payments.\n" | ||
"3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n" | ||
|
||
"\nResult:\n" | ||
"[\n" | ||
" {\n" | ||
" \"involvesWatchonly\" : true, (bool) Only returned if imported addresses were involved in transaction\n" | ||
" \"account\" : \"accountname\", (string) The account name of the receiving account\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, updated in 864a837.
doc/release-notes.md
Outdated
- `dumpwallet` no longer allows overwriting files. This is a security measure | ||
as well as prevents dangerous user mistakes. | ||
- `getnewaddress` and `addmultisigaddress` RPC `account` named parameters have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep an address
alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep an address alias around for this named parameter as well, along with a deprecation warning? Otherwise it should probably be marked as a breaking change.
Thanks. Somehow I missed this comment earlier. Added in 7ae567f
Needs rebase |
e958b9e
to
981ac92
Compare
Just so this isn't the last comment., I have been periodically rebasing this PR. The PR is just a big rename so it gets out of date pretty frequently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, why isn't RPC_INVALID_PARAMETER used instead?
I don't know the history, but I could imagine this was added because it was useful for some client to be able to distinguish this specific error from more generic errors. Similar to how RPC_WALLET_NOT_SPECIFIED was added for bitcoin-cli in #10931.
8ec5890
to
5bb0a7b
Compare
Rebased again 8ec5890 -> 5bb0a7b (pr/acct.9 -> pr/acct.10) due to conflict with #12062. |
This is a separate commit because changing the test at the same time as renaming it breaks git (and github) rename detection.
4c317d8 Document RPC method aliasing (Russell Yanofsky) Pull request description: Suggested by @Sjors in #11536 (comment) Tree-SHA512: 7bf16238e41b6c6c078e9103d8eac2ac76739a2c16b4f964be49bfde1f20f31a1fb30badf1faaa6ddc301a74f0d785d19567069b50de78c502144479143cb38c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment inline.
Why does this PR not add the getlabel
and getaddressesbylabel
rpcs?
I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py
-> wallet_labels.py
@@ -63,6 +63,15 @@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: language is perhaps a little clumsy: ...been added to replace to be deprecated...
. Perhaps prefer something like:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: language is perhaps a little clumsy
Updated in 843ee06
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 1 commits e2966ce -> 843ee06 (pr/acct.26 -> pr/acct.27, compare)
Squashed 843ee06 -> d2527bd (pr/acct.27 -> pr/acct.28)
Why does this PR not add the getlabel and getaddressesbylabel rpcs?
#7729 adds these with different behavior than the existing account rpcs. The idea is for this PR to just clean up stale "account" terminology and for #7729 to add new functionality for making labels work better.
I still think this would be clearer if you did all the code changes in the first commit, and then the second commit was just the rename wallet_accounts.py -> wallet_labels.py
Sure, moved rename to second commit.
@@ -63,6 +63,15 @@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: language is perhaps a little clumsy
Updated in 843ee06
- getaddressesbyaccount | ||
- listaddressgroupings | ||
- setlabel | ||
- sendfrom (with account arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the line below) can be updated to (with label arguments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the line below) can be updated to (with label arguments)
This is actually intentional. sendfrom
and move
are account-specific and deprecated and won't take label arguments. Idea is that we support receiving at addresses but not really sending from addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, makes sense. I was confused by the code lower down in the test case:
# Check that sendfrom label reduces listaccounts balances.
for i, label in enumerate(labels):
to_label = labels[(i+1) % len(labels)]
node.sendfrom(label.name, to_label.receive_address, amount_to_send)
Tested ACK d2527bd. Thanks for moving the commits around. Sorry - I've added one more nit in the test file. |
d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in #7729, by getting renaming out of the way and letting that change 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. --- There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff
Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin#11536 (comment)
Summary: Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin/bitcoin#11536 (comment) This is a backport of Core PR12700 Test Plan: make Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4169
Summary: Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin/bitcoin#11536 (comment) This is a backport of Core PR12700 Test Plan: make Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4169
Summary: Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin/bitcoin#11536 (comment) This is a backport of Core PR12700 Test Plan: make Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4169
Summary: Suggested by Sjors Provoost <sjors@sprovoost.nl> in bitcoin/bitcoin#11536 (comment) This is a backport of Core PR12700 Test Plan: make Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4169
4c317d8 Document RPC method aliasing (Russell Yanofsky) Pull request description: Suggested by @Sjors in bitcoin#11536 (comment) Tree-SHA512: 7bf16238e41b6c6c078e9103d8eac2ac76739a2c16b4f964be49bfde1f20f31a1fb30badf1faaa6ddc301a74f0d785d19567069b50de78c502144479143cb38c
4c317d8 Document RPC method aliasing (Russell Yanofsky) Pull request description: Suggested by @Sjors in bitcoin#11536 (comment) Tree-SHA512: 7bf16238e41b6c6c078e9103d8eac2ac76739a2c16b4f964be49bfde1f20f31a1fb30badf1faaa6ddc301a74f0d785d19567069b50de78c502144479143cb38c
d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate 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 that change 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. --- There is a rebased version of bitcoin#7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see bitcoin#7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff
d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate 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 that change 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. --- There is a rebased version of bitcoin#7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see bitcoin#7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff
d2527bd Rename wallet_accounts.py test (Russell Yanofsky) 045eeb8 Rename account to label where appropriate (Russell Yanofsky) Pull request description: Rename account to label where appropriate 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 that change 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. --- There is a rebased version of bitcoin#7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see bitcoin#7729 (comment). Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff
Rename account to label where appropriate
This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in #7729, by getting renaming out of the way and letting that change 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.
There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see #7729 (comment).