New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic multiwallet support #8694

Merged
merged 13 commits into from Jun 12, 2017

Conversation

@luke-jr
Member

luke-jr commented Sep 9, 2016

This allows running with multiple -wallet options to load more than one wallet.

GUI has independent comboboxen for the main GUI window and debug console to select which wallet to view/use. The comboboxes are not visible in the main GUI unless multiple wallets are loaded (the debug window's combobox is useful even for a single wallet, since it allows selecting "(none)" to block wallet access).

RPC can access only one wallet per user, but rpcauth is extended to accept a 4th field which controls which wallet, if any, the user has access to. I chose to do it this way because pre-multiwallet nodes will gracefully ignore such rpcauth rather than expose their wallet. The field can also be a single hyphen to block wallet access.

(RPC and GUI changes are moved to my multiwallet_rpc and multiwallet_gui branches)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 10, 2016

Member

Impressive changeset!
General Concept ACK.

Instead of the extending the RPC auth, I could imaging using different RPC endpoints would also work.

  • Accessing RPC at / would result in the default wallet.
  • Accessing RPC at /<walletid> would result in using the wallet with the id <walletid>.

bitcoin-cli could just support a new argument -walletid.
Something like bitcoin-cli -walletid=something getbalance would result in the same RPC call just against the endpoint /something.
Maybe this would result in a simple RPC access to the multiple wallets.

Will review soon.

Member

jonasschnelli commented Sep 10, 2016

Impressive changeset!
General Concept ACK.

Instead of the extending the RPC auth, I could imaging using different RPC endpoints would also work.

  • Accessing RPC at / would result in the default wallet.
  • Accessing RPC at /<walletid> would result in using the wallet with the id <walletid>.

bitcoin-cli could just support a new argument -walletid.
Something like bitcoin-cli -walletid=something getbalance would result in the same RPC call just against the endpoint /something.
Maybe this would result in a simple RPC access to the multiple wallets.

Will review soon.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 10, 2016

Member

Instead of the extending the RPC auth, I could imaging using different RPC endpoints would also work.

Indeed, although my main purpose in doing this was to isolate JoinMarket, and it's less isolated if it has auth to my real hotwallet. :)

(Also, endpoints seemed like they'd require more code.)

Accessing RPC at /<walletid> would result in using the wallet with the id <walletid>.

This isn't very future-proof. If we go this route, I'd suggest /?wallet=<walletid>

Member

luke-jr commented Sep 10, 2016

Instead of the extending the RPC auth, I could imaging using different RPC endpoints would also work.

Indeed, although my main purpose in doing this was to isolate JoinMarket, and it's less isolated if it has auth to my real hotwallet. :)

(Also, endpoints seemed like they'd require more code.)

Accessing RPC at /<walletid> would result in using the wallet with the id <walletid>.

This isn't very future-proof. If we go this route, I'd suggest /?wallet=<walletid>

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Sep 14, 2016

Member

Nice, Concept ACK. Will test.

Member

btcdrak commented Sep 14, 2016

Nice, Concept ACK. Will test.

Show outdated Hide outdated src/init.cpp
@@ -196,8 +196,9 @@ void Shutdown()
StopRPC();
StopHTTPServer();
#ifdef ENABLE_WALLET
if (pwalletMain)
pwalletMain->Flush(false);
for (CWallet_ptr pwallet : vpwallets) {

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 21, 2016

Member

I like the approach by @jonasschnelli better. Let's keep most of the multiwallet logic in wallet.cpp.

@MarcoFalke

MarcoFalke Sep 21, 2016

Member

I like the approach by @jonasschnelli better. Let's keep most of the multiwallet logic in wallet.cpp.

This comment has been minimized.

@luke-jr

luke-jr Mar 3, 2017

Member

It's not in wallet.cpp now. Moving it there seems like a good idea, but independent of this PR...

@luke-jr

luke-jr Mar 3, 2017

Member

It's not in wallet.cpp now. Moving it there seems like a good idea, but independent of this PR...

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

Wallet: Replace pwalletMain with a vector of wallet pointers
Github-Pull: #8694
Rebased-From: 17480ed9dbd535b174342d02e4a3627ff9ddc193

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

Wallet: Support loading multiple wallets if -wallet used more than once
Github-Pull: #8694
Rebased-From: 03b8a1380e2c91794c5ed4aac0ec50aa85405959

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

Qt: Load all wallets into WalletModels
Github-Pull: #8694
Rebased-From: 4d44a06ee74240a1616b6561767901f3d5e4ad91

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

Qt: Add a combobox to toolbar to select from multiple wallets
Github-Pull: #8694
Rebased-From: 6f3f87fe0fcb10929a9a4f367e72767a7b49aa98

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

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

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 3, 2017

Member

Rebased, but please prioritise #8775 review (which this is based on) first. Once that's merged, it may or may not make sense to split the Qt changes out of this.

Member

luke-jr commented Jan 3, 2017

Rebased, but please prioritise #8775 review (which this is based on) first. Once that's merged, it may or may not make sense to split the Qt changes out of this.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 7, 2017

Member

Concept ACK.

Syntax for the auth field should have a clear way to extend it to support a list of wallets (first being the default, I guess). I think. To support things like the path selected wallets suggested above later.

Member

gmaxwell commented Jan 7, 2017

Concept ACK.

Syntax for the auth field should have a clear way to extend it to support a list of wallets (first being the default, I guess). I think. To support things like the path selected wallets suggested above later.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 3, 2017

Member

Moved RPC/GUI changes out of this, and rebased.

Member

luke-jr commented Feb 3, 2017

Moved RPC/GUI changes out of this, and rebased.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 4, 2017

Member

Rebased and now the next-step in multiwallet integration.

Member

luke-jr commented Mar 4, 2017

Rebased and now the next-step in multiwallet integration.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 6, 2017

Member

Re-Concept ACK.
Binaries to play with: https://bitcoin.jonasschnelli.ch/build/PR/8694

Member

jonasschnelli commented Mar 6, 2017

Re-Concept ACK.
Binaries to play with: https://bitcoin.jonasschnelli.ch/build/PR/8694

Show outdated Hide outdated src/wallet/wallet.h
@@ -31,7 +31,8 @@
#include <boost/shared_ptr.hpp>
#include <boost/thread.hpp>
extern CWallet* pwalletMain;
typedef CWallet* CWallet_ptr;

This comment has been minimized.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

Curious, why have a CWallet_ptr typedef instead of using CWallet *? Is there a plan to switch to a different type of pointer later? Curious, because this seems to make the rest of the code more opaque. For example:

for (CWallet_ptr pwallet : vpwallets)

Because this says CWallet_ptr instead of CWallet* it's unclear whether the copy initialization has extra cost (which it would for a shared_ptr), implying a const reference should be used instead.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

Curious, why have a CWallet_ptr typedef instead of using CWallet *? Is there a plan to switch to a different type of pointer later? Curious, because this seems to make the rest of the code more opaque. For example:

for (CWallet_ptr pwallet : vpwallets)

Because this says CWallet_ptr instead of CWallet* it's unclear whether the copy initialization has extra cost (which it would for a shared_ptr), implying a const reference should be used instead.

This comment has been minimized.

@luke-jr

luke-jr Mar 6, 2017

Member

Indeed, a shared_ptr or similar would be needed for when/if we add the ability to close wallets at runtime.

@luke-jr

luke-jr Mar 6, 2017

Member

Indeed, a shared_ptr or similar would be needed for when/if we add the ability to close wallets at runtime.

Show outdated Hide outdated src/wallet/walletdb.cpp
bitdb.mapFileUseCount.erase(_mi++);
LogPrint("db", "Flushed %s %dms\n", strFile, GetTimeMillis() - nStart);
for (CWallet_ptr pwallet : vpwallets) {

This comment has been minimized.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

This thread is created from the non-static method CWallet::postInitProcess, so it seems like it makes more sense for it to continue flushing a single wallet, rather than going through the global wallets list. Instead of looping and accessing the global vpwallet variable, ThreadFlushWalletDB could just take a new CWallet* pwallet argument and be invoked with:

threadGroup.create_thread([this](){ThreadFlushWalletDB(this);});

from CWallet::postInitProcess.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

This thread is created from the non-static method CWallet::postInitProcess, so it seems like it makes more sense for it to continue flushing a single wallet, rather than going through the global wallets list. Instead of looping and accessing the global vpwallet variable, ThreadFlushWalletDB could just take a new CWallet* pwallet argument and be invoked with:

threadGroup.create_thread([this](){ThreadFlushWalletDB(this);});

from CWallet::postInitProcess.

This comment has been minimized.

@luke-jr

luke-jr Mar 6, 2017

Member

Having a dedicated thread per wallet is just needlessly inefficient.

@luke-jr

luke-jr Mar 6, 2017

Member

Having a dedicated thread per wallet is just needlessly inefficient.

Show outdated Hide outdated src/wallet/walletdb.cpp
}
nLastFlushed = CWalletDB::GetUpdateCounter();

This comment has been minimized.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

How come the nLastFlushed assignment is moving ouside of the _mi != bitdb.mapFileUseCount.end() condition? It seems unrelated to this change. Maybe a code comment explaining the assignment would be good here.

@ryanofsky

ryanofsky Mar 6, 2017

Contributor

How come the nLastFlushed assignment is moving ouside of the _mi != bitdb.mapFileUseCount.end() condition? It seems unrelated to this change. Maybe a code comment explaining the assignment would be good here.

This comment has been minimized.

@luke-jr

luke-jr Mar 6, 2017

Member

Because the condition is per-wallet. Maybe nLastFlushed ought to be as well...?

@luke-jr

luke-jr Mar 6, 2017

Member

Because the condition is per-wallet. Maybe nLastFlushed ought to be as well...?

This comment has been minimized.

@luke-jr

luke-jr Mar 9, 2017

Member

Sanitised this better in the rebase

@luke-jr

luke-jr Mar 9, 2017

Member

Sanitised this better in the rebase

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Mar 8, 2017

Contributor

(needs another rebase)

Contributor

ryanofsky commented Mar 8, 2017

(needs another rebase)

@laanwj laanwj self-assigned this Mar 9, 2017

@laanwj laanwj added this to the 0.15.0 milestone Mar 9, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 9, 2017

Member

Added 0.15.0 milestone.

Member

laanwj commented Mar 9, 2017

Added 0.15.0 milestone.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 9, 2017

Member

Significantly rebased. Also fixed minor race conditions surrounding wallet flush (it was bumping the "have update to flush" counter before doing the actual update).

Member

luke-jr commented Mar 9, 2017

Significantly rebased. Also fixed minor race conditions surrounding wallet flush (it was bumping the "have update to flush" counter before doing the actual update).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 10, 2017

Member

It would be preferable to increment the update counter after committing a database transaction, not after every call to a CWalletDB method.
So this would mean the destructor of CWalletDB and TxnCommit() . Which should be overridden in CWalletDB, or alternatively use composition not inheritance for CDB as I've done in the last commit in #9951.
This will simplify these changes (no need to do WriteIC/EraseIC everywhere), as well as makes sure that only changes that actually hit the database/disk are counted.

Member

laanwj commented Mar 10, 2017

It would be preferable to increment the update counter after committing a database transaction, not after every call to a CWalletDB method.
So this would mean the destructor of CWalletDB and TxnCommit() . Which should be overridden in CWalletDB, or alternatively use composition not inheritance for CDB as I've done in the last commit in #9951.
This will simplify these changes (no need to do WriteIC/EraseIC everywhere), as well as makes sure that only changes that actually hit the database/disk are counted.

Show outdated Hide outdated src/util.cpp
@@ -418,6 +418,7 @@ bool SoftSetArg(const std::string& strArg, const std::string& strValue)
if (mapArgs.count(strArg))
return false;
mapArgs[strArg] = strValue;
_mapMultiArgs[strArg].push_back(strValue);

This comment has been minimized.

@laanwj

laanwj Mar 10, 2017

Member

I think it is confusing for a "Set" function to add something to a list. If anything it should replace it.
What do you need this for, specifically?
In the past I've defined SoftSetMultiArg to be able to define a whole list argument at once.

@laanwj

laanwj Mar 10, 2017

Member

I think it is confusing for a "Set" function to add something to a list. If anything it should replace it.
What do you need this for, specifically?
In the past I've defined SoftSetMultiArg to be able to define a whole list argument at once.

This comment has been minimized.

@luke-jr

luke-jr Mar 10, 2017

Member

It can't get here unless the list is currently empty. It's used to initialise -wallet, which is accessed as a list only now.

@luke-jr

luke-jr Mar 10, 2017

Member

It can't get here unless the list is currently empty. It's used to initialise -wallet, which is accessed as a list only now.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 21, 2017

Contributor

I believe this is also not correct - other threads may be using mapMultiArgs or the vector in it at the same time

@TheBlueMatt

TheBlueMatt Mar 21, 2017

Contributor

I believe this is also not correct - other threads may be using mapMultiArgs or the vector in it at the same time

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 28, 2017

Contributor

Maybe get @jtimon to do it in #9494 and then just use it here?

@TheBlueMatt

TheBlueMatt Mar 28, 2017

Contributor

Maybe get @jtimon to do it in #9494 and then just use it here?

This comment has been minimized.

@laanwj

laanwj Apr 12, 2017

Member

It can't get here unless the list is currently empty.

Good point

@laanwj

laanwj Apr 12, 2017

Member

It can't get here unless the list is currently empty.

Good point

This comment has been minimized.

@luke-jr

luke-jr Apr 18, 2017

Member

@TheBlueMatt I don't see what your concern is... We have the lock here already.

@luke-jr

luke-jr Apr 18, 2017

Member

@TheBlueMatt I don't see what your concern is... We have the lock here already.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Apr 27, 2017

Contributor

We do not take the lock when accessing mapMultiArgs all over the codebase. You cannot write a new entry to this map without updating the various locations in the codebase which access mapMultiArgs to also go through cs_args.

@TheBlueMatt

TheBlueMatt Apr 27, 2017

Contributor

We do not take the lock when accessing mapMultiArgs all over the codebase. You cannot write a new entry to this map without updating the various locations in the codebase which access mapMultiArgs to also go through cs_args.

@@ -8,6 +8,8 @@
#include "wallet/db.h"
#include "wallet/wallet.h"
CWallet *pwalletMain;

This comment has been minimized.

@laanwj

laanwj Mar 10, 2017

Member

Why define pwalletMain here?

@laanwj

laanwj Mar 10, 2017

Member

Why define pwalletMain here?

This comment has been minimized.

@luke-jr

luke-jr Mar 10, 2017

Member

There's no need to modify the tests to use vpwallets in most cases?

@luke-jr

luke-jr Mar 10, 2017

Member

There's no need to modify the tests to use vpwallets in most cases?

This comment has been minimized.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers":

Maybe add a TODO comment for updating the tests to stop referencing pwalletMain, since this will be confusing to someone seeing this who isn't familiar with the pre-multiwallet history of the code.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers":

Maybe add a TODO comment for updating the tests to stop referencing pwalletMain, since this will be confusing to someone seeing this who isn't familiar with the pre-multiwallet history of the code.

This comment has been minimized.

@laanwj

laanwj Mar 10, 2017

Member

Depends on the reason why they are setting pwalletMain. Generally that is to communicate with stuff in wallet.cpp/walletdb.cpp I'd say.

@laanwj

laanwj Mar 10, 2017

Member

Depends on the reason why they are setting pwalletMain. Generally that is to communicate with stuff in wallet.cpp/walletdb.cpp I'd say.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 10, 2017

Member

I think the overall approach here is good. This is another step towards multi-wallet support. As it's not anywhere on the API yet, I guess it's too early to ask for a multiwallet test?

Member

laanwj commented Mar 10, 2017

I think the overall approach here is good. This is another step towards multi-wallet support. As it's not anywhere on the API yet, I guess it's too early to ask for a multiwallet test?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 10, 2017

Member

It would be preferable to increment the update counter after committing a database transaction, not after every call to a CWalletDB method.

IMO too much for this PR. Just trying to multiwallet here, not refactor stuff that doesn't need it. :)

Member

luke-jr commented Mar 10, 2017

It would be preferable to increment the update counter after committing a database transaction, not after every call to a CWalletDB method.

IMO too much for this PR. Just trying to multiwallet here, not refactor stuff that doesn't need it. :)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 10, 2017

Member

IMO too much for this PR. Just trying to multiwallet here, not refactor stuff that doesn't need it. :)

Maybe my recommendation goes to far but I just think your current approach there is messy. The mutator functions call WriteIC/EraseIC which call IncrementUpdateCounter which calls IncrementUpdateCounter(strFile) which indexes into a global map indexed by name. This makes the wallet code even more complex and hard to understand as it introduces another place where per-wallet(-database) information is stored.

We need to decide whether CWalletDBStateInfo belongs with the database wrapper, or with the wallet.

  • If it is part of the database statistics side of things, the place to put it would be CDBEnv, with the other maps mapFileUseCount and mapDb. After #9951 I'm going to refactor these to put them on a CDBWrapper object which represents a single database.
  • If it is specific to wallets, the structure could be put on CWallet, and a reference passed in where it is needed (e.g. to CWalletDB) instead of looking it up all the time. This makes the CWallet more self-contained.
Member

laanwj commented Mar 10, 2017

IMO too much for this PR. Just trying to multiwallet here, not refactor stuff that doesn't need it. :)

Maybe my recommendation goes to far but I just think your current approach there is messy. The mutator functions call WriteIC/EraseIC which call IncrementUpdateCounter which calls IncrementUpdateCounter(strFile) which indexes into a global map indexed by name. This makes the wallet code even more complex and hard to understand as it introduces another place where per-wallet(-database) information is stored.

We need to decide whether CWalletDBStateInfo belongs with the database wrapper, or with the wallet.

  • If it is part of the database statistics side of things, the place to put it would be CDBEnv, with the other maps mapFileUseCount and mapDb. After #9951 I'm going to refactor these to put them on a CDBWrapper object which represents a single database.
  • If it is specific to wallets, the structure could be put on CWallet, and a reference passed in where it is needed (e.g. to CWalletDB) instead of looking it up all the time. This makes the CWallet more self-contained.
@@ -8,6 +8,8 @@
#include "wallet/db.h"
#include "wallet/wallet.h"
CWallet *pwalletMain;

This comment has been minimized.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers":

Maybe add a TODO comment for updating the tests to stop referencing pwalletMain, since this will be confusing to someone seeing this who isn't familiar with the pre-multiwallet history of the code.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers":

Maybe add a TODO comment for updating the tests to stop referencing pwalletMain, since this will be confusing to someone seeing this who isn't familiar with the pre-multiwallet history of the code.

Show outdated Hide outdated src/wallet/walletdb.cpp
@@ -103,42 +94,40 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,
Erase(std::make_pair(std::string("key"), vchPubKey));
Erase(std::make_pair(std::string("wkey"), vchPubKey));
}
IncrementUpdateCounter();

This comment has been minimized.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Bugfix: wallet: Increment "update counter" only after actually making the applicable db changes to avoid potential races":

I'm probably missing something, but if the first write above succeeds and second write fails, the counter won't be incremented. Is this wrong? Would a possible fix be to drop this line and replace all the Write/Erases above with WriteIC/EraseIC?

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Bugfix: wallet: Increment "update counter" only after actually making the applicable db changes to avoid potential races":

I'm probably missing something, but if the first write above succeeds and second write fails, the counter won't be incremented. Is this wrong? Would a possible fix be to drop this line and replace all the Write/Erases above with WriteIC/EraseIC?

This comment has been minimized.

@luke-jr

luke-jr Mar 11, 2017

Member

Hmm, not sure what we want to happen in that case...

@luke-jr

luke-jr Mar 11, 2017

Member

Hmm, not sure what we want to happen in that case...

This comment has been minimized.

@laanwj

laanwj Mar 11, 2017

Member

This is one of the reasons why I suggested above to only update the update counter if a DB transaction actually hits the database. That's more robust than doing it per operation. It's possible for all the operations to succeed then TxnAbort() to be called and still no update happens.

@laanwj

laanwj Mar 11, 2017

Member

This is one of the reasons why I suggested above to only update the update counter if a DB transaction actually hits the database. That's more robust than doing it per operation. It's possible for all the operations to succeed then TxnAbort() to be called and still no update happens.

This comment has been minimized.

@luke-jr

luke-jr Apr 18, 2017

Member

I agree, but it's outside the scope of this PR. For now, I'll just increment the counter more aggressively (merely reordering the increment to fix the race issue).

@luke-jr

luke-jr Apr 18, 2017

Member

I agree, but it's outside the scope of this PR. For now, I'll just increment the counter more aggressively (merely reordering the increment to fix the race issue).

@@ -32,46 +32,38 @@ static std::atomic<unsigned int> nWalletDBUpdateCounter;
bool CWalletDB::WriteName(const std::string& strAddress, const std::string& strName)
{
nWalletDBUpdateCounter++;

This comment has been minimized.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Bugfix: wallet: Increment "update counter" only after actually making the applicable db changes to avoid potential races":

It seems good not to increment the counter when a write or erase fails. But I don't understand what the "potential races" are that could happen if the counter is incremented regardless. If you could clarify / expand the commit message on this point, I think that would be helpful.

@ryanofsky

ryanofsky Mar 10, 2017

Contributor

In commit "Bugfix: wallet: Increment "update counter" only after actually making the applicable db changes to avoid potential races":

It seems good not to increment the counter when a write or erase fails. But I don't understand what the "potential races" are that could happen if the counter is incremented regardless. If you could clarify / expand the commit message on this point, I think that would be helpful.

This comment has been minimized.

@luke-jr

luke-jr Mar 11, 2017

Member
  1. (thread 1) Increment the counter.
  2. (thread 2) DB gets flushed
  3. (thread 1) Actual change happens.
  4. (thread 2) Thinks DB hasn't changed, so doesn't flush again.
@luke-jr

luke-jr Mar 11, 2017

Member
  1. (thread 1) Increment the counter.
  2. (thread 2) DB gets flushed
  3. (thread 1) Actual change happens.
  4. (thread 2) Thinks DB hasn't changed, so doesn't flush again.
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 24, 2017

Member

Tested a bit and seems to work as intended.
The only point I found during testing that needs fixing is the SalvageWallet renaming to "wallet.%d.bak".
Step-debugged the flushing and SyncTransaction.

Conceptual:

This PR mostly goes into a direction that one has to choose the wallets-to-work-with before starting up Core. I miss the pre-work for run-time adding and removing of wallets. Sure, this may be added later.
Furthermore, the "globalness" of the startup arguments and action like -usehd or -zapwallettxes, --keypool, etc. are somehow conceptual issues that need to be solved before deploying multiwallet.

  • Ideally creating, loading and unloading of wallets happens during runtime over an RPC call (or though the GUI).
  • In the long run, actions like zapwallettx, salvagewallet, rescan, upgradewallet, etc should also be moved to runtime.
  • Per wallet settings like -usehd, -keypool, etc. should be values that can be set during creation (or loading of a wallet).
Member

jonasschnelli commented May 24, 2017

Tested a bit and seems to work as intended.
The only point I found during testing that needs fixing is the SalvageWallet renaming to "wallet.%d.bak".
Step-debugged the flushing and SyncTransaction.

Conceptual:

This PR mostly goes into a direction that one has to choose the wallets-to-work-with before starting up Core. I miss the pre-work for run-time adding and removing of wallets. Sure, this may be added later.
Furthermore, the "globalness" of the startup arguments and action like -usehd or -zapwallettxes, --keypool, etc. are somehow conceptual issues that need to be solved before deploying multiwallet.

  • Ideally creating, loading and unloading of wallets happens during runtime over an RPC call (or though the GUI).
  • In the long run, actions like zapwallettx, salvagewallet, rescan, upgradewallet, etc should also be moved to runtime.
  • Per wallet settings like -usehd, -keypool, etc. should be values that can be set during creation (or loading of a wallet).
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 24, 2017

Member

Gitian builds are failing (all three platforms): https://bitcoin.jonasschnelli.ch/build/147

Member

jonasschnelli commented May 24, 2017

Gitian builds are failing (all three platforms): https://bitcoin.jonasschnelli.ch/build/147

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 25, 2017

Member

zapwallettx, salvagewallet

These should probably become less visible or go away or move to a separate utility.

rescan

We can do this from the rpc now.

Not sure about upgradewallet.

Member

gmaxwell commented May 25, 2017

zapwallettx, salvagewallet

These should probably become less visible or go away or move to a separate utility.

rescan

We can do this from the rpc now.

Not sure about upgradewallet.

@jnewbery

A couple of small nits inline, and a couple of general comments:

  • I think this could do with both unit and integration tests
  • Please update the comment for CWallet::Verify() in wallet.h now that it's responsible for parsing the wallet arguments and populating the ArgsManager
Show outdated Hide outdated src/wallet/walletdb.cpp
nWalletDBUpdateCounter++;
if (!batch.Write(std::make_pair(std::string("keymeta"), vchPubKey),
if (!WriteIC(std::make_pair(std::string("keymeta"), vchPubKey),

This comment has been minimized.

@jnewbery

jnewbery May 29, 2017

Member

Please update to use braces. You can also combine this line with the one below.

Several other if statements below which you can update to use braces.

@jnewbery

jnewbery May 29, 2017

Member

Please update to use braces. You can also combine this line with the one below.

Several other if statements below which you can update to use braces.

Show outdated Hide outdated src/wallet/db.cpp
++nUpdateCounter;
}
unsigned int CWalletDBWrapper::GetUpdateCounter()

This comment has been minimized.

@jnewbery

jnewbery May 29, 2017

Member

You've changed MaybeCompactWalletDB() to access CDB::nUpdateCounter directly, so this function isn't used.

@jnewbery

jnewbery May 29, 2017

Member

You've changed MaybeCompactWalletDB() to access CDB::nUpdateCounter directly, so this function isn't used.

Show outdated Hide outdated src/wallet/wallet.cpp
{
// Recover readable keypairs:
CWallet dummyWallet;
if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter))

This comment has been minimized.

@jnewbery

jnewbery May 29, 2017

Member

I agree with @jonasschnelli . This seems dangerous if Recover() or VerifyDatabaseFile() is called for multiple wallets. Should be straightforward to update Recover() to name the backup file uniquely (the warning string in VerifyDatabaseFile() will also need to be updated)

@jnewbery

jnewbery May 29, 2017

Member

I agree with @jonasschnelli . This seems dangerous if Recover() or VerifyDatabaseFile() is called for multiple wallets. Should be straightforward to update Recover() to name the backup file uniquely (the warning string in VerifyDatabaseFile() will also need to be updated)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 5, 2017

Member

Rebased and issues addressed

Member

luke-jr commented Jun 5, 2017

Rebased and issues addressed

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 6, 2017

Member

Travis falure:

Running 255 test cases...
unknown location(0): fatal error: in "wallet_tests/importwallet_rescan": unknown type
wallet/test/wallet_tests.cpp(446): last checkpoint: "importwallet_rescan" entry.
*** 1 failure is detected in the test module "Bitcoin Test Suite"
Member

laanwj commented Jun 6, 2017

Travis falure:

Running 255 test cases...
unknown location(0): fatal error: in "wallet_tests/importwallet_rescan": unknown type
wallet/test/wallet_tests.cpp(446): last checkpoint: "importwallet_rescan" entry.
*** 1 failure is detected in the test module "Bitcoin Test Suite"
@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Jun 6, 2017

Contributor

Travis falure:

The importwallet_rescan test has another pwalletMain usage that needs to be changed to vpwallets.

Contributor

ryanofsky commented Jun 6, 2017

Travis falure:

The importwallet_rescan test has another pwalletMain usage that needs to be changed to vpwallets.

@ryanofsky

utACK 33f0b60 with the test fix. First 6 commits were basically unchanged since my previous review (just a member rename and atomic init fixes). 7 new commits have been added and seem fine.

@@ -19,6 +19,8 @@
#include <boost/test/unit_test.hpp>
#include <univalue.h>
extern CWallet* pwalletMain;

This comment has been minimized.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Replace pwalletMain with a vector of wallet"

This can probably be removed after the importwallet_rescan test is updated.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Replace pwalletMain with a vector of wallet"

This can probably be removed after the importwallet_rescan test is updated.

This comment has been minimized.

@ryanofsky

ryanofsky Jun 9, 2017

Contributor

This can probably be removed after the importwallet_rescan test is updated.

Never mind, it's used by the receiverequests test.

@ryanofsky

ryanofsky Jun 9, 2017

Contributor

This can probably be removed after the importwallet_rescan test is updated.

Never mind, it's used by the receiverequests test.

Show outdated Hide outdated src/wallet/db.cpp
@@ -439,11 +439,6 @@ void CWalletDBWrapper::IncrementUpdateCounter()
++nUpdateCounter;
}
unsigned int CWalletDBWrapper::GetUpdateCounter()

This comment has been minimized.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Wallet: Support loading multiple wallets if -wallet used more than once"

Removing GetUpdateCounter in this commit isn't related to the rest of the changes here. This change was probably meant for the "CWalletDB: Store the update counter per wallet" commit.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Wallet: Support loading multiple wallets if -wallet used more than once"

Removing GetUpdateCounter in this commit isn't related to the rest of the changes here. This change was probably meant for the "CWalletDB: Store the update counter per wallet" commit.

Show outdated Hide outdated src/wallet/wallet.cpp
@@ -440,30 +440,40 @@ bool CWallet::Verify()
if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET))
return true;
uiInterface.InitMessage(_("Verifying wallet..."));
std::string walletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
SoftSetArg("-wallet", DEFAULT_WALLET_DAT);

This comment has been minimized.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Wallet: Move multiwallet sanity checks to CWallet::Verify"

Consider squashing this commit into previous commit "Wallet: Support loading multiple wallets if -wallet used more than once", because the two commits are making parallel sets changes, and having them separate makes the interaction confusing to think about.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Wallet: Move multiwallet sanity checks to CWallet::Verify"

Consider squashing this commit into previous commit "Wallet: Support loading multiple wallets if -wallet used more than once", because the two commits are making parallel sets changes, and having them separate makes the interaction confusing to think about.

Show outdated Hide outdated src/wallet/db.h
@@ -122,11 +122,15 @@ class CWalletDBWrapper
void IncrementUpdateCounter();
unsigned int GetUpdateCounter();
std::atomic<unsigned int> nUpdateCounter;
unsigned int nLastSeen;

This comment has been minimized.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

Should initialize these values.

Note: comment no longer applies. This is now done in two CWalletDBWrapper constructors.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

Should initialize these values.

Note: comment no longer applies. This is now done in two CWalletDBWrapper constructors.

@@ -825,7 +825,7 @@ bool CWalletDB::VerifyEnvironment(const std::string& walletFile, const fs::path&
bool CWalletDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr)
{
return CDB::VerifyDatabaseFile(walletFile, dataDir, errorStr, warningStr, CWalletDB::Recover);
return CDB::VerifyDatabaseFile(walletFile, dataDir, warningStr, errorStr, CWalletDB::Recover);

This comment has been minimized.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Bugfix: wallet: Fix warningStr, errorStr argument order"

Note: the bug seems to have been introduced in #8574.

@ryanofsky

ryanofsky Jun 6, 2017

Contributor

In commit "Bugfix: wallet: Fix warningStr, errorStr argument order"

Note: the bug seems to have been introduced in #8574.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Jun 6, 2017

Member

I've had a quick scan and the new commits look generally good, although #10457 seems like a simpler solution to the wallet backup name issue. I don't think it's really worth passing the backup file name around through multiple function calls just so it can be printed in a single warning message.

I'll fully review once the the test has been fixed.

Member

jnewbery commented Jun 6, 2017

I've had a quick scan and the new commits look generally good, although #10457 seems like a simpler solution to the wallet backup name issue. I don't think it's really worth passing the backup file name around through multiple function calls just so it can be printed in a single warning message.

I'll fully review once the the test has been fixed.

@@ -3972,15 +3979,27 @@ bool CWallet::ParameterInteraction()
}
if (GetBoolArg("-salvagewallet", false) && SoftSetBoolArg("-rescan", true)) {
if (is_multiwallet) {
return InitError(strprintf("%s is only allowed with a single wallet file", "-salvagewallet"));

This comment has been minimized.

@achow101

achow101 Jun 7, 2017

Member

Here you say that -salvagewallet is only for one wallet file, but in CWallet::Verify(), the loop through all wallets checks for -salvagewallet and recovers all wallets if -salvagewallet is set. Which behavior is correct: salvage all wallets, or salvage only if there is one wallet?

For the latter, the -salvagewallet check in CWallet::Verify() can be moved out of the loop.

@achow101

achow101 Jun 7, 2017

Member

Here you say that -salvagewallet is only for one wallet file, but in CWallet::Verify(), the loop through all wallets checks for -salvagewallet and recovers all wallets if -salvagewallet is set. Which behavior is correct: salvage all wallets, or salvage only if there is one wallet?

For the latter, the -salvagewallet check in CWallet::Verify() can be moved out of the loop.

This comment has been minimized.

@luke-jr

luke-jr Jun 7, 2017

Member

Both are correct. If we get to ::Verify, we should salvage all wallets. But for now, we forbid this combination. Moving it out of the loop is error-prone, and creates unnecessary complexity.

@luke-jr

luke-jr Jun 7, 2017

Member

Both are correct. If we get to ::Verify, we should salvage all wallets. But for now, we forbid this combination. Moving it out of the loop is error-prone, and creates unnecessary complexity.

@ryanofsky

utACK c237bd7. Changes since previous review were fixing rescan test, updating a salvagewallet comment, and removing GetUpdateCounter in an earlier commit.

@@ -19,6 +19,8 @@
#include <boost/test/unit_test.hpp>
#include <univalue.h>
extern CWallet* pwalletMain;

This comment has been minimized.

@ryanofsky

ryanofsky Jun 9, 2017

Contributor

This can probably be removed after the importwallet_rescan test is updated.

Never mind, it's used by the receiverequests test.

@ryanofsky

ryanofsky Jun 9, 2017

Contributor

This can probably be removed after the importwallet_rescan test is updated.

Never mind, it's used by the receiverequests test.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2017

Member

utACK c237bd7
Seems ready to merge.

Member

laanwj commented Jun 12, 2017

utACK c237bd7
Seems ready to merge.

@laanwj laanwj merged commit c237bd7 into bitcoin:master Jun 12, 2017

1 check passed

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

laanwj added a commit that referenced this pull request Jun 12, 2017

Merge #8694: Basic multiwallet support
c237bd7 wallet: Update formatting (Luke Dashjr)
9cbe8c8 wallet: Forbid -salvagewallet, -zapwallettxes, and -upgradewallet with multiple wallets (Luke Dashjr)
a2a5f3f wallet: Base backup filenames on original wallet filename (Luke Dashjr)
b823a4c wallet: Include actual backup filename in recovery warning message (Luke Dashjr)
84dcb45 Bugfix: wallet: Fix warningStr, errorStr argument order (Luke Dashjr)
008c360 Wallet: Move multiwallet sanity checks to CWallet::Verify, and do other checks on all wallets (Luke Dashjr)
0f08575 Wallet: Support loading multiple wallets if -wallet used more than once (Luke Dashjr)
b124cf0 Wallet: Replace pwalletMain with a vector of wallet pointers (Luke Dashjr)
19b3648 CWalletDB: Store the update counter per wallet (Luke Dashjr)
74e8738 Bugfix: ForceSetArg should replace entr(ies) in mapMultiArgs, not append (Luke Dashjr)
23fb9ad wallet: Move nAccountingEntryNumber from static/global to CWallet (Luke Dashjr)
9d15d55 Bugfix: wallet: Increment "update counter" when modifying account stuff (Luke Dashjr)
f28eb80 Bugfix: wallet: Increment "update counter" only after actually making the applicable db changes to avoid potential races (Luke Dashjr)

Tree-SHA512: 23f5dda58477307bc07997010740f1dc729164cdddefd2f9a2c9c7a877111eb1516d3e2ad4f9b104621f0b7f17369c69fcef13d28b85cb6c01d35f09a8845f23

@fanquake fanquake moved this from In progress to Done in Multiwallet support Jun 12, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2017

Member

I've had a quick scan and the new commits look generally good, although #10457 seems like a simpler solution to the wallet backup name issue. I don't think it's really worth passing the backup file name around through multiple function calls just so it can be printed in a single warning message.

I agree. We should probably revert that part and do #10457 afterward.

Member

laanwj commented Jun 12, 2017

I've had a quick scan and the new commits look generally good, although #10457 seems like a simpler solution to the wallet backup name issue. I don't think it's really worth passing the backup file name around through multiple function calls just so it can be printed in a single warning message.

I agree. We should probably revert that part and do #10457 afterward.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

laanwj added a commit that referenced this pull request Jul 21, 2017

Merge #10604: [wallet] [tests] Add listwallets RPC, include wallet na…
…me in `getwalletinfo` and add multiwallet test

3707fcd [wallet] [tests] Add listwallets to multiwallet test (John Newbery)
9508761 [wallet] [rpc] Add listwallets RPC (John Newbery)
4a05715 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery)
09eacee [wallet] fix comment for CWallet::Verify() (John Newbery)

Pull request description:

  - fix comment for CWallet::Verify (cleanup after #8694)
  - expose the wallet name in `getwalletinfo` rpc
  - add `listwallets` rpc - returns array of wallet names
  - add functional test for multiwallet using new rpc functionality

Tree-SHA512: 52f864726bf8a28421d4f3604a6cb95fffb3f4e19edbce18efaef06142c48dd4adb9e7a65a10de2955c80f13c00803ce27c78ccbc8434d92ef12cd36c4ccb4aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment