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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Rebased 771b0e36de7dad225e9b664a9fd1c2a55f94ed8b -> 05875a8bc8e500322f9a54f07d60e9ed1e295282 (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)); });
nit, inline 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
nit, add the usual comment.
Code review ACK after bases get merged and rebase. Nice work.
Rebased f1afe0ac5d0a389e415e67f19f91d7e538b7ad18 -> 314395c17f4fc02a72af790f8d959fe8941c51ad (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)?
if (!AppInitInterfaces(node)) return false;
seems to be addressed
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 | -
ฮผNit: This empty line looks nice, IMO.
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);
Style-nit: Usually no space before (.
seems to be addressed
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.
//! 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 ๐
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 24bf17602c620445f76c3b407937751c8a894d37 ๐
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjmewv/dJqaPdgxC1U3bn+w8eyykTimZGhO7qQeEkDFIM/KtzP62aCYhPeIUw4P
5bvnVqwB7+xU6ulQ8bKQBfWIZQiyCehQ5xCrB6kdTO8U2jUnUqO4G3c3ypgqbUXd
oNhRKT2P+V3XtkpU2DbAz+7n+B5GeoJA7OxXksEN8U9q3EdymgmTVVfeBQB36gtt
GlfQYbCksyKl8cj6FN5l4D0pJKOqPgfS21jh7WEjHoKIPE8ttTLYelgXQeSijhci
Iuc4NTts7l0dwTescpwOpVqyk0Mz/k6TshEZSDGaf7fjY26s5wz7RZFUVtYtICf8
S2zYQDque8mwZTnaEs4dVo5wGG6dqpWK5fPEdQFDAskdsDiXLnumsYpZOH68V5ov
62cztTrLVECqErAPjT0P+P/mBTrp8oHoTagzvrSO71Y/pyoc3zoAdNimxmT1K8ya
GStgbd4W9ia6J8D1s/ov4rxmZBH2XPBBaTXpas4++8BYiPjmx+NkteVXjJsRNOX/
ZNWwcqg0
=TcZF
-----END PGP SIGNATURE-----
Timestamp of file with hash 77e30443c0b09185b05b8ce84203f4afece67e0a8bb120e17ec2f352a85547ac -
</details>
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.
doc-nit: Should the docs say that this is not allowed to be called in a non-wallet build or when the wallet is disabled?
A regression introduced in this PR fixed in https://github.com/bitcoin-core/gui/pull/102.