Remaining instances of ENABLE_WALLET in `libbitcoin_server.a` #7965

1. 
   laanwj commented at 2:31 PM on April 28, 2016: member

   We've been quite successful in eliminating these lately (#7905, #7691).

   It would be nice to disentangle the wallet and non-wallet initialization completely so that init.cpp no longer depends on the wallet (and thus, libbitcoin_server.a no longer depends on libbitcoin_wallet.a - the other way is fine, a circular dependency is not).

   Ignoring the GUI for now (which still needs some work in this direction), the following are left:

   • init.cpp: these are mostly self-contained and short. (#10762)
     • print BerkeleyDB version should probably be moved to CWallet::InitLoadWallet CWallet::Verify (fixed in #8036).
     • wallet-specfic option interaction (-walletbroadcast, -prune+-rescan) should move to wallet module (possibly a new call that can be called in addition to InitParameterInteraction())
     • printing setKeyPool size etc should probably be moved to CWallet::InitLoadWallet.
     • wallet initialization/verify/shutdown - maybe this could be replaced by a general registration system for initialization/interrupt/shutdown functions, called as signals in the appropriate places. The same could be used by other modules
     • -disablewallet could result in the wallet not being registered at all, and that can all be handled from outside init.
   • RPC still has some calls that vary depending on wallet support. We should split these up. This is the more annoying part as it will involve API changes. No non-wallet RPC call should make an assumption about "a wallet".
     • rpcmisc.cpp
       • getinfo - this call is already on the list to be deprecated for a long time (#10838)
       • validateaddress - should be split up into an utility-only validateaddress and a wallet-specific call getaddressinfo (name open for discussion) (#10583)
       • createmultisig - same. createmultisig and walletcreatemultisig or so (#11415)
     • rpcblockchain.cpp
       • signrawtransaction - should be split into an utility-only signrawtransaction and a wallet-specific call walletsignrawtransaction (#10579)
     • remove deprecated validateaddress and signrawtransaction wallet dependency completely (#12490)
   • Mining: no ENABLE_WALLET but an implicit assumption and a circuitous registration system. The generate call should be a wallet-specific call, whereas generatetoaddress is core. See discussion in #6481. (done in #10683).
   • in httprpc.cpp, there are #ifdef ENABLE_WALLETs protecting registration/unregistration of the wallet endpoint. These can trivially be replaced with calls into the WalletInitInterface.
   • in interfaces/node.cpp, #ifdef ENABLE_WALLET protects the calls to getWallets(). This can easily be replaced by calls into a wallet manager/dummy wallet manager.

   The first phase would be to introduce the new (wallet-only) methods, then a release later remove wallet functionality from the core-only calls.

2. laanwj added the label Wallet on Apr 28, 2016
3. laanwj cross-referenced this on Apr 28, 2016 from issue [mining] allow other signal listeners to provide scripts for mining by jonasschnelli
4. 
   laanwj commented at 3:34 PM on April 29, 2016: member

   I think it may be nice to implement getinfo (or something similar) client-side in bitcoin-cli. For users of the command line it has some value as a status overview of the entire node+wallet, and it's simple enough to gather the info from that various getXXXinfo calls and return it in one screen.

5. laanwj referenced this in commit 37e4fdfe64 on May 10, 2016
6. laanwj cross-referenced this on May 10, 2016 from issue init: Move berkeleydb version reporting to wallet by laanwj
7. laanwj referenced this in commit 3e2c946cfd on May 10, 2016
8. MarcoFalke added the label Refactoring on May 10, 2016
9. laanwj added this to the milestone 0.14 on Jun 22, 2016
10. MarcoFalke cross-referenced this on Aug 18, 2016 from issue Move CWallet::setKeyPool to private section of CWallet. by pstratem
11. MarcoFalke cross-referenced this on Sep 19, 2016 from issue [init] Get rid of some ENABLE_WALLET by MarcoFalke
12. MarcoFalke cross-referenced this on Sep 20, 2016 from issue init: Get rid of fDisableWallet by MarcoFalke
13. 
    MarcoFalke commented at 4:44 PM on September 20, 2016: member

    I have modified the OP to reflect the current status based on the pulls that point to this issue.

14. 
    laanwj commented at 11:29 AM on September 21, 2016: member

    Thanks!

15. 
    laanwj commented at 10:23 AM on September 28, 2016: member

    signrawtransaction - should be split into an utility-only signrawtransaction and a wallet-specific call walletsignrawtransaction.

    Related: #4844

16. 
    luke-jr commented at 10:53 AM on September 28, 2016: member

    IMO utility-only signrawtransaction belongs in bitcoin-tx, and leave the RPC as wallet-specific.

17. 
    laanwj commented at 11:02 AM on September 28, 2016: member

    I tend to agree, although deprecating utility-only RPCs is a bit of an orthogonal issue. The main practical problem there is that bitcoin-tx is not a library, and doesn't have language bindings, whereas RPC does. Calling out to a program is not always acceptable. I suppose people are using signrawtransaction when they don't have a library for their programming language to provide that functionality locally. And it's preferable to ask bitcoind than use a badly hand-rolled one, imagine how badly ECDSA signing could be implemented in PHP...

    So while I agree on the long-term vision there, I think on the short run it may make sense to split the call instead.

18. 
    luke-jr commented at 6:47 PM on September 28, 2016: member

    That's a good point. In that case, perhaps make the utility version be the awkward name, since the long-term plan is to move it out?

19. 
    laanwj commented at 7:33 AM on September 29, 2016: member

    Fine with me, I don't want to get into bikeshedding here. What is your proposal regarding naming?

20. laanwj cross-referenced this on Oct 18, 2016 from issue Fix init segfault where InitLoadWallet() calls ATMP before genesis by TheBlueMatt
21. laanwj cross-referenced this on Oct 25, 2016 from issue Split up AppInit2 into multiple phases, daemonize after datadir lock errors by laanwj
22. MarcoFalke cross-referenced this on Oct 29, 2016 from issue [Wallet] Refactor wallet/init interaction (Reaccept wtx, flush thread) by jonasschnelli
23. MarcoFalke added this to the milestone 0.15.0 on Jan 6, 2017
24. MarcoFalke removed this from the milestone 0.14.0 on Jan 6, 2017
25. 
    achow101 commented at 11:49 PM on June 12, 2017: member

    #10579 does the signrawtransaction part of this

26. laanwj cross-referenced this on Jun 22, 2017 from issue Make sure we only mine via the first wallet by jonasschnelli
27. laanwj cross-referenced this on Jun 24, 2017 from issue [RPC] Split part of validateaddress into getaddressinfo by achow101
28. laanwj cross-referenced this on Jun 27, 2017 from issue rpc: Move the `generate` RPC call to rpcwallet by laanwj
29. jnewbery cross-referenced this on Jun 28, 2017 from issue RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet by luke-jr
30. jnewbery cross-referenced this on Jul 4, 2017 from issue [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery
31. jnewbery cross-referenced this on Jul 7, 2017 from issue [wallet] Remove Wallet dependencies from init.cpp by jnewbery
32. 
    achow101 commented at 1:22 AM on July 15, 2017: member

    I think this should be moved to the future milestone as all that primarily remains are RPC calls which all will require deprecation for a few versions first

33. MarcoFalke added this to the milestone Future on Jul 18, 2017
34. MarcoFalke removed this from the milestone 0.15.0 on Jul 18, 2017
35. 
    jnewbery commented at 2:23 PM on July 21, 2017: member

    Please update the PR info to reference the relevant PRs:

    • #10762 removes wallet dependencies from init.cpp
    • #10838 removes getinfo
    • #10583 splits validateaddress into wallet/non-wallet parts
    • #10579 splits signrawtransaction into wallet/non-wallet parts

    createmultisig is the only remaining part that we don't have a PR for.

    I also think we should label this 0.16. Should be easily achievable.

36. laanwj cross-referenced this on Aug 29, 2017 from issue rpc: Push down safe mode checks by laanwj
37. laanwj removed this from the milestone Future on Sep 28, 2017
38. laanwj added this to the milestone 0.16.0 on Sep 28, 2017
39. achow101 cross-referenced this on Sep 29, 2017 from issue [RPC] Disallow using addresses in createmultisig by achow101
40. 
    achow101 commented at 4:30 AM on September 29, 2017: member

    #11415 completes the createmultisig aspect of this.

41. 
    laanwj commented at 7:49 AM on September 29, 2017: member

    @jnewbery @achow101 Thanks, added

42. laanwj cross-referenced this on Nov 30, 2017 from issue trivial: Mention that wallet needs to be enabled for gettransaction by hkjn
43. laanwj removed this from the milestone 0.16.0 on Jan 11, 2018
44. laanwj added this to the milestone 0.17.0 on Jan 11, 2018
45. laanwj referenced this in commit 69ec021969 on Jan 24, 2018
46. 
    jnewbery commented at 7:32 PM on February 5, 2018: member

    #11415 has been merged. Can someone strike it from the list in this PR?

47. 
    jnewbery commented at 2:24 PM on February 20, 2018: member

    #10579 has been merged. Please strike from the list.

    Also add #12490, which actually removes the wallet dependencies from rpc.

48. 
    laanwj commented at 11:05 AM on February 23, 2018: member

    @jnewbery Done

49. laanwj referenced this in commit 6d53663a43 on Mar 29, 2018
50. 
    laanwj commented at 4:55 AM on May 2, 2018: member

    Updated for #10762. We're so close now.

    This is the only open item according to the OP:

    remove deprecated validateaddress and signrawtransaction wallet dependency completely (#12490)

51. 
    jnewbery commented at 7:20 PM on May 2, 2018: member

    We're so close now.

    Yes! Very close. I'll reopen #12490 as soon as v0.17 has been branched.

    There are a few other #ifdef ENABLE_WALLETs introduced since this issue was opened:

    1. in init.cpp, if ENABLE_WALLET is not defined, then a dummy WalletInitInterface is defined and instantiated. I think this is fine.
    2. in httprpc.cpp, there are #ifdef ENABLE_WALLETs protecting registration/unregistration of the wallet endpoint. These can trivially be replaced with calls into the WalletInitInterface.
    3. in interfaces/node.cpp, #ifdef ENABLE_WALLET protects the calls to getWallets(). This can easily be replaced by calls into a wallet manager/dummy wallet manager.
52. 
    laanwj commented at 5:36 AM on May 3, 2018: member

    There are a few other #ifdef ENABLE_WALLETs introduced since this issue was opened:

    I noticed. Agree (1) is not really a problem as it doesn't introduce a dependency. Just the theoretical architectural concern that libbitcoin_server.a could be built independently of --enable/disable-wallet. But given the monolithic build I don't think that's a short term priority, certainly not the reason I created this issue :)

    But yes, the other two need to go - will update the OP.

53. MarcoFalke removed this from the milestone 0.17.0 on May 3, 2018
54. MarcoFalke added this to the milestone 0.18.0 on May 3, 2018
55. 
    MarcoFalke commented at 3:35 PM on May 3, 2018: member

    Updated milestone to 0.18.0 accordingly.

56. HashUnlimited cross-referenced this on Aug 1, 2018 from issue [wallet] Remove Wallet dependencies from init.cpp and custom modules by HashUnlimited
57. jnewbery cross-referenced this on Sep 7, 2018 from issue Remove ENABLE_WALLET from libbitcoin_server.a by jnewbery
58. 
    jnewbery commented at 8:34 PM on September 7, 2018: member

    I've opened #14168 to remove the final instances of ENABLE_WALLET in libbitcoin_server.

    in interfaces/node.cpp, #ifdef ENABLE_WALLET protects the calls to getWallets(). This can easily be replaced by calls into a wallet manager/dummy wallet manager.

    This is actually in libbitcoin_util, so I think it can be removed from this issue.

59. MarcoFalke referenced this in commit 4103cc3169 on Sep 11, 2018
60. 
    jnewbery commented at 5:43 PM on September 11, 2018: member

    #14168 is merged.

    The final instance of #ifdef ENABLE_WALLET is actually in libbitcoin_util, so I think we're done here. @laanwj - do you agree? Should we close this issue, or keep it open to track removing the #ifdef ENABLE_WALLET from libbitcoin_util?

61. 
    laanwj commented at 7:29 AM on September 12, 2018: member

    @jnewbery The issue mentions only libbitcoin_server.a but I think that is because I assumed the only instances of ENABLE_WALLET would be in the server and GUI, not utilities that are linked into the -cli and -tx as well. That's kind of surprising.

    I'm surprised, I don't think interfaces/node.cpp should be in libutil in the first place? What needs it, outside of bitcoin-qt and bitcoind?

    Edit: I've succesfully built the code after moving the interfaces to server. See #14204.

62. laanwj cross-referenced this on Sep 12, 2018 from issue [build] Actually remove ENABLE_WALLET by jnewbery
63. laanwj closed this on Sep 13, 2018

    ---

64. 
    laanwj commented at 1:52 PM on September 13, 2018: member

    The last step here was done in #14208, which was merged shortly ago. This means that this long-running issue can finally be closed. Yay!

65. ryanofsky cross-referenced this on Sep 18, 2018 from issue Newsletters: add #13 (2018-09-18) by harding
66. jnewbery cross-referenced this on Oct 4, 2018 from issue Breaking User Space by nopara73
67. PastaPastaPasta referenced this in commit 925865c66c on Apr 14, 2020
68. PastaPastaPasta referenced this in commit f475316ba9 on Apr 16, 2020
69. PastaPastaPasta referenced this in commit 2fa9f27b75 on Apr 16, 2020
70. UdjinM6 referenced this in commit 97742a14d8 on Apr 18, 2020
71. PastaPastaPasta referenced this in commit 13b34dde3e on May 14, 2020
72. UdjinM6 referenced this in commit 0b3c3e8406 on May 15, 2020
73. ckti referenced this in commit 7ce38d8cd2 on Mar 28, 2021
74. ckti referenced this in commit 8558a96dc7 on Mar 28, 2021
75. Munkybooty referenced this in commit 120bb6235d on Jul 7, 2021
76. 5tefan referenced this in commit c04523c70f on Aug 10, 2021
77. bitcoin locked this on Sep 8, 2021

Contributors

laanwjMarcoFalkeluke-jrachow101jnewbery

Milestone
0.18.0