refactor and optimize init.cpp/CWallet interaction #5990

pull jonasschnelli wants to merge 4 commits into bitcoin:master from jonasschnelli:2015/04/wallet_init_decoup changing 19 files +532 −319
  1. 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
  2. jonasschnelli force-pushed on Apr 9, 2015
  3. in src/init.cpp:None 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

  4. in src/wallet/wallet.cpp:None 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 :).

  5. in src/init.cpp:None 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?

  6. in src/wallet/wallet.cpp:None 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 ;).

  7. jonasschnelli renamed this:
    [Wallet] refactor and optimize init.cpp/CWallet interaction
    refactor and optimize init.cpp/CWallet interaction
    on Apr 10, 2015
  8. jgarzik commented at 3:36 PM on April 12, 2015: contributor

    ditto previous PR - code movement in separate commit

    concept ACK

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

  10. jgarzik commented at 4:07 PM on April 12, 2015: contributor

    fair enough

  11. jonasschnelli force-pushed on Apr 29, 2015
  12. 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.

  13. jonasschnelli force-pushed on Apr 30, 2015
  14. laanwj added the label Wallet on May 6, 2015
  15. jonasschnelli force-pushed on May 8, 2015
  16. jonasschnelli commented at 9:34 AM on May 8, 2015: contributor

    Rebased.

  17. jonasschnelli force-pushed on May 8, 2015
  18. 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.

  19. jonasschnelli force-pushed on May 8, 2015
  20. [Wallet] refactor and optimize init.cpp/CWallet interaction
    Reduced wallet/core coupling.
    
    - hides CWalletDB behind CWallet
    - reduces ENABLE_WALLET ifdefs
    9e80356ad8
  21. [squashme] remove whitespace 294cf336fa
  22. [squashme] remove overwriting/bypassing of member variable 95d89aa09e
  23. decouple wallet from core, allow multiple wallets through signaling fb5b746fce
  24. jonasschnelli force-pushed on Jun 15, 2015
  25. jonasschnelli commented at 12:53 PM on June 15, 2015: contributor

    Rebased

  26. in src/wallet/legacywallet.cpp:None 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).

  27. in src/wallet/wallet.h:None 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

  28. in src/wallet/legacywallet.cpp:None 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 :).

  29. jonasschnelli commented at 1:09 PM on June 15, 2015: contributor

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

  30. in src/wallet/legacywallet.cpp:None 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.

  31. jonasschnelli force-pushed on Jun 16, 2015
  32. jonasschnelli force-pushed on Jun 16, 2015
  33. 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.

  34. jonasschnelli force-pushed on Jun 16, 2015
  35. jonasschnelli force-pushed on Jun 16, 2015
  36. jonasschnelli force-pushed on Jun 16, 2015
  37. jonasschnelli force-pushed on Jun 16, 2015
  38. jonasschnelli force-pushed on Jun 19, 2015
  39. 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.

  40. 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.

  41. in src/init.cpp:None 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?

  42. in src/init.cpp:None 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

  43. in src/init.cpp:None 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

  44. in src/init.cpp:None 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.

  45. in src/qt/bitcoin.cpp:None 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

  46. in src/qt/signverifymessagedialog.cpp:None 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

  47. in src/wallet/legacywallet.cpp:None 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 ;)

  48. in src/wallet/legacywallet.cpp:None 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?

  49. in src/wallet/legacywallet.h:None 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.

  50. in src/wallet/legacywallet.h:None 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.

  51. dcousens commented at 7:00 AM on September 16, 2015: contributor

    concept ACK, will review some point soon

  52. dcousens commented at 10:16 PM on October 21, 2015: contributor

    light utACK, will review again after rebase @jonasschnelli

  53. sipa commented at 9:28 PM on November 28, 2015: member

    Needs rebase.

  54. 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

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

  56. jonasschnelli closed this on Apr 15, 2016

  57. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-30 09:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me