[bitcoin/bitcoin] [wallet] `loadwallet` RPC – load wallet at runtime (#10740)

TheBlueMatt commented on this pull request.

Should probably fix the various memory leaks in CreateWalletFromFile first – we didnt really use to care cause it would just persist through operation, now we do.

> – return InitError(strprintf(_(“Error loading wallet %s. Duplicate -wallet filename specified.”), walletFile));
– }

– std::string strError;
– if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError)) {
– return InitError(strError);
– }

– if (gArgs.GetBoolArg(“-salvagewallet”, false)) {
– // Recover readable keypairs:
– CWallet dummyWallet;
– std::string backup_filename;
– if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter, backup_filename)) {
– return false;
– }
+ return InitError(strprintf(_(“Error loading wallet %s. Duplicate wallet filename specified.”), walletFile));

This is redundant now, no?

> @@ -1080,6 +1080,9 @@ 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 VerifyWallet(std::string walletFile, bool salvage_wallet);

nit: I’d rather not make this a static CWallet member. wallet.cpp is already plenty big and the stuff that it does seems more external db-related stuff and less wallet-related stuff. Ideally it’d be nice to more clearly separate the DB stuff from the wallet logic itself by pulling things like the DB-related stuff out of CreateWalletFromFile.

> + ” “name” : , (string) The wallet name if loaded successfully.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 dummy_warning;
+ if (!CWallet::Verify(wallet_file, false, error, dummy_warning)) {

This needs some kind of lock to avoid double-loading a wallet in two rpc threads at once.

> @@ -2994,6 +2994,49 @@ 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.n”

Maybe some help messages noting that certain config options still apply here – eg if you started with something like zapwallettxes, upgradewallet, rescan, etc it will take the same action for newly-loaded wallets here.

This post was last modified on May 7, 2018, 5:52 pm