Basic multiwallet support

luke-jr commented at 9:05 pm on September 9, 2016: member

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)

luke-jr force-pushed on Sep 9, 2016

MarcoFalke added the label Wallet on Sep 9, 2016

luke-jr force-pushed on Sep 9, 2016

jonasschnelli commented at 6:54 am on September 10, 2016: contributor

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 commented at 10:22 am on September 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>

btcdrak commented at 9:21 pm on September 14, 2016: contributor

Nice, Concept ACK. Will test.

in src/init.cpp: in 996948af12 outdated

195@@ -196,8 +196,9 @@ void Shutdown()
196     StopRPC();
197     StopHTTPServer();
198 #ifdef ENABLE_WALLET
199-    if (pwalletMain)
200-        pwalletMain->Flush(false);
201+    for (CWallet_ptr pwallet : vpwallets) {

MarcoFalke commented at 9:17 am on September 21, 2016:

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

luke-jr commented at 11:09 pm on March 3, 2017:

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

luke-jr referenced this in commit d6ebde78c9 on Oct 20, 2016

luke-jr referenced this in commit ca93ab2c60 on Oct 20, 2016

luke-jr referenced this in commit aaf1507a9b on Oct 20, 2016

luke-jr referenced this in commit b0a256d73c on Oct 20, 2016

luke-jr referenced this in commit 1f62f13548 on Oct 20, 2016

luke-jr referenced this in commit 35fcc1a5c3 on Oct 20, 2016

luke-jr force-pushed on Oct 25, 2016

luke-jr referenced this in commit 9f8717cf68 on Dec 31, 2016

luke-jr force-pushed on Jan 3, 2017

luke-jr commented at 10:48 pm on January 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.

gmaxwell commented at 3:31 pm on January 7, 2017: contributor

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 force-pushed on Feb 3, 2017

luke-jr commented at 5:10 am on February 3, 2017: member

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

luke-jr force-pushed on Feb 6, 2017

luke-jr force-pushed on Feb 17, 2017

luke-jr force-pushed on Feb 25, 2017

luke-jr force-pushed on Feb 27, 2017

luke-jr force-pushed on Mar 3, 2017

luke-jr commented at 3:54 am on March 4, 2017: member

Rebased and now the next-step in multiwallet integration.

jonasschnelli commented at 8:34 am on March 6, 2017: contributor

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

in src/wallet/wallet.h: in 0d348d5a56 outdated

30@@ -31,7 +31,8 @@
31 #include <boost/shared_ptr.hpp>
32 #include <boost/thread.hpp>
33 
34-extern CWallet* pwalletMain;
35+typedef CWallet* CWallet_ptr;

ryanofsky commented at 4:37 pm on March 6, 2017:

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:

0for (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.

luke-jr commented at 9:08 pm on March 6, 2017:

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

in src/wallet/walletdb.cpp: in 0d348d5a56 outdated

831-                        bitdb.CloseDb(strFile);
832-                        bitdb.CheckpointLSN(strFile);
833-
834-                        bitdb.mapFileUseCount.erase(_mi++);
835-                        LogPrint("db", "Flushed %s %dms\n", strFile, GetTimeMillis() - nStart);
836+                    for (CWallet_ptr pwallet : vpwallets) {

ryanofsky commented at 5:07 pm on March 6, 2017:

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:

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

from CWallet::postInitProcess.

luke-jr commented at 9:10 pm on March 6, 2017:

Having a dedicated thread per wallet is just needlessly inefficient.

in src/wallet/walletdb.cpp: in 0d348d5a56 outdated

847+
848+                            bitdb.mapFileUseCount.erase(_mi++);
849+                            LogPrint("db", "Flushed %s %dms\n", strFile, GetTimeMillis() - nStart);
850+                        }
851                     }
852+                    nLastFlushed = CWalletDB::GetUpdateCounter();

ryanofsky commented at 5:08 pm on March 6, 2017:

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.

luke-jr commented at 9:12 pm on March 6, 2017:

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

luke-jr commented at 10:07 pm on March 9, 2017:

Sanitised this better in the rebase

luke-jr referenced this in commit 6e2d7c1c18 on Mar 7, 2017

luke-jr referenced this in commit d4718eb611 on Mar 7, 2017

luke-jr referenced this in commit b4ee0aa783 on Mar 7, 2017

ryanofsky commented at 5:00 pm on March 8, 2017: member

(needs another rebase)

laanwj assigned laanwj on Mar 9, 2017

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

laanwj commented at 9:22 am on March 9, 2017: member

Added 0.15.0 milestone.

luke-jr force-pushed on Mar 9, 2017

luke-jr commented at 10:08 pm on March 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).

laanwj commented at 7:40 am on March 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.

in src/util.cpp: in 3f096ca2ca outdated

417@@ -418,6 +418,7 @@ bool SoftSetArg(const std::string& strArg, const std::string& strValue)
418     if (mapArgs.count(strArg))
419         return false;
420     mapArgs[strArg] = strValue;
421+    _mapMultiArgs[strArg].push_back(strValue);

laanwj commented at 7:44 am on March 10, 2017:

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.

luke-jr commented at 5:16 pm on March 10, 2017:

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.

TheBlueMatt commented at 0:07 am on March 21, 2017:

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

TheBlueMatt commented at 3:58 pm on March 28, 2017:

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

laanwj commented at 9:55 am on April 12, 2017:

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

Good point

luke-jr commented at 3:16 pm on April 18, 2017:

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

TheBlueMatt commented at 3:24 pm on April 27, 2017:

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.

in src/wallet/test/wallet_test_fixture.cpp: in 3f096ca2ca outdated

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

laanwj commented at 7:44 am on March 10, 2017:

Why define pwalletMain here?

luke-jr commented at 5:17 pm on March 10, 2017:

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

ryanofsky commented at 5:44 pm on March 10, 2017:

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.

laanwj commented at 6:24 pm on March 10, 2017:

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 commented at 7:46 am on March 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?

luke-jr commented at 5:20 pm on March 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. :)

laanwj commented at 6:36 pm on March 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.

in src/wallet/walletdb.cpp: in f1052c224c outdated

93@@ -103,42 +94,40 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,
94         Erase(std::make_pair(std::string("key"), vchPubKey));
95         Erase(std::make_pair(std::string("wkey"), vchPubKey));
96     }
97+
98+    IncrementUpdateCounter();

ryanofsky commented at 6:39 pm on March 10, 2017:

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?

luke-jr commented at 1:26 am on March 11, 2017:

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

laanwj commented at 7:42 am on March 11, 2017:

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.

luke-jr commented at 2:41 pm on April 18, 2017:

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

in src/wallet/walletdb.cpp: in f1052c224c outdated

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

ryanofsky commented at 6:44 pm on March 10, 2017:

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.

luke-jr commented at 1:25 am on March 11, 2017:

(thread 1) Increment the counter.
(thread 2) DB gets flushed
(thread 1) Actual change happens.
(thread 2) Thinks DB hasn’t changed, so doesn’t flush again.

ryanofsky commented at 7:14 pm on March 10, 2017: member

utACK 3f096ca2cad68c8bc66a15142dabce28ed8e6998

luke-jr commented at 1:27 am on March 11, 2017: member

@laanwj I agree that stuff should be refactored, but I don’t think multiwallet needs to depend on it here.

(Why is CWalletDB not just a private parent class of CWallet anyway?)

laanwj commented at 7:47 am on March 11, 2017: member

(Why is CWalletDB not just a private parent class of CWallet anyway?)

It’s named wrongly. Because CWalletDB is not a database, it provides access to it. To be exact, it is a database transaction, similar to the CDBBatch in leveldbwrapper.h. It is commited when the object goes out of scope or TxnCommit() is called. It is not committed if TxnAbort() is called.
Should at least use composition there not inheritance. A wallet uses a wallet database, it is not a wallet database. This avoids low-level implementation details from leaking through.

I agree that stuff should be refactored, but I don’t think multiwallet needs to depend on it here.

As said I understand that. I’m not asking for you to do big changes. I just don’t want it to become even worse and more muddled.

I might pick this up myself if I have some time next week.

in src/wallet/wallet.h:33 in 3f096ca2ca outdated

29@@ -30,7 +30,8 @@
30 
31 #include <boost/shared_ptr.hpp>
32 
33-extern CWallet* pwalletMain;
34+typedef CWallet* CWallet_ptr;

TheBlueMatt commented at 0:03 am on March 21, 2017:

Why typedef this? Its literally /more/ characters now.

laanwj commented at 9:23 am on April 12, 2017:

Typedef-ing a pointer type doesn’t seem that useful to me either. I suppose the rationale is “what if we want to change it to e.g. std::shared_ptr later”. But a) I think the code is more readable if the kind of pointer is clear from the function/struct/class signatures using it b) You might want to have different kinds of pointers to the same kind of objects in different places.

NicolasDorier commented at 9:47 am on April 12, 2017:

not sure it make sense either, but if it is done, better naming it to CWalletRef to be consistent with CTransactionRef

in src/init.cpp:268 in 3f096ca2ca outdated

257@@ -256,8 +258,7 @@ void Shutdown()
258 #endif
259     UnregisterAllValidationInterfaces();
260 #ifdef ENABLE_WALLET
261-    delete pwalletMain;
262-    pwalletMain = NULL;
263+    vpwallets.clear();

TheBlueMatt commented at 0:04 am on March 21, 2017:

Don’t you still need to delete each wallet here?

TheBlueMatt commented at 0:32 am on March 21, 2017: member

Barely got started, will finish review later.

laanwj added this to the "Blockers" column in a project

in src/wallet/rpcwallet.cpp:36 in 3f096ca2ca outdated

28@@ -29,7 +29,8 @@
29 
30 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
31 {
32-    return pwalletMain;
33+    // TODO: Some way to access secondary wallets

NicolasDorier commented at 9:42 am on April 12, 2017:

I throwing an RPC error like RPC_METHOD_NOT_FOUND is better than returning NULL.

luke-jr commented at 2:47 pm on April 18, 2017:

It is not always an error for there to be no wallet (eg, signrawtransaction).

in src/wallet/wallet.cpp:3929 in 3f096ca2ca outdated

3733@@ -3724,24 +3734,17 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3734 bool CWallet::InitLoadWallet()
3735 {
3736     if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
3737-        pwalletMain = NULL;

NicolasDorier commented at 9:45 am on April 12, 2017:

vpwallets.clear() ?

EDIT: Meh, not really needed, just thought it would be more coherent.

NicolasDorier commented at 9:54 am on April 12, 2017: contributor

not blocking, but I think a CWallets instead of vpwallets it would make the code way easier to read and less error prone. Would also make locking easier to get right, and preventing some loop.

It would impact:

0std::map<std::string, CWalletDBStateInfo> WalletDBStateInfos
1std::vector<CWallet_ptr> vpwallets;
2IncrementUpdateCounter()

That said, this can be done in a separate PR.

in src/wallet/rpcwallet.cpp:33 in 3f096ca2ca outdated

28@@ -29,7 +29,8 @@
29 
30 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
31 {
32-    return pwalletMain;
33+    // TODO: Some way to access secondary wallets
34+    return vpwallets.empty() ? NULL : vpwallets[0];

laanwj commented at 9:58 am on April 12, 2017:

s/NULL/nullptr?

in src/wallet/wallet.cpp:457 in 3f096ca2ca outdated

449 
450-    std::string strError;
451-    if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError))
452-        return InitError(strError);
453+    for (const std::string& walletFile : mapMultiArgs.at("-wallet")) {
454+        if (walletFile.find_first_of("/\\") != std::string::npos) {

laanwj commented at 10:17 am on April 12, 2017:

walletFile.find_first_of("/\\") is not portable. Likely, boost::filesystem::path has a way to check a path for path separators.

luke-jr commented at 3:15 pm on April 18, 2017:

0        boost::filesystem::path walletFileBoost = walletFile;
1        if (walletFileBoost.has_root_path() || walletFileBoost.has_parent_path()) {

This would work, but off-topic here since this is existing code.

luke-jr force-pushed on Apr 18, 2017

luke-jr commented at 3:17 pm on April 18, 2017: member

In-scope nits addressed and rebased.

in src/util.cpp:488 in 20e5bfc444 outdated

484@@ -484,6 +485,8 @@ void ForceSetArg(const std::string& strArg, const std::string& strValue)
485 {
486     LOCK(cs_args);
487     mapArgs[strArg] = strValue;
488+    _mapMultiArgs[strArg].clear();

jtimon commented at 4:48 pm on April 18, 2017:

why clear here? afaik this is only used for unittests now.

luke-jr commented at 6:06 pm on April 18, 2017:

So that there’s only one value in _mapMultiArgs…

luke-jr referenced this in commit 6c67059c7d on Apr 21, 2017

luke-jr referenced this in commit 02254cab8a on Apr 21, 2017

luke-jr referenced this in commit 9a76eaf337 on Apr 21, 2017

luke-jr referenced this in commit cc7c34e66a on Apr 21, 2017

sipa removed this from the "Blockers" column in a project

luke-jr force-pushed on May 20, 2017

luke-jr commented at 0:42 am on May 21, 2017: member

Rebased

in src/wallet/walletdb.h:248 in ca8d241af3 outdated

244@@ -247,6 +245,7 @@ class CWalletDB
245     bool WriteVersion(int nVersion);
246 private:
247     CDB batch;
248+    CWalletDBWrapper& wrapper;

ryanofsky commented at 5:09 pm on May 22, 2017:

In commit “CWalletDB: Store the update counter per wallet”

Might be nice to call this member dbw instead of wrapper for consistency with the wallet member.

in src/wallet/walletdb.h:150 in 7772f23ad5 outdated

145+    bool WriteIC(const K& key, const T& value, bool fOverwrite = true)
146+    {
147+        if (!batch.Write(key, value, fOverwrite)) {
148+            return false;
149+        }
150+        IncrementUpdateCounter();

ryanofsky commented at 5:12 pm on May 22, 2017:

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

Would be helpful to have a comment mentioning that counter should be updated after the write operation, rather than before to prevent a race where the flush thread could run and store the higher counter value before the database is fully updated.

Could also mention the idea of moving the counter increment to the destructor / commit function so these write and erase wrappers would be unnecessary.

in src/wallet/db.h:128 in 68bc5b926f outdated

121@@ -122,11 +122,15 @@ class CWalletDBWrapper
122     void IncrementUpdateCounter();
123     unsigned int GetUpdateCounter();
124 
125+    std::atomic<unsigned int> nUpdateCounter;
126+    unsigned int nLastSeen;
127+    unsigned int nLastFlushed;
128+    int64_t nLastWalletUpdate;

ryanofsky commented at 5:21 pm on May 22, 2017:

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

It would be nice if conversion of these values from static variables to per-wallet members happened in the previous commit (“CWalletDB: Store the update counter per wallet”) instead of this one. Would make both commits easier to follow and avoid the unusual intermediate state where globals and per-wallet members are compared to each other.

in src/wallet/db.h:126 in 68bc5b926f outdated

121@@ -122,11 +122,15 @@ class CWalletDBWrapper
122     void IncrementUpdateCounter();
123     unsigned int GetUpdateCounter();
124 
125+    std::atomic<unsigned int> nUpdateCounter;
126+    unsigned int nLastSeen;

ryanofsky commented at 5:30 pm on May 22, 2017:

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

Should initialize these values.

ryanofsky commented at 2:56 pm on June 6, 2017:

Should initialize these values.

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

in src/wallet/db.h:129 in ca8d241af3 outdated

124+
125 private:
126     /** BerkeleyDB specific */
127     CDBEnv *env;
128     std::string strFile;
129+    std::atomic<unsigned int> nUpdateCounter;

ryanofsky commented at 5:32 pm on May 22, 2017:

In commit “CWalletDB: Store the update counter per wallet”:

Should initialize this value.

luke-jr commented at 9:11 pm on June 5, 2017:

It’s an atomic, should have a default constructor?

ryanofsky commented at 9:20 pm on June 5, 2017:

It’s an atomic, should have a default constructor?

Yes but it seems to not initialize the value, see https://stackoverflow.com/questions/36320008/whats-the-default-value-for-a-stdatomic or http://en.cppreference.com/w/cpp/atomic/atomic/atomic. IMO, adding = 0 to this line would make the code clearer anyway.

in src/wallet/walletdb.cpp:763 in ca8d241af3 outdated

759@@ -762,20 +760,22 @@ void MaybeCompactWalletDB()
760         return;
761     }
762 
763-    static unsigned int nLastSeen = CWalletDB::GetUpdateCounter();
764-    static unsigned int nLastFlushed = CWalletDB::GetUpdateCounter();
765+    CWalletDBWrapper& dbh = pwalletMain->GetDBHandle();

ryanofsky commented at 5:37 pm on May 22, 2017:

In commit “CWalletDB: Store the update counter per wallet”

Maybe call this dbw for consistency with existing wallet code.

ryanofsky commented at 5:40 pm on May 22, 2017: member

utACK 68bc5b926fadbe1d904b5138dc299d2dd06fdead. This is much simpler than before and the bugfixes and cleanup are nice even independent of the multiwallet changes.

in src/wallet/wallet.cpp:455 in 96089fffd8 outdated

459-    {
460-        // Recover readable keypairs:
461-        CWallet dummyWallet;
462-        if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter))
463+        std::string strError;
464+        if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError))

jonasschnelli commented at 7:03 am on May 24, 2017:

This redundantly opens/checks the datadir via BDB. Not sure if this is a problem…

in src/wallet/walletdb.cpp:766 in 96089fffd8 outdated

759@@ -785,22 +760,23 @@ void MaybeCompactWalletDB()
760         return;
761     }
762 
763-    static unsigned int nLastSeen = CWalletDB::GetUpdateCounter();
764-    static unsigned int nLastFlushed = CWalletDB::GetUpdateCounter();
765-    static int64_t nLastWalletUpdate = GetTime();
766+    for (CWalletRef pwallet : vpwallets) {

jonasschnelli commented at 7:48 am on May 24, 2017:

Without a lock we set the assumption that vpwallets is immutable.

in src/wallet/wallet.cpp:445 in 96089fffd8 outdated

439@@ -440,30 +440,40 @@ bool CWallet::Verify()
440     if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET))
441         return true;
442 
443+    SoftSetArg("-wallet", DEFAULT_WALLET_DAT);
444+
445     uiInterface.InitMessage(_("Verifying wallet..."));

jonasschnelli commented at 7:53 am on May 24, 2017:

nit: using Verifying wallet(s)...?

in src/wallet/wallet.cpp:462 in 96089fffd8 outdated

466+
467+        if (GetBoolArg("-salvagewallet", false))
468+        {
469+            // Recover readable keypairs:
470+            CWallet dummyWallet;
471+            if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter))

jonasschnelli commented at 7:57 am on May 24, 2017:

This renames an arbitrary wallet name to “wallet.%d.bak”, collisions may happen.

2017-05-24 07:56:04 Renamed test1.dat to wallet.1495612559.bak

jonasschnelli commented at 7:58 am on May 24, 2017:

Maybe we should use <filename>.<date>.bak (will result in something like test1.dat.1495612559.bak (which is preferable IMO).

jnewbery commented at 10:50 pm on May 29, 2017:

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)

jonasschnelli commented at 8:18 am on May 24, 2017: contributor

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 commented at 9:22 am on May 24, 2017: contributor

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

gmaxwell commented at 6:51 pm on May 25, 2017: contributor

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.

in src/wallet/walletdb.cpp:63 in 96089fffd8 outdated

77 bool CWalletDB::WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata& keyMeta)
78 {
79-    nWalletDBUpdateCounter++;
80-
81-    if (!batch.Write(std::make_pair(std::string("keymeta"), vchPubKey),
82+    if (!WriteIC(std::make_pair(std::string("keymeta"), vchPubKey),

jnewbery commented at 9:15 pm on May 25, 2017:

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.

in src/wallet/db.cpp:442 in 96089fffd8 outdated

433@@ -434,6 +434,16 @@ void CDB::Flush()
434     env->dbenv->txn_checkpoint(nMinutes ? GetArg("-dblogsize", DEFAULT_WALLET_DBLOGSIZE) * 1024 : 0, nMinutes, 0);
435 }
436 
437+void CWalletDBWrapper::IncrementUpdateCounter()
438+{
439+    ++nUpdateCounter;
440+}
441+
442+unsigned int CWalletDBWrapper::GetUpdateCounter()

jnewbery commented at 9:26 pm on May 29, 2017:

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

jnewbery commented at 10:50 pm on May 29, 2017: member

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

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

Also does all "update counter" access via IncrementUpdateCounter

f28eb8020e

Bugfix: wallet: Increment "update counter" when modifying account stuff 9d15d5548d

wallet: Move nAccountingEntryNumber from static/global to CWallet 23fb9adaea

Bugfix: ForceSetArg should replace entr(ies) in mapMultiArgs, not append 74e8738961

CWalletDB: Store the update counter per wallet 19b3648bb5

luke-jr force-pushed on Jun 5, 2017

luke-jr commented at 10:31 pm on June 5, 2017: member

Rebased and issues addressed

laanwj commented at 12:39 pm on June 6, 2017: member

Travis falure:

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

ryanofsky commented at 12:44 pm on June 6, 2017: member

Travis falure:

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

in src/wallet/test/wallet_tests.cpp:22 in 574e44eab4 outdated

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

ryanofsky commented at 2:36 pm on June 6, 2017:

In commit “Replace pwalletMain with a vector of wallet”

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

ryanofsky commented at 9:06 pm on June 9, 2017:

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

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

in src/wallet/db.cpp:442 in 60c5a3778d outdated

438@@ -439,11 +439,6 @@ void CWalletDBWrapper::IncrementUpdateCounter()
439     ++nUpdateCounter;
440 }
441 
442-unsigned int CWalletDBWrapper::GetUpdateCounter()

ryanofsky commented at 2:44 pm on June 6, 2017:

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.

in src/wallet/wallet.cpp:443 in 4c0f78c44d outdated

439@@ -440,30 +440,40 @@ bool CWallet::Verify()
440     if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET))
441         return true;
442 
443-    uiInterface.InitMessage(_("Verifying wallet..."));
444-    std::string walletFile = GetArg("-wallet", DEFAULT_WALLET_DAT);
445+    SoftSetArg("-wallet", DEFAULT_WALLET_DAT);

ryanofsky commented at 2:53 pm on June 6, 2017:

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.

in src/wallet/walletdb.cpp:831 in 9dacebee9e outdated

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

ryanofsky commented at 3:04 pm on June 6, 2017:

In commit “Bugfix: wallet: Fix warningStr, errorStr argument order”

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

ryanofsky commented at 3:27 pm on June 6, 2017: member

utACK 33f0b60b49305435c9a7fa353c91da15b3ca91a4 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.

jnewbery commented at 4:10 pm on June 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.

Wallet: Replace pwalletMain with a vector of wallet pointers b124cf04ea

Wallet: Support loading multiple wallets if -wallet used more than once 0f08575be2

Wallet: Move multiwallet sanity checks to CWallet::Verify, and do other checks on all wallets 008c360083

Bugfix: wallet: Fix warningStr, errorStr argument order 84dcb45017

wallet: Include actual backup filename in recovery warning message b823a4c9f6

wallet: Base backup filenames on original wallet filename a2a5f3f0f0

wallet: Forbid -salvagewallet, -zapwallettxes, and -upgradewallet with multiple wallets 9cbe8c80ba

wallet: Update formatting c237bd750e

luke-jr force-pushed on Jun 6, 2017

in src/wallet/wallet.cpp:3983 in c237bd750e

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

achow101 commented at 9:59 pm on June 7, 2017:

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.

luke-jr commented at 10:18 pm on June 7, 2017:

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 commented at 9:10 pm on June 9, 2017: member

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

laanwj commented at 11:27 am on June 12, 2017: member

utACK c237bd7 Seems ready to merge.

laanwj merged this on Jun 12, 2017

laanwj closed this on Jun 12, 2017

laanwj referenced this in commit 177433ad22 on Jun 12, 2017

fanquake moved this from the "In progress" to the "Done" column in a project

laanwj commented at 2:01 pm on June 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.

luke-jr referenced this in commit d9d0870725 on Jun 15, 2017

luke-jr referenced this in commit e2e288248a on Jun 15, 2017

luke-jr referenced this in commit 9ac761f8c4 on Jun 15, 2017

luke-jr referenced this in commit 06098cec5b on Jun 15, 2017

luke-jr referenced this in commit c296fb4ce0 on Jun 15, 2017

laanwj referenced this in commit 420238d310 on Jul 21, 2017

PastaPastaPasta referenced this in commit 7755051e56 on Jul 5, 2019

PastaPastaPasta referenced this in commit 67ee978a7f on Jul 5, 2019

PastaPastaPasta referenced this in commit cb530d289c on Jul 6, 2019

PastaPastaPasta referenced this in commit f712688e11 on Jul 8, 2019

PastaPastaPasta referenced this in commit 4e91eb990b on Jul 8, 2019

PastaPastaPasta referenced this in commit 071c659c63 on Jul 9, 2019

UdjinM6 referenced this in commit 2cbf726ab0 on Jul 11, 2019

PastaPastaPasta referenced this in commit a676e0db0d on Aug 6, 2019

PastaPastaPasta referenced this in commit 00124b96c1 on Aug 6, 2019

PastaPastaPasta referenced this in commit 24f5767922 on Aug 6, 2019

PastaPastaPasta referenced this in commit ffc08e135d on Aug 7, 2019

PastaPastaPasta referenced this in commit 56212875b4 on Aug 8, 2019

PastaPastaPasta referenced this in commit af4450ffeb on Aug 12, 2019

barrystyle referenced this in commit d1939c3c14 on Jan 22, 2020

barrystyle referenced this in commit 7cb3fe34bb on Jan 22, 2020

random-zebra referenced this in commit 096a0f9640 on May 17, 2021

DrahtBot locked this on Sep 8, 2021

Basic multiwallet support #8694

Conceptual: