Add WalletClient interface so node interface is cleaner and don’t need wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in #19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
pr/wclient.1 -> pr/wclient.2, compare) due to conflict with #17993, and including splashscreen init fix
Rebased 05875a8bc8e500322f9a54f07d60e9ed1e295282 -> 7f6b16c00feae2b7fbed41d6e091b7a84ad75dbf (pr/wclient.2 -> pr/wclient.3, compare) on updated base #19098 with no changes. Fixes UndefinedBehaviorSanitizer error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692541509#L4295 from base pr
Rebased 7f6b16c00feae2b7fbed41d6e091b7a84ad75dbf -> 7e2da82eee3cea75ff61f9a869e2791d78a7fe41 (pr/wclient.3 -> pr/wclient.4, compare) on rebased base pr #19098 pr/qtx.4
Rebased 7e2da82eee3cea75ff61f9a869e2791d78a7fe41 -> f1afe0ac5d0a389e415e67f19f91d7e538b7ad18 (pr/wclient.4 -> pr/wclient.5, compare) on rebased base pr #19098 pr/qtx.5
190+
191+void SplashScreen::handleLoadWallet()
192+{
193 #ifdef ENABLE_WALLET
194- m_handler_load_wallet = m_node.handleLoadWallet([this](std::unique_ptr<interfaces::Wallet> wallet) { ConnectWallet(std::move(wallet)); });
195+ m_handler_load_wallet = m_node.walletClient().handleLoadWallet([this](std::unique_ptr<interfaces::Wallet> wallet) { ConnectWallet(std::move(wallet)); });
SplashScreen::ConnectWallet here - remove from header and reduce ENABLE_WALLET usage.
re: #19099 (review)
nit, inline
SplashScreen::ConnectWallethere - remove from header and reduceENABLE_WALLETusage.
Inlined in followup commit
305@@ -303,6 +306,31 @@ class Wallet
306 virtual CWallet* wallet() { return nullptr; }
307 };
308
309+class WalletClient : public ChainClient
pr/wclient.5 -> pr/wclient.6, compare) on top of #19098 pr/qtx.7 with suggestions
Rebased 314395c17f4fc02a72af790f8d959fe8941c51ad -> 7d82b31f1ac8590d914968fcd9d455a0958d4ae4 (pr/wclient.6 -> pr/wclient.7, compare) on top of #19098 pr/qtx.8 due to conflicts with #18923
Rebased 7d82b31f1ac8590d914968fcd9d455a0958d4ae4 -> 75fc0cb723f06ec2d5144f99d55a2b844f1af080 (pr/wclient.7 -> pr/wclient.8, compare) after base PR #19098 merge
Rebased 75fc0cb723f06ec2d5144f99d55a2b844f1af080 -> 9ccb7b3f2fe7e6d924f807275b5a7567d7b49b11 (pr/wclient.8 -> pr/wclient.9, compare) due to conflict with #19011
Rebased 9ccb7b3f2fe7e6d924f807275b5a7567d7b49b11 -> 72acd6bca3fd408c9c7ca4801de817c69d59aca5 (pr/wclient.9 -> pr/wclient.10, compare) due to conflict with #15937
150@@ -152,6 +151,7 @@ static bool AppInit(int argc, char* argv[])
151 // If locking the data directory failed, exit immediately
152 return false;
153 }
154+ AppInitInterfaces(node);
Why ignore returned value (even if only true at this point)?
0 if (!AppInitInterfaces(node)) return false;
48-std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings);
49-WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, std::shared_ptr<CWallet>& result);
50-std::unique_ptr<interfaces::Handler> HandleLoadWallet(interfaces::Node::LoadWalletFn load_wallet);
51-
52 namespace interfaces {
53-
274- std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, WalletCreationStatus& status) override
275- {
276- std::shared_ptr<CWallet> wallet;
277- status = CreateWallet(*m_context->chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
278- return MakeWallet(wallet);
279+ assert (m_context->wallet_client);
(.
Conceptually looks like a good change, and codewise looks like a clean-up, but the PR description could use a bit more motivation as for why this is needed. It could simply be that I’m not aware of the bigger project, though.
Code review ACK
Add AppInitInterfaces function so wallet chain and chain client interfaces are
created earlier during initialization. This is needed in the next commit to
allow the gui splash screen to be able to register for wallet events through a
dedicated WalletClient interface instead managing wallets indirectly through
the Node interface. This only works if the wallet client interface is created
before the splash screen needs to use it.
Add WalletClient interface so node interface is cleaner and don't need
wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate
global wallet variables like ::vpwallets, because createWallet(), loadWallet(),
getWallets(), etc methods called by the GUI need a way to get a reference to
the list of open wallets if it is no longer a global variable.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
Suggested https://github.com/bitcoin/bitcoin/pull/19099#discussion_r450522201
129@@ -129,5 +130,7 @@ void WalletInit::Construct(NodeContext& node) const
130 settings.rw_settings["wallet"] = wallets;
131 });
132 }
133- node.chain_clients.emplace_back(interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet")));
134+ auto wallet_client = interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet"));
135+ node.wallet_client = wallet_client.get();
nit in e4f435047121886edb6e6a6c4e4998e44ed2e36a:
Would be good to assert !node.wallet_client to avoid accidentally re-seating the reference
40@@ -40,7 +41,11 @@ struct NodeContext {
41 std::unique_ptr<BanMan> banman;
42 ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
43 std::unique_ptr<interfaces::Chain> chain;
44+ //! List of all chain clients (wallet processes or other client) connected to node.
0 //! List of all chain clients (wallet client or other client) connected to node.
nit in e4f4350:
could simply say that there is at most a single wallet client in this list, not one for each wallet.
ACK 24bf17602c620445f76c3b407937751c8a894d37 ๐
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 24bf17602c620445f76c3b407937751c8a894d37 ๐
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjmewv/dJqaPdgxC1U3bn+w8eyykTimZGhO7qQeEkDFIM/KtzP62aCYhPeIUw4P
85bvnVqwB7+xU6ulQ8bKQBfWIZQiyCehQ5xCrB6kdTO8U2jUnUqO4G3c3ypgqbUXd
9oNhRKT2P+V3XtkpU2DbAz+7n+B5GeoJA7OxXksEN8U9q3EdymgmTVVfeBQB36gtt
10GlfQYbCksyKl8cj6FN5l4D0pJKOqPgfS21jh7WEjHoKIPE8ttTLYelgXQeSijhci
11Iuc4NTts7l0dwTescpwOpVqyk0Mz/k6TshEZSDGaf7fjY26s5wz7RZFUVtYtICf8
12S2zYQDque8mwZTnaEs4dVo5wGG6dqpWK5fPEdQFDAskdsDiXLnumsYpZOH68V5ov
1362cztTrLVECqErAPjT0P+P/mBTrp8oHoTagzvrSO71Y/pyoc3zoAdNimxmT1K8ya
14GStgbd4W9ia6J8D1s/ov4rxmZBH2XPBBaTXpas4++8BYiPjmx+NkteVXjJsRNOX/
15ZNWwcqg0
16=TcZF
17-----END PGP SIGNATURE-----
Timestamp of file with hash 77e30443c0b09185b05b8ce84203f4afece67e0a8bb120e17ec2f352a85547ac -
186- //! with handleLoadWallet.
187- virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
188-
189- //! Create a wallet from file
190- virtual std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, WalletCreationStatus& status) = 0;
191+ //! Get wallet client.
