Wallet refactoring leading up to multiwallet by luke-jr · Pull Request #8776 · bitcoin/bitcoin

luke-jr · 2016-09-21T10:01:05Z

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

jonasschnelli

Code Review ACK 682936c

jonasschnelli · 2016-09-22T06:53:31Z

src/wallet/wallet.cpp

nit: const CWallet *pwallet =

jonasschnelli · 2016-09-22T06:56:46Z

Needs rebase.

luke-jr · 2016-09-22T08:25:35Z

Rebased

Github-Pull: bitcoin#8776 Rebased-From: 4a8788f

Github-Pull: bitcoin#8776 Rebased-From: 24539dc

maflcko · 2016-10-22T17:45:31Z

utACK 24539dc

maflcko · 2016-11-02T21:35:31Z

src/init.cpp

    if (pwalletMain) {
        // Run a thread to flush wallet periodically
-        threadGroup.create_thread(boost::bind(&ThreadFlushWalletDB, boost::ref(pwalletMain->strWalletFile)));
+        threadGroup.create_thread(ThreadFlushWalletDB);


Needs merge conflict solved here

maflcko · 2016-11-07T09:49:00Z

@luke-jr Mind to solve the merge conflict?

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

luke-jr · 2016-11-11T11:46:36Z

Done

maflcko · 2016-11-11T12:19:18Z

utACK 5394b39

jonasschnelli · 2016-11-11T12:33:36Z

src/wallet/walletdb.cpp

                if (nRefCount == 0)
                {
                    boost::this_thread::interruption_point();
+                    const std::string& strFile = pwalletMain->strWalletFile;


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.

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

jonasschnelli · 2016-11-11T12:36:01Z

src/wallet/wallet.h

@@ -914,6 +917,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
    static std::string GetWalletHelpString(bool showDebug);

    /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */


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

jonasschnelli · 2016-11-11T12:36:24Z

src/wallet/wallet.h

    static std::string GetWalletHelpString(bool showDebug);

    /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
+    static CWallet* CreateWalletFromFile(const std::string walletFile);


Maybe declare CreateWalletFromFile() private to avoid layer confusion?

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

maflcko · 2016-11-17T23:37:12Z

@jonasschnelli Are your comments addressed by the feedback?

jonasschnelli · 2016-11-21T14:33:23Z

@jonasschnelli Are your comments addressed by the feedback?

Yes.
re-utACK 5394b39

NicolasDorier · 2016-12-28T09:44:59Z

Code Review ACK.

gmaxwell · 2017-01-03T17:47:52Z

utACK

sipa · 2017-01-03T22:02:58Z

utACK 5394b39

5394b39 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (Luke Dashjr) fb0c934 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr)

7e71759 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (random-zebra) e585dad [Wallet] refactor wallet/init interaction (random-zebra) 49646b2 [Refactor] Nuke zwalletMain global object (random-zebra) f5f9df9 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr) 74445e4 [wallet] Move hardcoded file name out of log messages (random-zebra) 3f1838d [Wallet] optimize return value of InitLoadWallet() (random-zebra) 539dea4 [Wallet] move "load wallet phase" to CWallet (random-zebra) 7644318 [Wallet] move wallet help string creation to CWallet (random-zebra) a9d69b8 [trivial] Reuse translation and cleanup DEFAULT_* values (random-zebra) cfd007a [Bug] Omit wallet-related options from -help when wallet not supported (random-zebra) da642e6 [Refactor] More constant default values cleanup (random-zebra) a45275a [Refactor] Implement CBaseChainParams::BaseParams(Network) (random-zebra) 09abb98 Constrain constant values to a single location in code (random-zebra) Pull request description: Adapts the following refactoring PRs: - bitcoin#6961 Constrain constant values to a single location in code - bitcoin#7576 [Wallet] move wallet help string creation to CWallet - bitcoin#7577 move "load wallet phase" to CWallet - bitcoin#7608 Move hardcoded file name out of log messages - bitcoin#7691 refactor wallet/init interaction - bitcoin#8776 Wallet refactoring leading up to multiwallet ACKs for top commit: furszy: re ACK 7e71759 Fuzzbawls: ACK 7e71759 Tree-SHA512: 5fad328b9ddf8187af97d3d5fb285d0b67e73d51ac1bc44a3d57d0af86bce8b34efaab539b7bdbc4f9a2fa575a936f83788cffcc9c6d6d04cd3e63b19d399400

maflcko added the Wallet label Sep 21, 2016

jonasschnelli approved these changes Sep 22, 2016

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

jonasschnelli Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const CWallet *pwallet =

luke-jr force-pushed the multiwallet_prefactor_wallet branch from 682936c to 24539dc Compare September 22, 2016 08:25

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Wallet: Let the interval-flushing thread figure out the filename

52f65d5

Github-Pull: bitcoin#8776 Rebased-From: 4a8788f

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile

d2e6329

Github-Pull: bitcoin#8776 Rebased-From: 24539dc

maflcko reviewed Nov 2, 2016

View reviewed changes

maflcko added this to the 0.14.0 milestone Nov 7, 2016

luke-jr added 2 commits November 11, 2016 11:35

Wallet: Let the interval-flushing thread figure out the filename

fb0c934

Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile

5394b39

luke-jr force-pushed the multiwallet_prefactor_wallet branch from 24539dc to 5394b39 Compare November 11, 2016 11:36

jonasschnelli reviewed Nov 11, 2016

View reviewed changes

jonasschnelli mentioned this pull request Nov 28, 2016

Make nWalletDBUpdated atomic to avoid a potential race. #9227

Merged

sipa merged commit 5394b39 into bitcoin:master Jan 3, 2017

PastaPastaPasta mentioned this pull request Nov 13, 2018

[WIP] Backport multiwallet functionality dashpay/dash#2439

Closed

random-zebra mentioned this pull request Aug 17, 2020

[Wallet][Refactoring] wallet/init refactoring backports PIVX-Project/PIVX#1811

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -914,6 +917,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
		static std::string GetWalletHelpString(bool showDebug);

		/* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */

Conversation

luke-jr commented Sep 21, 2016

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

luke-jr commented Sep 22, 2016

Uh oh!

maflcko commented Oct 22, 2016

Uh oh!

maflcko Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 7, 2016

Uh oh!

luke-jr commented Nov 11, 2016

Uh oh!

maflcko commented Nov 11, 2016

Uh oh!

jonasschnelli Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 17, 2016

Uh oh!

jonasschnelli commented Nov 21, 2016

Uh oh!

NicolasDorier commented Dec 28, 2016

Uh oh!

gmaxwell commented Jan 3, 2017

Uh oh!

sipa commented Jan 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants