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.
Concept ACK. Please remove the parameter interaction code from init; it seems you forgot to do this.
@MarcoFalke: thanks! Fixed.
New warning:
wallet/wallet.cpp:3222:1: warning: control may reach end of non-void function [-Wreturn-type]
}
^
1 warning generated.
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 https://github.com/jtimon/bitcoin/commit/3d03f15126b79bd9f22ccd27f1d8ca43d9e16bf5 ?
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
876 | @@ -875,8 +877,11 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface 877 | /* Returns the wallets help message */ 878 | static std::string GetWalletHelpString(bool showDebug); 879 | 880 | - /* initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ 881 | - static CWallet* InitLoadWallet(bool fDisableWallet, const std::string& strWalletFile, std::string& warningString, std::string& errorString); 882 | + /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ 883 | + 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.
991 | - nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); 992 | - bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); 993 | - fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS); 994 | - 995 | - std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT); 996 | + CWallet::ParameterInteraction();
You can't drop the return value here.
Right. Fixed.
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.
375 | +bool static UIWarning(const std::string &str) 376 | +{ 377 | + uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING); 378 | + return true; 379 | +} 380 | +
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.
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)
3218 | + nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET); 3219 | + bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE); 3220 | + fSendFreeTransactions = GetBoolArg("-sendfreetransactions", DEFAULT_SEND_FREE_TRANSACTIONS); 3221 | + 3222 | + std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT); 3223 | +
I think this line can be removed... the wallet file name is read at the beginning of InitLoadWallet.
3016 | @@ -2992,8 +3017,10 @@ std::string CWallet::GetWalletHelpString(bool showDebug) 3017 | return strUsage; 3018 | } 3019 | 3020 | -CWallet* CWallet::InitLoadWallet(bool fDisableWallet, const std::string& strWalletFile, std::string& warningString, std::string& errorString) 3021 | +bool CWallet::InitLoadWallet() 3022 | { 3023 | + std::string strWalletFile = GetArg("-wallet", DEFAULT_WALLET_DAT); 3024 | +
Nit: this local variable has the same name as the class member variable (I guess it is not a problem here since the compiler knows it can't access the class member from a static method?) Not a big deal but maybe want to give it a different name.
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.
Fixed nits. Should be ready for merge.
@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.
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!
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.
Great, thanks for addressing the other nits.
utACK 25340b7
3161 | @@ -3150,7 +3162,62 @@ CWallet* CWallet::InitLoadWallet(bool fDisableWallet, const std::string& strWall 3162 | } 3163 | walletInstance->SetBroadcastTransactions(GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); 3164 | 3165 | - return walletInstance; 3166 | + pwalletMain = walletInstance; 3167 | + return true; 3168 | +} 3169 | + 3170 | +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 https://github.com/bitcoin/bitcoin/pull/6423/commits/3d03f15126b79bd9f22ccd27f1d8ca43d9e16bf5#diff-b7702a084eb00fb47f9800fd68271951R315
utACK, all my nits can be solved later.
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.