/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Wallet refactoring leading up to multiwallet #8776

Merged
merged 2 commits into from Jan 3, 2017
+65 −31

Conversation

Reviewers

@jonasschnelli jonasschnelli

@MarcoFalke MarcoFalke

Assignees
No one assigned
Labels
Wallet
Projects

Done in Multiwallet support

Milestone
  0.14.0
6 participants
@luke-jr @jonasschnelli @MarcoFalke @NicolasDorier @gmaxwell @sipa
@luke-jr
Member

luke-jr commented Sep 21, 2016

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

@MarcoFalke MarcoFalke added the Wallet label Sep 21, 2016

@jonasschnelli

jonasschnelli approved these changes Sep 22, 2016
View changes

Code Review ACK 682936c

src/wallet/wallet.cpp
+{
+ std::string walletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
+
+ CWallet * const pwallet = CreateWalletFromFile(walletFile);
@jonasschnelli

jonasschnelli Sep 22, 2016

Member

nit: const CWallet *pwallet =

@jonasschnelli
Member

jonasschnelli commented Sep 22, 2016

Needs rebase.
@luke-jr
Member

luke-jr commented Sep 22, 2016

Rebased

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

@luke-jr
Wallet: Let the interval-flushing thread figure out the filename 
Github-Pull: #8776
Rebased-From: 4a8788f
52f65d5

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

@luke-jr
Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile 
Github-Pull: #8776
Rebased-From: 24539dc
d2e6329
@MarcoFalke
Member

MarcoFalke commented Oct 22, 2016

utACK 24539dc
src/init.cpp
@@ -1527,7 +1527,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
#ifdef ENABLE_WALLET
if (pwalletMain) {
// Run a thread to flush wallet periodically
- threadGroup.create_thread(boost::bind(&ThreadFlushWalletDB, boost::ref(pwalletMain->strWalletFile)));
+ threadGroup.create_thread(ThreadFlushWalletDB);
@MarcoFalke

MarcoFalke Nov 2, 2016

Member

Needs merge conflict solved here

@MarcoFalke MarcoFalke added this to the 0.14.0 milestone Nov 7, 2016

@MarcoFalke
Member

MarcoFalke commented Nov 7, 2016

@luke-jr Mind to solve the merge conflict?

Also, assigned 0.14 milestone, I think this is relatively trivial/uncontroversial refactoring.
@luke-jr
Member

luke-jr commented Nov 11, 2016

Done
@MarcoFalke
Member

MarcoFalke commented Nov 11, 2016

utACK 5394b39
src/wallet/walletdb.cpp
@@ -810,6 +810,7 @@ void ThreadFlushWalletDB(const string& strFile)
if (nRefCount == 0)
{
boost::this_thread::interruption_point();
+ const std::string& strFile = pwalletMain->strWalletFile;
@jonasschnelli

jonasschnelli Nov 11, 2016

Member

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

luke-jr Nov 11, 2016

Member

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

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 */
@jonasschnelli

jonasschnelli Nov 11, 2016

Member

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

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 */
+ static CWallet* CreateWalletFromFile(const std::string walletFile);
@jonasschnelli

jonasschnelli Nov 11, 2016

Member

Maybe declare CreateWalletFromFile() private to avoid layer confusion?

@luke-jr

luke-jr Nov 11, 2016

Member

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

@MarcoFalke
Member

MarcoFalke commented Nov 17, 2016

@jonasschnelli Are your comments addressed by the feedback?
@jonasschnelli
Member

jonasschnelli commented Nov 21, 2016

@jonasschnelli Are your comments addressed by the feedback?

Yes.
re-utACK 5394b39

@jonasschnelli jonasschnelli referenced this pull request Nov 28, 2016

Merged

Make nWalletDBUpdated atomic to avoid a potential race. #9227

@NicolasDorier
Member

NicolasDorier commented Dec 28, 2016

Code Review ACK.
@gmaxwell
Member

gmaxwell commented Jan 3, 2017

utACK
@sipa
Owner

sipa commented Jan 3, 2017

utACK 5394b39

@sipa sipa merged commit 5394b39 into bitcoin:master Jan 3, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jan 3, 2017

@sipa
Merge #8776: Wallet refactoring leading up to multiwallet 
5394b39 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (Luke Dashjr)
fb0c934 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr)
This user has not uploaded their public key yet.
GPG key ID: DBA1A67379A1A931 Learn about signing commits
2a524b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment