foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-04 13:55:23 -05:00
Code Issues Activity

wallet: migration, avoid loading wallet after failure when it wasn't loaded before

Browse source 
During migration failure, only load wallet back into memory when the
wallet was loaded prior to migration. This fixes the case where BDB
is not supported, which implies that no legacy wallet can be loaded
into memory due to the lack of db writing functionality.

This commit also improves migration backup related comments to better
document the current workflow.

Co-authored-by: Ava Chow <github@achow101.com>
This commit is contained in:
furszy 2024-12-10 15:03:37 -05:00
parent 35000e34cf
commit 589ed1a8ea
No known key found for this signature in database
GPG key ID: 5DD23CCC686AA623
3 changed files with 40 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
src/wallet/wallet.cpp
View file

@ -490,7 +490,9 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
return wallet; return wallet;
} }
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings) // Re-creates wallet from the backup file by renaming and moving it into the wallet's directory.
// If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context.
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore)
{ {
DatabaseOptions options; DatabaseOptions options;
ReadDatabaseArgs(*context.args, options); ReadDatabaseArgs(*context.args, options);
@ -515,13 +517,17 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
fs::copy_file(backup_file, wallet_file, fs::copy_options::none); fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); if (load_after_restore) {
wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
}
} catch (const std::exception& e) { } catch (const std::exception& e) {
assert(!wallet); assert(!wallet);
if (!error.empty()) error += Untranslated("\n"); if (!error.empty()) error += Untranslated("\n");
error += Untranslated(strprintf("Unexpected exception: %s", e.what())); error += Untranslated(strprintf("Unexpected exception: %s", e.what()));
} }
if (!wallet) {
// Remove created wallet path only when loading fails
if (load_after_restore && !wallet) {
fs::remove_all(wallet_path); fs::remove_all(wallet_path);
} }
@ -4523,7 +4529,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
} }
if (!success) { if (!success) {
// Migration failed, cleanup // Migration failed, cleanup
// Copy the backup to the actual wallet dir // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir
fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
@ -4560,17 +4566,24 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
} }
// Restore the backup // Restore the backup
DatabaseStatus status; // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
std::vector<bilingual_str> warnings; // Reload it into memory if the wallet was previously loaded.
if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) { bilingual_str restore_error;
error += _("\nUnable to restore backup of wallet."); const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
if (!restore_error.empty()) {
error += restore_error + _("\nUnable to restore backup of wallet.");
return util::Error{error}; return util::Error{error};
} }
// Move the backup to the wallet dir // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir
fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none); fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
fs::remove(temp_backup_location); fs::remove(temp_backup_location);
// Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
// This check is performed after restoration to avoid an early error before saving the backup.
bool wallet_reloaded = ptr_wallet != nullptr;
assert(was_loaded == wallet_reloaded);
return util::Error{error}; return util::Error{error};
} }
return res; return res;

2
src/wallet/wallet.h
View file

@ -95,7 +95,7 @@ std::shared_ptr<CWallet> GetDefaultWallet(WalletContext& context, size_t& count)
std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name); std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings); std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings); std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings); std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore = true);
std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>& wallet); void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);

20
test/functional/wallet_migration.py
View file

@ -894,9 +894,7 @@ class WalletMigrationTest(BitcoinTestFramework):
shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed") shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed")
assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed") assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
assert "failed" in self.master_node.listwallets() assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"])
assert "failed_watchonly" not in self.master_node.listwallets()
assert "failed_solvables" not in self.master_node.listwallets()
assert not (self.master_node.wallets_path / "failed_watchonly").exists() assert not (self.master_node.wallets_path / "failed_watchonly").exists()
# Since the file in failed_solvables is one that we put there, migration shouldn't touch it # Since the file in failed_solvables is one that we put there, migration shouldn't touch it
@ -910,6 +908,22 @@ class WalletMigrationTest(BitcoinTestFramework):
_, _, magic = struct.unpack("QII", data) _, _, magic = struct.unpack("QII", data)
assert_equal(magic, BTREE_MAGIC) assert_equal(magic, BTREE_MAGIC)
####################################################
# Perform the same test with a loaded legacy wallet.
# The wallet should remain loaded after the failure.
#
# This applies only when BDB is enabled, as the user
# cannot interact with the legacy wallet database
# without BDB support.
if self.is_bdb_compiled() is not None:
# Advance time to generate a different backup name
self.master_node.setmocktime(self.master_node.getblockheader(self.master_node.getbestblockhash())['time'] + 100)
assert "failed" not in self.master_node.listwallets()
self.master_node.loadwallet("failed")
assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
wallets = self.master_node.listwallets()
assert "failed" in wallets and all(wallet not in wallets for wallet in ["failed_watchonly", "failed_solvables"])
def test_blank(self): def test_blank(self):
self.log.info("Test that a blank wallet is migrated") self.log.info("Test that a blank wallet is migrated")
wallet = self.create_legacy_wallet("blank", blank=True) wallet = self.create_legacy_wallet("blank", blank=True)