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.
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.
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.
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: https://github.com/bitcoin/bitcoin/pull/6008/commits/fa6a5b28fcaa612c6512c6028184df6b346fdad4).
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).
306 }
307 }
308
309-void StopWallets() {
310+void WalletInit::StopWallets() {
311+ if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) return;
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.
6+#define WALLETINITINTERFACE_H
7+
8+class CScheduler;
9+class CRPCTable;
10+
11+class WalletInitInterface {
In commit “[wallet] Create wallet init interface.”
Just FYI, this PR is very similar to 6626e2fe114ce2aa861be1eeda84f0c9faa60527 from #10973 and the WalletInitInterface added here is basically identical to the Chain::Client interface added there:
Probably would replace this interface with that more generic one if this is merged first.
4@@ -5,6 +5,8 @@
5 #ifndef WALLETINITINTERFACE_H
6 #define WALLETINITINTERFACE_H
7
8+#include <string>
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.
11+class CRPCTable;
12+
13+class WalletInitInterface {
14+public:
15+ /** Get wallet help string */
16+ virtual std::string GetWalletHelpString(bool showDebug) {return (std::string)"";}
std::string{};
Updated PR description to no longer reference boost signals.
This is ready for review. It’s a fairly small changeset, and removes one of the last server->wallet dependencies.
414@@ -415,9 +415,9 @@ std::string HelpMessage(HelpMessageMode mode)
415 strUsage += HelpMessageOpt("-whitelist=<IP address or network>", _("Whitelist peers connecting from the given IP address (e.g. 1.2.3.4) or CIDR notated network (e.g. 1.2.3.0/24). Can be specified multiple times.") +
416 " " + _("Whitelisted peers cannot be DoS banned and their transactions are always relayed, even if they are already in the mempool, useful e.g. for a gateway"));
417
418-#ifdef ENABLE_WALLET
419- strUsage += GetWalletHelpString(showDebug);
420-#endif
421+ if (g_wallet_init_interface) {
422+ g_wallet_init_interface->GetWalletHelpString(showDebug);
strUsage += ... I believe.
8+#include <string>
9+
10+class CScheduler;
11+class CRPCTable;
12+
13+class WalletInitInterface {
Every method name in this interface has “Wallet” or “Wallets” in it, which is rather redundant given that this is a WalletInitInterface. I’d say drop them all, so “GetWalletHelpString” => “GetHelpString”, “WalletParameterInteraction” => “CheckParameterInteraction”, “RegisterWalletRPC” => “RegisterRPC”.
That alone would go a long way towards making this a generalized init module interface.
I’ve updated the function names, since as you point out, “Wallet” is redundant.
See my comment below about a generalized module init interface.
If WalletInitInterface is an interface class, maybe we can have an interface without method definitions. Have you considered an interface with pure virtual functions?
0 /** Get wallet help string */
1 virtual std::string GetWalletHelpString(bool showDebug) = 0;
2 /** Check wallet parameter interaction */
3 virtual bool WalletParameterInteraction() = 0;
4 /** Register wallet RPC*/
5 virtual void RegisterWalletRPC(CRPCTable &) = 0;
6 /** Verify wallets */
7 virtual bool VerifyWallets() = 0;
8 /** Open wallets*/
9 virtual bool OpenWallets() = 0;
10 /** Start wallets*/
11 virtual void StartWallets(CScheduler& scheduler) = 0;
12 /** Flush Wallets*/
13 virtual void FlushWallets() = 0;
14 /** Stop Wallets*/
15 virtual void StopWallets() = 0;
16 /** Close wallets */
17 virtual void CloseWallets() = 0;
Definitely an improvement, though I agree with @jonasschnelli that I’d rather see this be a general interface for init modules, rather than one specific to wallet initialization.
One way to do it is to name methods to correspond to their place in the init lifecycle, like BeforeDBOpen, AfterDBClose, BeforeNetStart, AfterNetStop, etc.
I’d rather see this be a general interface for init modules
Do you think that’s a hard requirement before this PR gets merged? This PR is simply changing function calls to an internal interface class, which can be updated or replaced later. It seems to me that creating a brand new general interface for modules is outside the scope of this.
Just merging this PR has a real benefit now - it removes one of the last remaining server -> wallet dependencies.
10+class CScheduler;
11+class CRPCTable;
12+
13+class WalletInitInterface {
14+public:
15+ /** Get wallet help string */
Implement a virtual destructor to protect against undefined behavior in the event that an instance of a derived WalletInitInterface instance is destroyed through a pointer to this interface.
0virtual ~WalletInitInterface() {}
Tested ACK https://github.com/bitcoin/bitcoin/pull/10762/commits/b3e482c65c5c4697b38bba6006dbcdc3c1bba836
-wallet=/path/to/backup (File -> Backup wallet…)
WalletInit method names, switching to pure virtual methods, removing -disablewallet check in StopWallets, fixing GetWalletHelpString bug.
overrides
11
12 class CScheduler;
13 class CWallet;
14
15+class WalletInitInterface;
16+extern std::unique_ptr<WalletInitInterface> g_wallet_init_interface;
187@@ -189,9 +188,9 @@ void Shutdown()
188 StopREST();
189 StopRPC();
190 StopHTTPServer();
191-#ifdef ENABLE_WALLET
192- FlushWallets();
193-#endif
194+ if (g_wallet_init_interface) {
This commit creates a global g_wallet_init_interface, which is created
in bitcoind and bitcoin-qt. g_wallet_init_interface is used to init
and destroy the wallet.
This removes the dependency from init.cpp on the wallet library.
Added a dummy wallet init class as requested here: #10762 (review)
I started trying to implement the suggestion of passing the wallet_init_interface into the AppInit* functions, but it ended up being a much larger change than I was expecting, since the wallet interface is called from several different functions in init.cpp, which are called from various places in qt.
I’m going to leave it as a global, since it’s limited to the init.cpp file, and this PR already has 4 ACKs. The global can be removed in a future PR if necessary.
1581@@ -1597,12 +1582,7 @@ bool AppInitMain()
1582 fFeeEstimatesInitialized = true;
1583
1584 // ********************************************************* Step 8: load wallet
1585-#ifdef ENABLE_WALLET
1586- if (!OpenWallets())
1587- return false;
1588-#else
1589- LogPrintf("No wallet support compiled in!\n");
