refactor: Move wallet methods out of chain.h and node.h #19099

ryanofsky · 2020-05-28T21:25:08Z

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.

DrahtBot · 2020-05-29T06:48:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet, gui: Reload previously loaded wallets on startup #19754 (wallet, gui: Reload previously loaded wallets on startup by achow101)
Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)
multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
Wallet passive startup #15719 (Wallet passive startup by ryanofsky)
Remove the automatic creation and loading of the default wallet #15454 (Remove the automatic creation and loading of the default wallet by achow101)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

ryanofsky · 2020-05-29T12:48:00Z

Rebased 771b0e3 -> 05875a8 (pr/wclient.1 -> pr/wclient.2, compare) due to conflict with #17993, and including splashscreen init fix
Rebased 05875a8 -> 7f6b16c (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 7f6b16c -> 7e2da82 (pr/wclient.3 -> pr/wclient.4, compare) on rebased base pr #19098 pr/qtx.4
Rebased 7e2da82 -> f1afe0a (pr/wclient.4 -> pr/wclient.5, compare) on rebased base pr #19098 pr/qtx.5

kallewoof

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

kallewoof · 2020-08-26T05:29:13Z

src/bitcoind.cpp

@@ -152,6 +151,7 @@ static bool AppInit(int argc, char* argv[])
            // If locking the data directory failed, exit immediately
            return false;
        }
+        AppInitInterfaces(node);


Why ignore returned value (even if only true at this point)?

Suggested change

AppInitInterfaces(node);

if (!AppInitInterfaces(node)) return false;

seems to be addressed

kallewoof · 2020-08-26T05:30:56Z

src/interfaces/node.cpp

 namespace interfaces {
-


μNit: This empty line looks nice, IMO.

kallewoof · 2020-08-26T05:31:58Z

src/interfaces/node.cpp

-        std::shared_ptr<CWallet> wallet;
-        status = CreateWallet(*m_context->chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
-        return MakeWallet(wallet);
+        assert (m_context->wallet_client);


Style-nit: Usually no space before (.

seems to be addressed

Suggested bitcoin#19099 (comment)

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 bitcoin#19099 (comment)

promag

Code review ACK 24bf176.

(actually reviewed in #19101)

maflcko

ACK 24bf176 🐚

Show signature and timestamp

Signature:

Timestamp of file with hash 77e30443c0b09185b05b8ce84203f4afece67e0a8bb120e17ec2f352a85547ac -

maflcko · 2020-08-31T08:04:06Z

src/wallet/init.cpp

@@ -129,5 +130,7 @@ void WalletInit::Construct(NodeContext& node) const
            settings.rw_settings["wallet"] = wallets;
        });
    }
-    node.chain_clients.emplace_back(interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet")));
+    auto wallet_client = interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet"));
+    node.wallet_client = wallet_client.get();


nit in e4f4350:

Would be good to assert !node.wallet_client to avoid accidentally re-seating the reference

maflcko · 2020-08-31T08:05:31Z

src/node/context.h

@@ -40,7 +41,11 @@ struct NodeContext {
    std::unique_ptr<BanMan> banman;
    ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
    std::unique_ptr<interfaces::Chain> chain;
+    //! List of all chain clients (wallet processes or other client) connected to node.


Suggested change

//! 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.

maflcko · 2020-08-31T16:22:19Z

src/interfaces/node.h

-
-    //! Create a wallet from file
-    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;
+    //! 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?

… node.h 24bf176 gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky) e4f4350 refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky) b266b3e refactor: Create interfaces earlier during initialization (Russell Yanofsky) Pull request description: 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 bitcoin#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. ACKs for top commit: promag: Code review ACK 24bf176. MarcoFalke: ACK 24bf176 🐚 Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf

hebasto · 2020-10-09T19:16:19Z

A regression introduced in this PR fixed in bitcoin-core/gui#102.

c056064 gui: Fix SplashScreen crash when run with -disablewallet (Hennadii Stepanov) Pull request description: This PR fixes the bug introduced in bitcoin/bitcoin#19099: ``` $ src/qt/bitcoin-qt -disablewallet bitcoin-qt: interfaces/node.cpp:236: auto interfaces::(anonymous namespace)::NodeImpl::walletClient()::(anonymous class)::operator()() const: Assertion `"m_context->wallet_client" && check' failed. Aborted (core dumped) ``` ACKs for top commit: Sjors: tACK c056064 promag: ACK c056064. Tree-SHA512: 263d9efd5899cc6e447dfc5142bf911ca627149fac0a1c5e5b58dd196aa5e0d12fe13e3f750fb5f3c4338222f7959935d2f77391263f967dbca2e0e79a416a29

Suggested bitcoin/bitcoin#19099 (comment)

Summary: 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. This is a backport of [[bitcoin/bitcoin#19099 | core#19099]] [1/3] bitcoin/bitcoin@b266b3e Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10141

Summary: bitcoin/bitcoin@e4f4350 > 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. bitcoin-core/gui@c056064 > gui: Fix SplashScreen crash when run with -disablewallet This is a backport of [[bitcoin/bitcoin#19099 | core#19099]] [2/3] and [[bitcoin-core/gui#102 | core-gui#102]] (small bugfix) Depends on D10141 Test Plan: `ninja all check-all` Test for bugfix core-gui#102: `src/qt/bitcoin-qt -disablewallet` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Klakurka, boardgamesaz, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10142

Summary: Suggested bitcoin/bitcoin#19099 (comment) This concludes backport of [[bitcoin/bitcoin#19099 | core#19099]] [3/3] bitcoin/bitcoin@24bf176 Depends on D10142 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10143

ryanofsky mentioned this pull request May 28, 2020

refactor: remove ::vpwallets and related global variables #19101

Merged

DrahtBot added GUI Refactoring Tests Wallet labels May 28, 2020

DrahtBot mentioned this pull request May 29, 2020

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Merged

DrahtBot added the Needs rebase label May 29, 2020

ryanofsky force-pushed the pr/wclient branch from 771b0e3 to 05875a8 Compare May 29, 2020 12:47

DrahtBot removed the Needs rebase label May 29, 2020

This was referenced May 30, 2020

test: Remove duplicate NodeContext hacks #19098

Merged

wallettool: Add dump and createfromdump commands #19137

Merged

Wallet passive startup #15719

Closed

Multiprocess bitcoin #10102

Draft

multiprocess: Add basic spawn and IPC support #19160

Merged

ryanofsky force-pushed the pr/wclient branch from 05875a8 to 7f6b16c Compare June 4, 2020 13:50

DrahtBot mentioned this pull request Jun 5, 2020

refactor: Error message bilingual_str consistency #19176

Merged

DrahtBot added the Needs rebase label Jun 9, 2020

ryanofsky force-pushed the pr/wclient branch from 7f6b16c to 7e2da82 Compare June 10, 2020 19:09

DrahtBot removed the Needs rebase label Jun 10, 2020

DrahtBot mentioned this pull request Jun 11, 2020

Replace automatic bans with discouragement filter #19219

Merged

DrahtBot removed the Needs rebase label Aug 17, 2020

This was referenced Aug 20, 2020

wallet, gui: Reload previously loaded wallets on startup #19754

Merged

Remove gArgs global from init #19779

Merged

kallewoof reviewed Aug 26, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 26, 2020

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 26, 2020

gui refactor: Inline SplashScreen::ConnectWallet

c6ac474

Suggested bitcoin#19099 (comment)

ryanofsky force-pushed the pr/wclient branch from 72acd6b to c6ac474 Compare August 26, 2020 23:58

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 26, 2020

gui refactor: Inline SplashScreen::ConnectWallet

3b2908d

Suggested bitcoin#19099 (comment)

ryanofsky added 3 commits August 27, 2020 14:33

gui refactor: Inline SplashScreen::ConnectWallet

24bf176

Suggested bitcoin#19099 (comment)

ryanofsky force-pushed the pr/wclient branch from c6ac474 to 24bf176 Compare August 28, 2020 06:16

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 28, 2020

gui refactor: Inline SplashScreen::ConnectWallet

1c45421

Suggested bitcoin#19099 (comment)

DrahtBot removed the Needs rebase label Aug 28, 2020

promag reviewed Aug 30, 2020

View reviewed changes

maflcko removed GUI Tests Wallet labels Aug 31, 2020

maflcko reviewed Aug 31, 2020

View reviewed changes

maflcko merged commit 269a7cc into bitcoin:master Aug 31, 2020

maflcko reviewed Aug 31, 2020

View reviewed changes

hebasto mentioned this pull request Oct 9, 2020

Fix SplashScreen crash when run with -disablewallet bitcoin-core/gui#102

Merged

janus pushed a commit to janus/bitgesell that referenced this pull request Dec 7, 2020

gui refactor: Inline SplashScreen::ConnectWallet

f1d2238

Suggested bitcoin/bitcoin#19099 (comment)

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Move wallet methods out of chain.h and node.h #19099

refactor: Move wallet methods out of chain.h and node.h #19099

Uh oh!

ryanofsky commented May 28, 2020 •

edited

Loading

Uh oh!

DrahtBot commented May 29, 2020 •

edited

Loading

Uh oh!

ryanofsky commented May 29, 2020 •

edited

Loading

Uh oh!

kallewoof left a comment

Uh oh!

kallewoof Aug 26, 2020

Uh oh!

maflcko Aug 31, 2020

Uh oh!

kallewoof Aug 26, 2020

Uh oh!

kallewoof Aug 26, 2020

Uh oh!

maflcko Aug 31, 2020

Uh oh!

promag left a comment

Uh oh!

maflcko left a comment

Uh oh!

maflcko Aug 31, 2020

Uh oh!

maflcko Aug 31, 2020

Uh oh!

maflcko Aug 31, 2020 •

edited

Loading

Uh oh!

hebasto commented Oct 9, 2020

Uh oh!

Uh oh!

	AppInitInterfaces(node);
	if (!AppInitInterfaces(node)) return false;

	//! 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.

refactor: Move wallet methods out of chain.h and node.h #19099

refactor: Move wallet methods out of chain.h and node.h #19099

Uh oh!

Conversation

ryanofsky commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 9, 2020

Uh oh!

Uh oh!

ryanofsky commented May 28, 2020 •

edited

Loading

DrahtBot commented May 29, 2020 •

edited

Loading

ryanofsky commented May 29, 2020 •

edited

Loading

maflcko Aug 31, 2020 •

edited

Loading