Hide accounts system behind deprecation switch #11497

1. 
   achow101 commented at 1:48 am on October 14, 2017: member

   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=acocunts startup option now in order to see account things.

   A couple of the tests use the accounts system for labeling things, so instead of changing those, I left them as is and set the tests to start the nodes with -deprecatedrpc=accounts. That switch can be removed as those RPCs are replaced with a labels system.

2. fanquake added the label RPC/REST/ZMQ on Oct 14, 2017
3. fanquake added the label Wallet on Oct 14, 2017
4. fanquake added this to the milestone 0.16.0 on Oct 14, 2017
5. 
   meshcollider commented at 8:55 am on October 14, 2017: contributor

   Concept ACK after label system has been merged
6. 
   TheBlueMatt commented at 6:35 pm on October 14, 2017: member

   I thought the plan was to have labels in place before removing (or, really, formally deprecating) accounts?
7. 
   achow101 commented at 7:02 pm on October 14, 2017: member

   I thought the plan was to have labels in place before removing (or, really, formally deprecating) accounts?

   I have no idea.

   Maybe it would be a good idea to have this and #7729 at the same time to allow for not using accounts and labels at the same time

8. 
   TheBlueMatt commented at 11:05 pm on October 14, 2017: member

   Yea, I mean this and labels in the same release sounds good, though I cant say I’m excited about removing accounts pre-labels.
9. 
   jnewbery commented at 6:45 pm on October 16, 2017: member

   Thanks for tackling this @achow101 . I’d echo @TheBlueMatt’s feedback that labels needs to be in place before we remove the accounts system. The -deprecatedrpc framework was put in place with a specific purpose: to make sure that RPCs can be removed or changed in a release without suddenly breaking clients. The idea is that the -deprecatedrpc option is only in place for a single release before the RPC is removed entirely. Without a plan already in place to migrate wallets to the label system and fully remove the accounts system, this feels like an abuse of the -deprecatedrpc mechanism.

   At a conceptual level, I’m not convinced that accounts system removal should depend on node version. It feels far more appropriate for it to be dependant on the wallet version than the node version.

10. Hide accounts system behind deprecation switch

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

    e572d3c9a9
11. in src/wallet/rpcwallet.cpp:1726 in 74085ad861 outdated

    1671@@ -1627,14 +1672,14 @@ UniValue listtransactions(const JSONRPCRequest& request)
    1672             "listtransactions ( \"account\" count skip include_watchonly)\n"
    1673             "\nReturns up to 'count' most recent transactions skipping the first 'from' transactions for account 'account'.\n"
    1674             "\nArguments:\n"
    1675-            "1. \"account\"    (string, optional) DEPRECATED. The account name. Should be \"*\".\n"
    1676+            "1. \"account\"    (string, optional) DEPRECATED. This argument will be removed in a future version. To use this deprecated argument, start bitcoind with -deprecatedrpc=accounts. The account name. Should be \"*\".\n"
    

    ---

    ---

    

    meshcollider commented at 5:47 am on October 20, 2017:

    Also, this should be re-purposed to label rather than removed, after #7729 merge, same as other RPCs with account arguments

    ---

    

    achow101 commented at 4:42 pm on November 30, 2017:

    Agreed.
12. achow101 force-pushed on Nov 30, 2017
13. 
    achow101 commented at 4:42 pm on November 30, 2017: member

    Rebased.

    This should be done after #7729 is merged. I will update it accordingly then.

14. laanwj removed this from the milestone 0.16.0 on Jan 11, 2018
15. laanwj added this to the milestone 0.17.0 on Jan 11, 2018
16. 
    ryanofsky commented at 10:33 am on April 11, 2018: member

    labels needs to be in place before we remove the accounts system

    Are we ready for this PR now that #12892 is merged?

17. 
    ryanofsky commented at 10:39 am on April 11, 2018: member

    Might be helpful for updating this PR: release notes https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-pr12892.md list the parts of the accounts api that are deprecated.
18. 
    Sjors commented at 10:44 am on April 11, 2018: member

    Concept ACK. Agree with @ryanofsky that all prerequisites seem to be in place now.
19. 
    jnewbery commented at 1:07 pm on April 11, 2018: member

    @achow101 - I’ve already started rebasing this on #12892. I’ll open my own PR later today unless you really want to keep hold of this one.
20. 
    achow101 commented at 4:10 pm on April 11, 2018: member

    Closing in favor of whatever @jnewbery does because I’m too lazy to rebase this.
21. achow101 closed this on Apr 11, 2018

    ---

22. MarcoFalke locked this on Sep 8, 2021

Contributors
achow101 meshcollider TheBlueMatt jnewbery ryanofsky Sjors

Labels
Wallet RPC/REST/ZMQ

Milestone
0.17.0