[wallet] `loadwallet` RPC - load wallet at runtime #10740

jnewbery · 2017-07-04T14:24:10Z

Adds a loadwallet RPCs. This allows wallets to be loaded dynamically during runtime without having to stop-start the node with new -wallet params.

Includes functional tests and release notes.

Limitations:

currently this functionality is only available through the RPC interface.
wallets loaded in this way will not be displayed in the GUI.

jonasschnelli · 2017-07-05T16:22:10Z

src/init.cpp

-        delete pwallet;
-    }
-    vpwallets.clear();
+    DeleteWallets();


It'd probably call this deallocWallets()

Yes, DeleteWallets() is a bad name. DeallocWallets() is good, or perhaps DetachWallets()

jonasschnelli · 2017-07-05T16:24:20Z

src/wallet/db.cpp

@@ -227,40 +227,12 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco

 bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr)
 {
-    LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));


What's the reasons for keeping this function (that now always returns true)?

You're right. This (and CWalletDB::VerifyEnvironment()) are now entirely vestigial and are not called. Both can be removed.

jonasschnelli · 2017-07-05T16:26:54Z

src/wallet/wallet.cpp

@@ -35,6 +35,8 @@
 #include <boost/algorithm/string/replace.hpp>
 #include <boost/thread.hpp>

+/** Protects the vpwallets vector */
+CCriticalSection cs_wallets;
 std::vector<CWalletRef> vpwallets;


I guess this belongs to walletinit.h now?

Why? vpwallets is accessed by functions in several of the wallet source files.

I had the idea that walletinit.h/.c is kind of the multiwallet interface (wallet manager). The functions we have there (DeleteWallets(), VerifyWallets(), ShutdownWallets();, etc.) sounded for me after vpwallets belongs there...

IMO, the multiwallet map should ideally not sit within the objects (CWallet) implementation.

Tend to agree with @jonasschnelli here, though I agree the barrier isn't exactly clear (yet). Ideally we'd move more towards clarifying the interface here - making CWallet less and less aware of all the things around it (including other wallets). @sipa thoughts here?

jnewbery · 2017-07-07T10:30:35Z

This overlaps significantly with #10762. Not sure how I should ask for review. Perhaps find out which one people think is the priority? Or I could split the walletinit.cpp commits into their own PR.

jnewbery · 2017-07-07T10:33:57Z

src/wallet/rpcwallet.cpp

+    return obj;
+}
+
+UniValue loadwallet(const JSONRPCRequest& request)


On further thought, I think I prefer the names attachwallet and detachwallet. That gives a bit more distinction from the dump/import RPCs and makes a stronger implication that the new .dat file is being loaded and attached as a separate wallet.

promag · 2017-07-07T10:43:28Z

@jnewbery IMO this one should rebase on the other.

jnewbery · 2017-07-07T10:46:52Z

IMO this one should rebase on the other.

Makes logical sense. Downside is that the other is a more significant change, so may take longer to get merged.

promag · 2017-07-07T10:54:19Z

If this really depends on that work then there's no other way than waiting. The other downside is that the other commits will show up here too.

jnewbery · 2017-07-07T15:09:16Z

I've changed the PR sequence, so this will now depend on #10767. If I get positive concept feedbac on #10767 I'll rebase this on top of it.

For now, I'm still just looking for concept feedback.

jnewbery · 2017-08-28T21:21:22Z

Rebased on #10767 . Reviewers, please review #10767 first.

…terface 5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery) 43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery) 290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery) 062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery) 77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery) 2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery) 1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery) 9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery) Pull request description: Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762 All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet. There should be no changes in behavior from this PR. Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

jnewbery · 2017-09-11T16:31:21Z

Rebased now that #10767 is merged. Still a work in progress. PR text updated.

jnewbery · 2017-09-20T14:16:16Z

Bad automatic merge of the testcase. Re-merged manually.

jnewbery · 2017-09-20T16:51:34Z

Manual rebase fixed the test bug. Travis now passes.

This is ready for initial review.

promag

Quick review. All touched code could be updated as per dev notes.

Most importantly, With the LOCK(cs_wallets) it will be impossible to have concurrent RPC to different wallets, not to mention to the same wallet. Is this expected behavior? - I'm assuming there is a thread pool for RPC.

promag · 2017-09-21T09:07:28Z

src/qt/bitcoin.cpp

-                             paymentServer, SLOT(fetchPaymentACK(CWallet*,const SendCoinsRecipient&,QByteArray)));
+            // TODO: Expose secondary wallets
+            LOCK(cs_wallets);
+            if (!vpwallets.empty())


promag · 2017-09-21T09:08:01Z

src/qt/bitcoin.cpp

+                window->addWallet(BitcoinGUI::DEFAULT_WALLET, walletModel);
+                window->setCurrentWallet(BitcoinGUI::DEFAULT_WALLET);
+
+                connect(walletModel, SIGNAL(coinsSent(CWallet*,SendCoinsRecipient,QByteArray)),


Missing spaces after ,?

promag · 2017-09-21T09:39:58Z

src/wallet/init.cpp

-            return InitError(strprintf(_("Error loading wallet %s. -wallet parameter must only specify a filename (not a path)."), walletFile));
-        }
+    // -salvagewallet can only be used when loading a single wallet
+    bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;


Throw an error if -salvagewallet and there are multiple wallets?

Sounds like a sensible change in behaviour. I think it's beyond the scope of this PR.

#10740 (comment)

In commit "Allow single wallet to be verified"

It looks like WalletParameterInteraction is already throwing an error in this case. I think it would be better to drop the comment and wallet_files check here. If you'd prefer to keep the check, I think the comment should be something clearer like "// parameter interaction code should have thrown an error if -salvagewallet was enabled with more than wallet file, so the wallet_files size check here should have no effect."

Updated comment as suggested by @ryanofsky .

Appears this comment update was lost somewhere?

promag · 2017-09-21T09:45:52Z

src/wallet/rpcwallet.cpp

@@ -37,6 +37,7 @@ static const std::string WALLET_ENDPOINT_BASE = "/wallet/";

 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
+    AssertLockHeld(cs_wallets);


Why not just LOCK(cs_wallets) too?

Because I'd prefer to avoid recursive locking if possible.

Just curious, why?

Yes, definitely. cs_wallets is locked before cs_main in global lock order, so putting it too many places is just asking for trouble, IMO.

jnewbery · 2017-09-21T13:43:28Z

Thanks for the review @promag!

Regarding the locking: Yes, I've implemented this as a mutex for now, which as you point out would limit wallet RPCs to be single-threaded. I'd like to update this to be something a bit better before v0.16. Suggestions are either a shared mutex or (suggested by @theuni) a version of the decay pointers in #10738. The problem with using a shared mutex is that C++ doesn't have a standard library shared mutex until C++14 (std::shared_timed_mutex) so I'd either have to use boost::shared_mutex or roll my own.

(apologies - I thought I'd already written a note in this PR explaining this, but I must have failed to hit send!)

For now, I think review of this PR with the mutex is helpful, and I'll change it around to use a shared_mutex later.

luke-jr · 2017-09-21T20:03:51Z

This seems to ignore the whole issue of long-term wallet references, and probably locks cs_wallets too long.

…in WalletInit directly Also move PS maintenance into wallet/init.cpp WalletInit::Start calls postInitProcess() for each wallet. Previously each call to postInitProcess() would attempt to schedule wallet background flushing. Just start wallet background flushing once from WalletInit::Start(). Signed-off-by: Pasta <pasta@dashboost.org>