Remaining instances of ENABLE_WALLET in `libbitcoin

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.

laanwj added the label Wallet on Apr 28, 2016

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.

laanwj referenced this in commit 37e4fdfe64 on May 10, 2016

laanwj referenced this in commit 3e2c946cfd on May 10, 2016

MarcoFalke added the label Refactoring on May 10, 2016

laanwj added this to the milestone 0.14 on Jun 22, 2016

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.

laanwj commented at 11:29 am on September 21, 2016: member

Thanks!

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

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.

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.

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?

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?

MarcoFalke added this to the milestone 0.15.0 on Jan 6, 2017

MarcoFalke removed this from the milestone 0.14.0 on Jan 6, 2017

achow101 commented at 11:49 pm on June 12, 2017: member

#10579 does the signrawtransaction part of this

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

MarcoFalke added this to the milestone Future on Jul 18, 2017

MarcoFalke removed this from the milestone 0.15.0 on Jul 18, 2017

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.

laanwj removed this from the milestone Future on Sep 28, 2017

laanwj added this to the milestone 0.16.0 on Sep 28, 2017

achow101 commented at 4:30 am on September 29, 2017: member

#11415 completes the createmultisig aspect of this.

laanwj commented at 7:49 am on September 29, 2017: member

@jnewbery @achow101 Thanks, added

laanwj removed this from the milestone 0.16.0 on Jan 11, 2018

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

laanwj referenced this in commit 69ec021969 on Jan 24, 2018

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?

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.

laanwj commented at 11:05 am on February 23, 2018: member

@jnewbery Done

laanwj referenced this in commit 6d53663a43 on Mar 29, 2018

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)

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:

in init.cpp, if ENABLE_WALLET is not defined, then a dummy WalletInitInterface is defined and instantiated. I think this is fine.
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.

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.

MarcoFalke removed this from the milestone 0.17.0 on May 3, 2018

MarcoFalke added this to the milestone 0.18.0 on May 3, 2018

MarcoFalke commented at 3:35 pm on May 3, 2018: member

Updated milestone to 0.18.0 accordingly.

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.

MarcoFalke referenced this in commit 4103cc3169 on Sep 11, 2018

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?

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.

laanwj closed this on Sep 13, 2018

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!

PastaPastaPasta referenced this in commit 925865c66c on Apr 14, 2020

PastaPastaPasta referenced this in commit f475316ba9 on Apr 16, 2020

PastaPastaPasta referenced this in commit 2fa9f27b75 on Apr 16, 2020

UdjinM6 referenced this in commit 97742a14d8 on Apr 18, 2020

PastaPastaPasta referenced this in commit 13b34dde3e on May 14, 2020

UdjinM6 referenced this in commit 0b3c3e8406 on May 15, 2020

ckti referenced this in commit 7ce38d8cd2 on Mar 28, 2021

ckti referenced this in commit 8558a96dc7 on Mar 28, 2021

Munkybooty referenced this in commit 120bb6235d on Jul 7, 2021

5tefan referenced this in commit c04523c70f on Aug 10, 2021

DrahtBot locked this on Sep 8, 2021

Remaining instances of ENABLE_WALLET in libbitcoin_server.a #7965

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