[wallet] Remove Wallet dependencies from init.cpp

[jnewbery]

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.

[promag]

Will take a look.

[jnewbery]

Thanks @promag - this is very rough-and-ready at the moment. It builds and passes tests, but is not ready for code review. I'd be very happy to get feedback about general concept though.

[jnewbery]

I see that @theuni has opened #10756 to replace CNodeSignals with an interface class. Would that be a better approach here?

[ryanofsky]

I see that @theuni has opened #10756 to replace CNodeSignals with an interface class. Would that be a better approach here?

Yeah I was just about to make this suggestion. You could delete the WalletInitSignals struct and replace the WalletInitSignals global variable with a std::unique_ptr<WalletInitInterface> global. This would get rid of all the boost signals code and make the control flow simpler.

Also you should prefix global variables with g_. And I think this PR would be less error prone and simpler to review if you just left the init code in wallet.cpp for now and moved the relevant functions to walletinit.cpp in followup MOVEONLY PR.

[jnewbery]

I think this PR would be less error prone and simpler to review if you just left the init code in wallet.cpp for now and moved the relevant functions to walletinit.cpp in followup MOVEONLY PR.

Thanks @ryanofsky, you've convinced me - I'll split this PR into two. But I think I'll do it the other way round - first move the wallet initialization/destruction functions into their own walletinit translation unit, and then introduce the interface class in a second PR. The reason is that #10740 is built on walletinit.cpp so this PR and #10740 will no longer be interdependent.

[jnewbery]

Rebased on master. I haven't swapped out the boost signals for an interface class yet. Still looking for high-level feedback on this approach.

The first three commits in this PR are shared with #10740

[jonasschnelli]

Concept ACK on the general direction, though unsure about the implementation (haven't checked the code in detail).

I was following a similar approach in #6008 (relatively old closed PR) (check commit: fa6a5b2).

Maybe, init/the-core only knows about modules. Some signal/hooks would allow initialising, shutdown of such modules. The wallet could be one, the miner another one, etc.
It would also allow to run with multiple wallet implementations (my idea was once to replace BDB with an append only file-format and removing all legacy code,... put this in a second wallet implementation and label it experimental).

If possible, I would prefer a more generic approach (a module interface rather then a wallet interface).

[TheBlueMatt]

Concept ACK, generally. Agree that I'd really prefer no boost::signals (especially in the header!).

[maflcko]

Needs rebase

[PierreRochard]

Concept ACK

[jnewbery]

Rebased. Feedback not yet addressed.

[jnewbery]

I've reimplemented this as an interface class. Should be ready for review.

[ryanofsky]

Looks good, conditional utACK 89d1d6ca293f602b88bf47fc2c3da37ddf6d4650 if you address the WalletInit::StopWallets comment below and also update the PR description to no longer refer to boost signals.

[ryanofsky]

In commit "[wallet] Move wallet init functions into WalletInit class."

Is adding this line here intentional? It doesn't seem to fit in with other changes in this commit, and seems unnecessary. Should add a comment describing rationale if it is useful to keep.

[jnewbery]

Yes, removed. This is a vestige of a previous branch.

[ryanofsky]

In commit "[wallet] Create wallet init interface."

Just FYI, this PR is very similar to 6626e2f from #10973 and the WalletInitInterface added here is basically identical to the Chain::Client interface added there:

bitcoin/src/interface/chain.h

Lines 18 to 40 in 6626e2f

	//! Interface to let node manage chain clients (wallets, or maybe tools for
	//! monitoring and analysis in the future).
	class Client
	{
	public:
	virtual ~Client() {}
	//! Register rpcs.
	virtual void registerRpcs() = 0;
	//! Prepare for execution, loading any needed state.
	virtual bool prepare() = 0;
	//! Start client execution and provide a scheduler. (Scheduler is
	//! ignored if client is out-of-process).
	virtual void start(CScheduler& scheduler) = 0;
	//! Stop client execution and prepare for shutdown.
	virtual void stop() = 0;
	//! Shut down client.
	virtual void shutdown() = 0;
	};

Probably would replace this interface with that more generic one if this is merged first.

[jnewbery]

Thanks for the heads up!

[ryanofsky]

In commit "[wallet] Use global g_wallet_init_interface to init/destroy the wallet."

This change would make a little more sense in the previous commit adding the file.

[jnewbery]

Agree. Fixed

[maflcko]

style-nit: Instead of casting a literal, you could directly return an empty std::string{};

[jnewbery]

better. Thanks!