Deprecate accounts #12953

pull jnewbery wants to merge 7 commits into bitcoin:master from jnewbery:deprecate_accounts changing 14 files +370 −91
  1. jnewbery commented at 7:02 pm on April 11, 2018: contributor

    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.

  2. maflcko added the label Wallet on Apr 11, 2018
  3. maflcko added the label RPC/REST/ZMQ on Apr 11, 2018
  4. jnewbery commented at 7:08 pm on April 11, 2018: contributor

    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.

  5. jnewbery force-pushed on Apr 11, 2018
  6. promag commented at 8:20 pm on April 11, 2018: member
    Concept ACK, will review.
  7. kallewoof commented at 1:01 am on April 12, 2018: member
    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. :/
  8. in src/wallet/rpcwallet.cpp:1047 in 14d2c093cc outdated 
    1043@@ -1039,13 +1044,54 @@ UniValue sendmany(const JSONRPCRequest& request)
1044         return NullUniValue;
1045     }
1046 
1047-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
1048-        throw std::runtime_error(
1049-            "sendmany \"fromaccount\" {\"address\":amount,...} ( minconf \"comment\" [\"address\",...] replaceable conf_target \"estimate_mode\")\n"
1050+    std::string help_text {};
    kallewoof commented at 1:05 am on April 12, 2018:
    Why {}?
    jnewbery commented at 1:36 pm on April 12, 2018:
    not required. I’ve now removed.
  9. in src/wallet/rpcwallet.cpp:1094 in 14d2c093cc outdated 
    1093+        help_text = "sendmany \"\" \"fromaccount\" {\"address\":amount,...} ( minconf \"comment\" [\"address\",...] replaceable conf_target \"estimate_mode\")\n"
1094             "\nSend multiple times. Amounts are double-precision floating point numbers."
1095             + HelpRequiringPassphrase(pwallet) + "\n"
1096             "\nArguments:\n"
1097-            "1. \"fromaccount\"         (string, required) DEPRECATED. The account to send the funds from. Should be \"\" for the default account\n"
1098+            "1. \"fromaccount\"         (string, required) DEPRECATED. The account to send the funds from. Should be \"\" for the default account.\n"
    kallewoof commented at 1:05 am on April 12, 2018:
    Remove the added . at end?
    jnewbery commented at 1:37 pm on April 12, 2018:
    removed
  10. in src/wallet/rpcwallet.cpp:1879 in 14d2c093cc outdated 
    1828+        help_text = "listtransactions (dummy count skip include_watchonly)\n"
1829+            "\nReturns up to 'count' most recent transactions skipping the first 'from' transactions for account 'account'.\n"
1830+            "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"
1831+            "bitcoind with -deprecatedrpc=accounts\n"
1832+            "\nArguments:\n"
1833+            "1. \"dummy\"    (string, optional) If set, should be \"*\" for backwards compatibility.\n"
    kallewoof commented at 1:08 am on April 12, 2018:

    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.

    promag commented at 1:38 pm on April 12, 2018:
    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.
    jnewbery commented at 2:05 pm on April 12, 2018:
    @promag - please see #12953 (comment) for rationale.
  11. in src/wallet/rpcwallet.cpp:1937 in 14d2c093cc outdated 
    1932@@ -1836,8 +1933,15 @@ UniValue listtransactions(const JSONRPCRequest& request)
1933     pwallet->BlockUntilSyncedToCurrentChain();
1934 
1935     std::string strAccount = "*";
1936-    if (!request.params[0].isNull())
1937-        strAccount = request.params[0].get_str();
1938+    if (!IsDeprecatedRPCEnabled("accounts")) {
1939+        if (request.params[0].get_str() != "*") {
    kallewoof commented at 1:12 am on April 12, 2018:

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

    0if (!request.params[0].isNull()) {
1  strAccount = request.params[0].get_str();
2  if (!IsDeprecatedRPCEnabled("accounts") && strAccount != "*") {
3    throw ...
4  }
5}
    promag commented at 1:40 pm on April 12, 2018:
    Correct, accounts is optional.
    jnewbery commented at 1:54 pm on April 12, 2018:
    Yes, much better. Thanks
  12. kallewoof commented at 1:12 am on April 12, 2018: member
    utACK
  13. fanquake added this to the milestone 0.17.0 on Apr 12, 2018
  14. laanwj commented at 3:44 am on April 12, 2018: member
    (Obvious) concept ACK
  15. in src/wallet/rpcwallet.cpp:1086 in 14d2c093cc outdated 
    1084+            "\nSend two amounts to two different addresses:\n"
1085+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\"") +
1086+            "\nSend two amounts to two different addresses setting the confirmation and comment:\n"
1087+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 6 \"testing\"") +
1088+            "\nSend two amounts to two different addresses, subtract fee from amount:\n"
1089+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 1 \"\" \"[\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\",\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\"]\"") +
    promag commented at 1:27 pm on April 12, 2018:
    Missing dummy ""?
    jnewbery commented at 1:57 pm on April 12, 2018:
    thanks. Fixed
  16. in src/rpc/server.cpp:477 in 14d2c093cc outdated 
    473@@ -474,13 +474,23 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
474 
475 UniValue CRPCTable::execute(const JSONRPCRequest &request) const
476 {
477+    static const std::vector<std::string> account_methods = {"getaccountaddress", "getaccount",
    promag commented at 1:27 pm on April 12, 2018:

    Alternative:

    0static const std::set<std::string> accounts_methods { "getaccountaddress", "getaccount", ... };
1...
2
3if (!IsDeprecatedRPCEnabled("accounts") && account_methods.count(request.strMethod)) {
    jnewbery commented at 2:03 pm on April 12, 2018:
    yes, that’s nicer. Thanks
  17. in src/wallet/rpcwallet.cpp:1088 in 14d2c093cc outdated 
    1086+            "\nSend two amounts to two different addresses setting the confirmation and comment:\n"
1087+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 6 \"testing\"") +
1088+            "\nSend two amounts to two different addresses, subtract fee from amount:\n"
1089+            + HelpExampleCli("sendmany", "\"{\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\":0.01,\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\":0.02}\" 1 \"\" \"[\\\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\\\",\\\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\\\"]\"") +
1090+            "\nAs a json rpc call\n"
1091+            + HelpExampleRpc("sendmany", "{\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\":0.01,\"1353tsE8YMTA4EuV7dgUXGjNFf9KpVvKHz\":0.02}, 6, \"testing\"");
    promag commented at 1:32 pm on April 12, 2018:
    Same as above.
    jnewbery commented at 2:04 pm on April 12, 2018:
    fixed
  18. in src/wallet/rpcwallet.cpp:1144 in 14d2c093cc outdated 
    1140@@ -1093,6 +1141,9 @@ UniValue sendmany(const JSONRPCRequest& request)
1141         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
1142     }
1143 
1144+    if (!IsDeprecatedRPCEnabled("accounts") && LabelFromValue(request.params[0]) != "") {
    promag commented at 1:34 pm on April 12, 2018:
    Remove LabelFromValue because it raises JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, "Invalid label name"). I suggest to just ... && !request.params[0].get_str().empty().
    jnewbery commented at 2:04 pm on April 12, 2018:
    agree. Thanks
  19. promag commented at 1:41 pm on April 12, 2018: member
    Partially tested, some comments.
  20. jnewbery commented at 1:53 pm on April 12, 2018: contributor

    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:
    0    { "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.

  21. jnewbery force-pushed on Apr 12, 2018
  22. jnewbery force-pushed on Apr 12, 2018
  23. jnewbery commented at 2:10 pm on April 12, 2018: contributor

    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.

  24. promag commented at 2:28 pm on April 12, 2018: member

    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.

  25. jnewbery commented at 2:39 pm on April 12, 2018: contributor

    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?

  26. promag commented at 2:47 pm on April 12, 2018: member

    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.

  27. jnewbery commented at 2:55 pm on April 12, 2018: contributor

    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)

  28. [wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCCommand table 
    Remove duplicate listreceivedby{account,label} methods.
    a28b907f8a
  29. [tests] Rename rpc_listtransactions.py to wallet_listtransactions.py 
    listtransactions is a wallet RPC. The test name should indicate that
this is a wallet test.
    4e671f0353
  30. [tests] Set -deprecatedrpc=accounts in tests 
    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.
    3db1ba01c7
  31. in src/rpc/server.cpp:488 in ba5bba9a18 outdated 
    483         LOCK(cs_rpcWarmup);
484         if (fRPCInWarmup)
485             throw JSONRPCError(RPC_IN_WARMUP, rpcWarmupStatus);
486     }
487 
488+    // Accounts RPC methods are deprecated
    laanwj commented at 2:45 pm on April 16, 2018:
    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.
    jnewbery commented at 2:54 pm on April 16, 2018:

    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.

    promag commented at 3:01 pm on April 16, 2018:

    but it saves a lot of code duplication

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

    laanwj commented at 5:14 pm on April 16, 2018:
    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.
    jnewbery commented at 5:29 pm on April 16, 2018:

    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 :)

  32. jnewbery force-pushed on Apr 16, 2018
  33. jnewbery commented at 6:46 pm on April 16, 2018: contributor

    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
  34. laanwj commented at 9:19 am on April 17, 2018: member

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

    0test_framework.authproxy.JSONRPCException: getaccount is deprecated and will be removed in V0.18. To use this command, start bitcoind with -deprecatedrpc=accounts. (-32)
12018-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.

  35. [wallet] [rpc] Deprecate account RPC methods 
    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
    3576ab1261
  36. [wallet] [rpc] Deprecate wallet 'account' API 
    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.
    109e05dcd1
  37. [wallet] [tests] Add tests for accounts/labels APIs 72c9575f7b
  38. jnewbery force-pushed on Apr 17, 2018
  39. [docs] Add release notes for deprecated 'account' API cead28bbec
  40. jnewbery commented at 6:25 pm on April 17, 2018: contributor

    Yes, --coverage caught a genuine bug. Hat tip to @jamesob .

    Now fixed, and regression tests and release notes added.

  41. jnewbery force-pushed on Apr 23, 2018
  42. laanwj commented at 1:09 pm on April 24, 2018: member
    utACK cead28bbecf032b49c01ca3ae4f47aa2536cbc55
  43. laanwj merged this on Apr 24, 2018
  44. laanwj closed this on Apr 24, 2018
  45. laanwj referenced this in commit d1d54ae6a3 on Apr 24, 2018
  46. sipa referenced this in commit f54f3738c8 on Jun 26, 2018
  47. sipa referenced this in commit ad552a54c5 on Jul 14, 2018
  48. red0bear commented at 1:06 pm on May 6, 2019: none
    i need know then …. in 0.18 there is no more -deprecatedrpc=accounts ?
  49. red0bear commented at 1:42 am on June 18, 2019: none
    well, i was looking to. just to clato clarify; as I could send from one label to another from 0.18 being that the sendfrom is no longer among the commands to accomplish this function. From 0.18 / 0.19 will bitcoin treat your wallet as only one person?
  50. sipa commented at 3:47 am on June 18, 2019: member
    @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).
  51. red0bear commented at 3:57 am on June 18, 2019: none
    So in these versions i can’t anymore sendfrom “fromaccount” “toaddress” amount will not be replaced by another command then. And only sendtoaddress limit label to only description wallet then.
  52. kallewoof commented at 4:25 am on June 18, 2019: member
    @red0bear Accounts were broken. Horribly. You are risking your funds every time you use them. Do not. This is why they were removed.
  53. luke-jr commented at 7:01 am on June 18, 2019: member
    Eh, they were never broken AFAIK. We just decided to stop supporting them.
  54. red0bear commented at 8:22 am on June 18, 2019: none
    @kallewoof , any external connection is a risk today.
  55. markblundeberg commented at 8:55 am on December 11, 2019: none
    ( note, introduces race fixed in #13130 )
  56. laanwj commented at 11:34 am on December 11, 2019: member
    a race in the python tests, not in the binary, to be clear
  57. inzider commented at 0:37 am on May 7, 2020: none

    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#issue-28982757 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?

  58. luke-jr commented at 0:51 am on May 7, 2020: member
    @inzider Please open a question on StackExchange explaining what you want to do. Accounts are no longer supported.
  59. bitcoin locked this on May 7, 2020
  60. jnewbery commented at 2:19 am on May 7, 2020: contributor
    @inzider - you need to use the -deprecatedrpc=accounts command-line or config option.
  61. jnewbery deleted the branch on May 7, 2020


jnewbery promag kallewoof laanwj red0bear sipa luke-jr markblundeberg inzider

Labels
Wallet RPC/REST/ZMQ

Milestone
0.17.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me