[Wallet] refactor wallet/init interaction #7691

jonasschnelli · 2016-03-15T09:35:30Z

This is another step to move the wallet code from init.cpp to wallet.cpp while keeping the behavior identical. Behavior simplification can be done once all code is available in wallet.cpp.

This is also a basic step into the direction of cloning the wallet, add BIP32 & multi-wallet in the second wallet implementation without introducing bugs in the current wallet implementation.

maflcko · 2016-03-15T12:26:02Z

Concept ACK. Please remove the parameter interaction code from init; it seems you forgot to do this.

jonasschnelli · 2016-03-15T12:37:53Z

@MarcoFalke: thanks! Fixed.

paveljanik · 2016-03-15T19:42:29Z

New warning:

wallet/wallet.cpp:3222:1: warning: control may reach end of non-void function [-Wreturn-type]
}
^
1 warning generated.

jtimon · 2016-03-16T18:27:22Z

Could CWallet::ParameterInteraction() take a const std::map<std::string, std::string>& mapArgs as parameter instead of using the mapArgs global from util.h like in jtimon@3d03f15 ?

In fact, I believe I got the idea from one of your older versions of this (and then @luke-jr suggested some improvements to std::vector<std::pair<std::string, std::string> > CStandardPolicy::GetOptionsHelp() that would allow him to create generic forms in qt that automatically extend themselves when new options are added).

Concept ACK

jtimon · 2016-03-16T18:30:38Z

src/wallet/wallet.h

-    /* initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
-    static CWallet* InitLoadWallet(bool fDisableWallet, const std::string& strWalletFile, std::string& warningString, std::string& errorString);
+    /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
+    static bool InitLoadWallet();


Why static instead of calling it from the pwalletMain global (which could be moved to wallet)?
Or are we turning the global into a singleton?

Why static instead of calling it from the pwalletMain global (which could be moved to wallet)?
Or are we turning the global into a singleton?

Right. This is the direction where I'm heading to. But I work in very tiny steps (as you can see). For now it just moves "static code" to a static function in CWallet. Later it should be within the pwalletMain context.

maflcko · 2016-03-17T10:32:07Z

src/init.cpp

-    fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS);
-
-    std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
+    CWallet::ParameterInteraction();


You can't drop the return value here.

Right. Fixed.

maflcko · 2016-03-18T17:44:28Z

You forgot to remove AmountErrMsg from init as well. Maybe it would help to create two separate commits or even pulls: One addressing the move-only and the other the refactoring Warning/Error messages?

I really want to get this merged quick but I am concerned that reviewers are turned off by the diff containing both moves and refactors.

mrbandrews · 2016-03-21T15:32:44Z

src/wallet/wallet.cpp

+    uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING);
+    return true;
+}
+


I think this should be void, as the return value is not used as far as I can tell.

It is mirroring bool static InitWarning(), so probably fine. Still, the code duplication could be avoided somehow.

I have thought about that and if we want to decouple the wallet, we probably need to accept short term duplications of some code. In that case, the factored out shared code is uiInterface.ThreadSafeMessageBox. A duplicated entry point only used by CWallet should be okay.

But I agree with @mrbandrews: the return code should be switched to void (for UIWarning, keep it for UIError for now).
Will fix that.

mrbandrews · 2016-03-21T15:37:53Z

AmountErrMsg still used once in init.cpp at line 937 of the revised file. So it's defined as a static function in both wallet.cpp and init.cpp, which seems to work fine, although I don't know whether it's the best way to do it (maybe it is)

mrbandrews · 2016-03-21T17:29:19Z

src/wallet/wallet.cpp

+    fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS);
+
+    std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
+


I think this line can be removed... the wallet file name is read at the beginning of InitLoadWallet.

mrbandrews · 2016-03-21T17:44:11Z

Other than a couple of nits, this looks good to me. Code review ACK, concept ACK, and I tested it with an unpruned node, pruned node with wallet, and a pruned node with an outdated wallet - meaning it should suggest a reindex (qt) or shut down (bitcoind) - with no problems. So it's an ACK from me.

jonasschnelli · 2016-03-22T07:48:38Z

Fixed nits.
Should be ready for merge.

morcos · 2016-03-22T13:58:19Z

@jonasschnelli I don't necessarily object to this but it seems like if you're eventually going to change these from static functions to methods called on pWalletMain, that you might as well make all those changes now instead of having to review twice.
If you feel like its cleaner to break it up, perhaps it could be 2 commits in the same PR, but why not move closer to where you're trying to go.

jonasschnelli · 2016-03-23T09:47:48Z

Okay. Fair enough.
Added two commits.
First added commit will refactor the static methods to take them into the CWallet instance context. This looks much cleaner now.

Second commit is a cosmetic BDB version logprint change.

A migration from the global pwalletMain (called from everywhere) to a singleton is out-of-scope for now.

Thanks for the reviews!

jonasschnelli · 2016-03-24T20:00:25Z

Rejecting @morcos proposal to integrate the static-to-context change. Rolling back to original commit. This would get to big and not enough reviews. Further refactor can be done after this PR.

maflcko · 2016-03-24T20:01:50Z

Great, thanks for addressing the other nits.

utACK 25340b7

jtimon · 2016-03-29T12:51:41Z

src/wallet/wallet.cpp

+    return true;
+}
+
+bool CWallet::ParameterInteraction()


Why not pass mapArgs as parameter here?

The goal was it to keep it at a tiny scope (almost moveonly). I would strongly advise to keep this PR as it is and refactor later.

Appears out of scope for this pull.

Never mind, it won't make much sense until you can explicitly pass it to GetArg(), like in 3d03f15#diff-b7702a084eb00fb47f9800fd68271951R315

jtimon · 2016-03-29T12:55:53Z

utACK, all my nits can be solved later.

laanwj · 2016-04-02T06:22:16Z

utACK 25340b7

A migration from the global pwalletMain (called from everywhere) to a singleton is out-of-scope for now.

Seems like a useless intermediate step to me.
Singletons are just nicely dressed-up globals.
But we'd like to get rid of a single, global wallet, in any form. Just pass the thing around when you need it.

25340b7 [Wallet] refactor wallet/init interaction (Jonas Schnelli)

Bitcoin wallet PRs 2 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7576 - bitcoin/bitcoin#7577 - bitcoin/bitcoin#7608 - bitcoin/bitcoin#7691 - bitcoin/bitcoin#7905

7e71759 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (random-zebra) e585dad [Wallet] refactor wallet/init interaction (random-zebra) 49646b2 [Refactor] Nuke zwalletMain global object (random-zebra) f5f9df9 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr) 74445e4 [wallet] Move hardcoded file name out of log messages (random-zebra) 3f1838d [Wallet] optimize return value of InitLoadWallet() (random-zebra) 539dea4 [Wallet] move "load wallet phase" to CWallet (random-zebra) 7644318 [Wallet] move wallet help string creation to CWallet (random-zebra) a9d69b8 [trivial] Reuse translation and cleanup DEFAULT_* values (random-zebra) cfd007a [Bug] Omit wallet-related options from -help when wallet not supported (random-zebra) da642e6 [Refactor] More constant default values cleanup (random-zebra) a45275a [Refactor] Implement CBaseChainParams::BaseParams(Network) (random-zebra) 09abb98 Constrain constant values to a single location in code (random-zebra) Pull request description: Adapts the following refactoring PRs: - bitcoin#6961 Constrain constant values to a single location in code - bitcoin#7576 [Wallet] move wallet help string creation to CWallet - bitcoin#7577 move "load wallet phase" to CWallet - bitcoin#7608 Move hardcoded file name out of log messages - bitcoin#7691 refactor wallet/init interaction - bitcoin#8776 Wallet refactoring leading up to multiwallet ACKs for top commit: furszy: re ACK 7e71759 Fuzzbawls: ACK 7e71759 Tree-SHA512: 5fad328b9ddf8187af97d3d5fb285d0b67e73d51ac1bc44a3d57d0af86bce8b34efaab539b7bdbc4f9a2fa575a936f83788cffcc9c6d6d04cd3e63b19d399400

jonasschnelli added Brainstorming Block storage Refactoring Wallet and removed Block storage Brainstorming labels Mar 15, 2016

jonasschnelli force-pushed the 2016/03/wallet_mod branch from e7b65e7 to c23db88 Compare March 15, 2016 10:01

jonasschnelli force-pushed the 2016/03/wallet_mod branch from c23db88 to 0a624e0 Compare March 15, 2016 12:35

jtimon reviewed Mar 16, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/03/wallet_mod branch from 0a624e0 to 93df308 Compare March 17, 2016 07:59

maflcko reviewed Mar 17, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/03/wallet_mod branch from 93df308 to b87c44b Compare March 17, 2016 12:10

mrbandrews reviewed Mar 21, 2016
View reviewed changes

[Wallet] refactor wallet/init interaction

25340b7

jonasschnelli force-pushed the 2016/03/wallet_mod branch from b87c44b to 25340b7 Compare March 22, 2016 07:21

jonasschnelli force-pushed the 2016/03/wallet_mod branch from 1005c47 to 094b8a4 Compare March 23, 2016 09:45

jonasschnelli force-pushed the 2016/03/wallet_mod branch from 094b8a4 to 25340b7 Compare March 24, 2016 20:01

maflcko mentioned this pull request Mar 28, 2016

Encapsulate options for mempool policy #7557

Closed

jtimon reviewed Mar 29, 2016
View reviewed changes

laanwj merged commit 25340b7 into bitcoin:master Apr 2, 2016

laanwj added a commit that referenced this pull request Apr 2, 2016

Merge #7691: [Wallet] refactor wallet/init interaction

30c2dd8

25340b7 [Wallet] refactor wallet/init interaction (Jonas Schnelli)

vlamer added a commit to vlamer/bitcoin that referenced this pull request Apr 2, 2016

Merge bitcoin#7691: [Wallet] refactor wallet/init interaction

4451ecb

jonasschnelli mentioned this pull request Apr 15, 2016

refactor and optimize init.cpp/CWallet interaction #5990

Closed

laanwj mentioned this pull request Apr 28, 2016

Remaining instances of ENABLE_WALLET in libbitcoin_server.a #7965

Closed

14 tasks

codablock pushed a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge bitcoin#7691: [Wallet] refactor wallet/init interaction

298224b

25340b7 [Wallet] refactor wallet/init interaction (Jonas Schnelli)

Empact mentioned this pull request Apr 30, 2019

utils and libraries: Replace deprecated Boost Filesystem functions #15880

Merged

str4d mentioned this pull request Dec 18, 2019

Bitcoin wallet PRs 2 zcash/zcash#4270

Merged

random-zebra mentioned this pull request Aug 17, 2020

[Wallet][Refactoring] wallet/init refactoring backports PIVX-Project/PIVX#1811

Merged

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

		fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS);

		std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);

[Wallet] refactor wallet/init interaction #7691

[Wallet] refactor wallet/init interaction #7691

Uh oh!

Conversation

jonasschnelli commented Mar 15, 2016

Uh oh!

maflcko commented Mar 15, 2016

Uh oh!

jonasschnelli commented Mar 15, 2016

Uh oh!

paveljanik commented Mar 15, 2016

Uh oh!

jtimon commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrbandrews commented Mar 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrbandrews commented Mar 21, 2016

Uh oh!

jonasschnelli commented Mar 22, 2016

Uh oh!

morcos commented Mar 22, 2016

Uh oh!

jonasschnelli commented Mar 23, 2016

Uh oh!

jonasschnelli commented Mar 24, 2016

Uh oh!

maflcko commented Mar 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 29, 2016

Uh oh!

laanwj commented Apr 2, 2016

Uh oh!

Uh oh!