highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-08-24 14:01:23 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#32273: wallet: Fix relative path backup during migration.

Browse Source 
76fe0e59ec test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fe test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d36437 test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c90 wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6 test: wallet: Check direct file backup name. (David Gumberg)

Pull request description:

  Support for wallets outside of the default wallet directory was added in #11687, and these external wallets can be specified with paths relative to the wallet directory, e.g.  `bitcoin-cli loadwallet ../../mywallet`. In the RPC commands, there is no distinction between a wallet's 'name' and a wallet's 'path'. This PR fixes an issue with wallet backup during migration where the wallet's 'name-path' is used in the backup filename. This goes south when that filename is appended to the directory where we want to put the file and the wallet's 'name' actually gets treated as a path:

  ```cpp
      fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
      fs::path backup_path = this_wallet_dir / backup_filename;
  ```

  Attempting to migrate a wallet with the 'name' `../../../mywallet` results in a backup being placed in `datadir/wallets/../../../mywallet/../../../mywallet_1744683963.legacy.bak`.

  If permissions don't exist to write to that folder, migration can fail.

  The solution implemented here is to put backup files in the top-level of the node's `walletdir` directory, using the folder name (and in some rare cases the file name)  of the wallet to name the backup file:

  9fa5480fc4/src/wallet/wallet.cpp (L4254-L4268)

  ##### Steps to reproduce on master
  Build and run `bitcoind` with legacy wallet creation enabled:
  ```bash
  $ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
  $ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb
  ```

  Create a wallet with some relative path specifiers (exercise caution with where this file may be written)

  ```bash
  $ ./build/bin/bitcoin-cli -regtest -named createwallet wallet_name="../../../myrelativewallet" descriptors=false
  ```

  Try to migrate the wallet:
  ```bash
  $ ./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="../../../myrelativewallet"
  ```

  You will see a message in `debug.log` about trying to backup a file somewhere like: `/home/user/.bitcoin/regtest/wallets/../../../myrelativewallet/../../../myrelativewallet_1744686627.legacy.bak` and migration might fail because `bitcoind` doesn't have permissions to write the backup file.

ACKs for top commit:
  pablomartin4btc:
    tACK 76fe0e59ec
  achow101:
    ACK 76fe0e59ec
  ryanofsky:
    Code review ACK 76fe0e59ec. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.

Tree-SHA512: 5cf6ed9f44ac7d204e4e9854edd3fb9b43812e930f76343b142b3c19df3de2ae5ca1548d4a8d26226d537bca231e3a50b3ff0d963c200303fb761f2b4eb3f0d9
This commit is contained in:
Ava Chow
2025-07-29 11:15:59 -07:00
parent 6b99670e3c 76fe0e59ec
commit 91058877ff
2 changed files with 182 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
src/wallet/wallet.cpp
View File

@@ -4201,10 +4201,20 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
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.empty() ? "default_wallet" : wallet_name), GetTime()));
fs::path backup_path = this_wallet_dir / backup_filename;
// Make a backup of the DB in the wallet's directory with a unique filename
// using the wallet name and current timestamp. The backup filename is based
// on the name of the parent directory containing the wallet data in most
// cases, but in the case where the wallet name is a path to a data file,
// the name of the data file is used, and in the case where the wallet name
// is blank, "default_wallet" is used.
const std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
// fs::weakly_canonical resolves relative specifiers and remove trailing slashes.
const auto legacy_wallet_path = fs::weakly_canonical(GetWalletDir() / fs::PathFromString(wallet_name));
return fs::PathToString(legacy_wallet_path.filename());
}();
fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", backup_prefix, GetTime()));
fs::path backup_path = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) {
return util::Error{_("Error: Unable to make a backup of your wallet")};
}
@@ -4286,11 +4296,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
}
if (!success) {
// Migration failed, cleanup
// 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::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
// Make list of wallets to cleanup
std::vector<std::shared_ptr<CWallet>> created_wallets;
if (local_wallet) created_wallets.push_back(std::move(local_wallet));
@@ -4326,7 +4331,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Restore the backup
// Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
bilingual_str restore_error;
const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false);
const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/false);
if (!restore_error.empty()) {
error += restore_error + _("\nUnable to restore backup of wallet.");
return util::Error{error};
@@ -4334,10 +4339,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Verify that the legacy wallet is not loaded after restoring from the backup.
assert(!ptr_wallet);
// 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::remove(temp_backup_location);
return util::Error{error};
}
return res;

172
test/functional/wallet_migration.py
View File

@@ -118,23 +118,39 @@ class WalletMigrationTest(BitcoinTestFramework):
if wallet_name == "":
shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat")
else:
shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name)
src = os.path.abspath(self.old_node.wallets_path / wallet_name)
dst = os.path.abspath(self.master_node.wallets_path / wallet_name)
if src != dst :
shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name, dirs_exist_ok=True)
# Check that the wallet shows up in listwalletdir with a warning about migration
wallets = self.master_node.listwalletdir()
for w in wallets["wallets"]:
if w["name"] == wallet_name:
assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
# Mock time so that we can check the backup filename.
mocked_time = int(time.time())
self.master_node.setmocktime(mocked_time)
# Migrate, checking that rescan does not occur
with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
self.master_node.setmocktime(0)
# Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
# (in which case the wallet name would be suffixed by the 'watchonly' term)
wallet_name = migrate_info['wallet_name']
wallet = self.master_node.get_wallet_rpc(wallet_name)
migrated_wallet_name = migrate_info['wallet_name']
wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
assert_equal(wallet.getwalletinfo()["descriptors"], True)
self.assert_is_sqlite(wallet_name)
self.assert_is_sqlite(migrated_wallet_name)
# Always verify the backup path exist after migration
assert os.path.exists(migrate_info['backup_path'])
if wallet_name == "":
backup_prefix = "default_wallet"
else:
backup_prefix = os.path.basename(os.path.realpath(self.old_node.wallets_path / wallet_name))
expected_backup_path = self.master_node.wallets_path / f"{backup_prefix}_{mocked_time}.legacy.bak"
assert_equal(str(expected_backup_path), migrate_info['backup_path'])
return migrate_info, wallet
def test_basic(self):
@@ -553,6 +569,98 @@ class WalletMigrationTest(BitcoinTestFramework):
assert_equal(bals, wallet.getbalances())
def test_wallet_with_relative_path(self):
self.log.info("Test migration of a wallet that isn't loaded, specified by a relative path")
# Get the nearest common path of both nodes' wallet paths.
common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
# This test assumes that the relative path from each wallet directory to the common path is identical.
assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))
wallet_name = "relative"
absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name))
relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path)
wallet = self.create_legacy_wallet(relative_name)
# listwalletdirs only returns wallets in the wallet directory
assert {"name": relative_name} not in wallet.listwalletdir()["wallets"]
assert relative_name in wallet.listwallets()
default = self.master_node.get_wallet_rpc(self.default_wallet_name)
addr = wallet.getnewaddress()
txid = default.sendtoaddress(addr, 1)
self.generate(self.master_node, 1)
bals = wallet.getbalances()
# migratewallet uses current time in naming the backup file, set a mock time
# to check that this works correctly.
curr_time = int(time.time())
self.master_node.setmocktime(curr_time)
migrate_res, wallet = self.migrate_and_get_rpc(relative_name)
self.master_node.setmocktime(0)
# Check that the wallet was migrated, knows the right txid, and has the right balance.
assert wallet.gettransaction(txid)
assert_equal(bals, wallet.getbalances())
# The migrated wallet should not be in the wallet dir, but should be in the list of wallets.
info = wallet.getwalletinfo()
walletdirlist = wallet.listwalletdir()
assert {"name": info["walletname"]} not in walletdirlist["wallets"]
walletlist = wallet.listwallets()
assert info["walletname"] in walletlist
# Check that old node can restore from the backup.
self.old_node.restorewallet("relative_restored", migrate_res['backup_path'])
wallet = self.old_node.get_wallet_rpc("relative_restored")
assert wallet.gettransaction(txid)
assert_equal(bals, wallet.getbalances())
info = wallet.getwalletinfo()
assert_equal(info["descriptors"], False)
assert_equal(info["format"], "bdb")
def test_wallet_with_path_ending_in_slash(self):
self.log.info("Test migrating a wallet with a name/path ending in '/'")
# The last directory in the wallet's path
final_dir = "mywallet"
wallet_name = f"path/to/{final_dir}/"
wallet = self.create_legacy_wallet(wallet_name)
default = self.master_node.get_wallet_rpc(self.default_wallet_name)
addr = wallet.getnewaddress()
txid = default.sendtoaddress(addr, 1)
self.generate(self.master_node, 1)
bals = wallet.getbalances()
_, wallet = self.migrate_and_get_rpc(wallet_name)
assert wallet.gettransaction(txid)
assert_equal(bals, wallet.getbalances())
def test_wallet_with_path_ending_in_relative_specifier(self):
self.log.info("Test migrating a wallet with a name/path ending in a relative specifier, '..'")
wallet_ending_in_relative = "path/that/ends/in/.."
# the wallet data is actually inside of path/that/ends/
wallet = self.create_legacy_wallet(wallet_ending_in_relative)
default = self.master_node.get_wallet_rpc(self.default_wallet_name)
addr = wallet.getnewaddress()
txid = default.sendtoaddress(addr, 1)
self.generate(self.master_node, 1)
bals = wallet.getbalances()
_, wallet = self.migrate_and_get_rpc(wallet_ending_in_relative)
assert wallet.gettransaction(txid)
assert_equal(bals, wallet.getbalances())
def test_default_wallet(self):
self.log.info("Test migration of the wallet named as the empty string")
wallet = self.create_legacy_wallet("")
@@ -590,7 +698,10 @@ class WalletMigrationTest(BitcoinTestFramework):
)
assert (self.master_node.wallets_path / "plainfile").is_file()
self.master_node.migratewallet("plainfile")
mocked_time = int(time.time())
self.master_node.setmocktime(mocked_time)
migrate_res = self.master_node.migratewallet("plainfile")
assert_equal(f"plainfile_{mocked_time}.legacy.bak", os.path.basename(migrate_res["backup_path"]))
wallet = self.master_node.get_wallet_rpc("plainfile")
info = wallet.getwalletinfo()
assert_equal(info["descriptors"], True)
@@ -924,6 +1035,53 @@ class WalletMigrationTest(BitcoinTestFramework):
# Check the wallet we tried to migrate is still BDB
self.assert_is_bdb("failed")
def test_failed_migration_cleanup_relative_path(self):
self.log.info("Test that a failed migration with a relative path is cleaned up")
# Get the nearest common path of both nodes' wallet paths.
common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
# This test assumes that the relative path from each wallet directory to the common path is identical.
assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))
wallet_name = "relativefailure"
absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name))
relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path)
wallet = self.create_legacy_wallet(relative_name)
# Make a copy of the wallet with the solvables wallet name so that we are unable
# to create the solvables wallet when migrating, thus failing to migrate
wallet.unloadwallet()
solvables_path = os.path.join(common_parent, f"{wallet_name}_solvables")
shutil.copytree(self.old_node.wallets_path / relative_name, solvables_path)
original_shasum = sha256sum_file(os.path.join(solvables_path, "wallet.dat"))
self.old_node.loadwallet(relative_name)
# Add a multisig so that a solvables wallet is created
wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey])
wallet.importaddress(get_generate_key().p2pkh_addr)
self.old_node.unloadwallet(relative_name)
assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, relative_name)
assert all(wallet not in self.master_node.listwallets() for wallet in [f"{wallet_name}", f"{wallet_name}_watchonly", f"{wallet_name}_solvables"])
assert not (self.master_node.wallets_path / f"{wallet_name}_watchonly").exists()
# Since the file in failed_solvables is one that we put there, migration shouldn't touch it
assert os.path.exists(solvables_path)
new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
assert_equal(original_shasum, new_shasum)
# Check the wallet we tried to migrate is still BDB
datfile = os.path.join(absolute_path, "wallet.dat")
with open(datfile, "rb") as f:
data = f.read(16)
_, _, magic = struct.unpack("QII", data)
assert_equal(magic, BTREE_MAGIC)
def test_blank(self):
self.log.info("Test that a blank wallet is migrated")
wallet = self.create_legacy_wallet("blank", blank=True)
@@ -1411,6 +1569,9 @@ class WalletMigrationTest(BitcoinTestFramework):
self.test_encrypted()
self.test_nonexistent()
self.test_unloaded_by_path()
self.test_wallet_with_relative_path()
self.test_wallet_with_path_ending_in_slash()
self.test_wallet_with_path_ending_in_relative_specifier()
self.test_default_wallet()
self.test_direct_file()
self.test_addressbook()
@@ -1418,6 +1579,7 @@ class WalletMigrationTest(BitcoinTestFramework):
self.test_conflict_txs()
self.test_hybrid_pubkey()
self.test_failed_migration_cleanup()
self.test_failed_migration_cleanup_relative_path()
self.test_avoidreuse()
self.test_preserve_tx_extra_info()
self.test_blank()