[wallet] Clarify wallet initialization / destruction interface #10767

jnewbery · 2017-07-07T15:05:00Z

Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762

All wallet component initialization/destruction functions are now in their own wallet/init.cpp translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet.

There should be no changes in behavior from this PR.

promag · 2017-07-07T15:17:55Z

src/init.cpp

@@ -187,11 +187,6 @@ void Shutdown()
    StopREST();
    StopRPC();
    StopHTTPServer();
-#ifdef ENABLE_WALLET


Nothing replacing this code?

Correct - I don't believe the early wallet flush is necessary (although I may be wrong). See PR description:

The most important one is that Flush() no longer gets called twice on shutdown. I think this is fine, and all the tests pass, but this probably requires some careful consideration from people who are more familiar with the wallet code.

promag · 2017-07-07T15:24:11Z

src/init.cpp

@@ -230,9 +225,7 @@ void Shutdown()
        pblocktree = NULL;
    }
 #ifdef ENABLE_WALLET


What about moving these #ifdef ENABLE_WALLET to walletinit.cpp?

Because walletinit.cpp is in libbitcoin_wallet, so is not linked if ENABLE_WALLET isn't defined. #10762 will remove the libbitcoin_server -> libbitcoin_wallet dependencies entirely from init.cpp.

practicalswift · 2017-07-17T12:52:03Z

Rebase needed :-)

jnewbery · 2017-07-17T13:10:51Z

This isn't going in until after 0.15. which includes some wallet changes. I'll rebase after 0.15 has been cut.

practicalswift · 2017-07-17T13:19:38Z

@jnewbery Got it! Thanks for the clarification.

jnewbery · 2017-08-14T15:35:14Z

This overlaps significantly with #10976, which is easier to review since it's mostly move-only.

I'll rebase this on #10976 once that gets merged.

jnewbery · 2017-08-28T18:12:34Z

Rebased on @ryanofsky's #10976 and reduced scope. This PR now only moves the remaining wallet startup/shutdown functions to wallet/init.cpp in preparation for #10740 (dynamic loading/unloading of wallets) and #10762 (remove wallet dependencies from init.cpp).

There should be no change in behavior from this PR.

ryanofsky

utACK 3f425ee4263f66ab039c089d189b6cad0abbc23c. Looks like a simple cleanup with no change in behavior. FWIW I have similar functions in #10973, but am using {start, stop, shutdown} instead of {WalletCompleteStartup, FlushWallets, ShutdownWallets}. Links: declarations, definitions

ryanofsky · 2017-08-28T21:53:42Z

src/wallet/init.h

@@ -22,4 +22,7 @@ bool WalletVerify();
 //! Load wallet databases.
 bool InitLoadWallet();

+//! Flush all wallets in preparation for shutdown.
+//  Call with shutdown = true to actually shutdown the wallet.


In commit "[wallet] move wallet flush calls to wallet/init.cpp"

I think you need to use //! prefix on each line of the comment for doxygen to pick it up.

ryanofsky · 2017-08-28T21:59:19Z

src/init.cpp

-        delete pwallet;
-    }
-    vpwallets.clear();
+    DetachWallets();


In commit "[wallet] move wallet detach calls to wallet/init.cpp"

Maybe go with "close" or "free" instead of "detach". Detach sounds like it could be keeping the wallets open but handing them off.

👍 FreeWallets.

promag · 2017-08-28T22:18:47Z

src/wallet/init.h

@@ -6,14 +6,22 @@
 #ifndef BITCOIN_WALLET_INIT_H
 #define BITCOIN_WALLET_INIT_H

+#include "wallet/rpcwallet.h"


promag · 2017-08-28T22:21:47Z

src/wallet/init.cpp

@@ -245,3 +253,28 @@ bool InitLoadWallet()

    return true;
 }
+
+void WalletCompleteStartup(CScheduler& scheduler) {
+    for (CWalletRef pwallet : vpwallets) {


Offtopic, we could ditch CWalletRef.

promag · 2017-08-28T22:28:44Z

src/init.cpp

-        delete pwallet;
-    }
-    vpwallets.clear();
+    DetachWallets();


👍 FreeWallets.

promag · 2017-08-28T22:29:30Z

src/wallet/init.h

+void ShutdownWallets();
+
+//! Detach all wallets
+void DetachWallets();


Newline after.

promag · 2017-08-28T22:29:37Z

src/wallet/init.h

+//! Shutdown all wallets. Wallets will be flushed first.
+void ShutdownWallets();
+
+//! Detach all wallets


Same as above.

promag · 2017-08-28T22:29:43Z

src/wallet/init.h

@@ -22,4 +30,15 @@ bool WalletVerify();
 //! Load wallet databases.
 bool InitLoadWallet();

+//! Complete startup of wallets


Same as above.

promag · 2017-08-28T22:31:39Z

src/wallet/init.h

 //! Return the wallets help message.
 std::string GetWalletHelpString(bool showDebug);

 //! Wallets parameter interaction
 bool WalletParameterInteraction();

+//! Register wallet RPCs


Missing period?

Fix above too.

jnewbery · 2017-08-29T14:11:06Z

Re: naming of unload/close/free/detach wallet function. I want the wallet loading/unloading RPC verbs to be antonyms so it's obvious they're carrying out opposite actions. I started with load/unload, but dropped that to avoid any ambiguity with adding keys and addresses to an already open wallet. attach/detach is bad for the reason Russ mentioned. Is everyone happy with open/close?

(I guess the name of the function doesn't need to map exactly to the name of the RPC, but I think it makes sense to do that if we can)

ryanofsky · 2017-08-29T14:20:18Z

open/close or load/unload is what I would call it

jnewbery · 2017-08-29T16:21:39Z

I've addressed the comments and changed names a little. @ryanofsky do you mind taking a look?

ryanofsky

utACK ca94bf2c39b06f1ed643c24dfcd78ef5af00562a. Note that what you're calling flush, I'm calling stop in #10973, and what you're calling stop, I'm calling shutdown. But I'm not too worried about the names.

#10973 links: declarations, definitions

ryanofsky · 2017-08-29T17:41:47Z

Also just a suggestion, but this might be easier to review if it were just 2 commits, one commit for RPC stuff and one commit for the start/stop/flush stuff, so you could see the related parts together.

jnewbery · 2017-08-29T18:15:24Z

Thanks @ryanofsky .

Other reviewers/maintainers - I'm happy to rebase/squash as Russ suggests if that makes things easier to review. Please let me know if you want that.

meshcollider · 2017-08-30T21:05:29Z

utACK ca94bf2

TheBlueMatt

utACK ca94bf2c39b06f1ed643c24dfcd78ef5af00562a. I'm willing to bet this materially reduces the memory usage/compile time of init.cpp due to the wallet.h import removal.

TheBlueMatt · 2017-09-07T20:08:08Z

src/init.cpp

@@ -44,7 +44,6 @@
 #include "validationinterface.h"
 #ifdef ENABLE_WALLET
 #include "wallet/init.h"
-#include "wallet/wallet.h"


Holy fuck yes, kill the imports.

maflcko

utACK. Is mostly move-only

maflcko · 2017-09-07T20:18:19Z

src/init.cpp

@@ -1567,7 +1559,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)

    // ********************************************************* Step 8: load wallet
 #ifdef ENABLE_WALLET
-    if (!InitLoadWallet())
+    if (!OpenWallets())


Can you add the rationale for rename in the commit message? Why not keep it?

@jnewbery Or add it to the OP, so it can be seen in the merge commit.

maflcko · 2017-09-07T20:26:38Z

src/wallet/init.h

 //! Return the wallets help message.
 std::string GetWalletHelpString(bool showDebug);

 //! Wallets parameter interaction
 bool WalletParameterInteraction();

+//! Register wallet RPCs.
+void RegisterWalletRPC(CRPCTable &tableRPC);
+
 //! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
 //  This function will perform salvage on the wallet if requested, as long as only one wallet is
 //  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).


Comment should say "WalletParameterInteraction"

added as trivial commit

Rationale: - this init function can now open multiple wallets (hence Wallet->Wallets) - This is named as the antonym to CloseWallets(), which carries out the opposite action.

This function can now verify multiple wallets.

jnewbery · 2017-09-07T23:24:48Z

@MarcoFalke - nits addressed. Should be ready for merge if Travis agrees.

maflcko · 2017-09-07T23:29:02Z

re-utACK commit-by-commit 5d2a399

…terface 5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery) 43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery) 290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery) 062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery) 77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery) 2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery) 1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery) 9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery) Pull request description: Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762 All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet. There should be no changes in behavior from this PR. Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

…tion interface 5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery) 43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery) 290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery) 062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery) 77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery) 2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery) 1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery) 9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery) Pull request description: Apologies for the mostly code move only PR. This is a pre-req for both bitcoin#10740 and bitcoin#10762 All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet. There should be no changes in behavior from this PR. Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

fanquake added Wallet Refactoring labels Jul 7, 2017

This was referenced Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

[wallet] loadwallet RPC - load wallet at runtime #10740

Merged

promag reviewed Jul 7, 2017

View reviewed changes

jnewbery changed the title ~~Clarify wallet initialization / destruction interface~~ [wallet] Clarify wallet initialization / destruction interface Jul 11, 2017

jnewbery mentioned this pull request Aug 2, 2017

[MOVEONLY] Move some static functions out of wallet.h/cpp #10976

Merged

jnewbery force-pushed the walletinit2 branch from 3696ed0 to 3f425ee Compare August 28, 2017 18:09

ryanofsky reviewed Aug 28, 2017

View reviewed changes

promag reviewed Aug 28, 2017

View reviewed changes

jnewbery force-pushed the walletinit2 branch from 3f425ee to ca94bf2 Compare August 29, 2017 16:21

ryanofsky reviewed Aug 29, 2017

View reviewed changes

TheBlueMatt reviewed Sep 7, 2017

View reviewed changes

maflcko reviewed Sep 7, 2017

View reviewed changes

laanwj self-assigned this Sep 7, 2017

jnewbery added 4 commits September 7, 2017 16:19

[wallet] Rename InitLoadWallet() to OpenWallets()

9c76ba1

Rationale: - this init function can now open multiple wallets (hence Wallet->Wallets) - This is named as the antonym to CloseWallets(), which carries out the opposite action.

[wallet] Rename WalletVerify() to VerifyWallets()

1b9cee6

This function can now verify multiple wallets.

[wallet] Add FlushWallets() function to wallet/init.cpp

2da5eaf

[wallet] Add StopWallets() function to wallet/init.cpp

77fe07c

jnewbery added 4 commits September 7, 2017 16:21

[wallet] Add CloseWallets() function to wallet/init.cpp

062d631

[wallet] Add RegisterWalletRPC() function to wallet/init.cpp

290f3c5

[wallet] Add StartWallets() function to wallet/init.cpp

43b0e81

[trivial] fixup comment for VerifyWallets()

5d2a399

jnewbery force-pushed the walletinit2 branch from ca94bf2 to 5d2a399 Compare September 7, 2017 23:22

maflcko merged commit 5d2a399 into bitcoin:master Sep 7, 2017

jnewbery deleted the walletinit2 branch September 10, 2017 18:51

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[wallet] Clarify wallet initialization / destruction interface #10767

[wallet] Clarify wallet initialization / destruction interface #10767

Conversation

jnewbery commented Jul 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

practicalswift commented Jul 17, 2017

jnewbery commented Jul 17, 2017

practicalswift commented Jul 17, 2017

jnewbery commented Aug 14, 2017

jnewbery commented Aug 28, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Aug 29, 2017

ryanofsky commented Aug 29, 2017

jnewbery commented Aug 29, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 29, 2017

jnewbery commented Aug 29, 2017

meshcollider commented Aug 30, 2017

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Sep 7, 2017

maflcko commented Sep 7, 2017

jnewbery commented Jul 7, 2017 •

edited