Deprecate accounts #12953

jnewbery · 2018-04-11T19:02:28Z

Deprecate all accounts functionality and make it only accessible by using -deprecatedrpc=accounts.

Accounts specific RPCs, account arguments, and account related results all require the -deprecatedrpc=accunts startup option now in order to see account things.

Several wallet functional tests use the accounts system. Those tests are unchanged, except to start the nodes with -deprecatedrpc=accounts. We can slowly migrate those tests to use the 'label' API instead of the 'account' API before accounts are fully removed.

jnewbery · 2018-04-11T19:08:58Z

This is @achow101's #11497 rebased and reworked. Notable differences:

I've moved the RPC method deprecation up into the CRPCTable::execute() function. That's perhaps a little bit messy (rpc/server.cpp ideally wouldn't have knowledge of individual Bitcoin RPCs), but it saves a lot of code duplication and will be removed in V0.18, so I decided it was acceptable to have it there.
Some methods (sendmany and listtransactions) have alternative help text depending on whether the -deprecatedrpcs=accounts switch is used. That means some duplicate text, but it makes the help text a lot clearer in all cases (since it's not filed with "if using accounts this else that").
I've avoided the argument repositioning that achow101 used, since that breaks the way rpc server fills in positional arguments from named arguments. That means that sendmany and listtransactions now have a dummy argument in the same way that move did. We can do another API change in a future version if that's offensive to people.

This PR also renames rpc_listtransactions.py to wallet_listtransactions.py (since it's a wallet test). This makes it clear that only wallet tests are affected by this change.

promag · 2018-04-11T20:20:56Z

Concept ACK, will review.

kallewoof · 2018-04-12T01:01:44Z

Review hint: recommend append ?w=1 to end, to avoid having to review whitespace changes. Edit: I noticed you can't actually put review comments when you do, so limited use. :/

kallewoof

utACK

kallewoof · 2018-04-12T01:05:19Z

src/wallet/rpcwallet.cpp

-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
-        throw std::runtime_error(
-            "sendmany \"fromaccount\" {\"address\":amount,...} ( minconf \"comment\" [\"address\",...] replaceable conf_target \"estimate_mode\")\n"
+    std::string help_text {};


not required. I've now removed.

kallewoof · 2018-04-12T01:05:56Z

src/wallet/rpcwallet.cpp

            "\nSend multiple times. Amounts are double-precision floating point numbers."
            + HelpRequiringPassphrase(pwallet) + "\n"
            "\nArguments:\n"
-            "1. \"fromaccount\"         (string, required) DEPRECATED. The account to send the funds from. Should be \"\" for the default account\n"
+            "1. \"fromaccount\"         (string, required) DEPRECATED. The account to send the funds from. Should be \"\" for the default account.\n"


Remove the added . at end?

kallewoof · 2018-04-12T01:08:33Z

src/wallet/rpcwallet.cpp

+            "Note that the \"account\" argument and \"otheraccount\" return value have been removed in V0.17. To use this RPC with an \"account\" argument, restart\n"
+            "bitcoind with -deprecatedrpc=accounts\n"
+            "\nArguments:\n"
+            "1. \"dummy\"    (string, optional) If set, should be \"*\" for backwards compatibility.\n"


Since we're splitting the function anyway, why have the dummy at all?

I also wonder if we should have some form of upgrade plan for when we do this. I know e.g. @luke-jr prefers option objects.

I agree, I don't see a reason to support the case "I want the new behavior without accounts but keep the RPC argument". People that wants to avoid breaking change launch with -deprecatedrpc=accounts.

@promag - please see #12953 (comment) for rationale.

kallewoof · 2018-04-12T01:12:00Z

src/wallet/rpcwallet.cpp

-    if (!request.params[0].isNull())
-        strAccount = request.params[0].get_str();
+    if (!IsDeprecatedRPCEnabled("accounts")) {
+        if (request.params[0].get_str() != "*") {


If params[0] is unset, this will throw an error at get_str() won't it?
Looks like you want

if (!request.params[0].isNull()) { strAccount = request.params[0].get_str(); if (!IsDeprecatedRPCEnabled("accounts") && strAccount != "*") { throw ... } }

Correct, accounts is optional.

Yes, much better. Thanks

laanwj · 2018-04-12T03:44:18Z

(Obvious) concept ACK

promag

Partially tested, some comments.

promag · 2018-04-12T13:27:51Z

src/wallet/rpcwallet.cpp

+            "\nSend two amounts to two different addresses setting the confirmation and comment:\n"
+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 6 \"testing\"") +
+            "\nSend two amounts to two different addresses, subtract fee from amount:\n"
+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 1 \"\" \"[\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\",\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\"]\"") +


Missing dummy ""?

thanks. Fixed

promag · 2018-04-12T13:27:56Z

src/rpc/server.cpp

@@ -474,13 +474,23 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c

 UniValue CRPCTable::execute(const JSONRPCRequest &request) const
 {
+    static const std::vector<std::string> account_methods = {"getaccountaddress", "getaccount",


Alternative:

static const std::set<std::string> accounts_methods { "getaccountaddress", "getaccount", ... }; ... if (!IsDeprecatedRPCEnabled("accounts") && account_methods.count(request.strMethod)) {

yes, that's nicer. Thanks

promag · 2018-04-12T13:32:17Z

src/wallet/rpcwallet.cpp

+            "\nSend two amounts to two different addresses, subtract fee from amount:\n"
+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 1 \"\" \"[\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\",\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\"]\"") +
+            "\nAs a json rpc call\n"
+            + HelpExampleRpc("sendmany", "{\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\":0.01,\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\":0.02}, 6, \"testing\"");


Same as above.

promag · 2018-04-12T13:34:06Z

src/wallet/rpcwallet.cpp

@@ -1093,6 +1141,9 @@ UniValue sendmany(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
    }

+    if (!IsDeprecatedRPCEnabled("accounts") && LabelFromValue(request.params[0]) != "") {


Remove LabelFromValue because it raises JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, "Invalid label name"). I suggest to just ... && !request.params[0].get_str().empty().

agree. Thanks

promag · 2018-04-12T13:38:18Z

src/wallet/rpcwallet.cpp

+            "Note that the \"account\" argument and \"otheraccount\" return value have been removed in V0.17. To use this RPC with an \"account\" argument, restart\n"
+            "bitcoind with -deprecatedrpc=accounts\n"
+            "\nArguments:\n"
+            "1. \"dummy\"    (string, optional) If set, should be \"*\" for backwards compatibility.\n"


I agree, I don't see a reason to support the case "I want the new behavior without accounts but keep the RPC argument". People that wants to avoid breaking change launch with -deprecatedrpc=accounts.

promag · 2018-04-12T13:40:31Z

src/wallet/rpcwallet.cpp

-    if (!request.params[0].isNull())
-        strAccount = request.params[0].get_str();
+    if (!IsDeprecatedRPCEnabled("accounts")) {
+        if (request.params[0].get_str() != "*") {


Correct, accounts is optional.

jnewbery · 2018-04-12T13:53:44Z

Since we're splitting the function anyway, why have the dummy at all?

I mentioned this briefly in the PR text:

I've avoided the argument repositioning that achow101 used, since that breaks the way rpc server fills in positional arguments from named arguments.

(@achow101's argument repositioning code is here: https://github.com/bitcoin/bitcoin/pull/11497/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR1086 for example)

In more detail:

the rpc/server.cpp function transformNamedArguments() uses the list of arguments from CRPCTable to convert a keyed map of arguments into a univalue array.
That CRPCTable is constructed from the various CRPCCommand arrays in the rpc .cpp files. For sendmany, the entry is:

    { "wallet",             "sendmany",                         &sendmany,                      {"fromaccount|dummy","amounts","minconf","comment","subtractfeefrom","replaceable","conf_target","estimate_mode"} },

that means that when called with named arguments, the rpc server will eg place the fromaccount named argument into index 0 in the array, amount into index 1, and so on.
there's no way to change CRPCTable at runtime based on whether or not -deprecatedrpc was used. Changing that in rpc/server.cpp would be a very large change, and would involve placing more bitcoin-specific knowledge into the rpc server code.

Functionally, I think this is good behaviour anyway - users who were calling sendmany with an empty string as the account argument will not see any difference when they upgrade. Users who were using sendmany with accounts will see an immediate obvious break, and can either change to stop using accounts or use -deprecatedrpc=accounts for one more release.

jnewbery · 2018-04-12T14:10:05Z

Thanks for the review @kallewoof and @promag! Your comments are addressed in the latest push.

This requires an update to the release notes. I'll do that once the new code/functionality is stable.

promag · 2018-04-12T14:28:38Z

Thanks @jnewbery for the details.

IIUC:

accounts are deprecated but API wise the arguments are still there;
in 0.18 accounts will be gone even dummy arguments.

If that's the case then I'm sorry to NACK - it's not possible to refactor existing software to face 0.18. In 0.17 we should provide both scenarios API wise!

I haven't thought about the positional and named arguments thing with CRPCTable but I believe we should find a solution.

jnewbery · 2018-04-12T14:39:08Z

in 0.18 accounts will be gone even dummy arguments.

No - there's no firm plan to remove the dummy arguments. At some point, we could remove them, but that would be a breaking API change.

The API change in this PR should be minimally invasive. For users calling sendmany and listtransactions without accounts, there's no change in the way they need to call the RPC. Changing the argument ordering as you've suggested would require all users of those PRC methods to change their client code, regardless of whether they're currently using accounts.

Does that address your concern? If not, could you elaborate a bit more?

promag · 2018-04-12T14:47:36Z

No - there's no firm plan to remove the dummy arguments

If we do then:

it will be breaking change - and there is nothing wrong with that IMO
we must provide a new deprecation flag in 0.18 and only remove in 0.19.

Does that address your concern?

Yes, all clear.

Restarted travis job. Will review again.

jnewbery · 2018-04-12T14:55:24Z

we must provide a new deprecation flag in 0.18 and only remove in 0.19.

Yes - any breaking API change should be protected by a deprecation switch (or API versions if we ever implement something like that)

laanwj · 2018-04-16T14:45:52Z

src/rpc/server.cpp

    // Return immediately if in warmup
    {
        LOCK(cs_rpcWarmup);
        if (fRPCInWarmup)
            throw JSONRPCError(RPC_IN_WARMUP, rpcWarmupStatus);
    }

+    // Accounts RPC methods are deprecated


This is a smaller change, however because rpc/server.cpp is supposed to not be specific to bitcoin, I'd prefer to put the deprecation check in the individual methods.

I agree that it's more correct to have these in the individual methods, but see my rationale above:

That's perhaps a little bit messy (rpc/server.cpp ideally wouldn't have knowledge of individual Bitcoin RPCs), but it saves a lot of code duplication and will be removed in V0.18, so I decided it was acceptable to have it there.

Are you strongly opposed to have this code in rpc/server.cpp, even if just temporarily? If so, I can change the PR to move this code to individual methods.

but it saves a lot of code duplication

Well it's just 8 deprecated methods. I'm fine either way.

To be honest I prefer the duplication (then you can remove the entire methods, duplications and al after the deprecation phase). If it's the about the amount of work just say so and I'll provide a patch.

If it's the about the amount of work

no, mostly amount of review. I'm happy to write the code if you're happy to review it :)

…d table Remove duplicate listreceivedby{account,label} methods.

listtransactions is a wallet RPC. The test name should indicate that this is a wallet test.

Future commits will deprecate the accounts RPC methods, arguments and return objects. Set the -deprecatedrpc=accounts switch now so tests don't break in intermediate commits.

jnewbery · 2018-04-16T18:46:27Z

Moved the RPC method deprecation into rpcwallet.cpp as requested by @laanwj . Also added tests for those deprecated methods.

Still to do:

deprecation tests for the changes in Deprecate wallet 'account' API
~~release notes~~

laanwj · 2018-04-17T09:19:53Z

Looks like test/functional/test_runner.py --coverage fails:

test_framework.authproxy.JSONRPCException: getaccount is deprecated and will be removed in V0.18. To use this command, start bitcoind with -deprecatedrpc=accounts. (-32)
2018-04-17T09:18:09.208000Z TestFramework (INFO): Stopping nodes

I think this is due to the help command failing, while getting help for the account calls.

All account RPC methods are now deprecated and can only be called if bitcoind has been started with the -deprecatedrpc=accounts switch. Affected RPC methods are: - getaccount - getaccountaddress - getaddressesbyaccount - getreceivedbyaccount - listaccouts - listreceivedbyaccount - move - setaccount

This commit finalizes the deprecation of the wallet 'accounts' API by removing all account arguments and return values. RPC behaviour is slightly different if the 'accounts' or 'labels' API is being used. Those behaviour changes are fully documented in the RPC help text.

laanwj · 2018-04-24T13:09:31Z

utACK cead28b

cead28b [docs] Add release notes for deprecated 'account' API (John Newbery) 72c9575 [wallet] [tests] Add tests for accounts/labels APIs (John Newbery) 109e05d [wallet] [rpc] Deprecate wallet 'account' API (John Newbery) 3576ab1 [wallet] [rpc] Deprecate account RPC methods (John Newbery) 3db1ba0 [tests] Set -deprecatedrpc=accounts in tests (John Newbery) 4e671f0 [tests] Rename rpc_listtransactions.py to wallet_listtransactions.py (John Newbery) a28b907 [wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCCommand table (John Newbery) Pull request description: Deprecate all accounts functionality and make it only accessible by using `-deprecatedrpc=accounts`. Accounts specific RPCs, account arguments, and account related results all require the `-deprecatedrpc=accunts` startup option now in order to see account things. Several wallet functional tests use the accounts system. Those tests are unchanged, except to start the nodes with `-deprecatedrpc=accounts`. We can slowly migrate those tests to use the 'label' API instead of the 'account' API before accounts are fully removed. Tree-SHA512: 89f4ae2fe6de4a1422f1817b0997ae22d63ab5a1a558362ce923a3871f3e42963405d6573c69c27f1764679cdee5b51bf52202cc407f1361bfd8066d652f3f37

df10f07 [wallet] Don't use accounts when checking balance in sendmany (John Newbery) e209184 [wallet] deprecate sendfrom RPC method. (John Newbery) Pull request description: A couple of fixups from the accounts API deprecation PR (#12953): - properly deprecate `sendfrom` - don't use accounts when calculating balance in `sendmany` (unless the `-deprecatedrpc=accounts` flag is being used) Tree-SHA512: 1befde055067438c4c3391bbff1aaed0e6249efd708c567db3f1faad40a0f28e64f95e5bad0679ae826d24a0239e4bc8a1c392dc93e2e7502343a7f6b1d1845c

702ae1e [RPC] [wallet] allow getbalance to use min_conf and watch_only without accounts. (John Newbery) cf15761 [wallet] GetBalance can take a min_depth argument. (John Newbery) 0f3d6e9 [wallet] factor out GetAvailableWatchOnlyBalance() (John Newbery) 7110c83 [wallet] deduplicate GetAvailableCredit logic (John Newbery) ef7bc88 [wallet] Factor out GetWatchOnlyBalance() (John Newbery) 4279da4 [wallet] GetBalance can take an isminefilter filter. (John Newbery) Pull request description: #12953 inadvertently removed the functionality to call `getbalance "*" <int> <bool>` to get the wallet's balance with either minconfs or include_watchonly. This restores that functionality (when `-deprecatedrpc=accounts`), and also makes it possible to call ``getbalance minconf=<int> include_watchonly=<bool>` when accounts are not being used. Tree-SHA512: 67e84de9291ed6d34b23c626f4dc5988ba0ae6c99708d02b87dd3aaad3f4b6baa6202a66cc2dadd30dd993a39de8036ee920fcaa8cbb1c5dfe606e6fac183344

sipa · 2019-06-18T03:47:08Z

@red0bear It always treated your wallet as one person. The only difference between sendtoaddress and sendfrom is that the latter also subtracted the amount being sent from the specified accounts' balance (but that was just virtual bean counter, it never had any relation to what UTXOs existed on-chain).

kallewoof · 2019-06-18T04:25:58Z

@red0bear Accounts were broken. Horribly. You are risking your funds every time you use them. Do not. This is why they were removed.

luke-jr · 2019-06-18T07:01:13Z

Eh, they were never broken AFAIK. We just decided to stop supporting them.

markblundeberg · 2019-12-11T08:55:27Z

( note, introduces race fixed in #13130 )

laanwj · 2019-12-11T11:34:25Z

a race in the python tests, not in the binary, to be clear

inzider · 2020-05-07T00:37:31Z

Im trying to start version 0.19.1 with -deprecated=accounts so I can use sendfrom method.
Still, bitcoind-cli return method not found.

Those are useful options for many reason - making a "blind" remove is odd.

Maybe I miss something but I don't feel this #3816 (comment) is enough of a good reason to remove those - some kind of confusion on the user part.

What is the solution here!? Downgrade 0.16.3 or im doing something wrong?

luke-jr · 2020-05-07T00:51:32Z

@inzider Please open a question on StackExchange explaining what you want to do. Accounts are no longer supported.

jnewbery · 2020-05-07T02:19:13Z

@inzider - you need to use the -deprecatedrpc=accounts command-line or config option.

maflcko added Wallet RPC/REST/ZMQ labels Apr 11, 2018

This was referenced Apr 11, 2018

Remove wallet 'account' API #12952

Closed

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

Merged

jnewbery force-pushed the deprecate_accounts branch from e4aa62c to 14d2c09 Compare April 11, 2018 19:33

kallewoof reviewed Apr 12, 2018

View reviewed changes

fanquake added this to the 0.17.0 milestone Apr 12, 2018

promag reviewed Apr 12, 2018

View reviewed changes

jnewbery force-pushed the deprecate_accounts branch 2 times, most recently from 2dcbccd to ba5bba9 Compare April 12, 2018 14:09

laanwj reviewed Apr 16, 2018

View reviewed changes

jnewbery added 3 commits April 16, 2018 14:43

[wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCComman…

a28b907

…d table Remove duplicate listreceivedby{account,label} methods.

[tests] Rename rpc_listtransactions.py to wallet_listtransactions.py

4e671f0

listtransactions is a wallet RPC. The test name should indicate that this is a wallet test.

[tests] Set -deprecatedrpc=accounts in tests

3db1ba0

Future commits will deprecate the accounts RPC methods, arguments and return objects. Set the -deprecatedrpc=accounts switch now so tests don't break in intermediate commits.

jnewbery force-pushed the deprecate_accounts branch from ba5bba9 to 5b55c12 Compare April 16, 2018 18:44

jnewbery added 3 commits April 17, 2018 12:42

[wallet] [tests] Add tests for accounts/labels APIs

72c9575

jnewbery mentioned this pull request Apr 23, 2018

[wallet] [rpc] Remove getlabeladdress RPC #13060

Merged

laanwj merged commit cead28b into bitcoin:master Apr 24, 2018

Bushstar mentioned this pull request Apr 25, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#308

Merged

ryanofsky mentioned this pull request Apr 25, 2018

Refactor: separate wallet from node #10973

Merged

marcoagner mentioned this pull request May 17, 2018

Account inconsistency in regtest with raw Tx #13261

Closed

This was referenced Jun 18, 2018

[wallet] Fixups from account API deprecation #13498

Merged

Wallet: correctly deprecate accounts in getbalance, re-add minconf / include-watch-only #13461

Closed

jnewbery mentioned this pull request Jun 28, 2018

Fix get balance #13566

Merged

This was referenced Sep 11, 2018

[wallet] Fixups from account API deprecation chaincoin/chaincoin#460

Merged

Fix get balance chaincoin/chaincoin#516

Merged

jnewbery mentioned this pull request Oct 8, 2018

[wallet] Restore ability to list incoming transactions by label #14411

Merged

This comment was marked as spam.

Sign in to view

Fuzzbawls mentioned this pull request Jan 8, 2020

Timeline: Removal of outdated internal accounting system PIVX-Project/PIVX#1248

Closed

bitcoin locked as resolved and limited conversation to collaborators May 7, 2020

jnewbery deleted the deprecate_accounts branch May 7, 2020 02:19

Deprecate accounts #12953

Deprecate accounts #12953

Uh oh!

Conversation

jnewbery commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 11, 2018

Uh oh!

promag commented Apr 11, 2018

Uh oh!

kallewoof commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 12, 2018

Uh oh!

jnewbery commented Apr 12, 2018

Uh oh!

promag commented Apr 12, 2018

Uh oh!

jnewbery commented Apr 12, 2018

Uh oh!

promag commented Apr 12, 2018

Uh oh!

jnewbery commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 11, 2018 •

edited

Loading

kallewoof commented Apr 12, 2018 •

edited

Loading

laanwj Apr 16, 2018 •

edited

Loading

jnewbery commented Apr 16, 2018 •

edited

Loading

inzider commented May 7, 2020 •

edited

Loading