refactor and optimize init.cpp/CWallet interaction

jonasschnelli commented at 8:20 pm on April 9, 2015: contributor

Reduced wallet/core coupling. This would also slowly lay groundwork for a possible new wallet candidate.

hides CWalletDB behind CWallet
reduces ENABLE_WALLET ifdefs

jonasschnelli force-pushed on Apr 9, 2015

in src/init.cpp: in 4e5c56ebad outdated

1191-    } // (!fDisableWallet)
1192+        pwalletMain = new CWallet();
1193+        
1194+        std::string warningString, errorString;
1195+        
1196+        if(!pwalletMain->LoadWallet(warningString, errorString))

Diapolo commented at 6:15 am on April 10, 2015:

Would a !pwalletMain help with a NULL pointer here?

jonasschnelli commented at 6:23 am on April 10, 2015:

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

in src/wallet/wallet.cpp: in 4e5c56ebad outdated

973+
974+void CWallet::LogGeneralInfos()
975+{
976+    LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
977+}
978+void CWallet::LogInfos() const

Diapolo commented at 6:18 am on April 10, 2015:

Nit: Missing new-line :).

in src/init.cpp: in 4e5c56ebad outdated

1200+            InitWarning(warningString);
1201+        if (!errorString.empty())
1202+            return InitError(errorString);
1203+    }
1204 #else // ENABLE_WALLET
1205     LogPrintf("No wallet compiled in!\n");

Diapolo commented at 6:19 am on April 10, 2015:

I always wanted this to change into No wallet support compiled in!, what do you think?

jonasschnelli commented at 6:26 am on April 10, 2015:

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?

in src/wallet/wallet.cpp: in 4e5c56ebad outdated

910@@ -908,6 +911,100 @@ CAmount CWallet::GetChange(const CTransaction& tx) const
911     return nChange;
912 }
913 
914+bool CWallet::IsDisabled()

Diapolo commented at 6:27 am on April 10, 2015:

I deleted my comment after discovering the declaration, sorry ;).

jonasschnelli renamed this:
~~[Wallet] refactor and optimize init.cpp/CWallet interaction~~
refactor and optimize init.cpp/CWallet interaction
on Apr 10, 2015

jgarzik commented at 3:36 pm on April 12, 2015: contributor

ditto previous PR - code movement in separate commit

concept ACK

jonasschnelli commented at 4:01 pm on April 12, 2015: contributor

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)

jgarzik commented at 4:07 pm on April 12, 2015: contributor

fair enough

jonasschnelli force-pushed on Apr 29, 2015

jonasschnelli commented at 12:37 pm on April 29, 2015: contributor

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.

jonasschnelli force-pushed on Apr 30, 2015

laanwj added the label Wallet on May 6, 2015

jonasschnelli force-pushed on May 8, 2015

jonasschnelli commented at 9:34 am on May 8, 2015: contributor

Rebased.

jonasschnelli force-pushed on May 8, 2015

jonasschnelli commented at 9:57 am on May 8, 2015: contributor

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.

jonasschnelli force-pushed on May 8, 2015

[Wallet] refactor and optimize init.cpp/CWallet interaction

Reduced wallet/core coupling.

- hides CWalletDB behind CWallet
- reduces ENABLE_WALLET ifdefs

9e80356ad8

[squashme] remove whitespace 294cf336fa

[squashme] remove overwriting/bypassing of member variable 95d89aa09e

decouple wallet from core, allow multiple wallets through signaling fb5b746fce

jonasschnelli force-pushed on Jun 15, 2015

jonasschnelli commented at 12:53 pm on June 15, 2015: contributor

Rebased

in src/wallet/legacywallet.cpp: in fb5b746fce

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"))

Diapolo commented at 1:07 pm on June 15, 2015:

Why are we using .count here and not e.g. GetArg()?

jonasschnelli commented at 1:08 pm on June 15, 2015:

This code has been moved 1:1 (better review capabilities).

in src/wallet/wallet.h: in fb5b746fce

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+

Diapolo commented at 1:08 pm on June 15, 2015:

Nit: Unneeded newline

in src/wallet/legacywallet.cpp: in fb5b746fce

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);

Diapolo commented at 1:09 pm on June 15, 2015:

Nit: Missing indentation

jonasschnelli commented at 1:12 pm on June 15, 2015:

Oops. Merge issue. This is even more than a “nit”. Will fix.

Diapolo commented at 9:16 pm on September 15, 2015:

Still needs fixing :).

jonasschnelli commented at 1:09 pm on June 15, 2015: contributor

Travis issue seems to be non-related to this PR (race condition)

in src/wallet/legacywallet.cpp: in fb5b746fce

19+
20+namespace CLegacyWalletModule {
21+
22+std::string GetWalletFile()
23+{
24+    return GetArg("-wallet", DEFAULT_WALLET_FILE);

Diapolo commented at 1:11 pm on June 15, 2015:

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.

jonasschnelli force-pushed on Jun 16, 2015

jonasschnelli commented at 6:38 pm on June 16, 2015: contributor

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.

jonasschnelli force-pushed on Jun 16, 2015

jonasschnelli force-pushed on Jun 19, 2015

jgarzik commented at 6:24 pm on September 15, 2015: contributor

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.

jonasschnelli commented at 6:33 pm on September 15, 2015: contributor

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.

in src/init.cpp: in fb5b746fce

874-            return InitError(warningString);
875-
876-    } // (!fDisableWallet)
877-#endif // ENABLE_WALLET
878+    // ********************************************************* Step 5: verify integrity of connected devices
879+    warningString.clear();

Diapolo commented at 9:11 pm on September 15, 2015:

NIT: Add a newline before this and before the next step?

in src/init.cpp: in fb5b746fce

882+    GetMainSignals().VerifyIntegrity(warningString, errorString, stopInit);
883+    if (stopInit)
884+        return false;
885+    
886+    if (!warningString.empty())
887+    InitWarning(warningString);

Diapolo commented at 9:11 pm on September 15, 2015:

Nit: Missing indentation

in src/init.cpp: in fb5b746fce

884+        return false;
885+    
886+    if (!warningString.empty())
887+    InitWarning(warningString);
888+    if (!errorString.empty())
889+    return InitError(errorString);

Diapolo commented at 9:11 pm on September 15, 2015:

Nit: Missing indentation

in src/init.cpp: in fb5b746fce

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

Diapolo commented at 9:12 pm on September 15, 2015:

Nit: Same as with step 5.

in src/qt/bitcoin.cpp: in fb5b746fce

30@@ -31,6 +31,7 @@
31 #include "util.h"
32 
33 #ifdef ENABLE_WALLET
34+#include "wallet/legacywallet.h"
35 #include "wallet/wallet.h"

Diapolo commented at 9:13 pm on September 15, 2015:

legacywallet.h includes wallet.h

in src/qt/signverifymessagedialog.cpp: in fb5b746fce

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"

Diapolo commented at 9:13 pm on September 15, 2015:

legacywallet.h includes wallet.h… true for all other places below

in src/wallet/legacywallet.cpp: in fb5b746fce

0@@ -0,0 +1,233 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin Core developers

Diapolo commented at 9:15 pm on September 15, 2015:

Nit: 2015 ;)

in src/wallet/legacywallet.cpp: in fb5b746fce

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))

Diapolo commented at 9:17 pm on September 15, 2015:

Could be a one-line check?

in src/wallet/legacywallet.h: in fb5b746fce

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

Diapolo commented at 9:18 pm on September 15, 2015:

Needs to be BITCOIN_WALLET_LEGACYWALLET_H, also for header end comment.

in src/wallet/legacywallet.h: in fb5b746fce

12+
13+namespace CLegacyWalletModule {
14+    void RegisterSignals();
15+    void UnregisterSignals();
16+    
17+    std::string GetWalletFile();

Diapolo commented at 9:19 pm on September 15, 2015:

Could return as const.

dcousens commented at 7:00 am on September 16, 2015: contributor

concept ACK, will review some point soon

dcousens commented at 10:16 pm on October 21, 2015: contributor

light utACK, will review again after rebase @jonasschnelli

sipa commented at 9:28 pm on November 28, 2015: member

Needs rebase.

laanwj commented at 2:19 pm on April 15, 2016: member

Is this pull still relevant with recent wallet/init.cpp changes? If so needs rebase, otherwise let’s close

jonasschnelli commented at 4:34 pm on April 15, 2016: contributor

Right. Closing. #7691 (merged) did contain most of this changes (a slightly different concept though).

jonasschnelli closed this on Apr 15, 2016

MarcoFalke locked this on Sep 8, 2021

refactor and optimize init.cpp/CWallet interaction #5990