Merge #10740: [wallet] loadwallet RPC - load wallet at runtime

cd53981 [docs] Add release notes for `loadwallet` RPC. (John Newbery)
a46aeb6 [wallet] [tests] Test loadwallet (John Newbery)
5d15260 [wallet] [rpc] Add loadwallet RPC (John Newbery)
876eb64 [wallet] Pass error message back from CWallet::Verify() (John Newbery)
e0e90db [wallet] Add CWallet::Verify function (John Newbery)
470316c [wallet] setup wallet background flushing in WalletInit directly (John Newbery)
59b87a2 [wallet] Fix potential memory leak in CreateWalletFromFile (John Newbery)

Pull request description:

  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.

Tree-SHA512: f80dfe32b77f5c97ea3732ac538de7d6ed7e7cd0413c2ec91096bb652ad9bccf05d847ddbe81e7cd3cd44eb8030a51a5f00083871228b1b9b0b8398994f6f9f1

Author: laanwj

doc/release-notes-pr10740.md

@@ -0,0 +1,8 @@
+Dynamic loading of wallets
+--------------------------
+
+Previously, wallets could only be loaded at startup, by specifying `-wallet` parameters on the command line or in the bitcoin.conf file. It is now possible to load wallets dynamically at runtime by calling the `loadwallet` RPC.
+
+The wallet can be specified as file/directory basename (which must be located in the `walletdir` directory), or as an absolute path to a file/directory.
+
+This feature is currently only available through the RPC interface. Wallets loaded in this way will not display in the bitcoin-qt GUI.

src/wallet/init.cpp

@@ -6,6 +6,7 @@
 #include <chainparams.h>
 #include <init.h>
 #include <net.h>
+#include <scheduler.h>
 #include <util.h>
 #include <utilmoneystr.h>
 #include <validation.h>
@@ -189,55 +190,29 @@ bool WalletInit::Verify() const
 
     uiInterface.InitMessage(_("Verifying wallet(s)..."));
 
+    std::vector<std::string> wallet_files = gArgs.GetArgs("-wallet");
+
+    // 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.
+    bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;
+
     // Keep track of each wallet absolute path to detect duplicates.
     std::set<fs::path> wallet_paths;
 
-    for (const std::string& walletFile : gArgs.GetArgs("-wallet")) {
-        // Do some checking on wallet path. It should be either a:
-        //
-        // 1. Path where a directory can be created.
-        // 2. Path to an existing directory.
-        // 3. Path to a symlink to a directory.
-        // 4. For backwards compatibility, the name of a data file in -walletdir.
-        fs::path wallet_path = fs::absolute(walletFile, GetWalletDir());
-        fs::file_type path_type = fs::symlink_status(wallet_path).type();
-        if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
-              (path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
-              (path_type == fs::regular_file && fs::path(walletFile).filename() == walletFile))) {
-            return InitError(strprintf(
-                _("Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and "
-                  "database/log.?????????? files can be stored, a location where such a directory could be created, "
-                  "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)"),
-                walletFile, GetWalletDir()));
-        }
+    for (const auto wallet_file : wallet_files) {
+        fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir());
 
         if (!wallet_paths.insert(wallet_path).second) {
-            return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), walletFile));
-        }
-
-        std::string strError;
-        if (!WalletBatch::VerifyEnvironment(wallet_path, strError)) {
-            return InitError(strError);
+            return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file));
         }
 
-        if (gArgs.GetBoolArg("-salvagewallet", false)) {
-            // Recover readable keypairs:
-            CWallet dummyWallet("dummy", WalletDatabase::CreateDummy());
-            std::string backup_filename;
-            if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) {
-                return false;
-            }
-        }
-
-        std::string strWarning;
-        bool dbV = WalletBatch::VerifyDatabaseFile(wallet_path, strWarning, strError);
-        if (!strWarning.empty()) {
-            InitWarning(strWarning);
-        }
-        if (!dbV) {
-            InitError(strError);
-            return false;
-        }
+        std::string error_string;
+        std::string warning_string;
+        bool verify_success = CWallet::Verify(wallet_file, salvage_wallet, error_string, warning_string);
+        if (!error_string.empty()) InitError(error_string);
+        if (!warning_string.empty()) InitWarning(warning_string);
+        if (!verify_success) return false;
     }
 
     return true;
@@ -264,8 +239,11 @@ bool WalletInit::Open() const
 void WalletInit::Start(CScheduler& scheduler) const
 {
     for (CWallet* pwallet : GetWallets()) {
-        pwallet->postInitProcess(scheduler);
+        pwallet->postInitProcess();
     }
+
+    // Run a thread to flush wallet periodically
+    scheduler.scheduleEvery(MaybeCompactWalletDB, 500);
 }
 
 void WalletInit::Flush() const

src/wallet/rpcwallet.cpp

@@ -2994,6 +2994,53 @@ static UniValue listwallets(const JSONRPCRequest& request)
     return obj;
 }
 
+UniValue loadwallet(const JSONRPCRequest& request)
+{
+    if (request.fHelp || request.params.size() != 1)
+        throw std::runtime_error(
+            "loadwallet \"filename\"\n"
+            "\nLoads a wallet from a wallet file or directory."
+            "\nNote that all wallet command-line options used when starting bitcoind will be"
+            "\napplied to the new wallet (eg -zapwallettxes, upgradewallet, rescan, etc).\n"
+            "\nArguments:\n"
+            "1. \"filename\"    (string, required) The wallet directory or .dat file.\n"
+            "\nResult:\n"
+            "{\n"
+            "  \"name\" :    <wallet_name>,        (string) The wallet name if loaded successfully.\n"
+            "  \"warning\" : <warning>,            (string) Warning message if wallet was not loaded cleanly.\n"
+            "}\n"
+            "\nExamples:\n"
+            + HelpExampleCli("loadwallet", "\"test.dat\"")
+            + HelpExampleRpc("loadwallet", "\"test.dat\"")
+        );
+    std::string wallet_file = request.params[0].get_str();
+    std::string error;
+
+    fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir());
+    if (fs::symlink_status(wallet_path).type() == fs::file_not_found) {
+        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found.");
+    }
+
+    std::string warning;
+    if (!CWallet::Verify(wallet_file, false, error, warning)) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error);
+    }
+
+    CWallet * const wallet = CWallet::CreateWalletFromFile(wallet_file, fs::absolute(wallet_file, GetWalletDir()));
+    if (!wallet) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Wallet loading failed.");
+    }
+    AddWallet(wallet);
+
+    wallet->postInitProcess();
+
+    UniValue obj(UniValue::VOBJ);
+    obj.pushKV("name", wallet->GetName());
+    obj.pushKV("warning", warning);
+
+    return obj;
+}
+
 static UniValue resendwallettransactions(const JSONRPCRequest& request)
 {
     CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
@@ -4197,6 +4244,7 @@ static const CRPCCommand commands[] =
     { "wallet",             "listtransactions",                 &listtransactions,              {"account|dummy","count","skip","include_watchonly"} },
     { "wallet",             "listunspent",                      &listunspent,                   {"minconf","maxconf","addresses","include_unsafe","query_options"} },
     { "wallet",             "listwallets",                      &listwallets,                   {} },
+    { "wallet",             "loadwallet",                       &loadwallet,                    {"filename"} },
     { "wallet",             "lockunspent",                      &lockunspent,                   {"unlock","transactions"} },
     { "wallet",             "sendfrom",                         &sendfrom,                      {"fromaccount","toaddress","amount","minconf","comment","comment_to"} },
     { "wallet",             "sendmany",                         &sendmany,                      {"fromaccount|dummy","amounts","minconf","comment","subtractfeefrom","replaceable","conf_target","estimate_mode"} },

src/wallet/wallet.cpp

@@ -23,11 +23,11 @@
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
-#include <scheduler.h>
 #include <timedata.h>
 #include <txmempool.h>
 #include <utilmoneystr.h>
 #include <wallet/fees.h>
+#include <wallet/walletutil.h>
 
 #include <algorithm>
 #include <assert.h>
@@ -3990,6 +3990,52 @@ void CWallet::MarkPreSplitKeys()
     }
 }
 
+bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string)
+{
+    // Do some checking on wallet path. It should be either a:
+    //
+    // 1. Path where a directory can be created.
+    // 2. Path to an existing directory.
+    // 3. Path to a symlink to a directory.
+    // 4. For backwards compatibility, the name of a data file in -walletdir.
+    LOCK(cs_wallets);
+    fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir());
+    fs::file_type path_type = fs::symlink_status(wallet_path).type();
+    if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
+          (path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
+          (path_type == fs::regular_file && fs::path(wallet_file).filename() == wallet_file))) {
+        error_string = strprintf(
+              "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and "
+              "database/log.?????????? files can be stored, a location where such a directory could be created, "
+              "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)",
+              wallet_file, GetWalletDir());
+        return false;
+    }
+
+    // Make sure that the wallet path doesn't clash with an existing wallet path
+    for (auto wallet : GetWallets()) {
+        if (fs::absolute(wallet->GetName(), GetWalletDir()) == wallet_path) {
+            error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", wallet_file);
+            return false;
+        }
+    }
+
+    if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) {
+        return false;
+    }
+
+    if (salvage_wallet) {
+        // Recover readable keypairs:
+        CWallet dummyWallet("dummy", WalletDatabase::CreateDummy());
+        std::string backup_filename;
+        if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) {
+            return false;
+        }
+    }
+
+    return WalletBatch::VerifyDatabaseFile(wallet_path, warning_string, error_string);
+}
+
 CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path& path)
 {
     const std::string& walletFile = name;
@@ -4012,7 +4058,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
 
     int64_t nStart = GetTimeMillis();
     bool fFirstRun = true;
-    CWallet *walletInstance = new CWallet(name, WalletDatabase::Create(path));
+    // Make a temporary wallet unique pointer so memory doesn't get leaked if
+    // wallet creation fails.
+    auto temp_wallet = MakeUnique<CWallet>(name, WalletDatabase::Create(path));
+    CWallet* walletInstance = temp_wallet.get();
     DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
     if (nLoadWalletRet != DBErrors::LOAD_OK)
     {
@@ -4224,7 +4273,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
     }
 
     walletInstance->m_last_block_processed = chainActive.Tip();
-    RegisterValidationInterface(walletInstance);
 
     if (chainActive.Tip() && chainActive.Tip() != pindexRescan)
     {
@@ -4290,6 +4338,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
             }
         }
     }
+
+    // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.
+    RegisterValidationInterface(temp_wallet.release());
+
     walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
 
     {
@@ -4302,18 +4354,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
     return walletInstance;
 }
 
-std::atomic<bool> CWallet::fFlushScheduled(false);
-
-void CWallet::postInitProcess(CScheduler& scheduler)
+void CWallet::postInitProcess()
 {
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
     ReacceptWalletTransactions();
-
-    // Run a thread to flush wallet periodically
-    if (!CWallet::fFlushScheduled.exchange(true)) {
-        scheduler.scheduleEvery(MaybeCompactWalletDB, 500);
-    }
 }
 
 bool CWallet::BackupWallet(const std::string& strDest)

src/wallet/wallet.h

@@ -68,7 +68,6 @@ class CCoinControl;
 class COutput;
 class CReserveKey;
 class CScript;
-class CScheduler;
 class CTxMemPool;
 class CBlockPolicyEstimator;
 class CWalletTx;
@@ -675,7 +674,6 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions
 class CWallet final : public CCryptoKeyStore, public CValidationInterface
 {
 private:
-    static std::atomic<bool> fFlushScheduled;
     std::atomic<bool> fAbortRescan{false};
     std::atomic<bool> fScanningWallet{false}; // controlled by WalletRescanReserver
     std::mutex mutexScanning;
@@ -1120,14 +1118,17 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
     /** Mark a transaction as replaced by another transaction (e.g., BIP 125). */
     bool MarkReplaced(const uint256& originalHash, const uint256& newHash);
 
+    //! Verify wallet naming and perform salvage on the wallet if required
+    static bool Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string);
+
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
     static CWallet* CreateWalletFromFile(const std::string& name, const fs::path& path);
 
     /**
      * Wallet post-init setup
      * Gives the wallet a chance to register repetitive tasks and complete post-init tasks
      */
-    void postInitProcess(CScheduler& scheduler);
+    void postInitProcess();
 
     bool BackupWallet(const std::string& strDest);
 

test/functional/wallet_multiwallet.py

@@ -170,5 +170,46 @@ def run_test(self):
         assert_equal(w1.getwalletinfo()['paytxfee'], 0)
         assert_equal(w2.getwalletinfo()['paytxfee'], 4.0)
 
+        self.log.info("Test dynamic wallet loading")
+
+        self.restart_node(0, ['-nowallet'])
+        assert_equal(node.listwallets(), [])
+        assert_raises_rpc_error(-32601, "Method not found", node.getwalletinfo)
+
+        self.log.info("Load first wallet")
+        loadwallet_name = node.loadwallet(wallet_names[0])
+        assert_equal(loadwallet_name['name'], wallet_names[0])
+        assert_equal(node.listwallets(), wallet_names[0:1])
+        node.getwalletinfo()
+        w1 = node.get_wallet_rpc(wallet_names[0])
+        w1.getwalletinfo()
+
+        self.log.info("Load second wallet")
+        loadwallet_name = node.loadwallet(wallet_names[1])
+        assert_equal(loadwallet_name['name'], wallet_names[1])
+        assert_equal(node.listwallets(), wallet_names[0:2])
+        assert_raises_rpc_error(-19, "Wallet file not specified", node.getwalletinfo)
+        w2 = node.get_wallet_rpc(wallet_names[1])
+        w2.getwalletinfo()
+
+        self.log.info("Load remaining wallets")
+        for wallet_name in wallet_names[2:]:
+            loadwallet_name = self.nodes[0].loadwallet(wallet_name)
+            assert_equal(loadwallet_name['name'], wallet_name)
+
+        assert_equal(set(self.nodes[0].listwallets()), set(wallet_names))
+
+        # Fail to load if wallet doesn't exist
+        assert_raises_rpc_error(-18, 'Wallet wallets not found.', self.nodes[0].loadwallet, 'wallets')
+
+        # Fail to load duplicate wallets
+        assert_raises_rpc_error(-4, 'Wallet file verification failed: Error loading wallet w1. Duplicate -wallet filename specified.', self.nodes[0].loadwallet, wallet_names[0])
+
+        # Fail to load if one wallet is a copy of another
+        assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy')
+
+        # Fail to load if wallet file is a symlink
+        assert_raises_rpc_error(-4, "Wallet file verification failed: Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink')
+
 if __name__ == '__main__':
     MultiWalletTest().main()