Reduced wallet/core coupling. This would also slowly lay groundwork for a possible new wallet candidate.
- hides CWalletDB behind CWallet
- reduces ENABLE_WALLET ifdefs
Reduced wallet/core coupling. This would also slowly lay groundwork for a possible new wallet candidate.
1191 | - } // (!fDisableWallet) 1192 | + pwalletMain = new CWallet(); 1193 | + 1194 | + std::string warningString, errorString; 1195 | + 1196 | + if(!pwalletMain->LoadWallet(warningString, errorString))
Would a !pwalletMain help with a NULL pointer here?
It's within a if (CWallet::IsDisabled()) else control structure. pwalletMain can't be NULL there. But i assume it won't hurt adding a !pwalletMain
973 | + 974 | +void CWallet::LogGeneralInfos() 975 | +{ 976 | + LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0)); 977 | +} 978 | +void CWallet::LogInfos() const
Nit: Missing new-line :).
1200 | + InitWarning(warningString); 1201 | + if (!errorString.empty()) 1202 | + return InitError(errorString); 1203 | + } 1204 | #else // ENABLE_WALLET 1205 | LogPrintf("No wallet compiled in!\n");
I always wanted this to change into No wallet support compiled in!, what do you think?
I'd like to keep the log output in the PR as exact as possible to allow log file comparison between this PR and the master. But your string is def. better. Why not open a PR @ trivial?
910 | @@ -908,6 +911,100 @@ CAmount CWallet::GetChange(const CTransaction& tx) const 911 | return nChange; 912 | } 913 | 914 | +bool CWallet::IsDisabled()
I deleted my comment after discovering the declaration, sorry ;).
ditto previous PR - code movement in separate commit
concept ACK
Here the moved code did need some adaption. I was trying to make more smaller commits. This was not possible with the fact that every commit should also passes travis (and result in a proper compilable bitcoind/qt)
fair enough
Rebased.
I'm not sure if i should include another commit which would then completely decouple the wallet over signaling to allow multiple wallet devices: https://github.com/jonasschnelli/bitcoin/commit/15ccf479f190786d0fc97a0253bfdbd080074ff1#diff-e74648dfe36ec1841b222ca921ad61c0
With this commit it would be a useful groundwork for ZMQ and a wallet rewriting without breaking the current wallet.
Rebased.
Added another commit on top.
This would decouple the wallet from the core and would allow to have multiple wallets (validation devices) over signaling.
Together with #5994 it would basically remove (almost) all wallet #ifdefs. The goal is to just have one wallet #ifdef at the point where we register for signals.
IMO this is a massiv step forward of core/wallet separation.
Reduced wallet/core coupling.
- hides CWalletDB behind CWallet
- reduces ENABLE_WALLET ifdefs
Rebased
134 | + if (GetBoolArg("-zapwallettxes", false)) { 135 | + if (SoftSetBoolArg("-rescan", true)) 136 | + LogPrintf("%s: parameter interaction: -zapwallettxes=<mode> -> setting -rescan=1\n", __func__); 137 | + } 138 | + 139 | + if (mapArgs.count("-mintxfee"))
Why are we using .count here and not e.g. GetArg()?
This code has been moved 1:1 (better review capabilities).
708 | + 709 | //! Verify the wallet database and perform salvage if required 710 | static bool Verify(const std::string& walletFile, std::string& warningString, std::string& errorString); 711 | 712 | + static void StartWalletTasks(boost::thread_group& threadGroup); 713 | +
Nit: Unneeded newline
84 | +void SanityCheck(std::string& errorString) 85 | +{ 86 | + std::string strWalletFile = GetWalletFile(); 87 | + // Wallet file must be a plain filename without a directory 88 | + if (strWalletFile != boost::filesystem::basename(strWalletFile) + boost::filesystem::extension(strWalletFile)) 89 | + errorString += strprintf(_("Wallet %s resides outside data directory"), strWalletFile);
Nit: Missing indentation
Oops. Merge issue. This is even more than a "nit". Will fix.
Still needs fixing :).
Travis issue seems to be non-related to this PR (race condition)
19 | + 20 | +namespace CLegacyWalletModule { 21 | + 22 | +std::string GetWalletFile() 23 | +{ 24 | + return GetArg("-wallet", DEFAULT_WALLET_FILE);
Not that it would matter that much, but how about using a static variable, that would allow to use GetWalletFile() without the need to store it into another std::string when using the function, which happens quite a few times.
Rebased this PR. This PR is required to support two (or more) wallet implementations. This PR would allow to selective compile-in a potential 2nd wallet code.
ut ACK - this seems a reasonable refactor to merge soonish even considering my comments on bitcoin-dev about refactoring - @jonasschnelli seems like the main person working on improving CoreWallet right now, so downstream developer breakage seems unlikely.
This would be a major step in decoupling the wallet from the core (codebase and not process decoupling). I'm ready to continue, but would like to get some concept ACK because this PR is itself relatively "heavy" and requires rebases all the time because it moves code away from init.cpp where often changes happen.
874 | - return InitError(warningString); 875 | - 876 | - } // (!fDisableWallet) 877 | -#endif // ENABLE_WALLET 878 | + // ********************************************************* Step 5: verify integrity of connected devices 879 | + warningString.clear();
NIT: Add a newline before this and before the next step?
882 | + GetMainSignals().VerifyIntegrity(warningString, errorString, stopInit); 883 | + if (stopInit) 884 | + return false; 885 | + 886 | + if (!warningString.empty()) 887 | + InitWarning(warningString);
Nit: Missing indentation
884 | + return false; 885 | + 886 | + if (!warningString.empty()) 887 | + InitWarning(warningString); 888 | + if (!errorString.empty()) 889 | + return InitError(errorString);
Nit: Missing indentation
1287 | - pwalletMain->SetBroadcastTransactions(GetBoolArg("-walletbroadcast", true)); 1288 | - } // (!fDisableWallet) 1289 | -#else // ENABLE_WALLET 1290 | - LogPrintf("No wallet support compiled in!\n"); 1291 | -#endif // !ENABLE_WALLET 1292 | + // ********************************************************* Step 8: load wallets/modules
Nit: Same as with step 5.
30 | @@ -31,6 +31,7 @@ 31 | #include "util.h" 32 | 33 | #ifdef ENABLE_WALLET 34 | +#include "wallet/legacywallet.h" 35 | #include "wallet/wallet.h"
legacywallet.h includes wallet.h
12 | @@ -13,6 +13,7 @@ 13 | #include "base58.h" 14 | #include "init.h" 15 | #include "main.h" // For strMessageMagic 16 | +#include "wallet/legacywallet.h" 17 | #include "wallet/wallet.h"
legacywallet.h includes wallet.h... true for all other places below
0 | @@ -0,0 +1,233 @@ 1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto 2 | +// Copyright (c) 2009-2014 The Bitcoin Core developers
Nit: 2015 ;)
130 | + LogPrintf("%s: parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__); 131 | + } 132 | + 133 | + // -zapwallettx implies a rescan 134 | + if (GetBoolArg("-zapwallettxes", false)) { 135 | + if (SoftSetBoolArg("-rescan", true))
Could be a one-line check?
0 | @@ -0,0 +1,20 @@ 1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto 2 | +// Copyright (c) 2009-2015 The Bitcoin Core developers 3 | +// Distributed under the MIT software license, see the accompanying 4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 5 | + 6 | +#ifndef BITCOIN_LEGACYWALLET_H
Needs to be BITCOIN_WALLET_LEGACYWALLET_H, also for header end comment.
12 | + 13 | +namespace CLegacyWalletModule { 14 | + void RegisterSignals(); 15 | + void UnregisterSignals(); 16 | + 17 | + std::string GetWalletFile();
Could return as const.
concept ACK, will review some point soon
light utACK, will review again after rebase @jonasschnelli
Needs rebase.
Is this pull still relevant with recent wallet/init.cpp changes? If so needs rebase, otherwise let's close
Right. Closing. #7691 (merged) did contain most of this changes (a slightly different concept though).