Wallet refactoring leading up to multiwallet #8776

1. 
   luke-jr commented at 10:01 AM on September 21, 2016: member

   Part of the refactorings needed for basic multiwallet (#8694)

2. MarcoFalke added the label Wallet on Sep 21, 2016
3. in src/wallet/wallet.cpp:None in 682936cd8f outdated

   3477 | +
   3478 | +bool CWallet::InitLoadWallet()
   3479 | +{
   3480 | +    std::string walletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
   3481 | +
   3482 | +    CWallet * const pwallet = CreateWalletFromFile(walletFile);
   

   ---

   ---

   

   jonasschnelli commented at 6:53 AM on September 22, 2016:

   nit: const CWallet *pwallet =

4. jonasschnelli approved
5. 
   jonasschnelli commented at 6:56 AM on September 22, 2016: contributor

   Code Review ACK https://github.com/bitcoin/bitcoin/pull/8776/commits/682936cd8f2184917aea70174fb8ac5e9cca85f6

6. 
   jonasschnelli commented at 6:56 AM on September 22, 2016: contributor

   Needs rebase.

7. 
   luke-jr commented at 8:25 AM on September 22, 2016: member

   Rebased

8. luke-jr force-pushed on Sep 22, 2016
9. luke-jr referenced this in commit 52f65d5971 on Oct 20, 2016
10. luke-jr referenced this in commit d2e63293ef on Oct 20, 2016
11. 
    MarcoFalke commented at 5:45 PM on October 22, 2016: member

    utACK 24539dc21fe5969c619f20

12. in src/init.cpp:None in 24539dc21f outdated

    1526 | @@ -1527,7 +1527,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
    1527 |  #ifdef ENABLE_WALLET
    1528 |      if (pwalletMain) {
    1529 |          // Run a thread to flush wallet periodically
    1530 | -        threadGroup.create_thread(boost::bind(&ThreadFlushWalletDB, boost::ref(pwalletMain->strWalletFile)));
    1531 | +        threadGroup.create_thread(ThreadFlushWalletDB);
    

    ---

    ---

    

    MarcoFalke commented at 9:35 PM on November 2, 2016:

    Needs merge conflict solved here

13. MarcoFalke added this to the milestone 0.14.0 on Nov 7, 2016
14. 
    MarcoFalke commented at 9:49 AM on November 7, 2016: member

    @luke-jr Mind to solve the merge conflict?

    Also, assigned 0.14 milestone, I think this is relatively trivial/uncontroversial refactoring.

15. Wallet: Let the interval-flushing thread figure out the filename fb0c934d1b
16. Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile 5394b3940d
17. luke-jr force-pushed on Nov 11, 2016
18. 
    luke-jr commented at 11:46 AM on November 11, 2016: member

    Done

19. 
    MarcoFalke commented at 12:19 PM on November 11, 2016: member

    utACK 5394b39

20. in src/wallet/walletdb.cpp:None in 5394b3940d

     809 | @@ -810,6 +810,7 @@ void ThreadFlushWalletDB(const string& strFile)
     810 |                  if (nRefCount == 0)
     811 |                  {
     812 |                      boost::this_thread::interruption_point();
     813 | +                    const std::string& strFile = pwalletMain->strWalletFile;
    

    ---

    ---

    

    jonasschnelli commented at 12:33 PM on November 11, 2016:

    In theory, this should only be done when holding cs_wallet. I guess changing CWallet::strWalletFile to a const std::string is not possible with the current concept of setting the filename in a instance method. But maybe I'm wrong.

    ---

    

    luke-jr commented at 1:12 PM on November 11, 2016:

    wallet.h says "This lock protects all the fields added by CWallet except for: ... strWalletFile (immutable after instantiation)"

21. in src/wallet/wallet.h:None in 5394b3940d

     916 | @@ -914,6 +917,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     917 |      static std::string GetWalletHelpString(bool showDebug);
     918 |  
     919 |      /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
    

    ---

    ---

    

    jonasschnelli commented at 12:36 PM on November 11, 2016:

    Heh. This (unchanged) comment was wrong before and correct after this PR. :)

22. in src/wallet/wallet.h:None in 5394b3940d

     916 | @@ -914,6 +917,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     917 |      static std::string GetWalletHelpString(bool showDebug);
     918 |  
     919 |      /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
     920 | +    static CWallet* CreateWalletFromFile(const std::string walletFile);
    

    ---

    ---

    

    jonasschnelli commented at 12:36 PM on November 11, 2016:

    Maybe declare CreateWalletFromFile() private to avoid layer confusion?

    ---

    

    luke-jr commented at 1:13 PM on November 11, 2016:

    It's not really intended to be private...

23. 
    MarcoFalke commented at 11:37 PM on November 17, 2016: member

    @jonasschnelli Are your comments addressed by the feedback?

24. 
    jonasschnelli commented at 2:33 PM on November 21, 2016: contributor

    @jonasschnelli Are your comments addressed by the feedback?

    Yes. re-utACK 5394b3940dec1fd35952d344e6373fb7115c5490

25. 
    NicolasDorier commented at 9:44 AM on December 28, 2016: contributor

    Code Review ACK.

26. 
    gmaxwell commented at 5:47 PM on January 3, 2017: contributor

    utACK

27. 
    sipa commented at 10:02 PM on January 3, 2017: member

    utACK 5394b3940dec1fd35952d344e6373fb7115c5490

28. sipa merged this on Jan 3, 2017
29. sipa closed this on Jan 3, 2017

    ---

30. sipa referenced this in commit 2a524b8e8f on Jan 3, 2017
31. codablock referenced this in commit da209546df on Jan 18, 2018
32. andvgal referenced this in commit 22790ae3e0 on Jan 6, 2019
33. CryptoCentric referenced this in commit 1103008ee9 on Feb 26, 2019
34. random-zebra referenced this in commit e89e20eca1 on Aug 25, 2020
35. MarcoFalke locked this on Sep 8, 2021

Contributors

luke-jrjonasschnelliMarcoFalkeNicolasDoriergmaxwellsipa

Milestone
0.14.0