From dbfa34540372033d95036a02b7025ddd33f540aa Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 17:01:50 -0500 Subject: [PATCH 1/5] wallet: Allow MigrateLegacyToDescriptor to take a wallet name An overload of MigrateLegacyToDescriptor is added which takes the wallet name. The original that took a wallet pointer is still available, it just gets the name, closes the wallet, and calls the new overload. --- src/wallet/rpc/wallet.cpp | 14 ++++++++----- src/wallet/wallet.cpp | 42 ++++++++++++++++++++------------------- src/wallet/wallet.h | 2 +- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 23a88cd51b1..9af7723e31a 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -738,16 +738,20 @@ static RPCHelpMan migratewallet() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr wallet = GetWalletForJSONRPCRequest(request); - if (!wallet) return NullUniValue; + std::string wallet_name; + { + std::shared_ptr wallet = GetWalletForJSONRPCRequest(request); + if (!wallet) return NullUniValue; - if (wallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); + if (wallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); + } + wallet_name = wallet->GetName(); } WalletContext& context = EnsureWalletContext(request.context); - util::Result res = MigrateLegacyToDescriptor(std::move(wallet), context); + util::Result res = MigrateLegacyToDescriptor(wallet_name, context); if (!res) { throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5a92dbe4288..52666eac66a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4165,32 +4165,19 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return true; } -util::Result MigrateLegacyToDescriptor(std::shared_ptr&& wallet, WalletContext& context) +util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, WalletContext& context) { - // Before anything else, check if there is something to migrate. - if (!wallet->GetLegacyScriptPubKeyMan()) { - return util::Error{_("Error: This wallet is already a descriptor wallet")}; - } - MigrationResult res; bilingual_str error; std::vector warnings; - // Make a backup of the DB - std::string wallet_name = wallet->GetName(); - fs::path this_wallet_dir = fs::absolute(fs::PathFromString(wallet->GetDatabase().Filename())).parent_path(); - fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", wallet_name, GetTime())); - fs::path backup_path = this_wallet_dir / backup_filename; - if (!wallet->BackupWallet(fs::PathToString(backup_path))) { - return util::Error{_("Error: Unable to make a backup of your wallet")}; + // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it + if (auto wallet = GetWallet(context, wallet_name)) { + if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { + return util::Error{_("Unable to unload the wallet before migrating")}; + } + UnloadWallet(std::move(wallet)); } - res.backup_path = backup_path; - - // Unload the wallet so that nothing else tries to use it while we're changing it - if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { - return util::Error{_("Unable to unload the wallet before migrating")}; - } - UnloadWallet(std::move(wallet)); // Load the wallet but only in the context of this function. // No signals should be connected nor should anything else be aware of this wallet @@ -4204,11 +4191,26 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr return util::Error{Untranslated("Wallet file verification failed.") + Untranslated(" ") + error}; } + // Make the local wallet std::shared_ptr local_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); if (!local_wallet) { return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; } + // Before anything else, check if there is something to migrate. + if (!local_wallet->GetLegacyScriptPubKeyMan()) { + return util::Error{_("Error: This wallet is already a descriptor wallet")}; + } + + // Make a backup of the DB + fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); + fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", wallet_name, GetTime())); + fs::path backup_path = this_wallet_dir / backup_filename; + if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { + return util::Error{_("Error: Unable to make a backup of your wallet")}; + } + res.backup_path = backup_path; + bool success = false; { LOCK(local_wallet->cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f104a15f98e..40720acc8e5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1007,7 +1007,7 @@ struct MigrationResult { }; //! Do all steps to migrate a legacy wallet to a descriptor wallet -util::Result MigrateLegacyToDescriptor(std::shared_ptr&& wallet, WalletContext& context); +util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, WalletContext& context); } // namespace wallet #endif // BITCOIN_WALLET_WALLET_H From 6bdbc5ff590de18dfb47c31190baad879f68fef7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 17:03:35 -0500 Subject: [PATCH 2/5] rpc: Allow users to specify wallet name for migratewallet --- src/wallet/rpc/wallet.cpp | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 9af7723e31a..06dfd1106be 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -722,7 +722,9 @@ static RPCHelpMan migratewallet() "file will be named -.legacy.bak and can be found in the directory\n" "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet." + HELP_REQUIRING_PASSPHRASE, - {}, + { + {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to migrate. If provided both here and in the RPC endpoint, the two must be identical."}, + }, RPCResult{ RPCResult::Type::OBJ, "", "", { @@ -739,17 +741,24 @@ static RPCHelpMan migratewallet() [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { std::string wallet_name; - { - std::shared_ptr wallet = GetWalletForJSONRPCRequest(request); - if (!wallet) return NullUniValue; - - if (wallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); + if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { + if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets"); } - wallet_name = wallet->GetName(); + } else { + if (request.params[0].isNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Either RPC endpoint wallet or wallet_name parameter must be provided"); + } + wallet_name = request.params[0].get_str(); } WalletContext& context = EnsureWalletContext(request.context); + { + std::shared_ptr wallet = GetWallet(context, wallet_name); + if (wallet && wallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); + } + } util::Result res = MigrateLegacyToDescriptor(wallet_name, context); if (!res) { From 7fd125b27d48e410509f3009e2eb9fa5cd6729dd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 17:10:44 -0500 Subject: [PATCH 3/5] wallet: Be able to unlock the wallet for migration Since migration reloads the wallet, the wallet will always be locked unless the passphrase is given. migratewallet can now take the passphrase in order to unlock the wallet for migration. --- src/wallet/rpc/wallet.cpp | 14 +++++++------- src/wallet/wallet.cpp | 17 +++++++++++++++-- src/wallet/wallet.h | 2 +- test/functional/wallet_migration.py | 1 - 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 06dfd1106be..f59b532cef4 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -724,6 +724,7 @@ static RPCHelpMan migratewallet() HELP_REQUIRING_PASSPHRASE, { {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to migrate. If provided both here and in the RPC endpoint, the two must be identical."}, + {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The wallet passphrase"}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -752,15 +753,14 @@ static RPCHelpMan migratewallet() wallet_name = request.params[0].get_str(); } - WalletContext& context = EnsureWalletContext(request.context); - { - std::shared_ptr wallet = GetWallet(context, wallet_name); - if (wallet && wallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); - } + SecureString wallet_pass; + wallet_pass.reserve(100); + if (!request.params[1].isNull()) { + wallet_pass = std::string_view{request.params[1].get_str()}; } - util::Result res = MigrateLegacyToDescriptor(wallet_name, context); + WalletContext& context = EnsureWalletContext(request.context); + util::Result res = MigrateLegacyToDescriptor(wallet_name, wallet_pass, context); if (!res) { throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 52666eac66a..c999adf8caf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3875,7 +3875,7 @@ std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& err std::optional res = legacy_spkm->MigrateToDescriptor(); if (res == std::nullopt) { - error = _("Error: Unable to produce descriptors for this legacy wallet. Make sure the wallet is unlocked first"); + error = _("Error: Unable to produce descriptors for this legacy wallet. Make sure to provide the wallet's passphrase if it is encrypted."); return std::nullopt; } return res; @@ -4165,7 +4165,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return true; } -util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, WalletContext& context) +util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context) { MigrationResult res; bilingual_str error; @@ -4215,6 +4215,19 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle { LOCK(local_wallet->cs_wallet); + // Unlock the wallet if needed + if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { + if (passphrase.find('\0') == std::string::npos) { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; + } else { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " + "The passphrase contains a null character (ie - a zero byte). " + "If this passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character.")}; + } + } + // First change to using SQLite if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 40720acc8e5..cab516f382e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1007,7 +1007,7 @@ struct MigrationResult { }; //! Do all steps to migrate a legacy wallet to a descriptor wallet -util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, WalletContext& context); +util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context); } // namespace wallet #endif // BITCOIN_WALLET_WALLET_H diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 72c5fe7b840..24373f81131 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -403,7 +403,6 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.encryptwallet("pass") - assert_raises_rpc_error(-15, "Error: migratewallet on encrypted wallets is currently unsupported.", wallet.migratewallet) # TODO: Fix migratewallet so that we can actually migrate encrypted wallets def run_test(self): From aaf02b5721a8b5d3d9280dc3146fa5e44ea671b6 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 18:26:38 -0500 Subject: [PATCH 4/5] tests: Tests for migrating wallets by name, and providing passphrase --- test/functional/wallet_migration.py | 69 ++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 24373f81131..7c2959bb895 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -400,10 +400,75 @@ class WalletMigrationTest(BitcoinTestFramework): def test_encrypted(self): self.log.info("Test migration of an encrypted wallet") wallet = self.create_legacy_wallet("encrypted") + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) wallet.encryptwallet("pass") + addr = wallet.getnewaddress() + txid = default.sendtoaddress(addr, 1) + self.generate(self.nodes[0], 1) + bals = wallet.getbalances() - # TODO: Fix migratewallet so that we can actually migrate encrypted wallets + assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet) + assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet, None, "badpass") + assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null") + + wallet.migratewallet(passphrase="pass") + + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + assert_equal(info["unlocked_until"], 0) + wallet.gettransaction(txid) + + assert_equal(bals, wallet.getbalances()) + + def test_unloaded(self): + self.log.info("Test migration of a wallet that isn't loaded") + wallet = self.create_legacy_wallet("notloaded") + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + addr = wallet.getnewaddress() + txid = default.sendtoaddress(addr, 1) + self.generate(self.nodes[0], 1) + bals = wallet.getbalances() + + wallet.unloadwallet() + + assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", wallet.migratewallet, "someotherwallet") + assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.nodes[0].migratewallet) + self.nodes[0].migratewallet("notloaded") + + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + wallet.gettransaction(txid) + + assert_equal(bals, wallet.getbalances()) + + def test_unloaded_by_path(self): + self.log.info("Test migration of a wallet that isn't loaded, specified by path") + wallet = self.create_legacy_wallet("notloaded2") + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + addr = wallet.getnewaddress() + txid = default.sendtoaddress(addr, 1) + self.generate(self.nodes[0], 1) + bals = wallet.getbalances() + + wallet.unloadwallet() + + wallet_file_path = os.path.join(self.nodes[0].datadir, "regtest", "wallets", "notloaded2") + self.nodes[0].migratewallet(wallet_file_path) + + # Because we gave the name by full path, the loaded wallet's name is that path too. + wallet = self.nodes[0].get_wallet_rpc(wallet_file_path) + + info = wallet.getwalletinfo() + assert_equal(info["descriptors"], True) + assert_equal(info["format"], "sqlite") + wallet.gettransaction(txid) + + assert_equal(bals, wallet.getbalances()) def run_test(self): self.generate(self.nodes[0], 101) @@ -415,6 +480,8 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_no_privkeys() self.test_pk_coinbases() self.test_encrypted() + self.test_unloaded() + self.test_unloaded_by_path() if __name__ == '__main__': WalletMigrationTest().main() From 9486509be65f09174a0cb50a337cac58a0c09de4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 6 Feb 2023 00:27:39 -0500 Subject: [PATCH 5/5] wallet, rpc: Update migratewallet help text for encrypted wallets --- src/wallet/rpc/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index f59b532cef4..3162cce9873 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -720,8 +720,8 @@ static RPCHelpMan migratewallet() "A new wallet backup will need to be made.\n" "\nThe migration process will create a backup of the wallet before migrating. This backup\n" "file will be named -.legacy.bak and can be found in the directory\n" - "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet." + - HELP_REQUIRING_PASSPHRASE, + "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet." + "\nEncrypted wallets must have the passphrase provided as an argument to this call.", { {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to migrate. If provided both here and in the RPC endpoint, the two must be identical."}, {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The wallet passphrase"},