Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
laanwj opened this issue Apr 28, 2016 · 24 comments

Comments

Projects
None yet
5 participants
@laanwj
Copy link
Member

commented Apr 28, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

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 added a commit to laanwj/bitcoin that referenced this issue May 10, 2016

init: Move berkeleydb version reporting to wallet
Move the version reporting to Wallet::Verify, before starting
verification of the wallet.

This removes the dependency of init on a specific wallet database
library.

A further, trivial step towards resolving bitcoin#7965.

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

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

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2016

Thanks!

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2016

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

Related: #4844

@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

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

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

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

@achow101

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

#10579 does the signrawtransaction part of this

@achow101

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

#11415 completes the createmultisig aspect of this.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2017

@jnewbery @achow101 Thanks, added

@laanwj laanwj modified the milestones: 0.16.0, 0.17.0 Jan 11, 2018

laanwj added a commit that referenced this issue Jan 24, 2018

Merge #11415: [RPC] Disallow using addresses in createmultisig
1df206f Disallow using addresses in createmultisig (Andrew Chow)

Pull request description:

  This PR should be the last part of #7965.

  This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated.

  It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode).

  `addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript.

Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969
@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

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

@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

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

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

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2018

@jnewbery Done

laanwj added a commit that referenced this issue Mar 29, 2018

Merge #10762: [wallet] Remove Wallet dependencies from init.cpp
c7ec524 [wallet] Add dummy wallet init class (John Newbery)
49baa4a [wallet] Use global g_wallet_init_interface to init/destroy the wallet. (John Newbery)
caaf972 [wallet] Create wallet init interface. (John Newbery)
5fb5421 [wallet] Move wallet init functions into WalletInit class. (John Newbery)

Pull request description:

  This continues the work of #7965. This PR, along with several others, would remove the remaining dependencies from libbitcoin_server.a on libbitcoin_wallet.a.

  To create the interface, I've just translated all the old init.cpp wallet function calls into an interface class. I've not done any thinking about whether it makes sense to change that interface by combining/splitting those calls. This is a purely internal interface, so there's no problem in changing it later.

Tree-SHA512: 32ea57615229c33fd1a7f2f29ebc11bf30337685f7211baffa899823ef74b65dcbf068289c557a161c5afffb51fdc38a2ee8180720371f64d433b12b0615cf3f
@laanwj

This comment has been minimized.

Copy link
Member Author

commented May 2, 2018

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

This comment has been minimized.

Copy link
Member

commented May 2, 2018

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.
@laanwj

This comment has been minimized.

Copy link
Member Author

commented May 3, 2018

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 MarcoFalke modified the milestones: 0.17.0, 0.18.0 May 3, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 3, 2018

Updated milestone to 0.18.0 accordingly.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

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 added a commit to MarcoFalke/bitcoin that referenced this issue Sep 11, 2018

Merge bitcoin#14168: Remove ENABLE_WALLET from libbitcoin_server.a
7d038dc [build] remove ENABLE_WALLET ifdef from httprpc.cpp (John Newbery)
3076556 [build] Move dummy wallet into its own .cpp file. (John Newbery)

Pull request description:

  This removes the final instances of ENABLE_WALLET in libbitcoin_server and so completes bitcoin#7965.

Tree-SHA512: a49128b7c17f4f69940d5843e6b785f08687efb377b5157d5b267d1205e596eb5c1966f1afb8ab36bcc2491c46252099e3e844c91f5623da8ded2e358d46338d
@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

#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

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

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!

@ryanofsky ryanofsky referenced this issue Sep 18, 2018

Merged

Newsletters: add #13 (2018-09-18) #67

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.