wallet: Fix relative path backup during migration.

davidgumberg commented at 3:32 am on April 15, 2025: contributor

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’. I suspect this may not be the only bug like this, since everywhere that a wallet’s name appears in code, that name might be a path, and that path may contain relative specifiers, and the consequences of that are not always obvious, and test coverage for wallets with relative paths seems like it could be improved.

This PR fixes an issue with wallet backup during migration where the wallet’s ’name’ 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:

0    fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
1    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.

The solution implemented here is to use the wallet directory’s name rather than the wallet_name when constructing the backup filename:

0    std::string legacy_wallet_dir_name = fs::PathToString(legacy_wallet_dir.filename());
1    fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : legacy_wallet_dir_name), GetTime()));

An alternative solution would have been to escape the path characters in the wallet’s name, but that seemed both more difficult to implement correctly, and like it might cause confusion for users.

This does not cause an issue for wallets with absolute pathnames because std::filesystem::operator/(path lhs, path rhs) returns rhs if rhs.is_absolute(). (https://en.cppreference.com/w/cpp/filesystem/path/append)

Steps to reproduce on master

Build and run bitcoind with legacy wallet creation enabled:

0$ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
1$ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb

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

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

Try to migrate the wallet:

0$ ./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.

DrahtBot commented at 3:32 am on April 15, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32273.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	shahsb
Stale ACK	ryanofsky, pseudoramdom

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Apr 15, 2025

in src/wallet/wallet.cpp:4504 in db3974ee11 outdated

4499@@ -4500,9 +4500,11 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4500     }
4501 
4502     // Make a backup of the DB
4503-    fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path();
4504-    fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
4505-    fs::path backup_path = this_wallet_dir / backup_filename;
4506+    fs::path legacy_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path();
4507+    // Use the wallet directory's name, rather than the wallet name, to avoid issues with wallet's whose names contain relative file paths.

ryanofsky commented at 1:27 pm on April 15, 2025:

In commit “wallet: Fix relative path backup during migration.” (db3974ee11b50c5cdccd277aa6d5cce660032718)

This comment doesn’t seem right. A relative file path like “foo” would not cause issues here. The problems happen with paths that contains slashes, which is why the line below is calling filename() to drop everything before the last slash.

Would suggest a different comment that explains what this is trying to do. Maybe: “Create a backup copy of wallet data inside the wallet directory with a unique and descriptive filename, based on the name of the wallet and the current timestamp. If this wallet is not the default wallet, the last component of the wallet’s directory name is used as the backup filename since the full wallet name may contain slashes.”

davidgumberg commented at 5:59 pm on April 15, 2025:

This comment doesn’t seem right.

This was wrong, thanks! I’ve added a variation of the comment you suggested that I edited for brevity. I think it should be more correct now.

0    // Make a backup of the DB in the wallet's directory with a unique filename using the wallet
1    // name and current timestamp. If not the default wallet, the name of the folder containing
2    // wallet data is used as the wallet's name, since its full name may contain slashes.

ryanofsky approved

ryanofsky commented at 1:31 pm on April 15, 2025: contributor

Code review ACK 1daaeedfafa0ee480a10756fa2a5cb25a268a3fa. Nice find and fix!

I suspect this may not be the only bug like this, since everywhere that a wallet’s name appears in code, that name might be a path

Wallet paths are meant to be passed to the database driver once during opening and not manipulated or interpreted after that. Backup and migration code are an exception because they do interpret the paths after they are opened, so that code needs to be written in a consistent way, but it should be a small amount of code.

davidgumberg force-pushed on Apr 15, 2025

wallet: Fix relative path backup during migration. 47174476c0

davidgumberg force-pushed on Apr 15, 2025

ryanofsky approved

ryanofsky commented at 3:34 pm on April 16, 2025: contributor

Code review ACK 7343538aa35101815f2823d1f102bc990abc1232, just changing a comment since the review

re: #32273#issue-2994969229

This does not cause an issue for wallets with absolute pathnames

IIUC, this is mostly true, but not totally. If intention for previous code was to place the wallet backup file inside the wallet directory, it looks like that was not previously happening, and that was also a bug which is now fixed.

Also IIUC, there wasn’t an issue with relative paths generally like “a/b/c”, only an issue with relative paths containing .. pointing outside of the wallets directory? Might be good idea to change title to something like “Fix external wallet backups during migration”. But title also seems ok currently since the detailed description is pretty clear.

achow101 commented at 6:23 pm on April 16, 2025: member

Can you add a test for for failing to migrate a wallet specified by relative path? It’s possible that restoration won’t work as we expect it to.

test: Migrating wallet with `../` in path. c6d439df6f

davidgumberg force-pushed on Apr 16, 2025

DrahtBot added the label CI failed on Apr 16, 2025

DrahtBot commented at 11:55 pm on April 16, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40692364945

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

davidgumberg commented at 0:04 am on April 17, 2025: contributor

Can you add a test for for failing to migrate a wallet specified by relative path? It’s possible that restoration won’t work as we expect it to.

Added a test that the ‘automatic’ restoration from a backup after a failed migration works, and added a test that restoring a backup made from an external relative wallet works.

Also IIUC, there wasn’t an issue with relative paths generally like “a/b/c”, only an issue with relative paths containing .. pointing outside of the wallets directory?

I thought so too, but checked after reading your comment and this is also broken on master:

0./build/bin/bitcoin-cli -regtest -named createwallet wallet_name="a/b/c/d" descriptors=false
1./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="a/b/c/d"

results in:

0error copying /home/user/.bitcoin/regtest/wallets/a/b/c/d/wallet.dat to
1/home/user/.bitcoin/regtest/wallets/a/b/c/d/a/b/c/d_1744848074.legacy.bak

pseudoramdom commented at 2:28 am on April 17, 2025: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/32273/commits/db4ea2632f0c1a5a5d6885526545cbddae7305d7

The patch works for both paths containing "../" and "a/b/c/d"

I thought so too, but checked after reading your comment and this is also broken on master:

+1. master does fail migrate for "a/b/c" style.

Migrating with .. style

02025-04-17T01:49:47Z copied /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelativewallet/wallet.dat to /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelativewallet/../../../amyrelativewallet_1744854587.legacy.bak

Migrating with a/b/c style

02025-04-17T02:12:59Z error copying /Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet/wallet.dat to /Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet/a/b/c/myrelativewallet_1744855979.legacy.bak - filesystem error: in copy_file: No such file or directory ["/Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet/a/b/c/myrelativewallet_1744855979.legacy.bak"] ["/Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet/wallet.dat"]

Migrating with .. style

02025-04-17T01:58:38Z copied /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelativewallet2/wallet.dat to /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelativewallet2/myrelativewallet2_1744855118.legacy.bak

Migrating with a/b/c style

02025-04-17T02:15:39Z copied /Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet2/wallet.dat to /Library/Application Support/Bitcoin/regtest/wallets/a/b/c/myrelativewallet2/myrelativewallet2_1744856139.legacy.bak

DrahtBot requested review from ryanofsky on Apr 17, 2025

test: Migration fail recovery w/ `../` in path 6b4b7ac211

davidgumberg force-pushed on Apr 17, 2025

in test/functional/wallet_migration.py:1034 in 6b4b7ac211

1029+        assert os.path.exists(solvables_path)
1030+        new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
1031+        assert_equal(original_shasum, new_shasum)
1032+
1033+        # Check the wallet we tried to migrate is still BDB
1034+        datfile = os.path.join(absolute_path, "wallet.dat")

davidgumberg commented at 6:32 am on April 17, 2025:

Writing as a review comment since I didn’t think a code comment was warranted here, the reason for putting this on a separate line even though it is only used once is to avoid the wrath of the slightly imprecise python-utf8-encoding lint check which otherwise won’t catch that the file is being opened in binary mode.

DrahtBot removed the label CI failed on Apr 17, 2025

in test/functional/wallet_migration.py:549 in 6b4b7ac211

544+        wallet_name = "relative"
545+        absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name))
546+        relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path)
547+
548+        wallet = self.create_legacy_wallet(relative_name)
549+        # listwalletdirs only returns wallets in the wallet directory

shahsb commented at 4:22 am on April 18, 2025:

You could reframe the comment to explain why this is significant:

0# Wallet created using relative path won't reside in the default wallet dir,
1# hence it should not appear in listwalletdir results.

in test/functional/wallet_migration.py:542 in 6b4b7ac211

537+
538+        # Get the nearest common path of both nodes' wallet paths.
539+        common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
540+
541+        # This test assumes that the relative path from each wallet directory to the common path is identical.
542+        assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))

shahsb commented at 4:24 am on April 18, 2025:

If this assumption ever fails (say due to environment setup differences), the test will break early. Consider failing gracefully with a more informative message:

0assert_equal(..., ..., "Relative paths from wallet dirs to common parent must be identical for this test.")

in test/functional/wallet_migration.py:592 in 6b4b7ac211

587+        assert {"name": backup_filename} not in walletdirlist["wallets"]
588+
589+        # Check that old node can restore from the backup.
590+        self.old_node.restorewallet("relative_restored", migrate_res['backup_path'])
591+        wallet = self.old_node.get_wallet_rpc("relative_restored")
592+        assert wallet.gettransaction(txid)

shahsb commented at 4:31 am on April 18, 2025:

Can we add more stricter checks on the transaction received here.

For example:

0tx = wallet.gettransaction(txid)
1assert_equal(tx["amount"], 1)
2assert_equal(tx["confirmations"], 1)

This gives you tighter guarantees and helps with debugging if the test fails.

in test/functional/wallet_migration.py:1027 in 6b4b7ac211

1022+        self.old_node.unloadwallet(relative_name)
1023+        assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, relative_name)
1024+
1025+        assert all(wallet not in self.master_node.listwallets() for wallet in [f"{wallet_name}", f"{wallet_name}_watchonly", f"{wallet_name}_solvables"])
1026+
1027+        assert not (self.master_node.wallets_path / "failed_watchonly").exists()

shahsb commented at 4:36 am on April 18, 2025:

Nit: May be a small comment with reason would help here:

0# No 'failed_watchonly' directory should exist because migration cleanup should be complete

in test/functional/wallet_migration.py:1000 in 6b4b7ac211

 995+
 996+        # Get the nearest common path of both nodes' wallet paths.
 997+        common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
 998+
 999+        # This test assumes that the relative path from each wallet directory to the common path is identical.
1000+        assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))

shahsb commented at 4:38 am on April 18, 2025:

Similar to previous test and all other places, can we fail gracefully with more informative message:

0assert_equal(..., ..., "Test requires that both wallet paths have the same relative path to the common parent")

in test/functional/wallet_migration.py:1035 in 6b4b7ac211

1030+        new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
1031+        assert_equal(original_shasum, new_shasum)
1032+
1033+        # Check the wallet we tried to migrate is still BDB
1034+        datfile = os.path.join(absolute_path, "wallet.dat")
1035+        with open(datfile, "rb") as f:

shahsb commented at 4:41 am on April 18, 2025:

We should first validate that file is present at the specified location and accessible and then only try to open and validate other things

shahsb commented at 4:43 am on April 18, 2025:

Minor / Nit: A separate helper function could be introduced to validate it’s BDB for better readability. Example:

0def is_bdb_wallet(filepath):
1    with open(filepath, "rb") as f:
2        data = f.read(16)
3        _, _, magic = struct.unpack("QII", data)
4        return magic == BTREE_MAGIC

and then:

0assert is_bdb_wallet(datfile)

shahsb commented at 4:44 am on April 18, 2025: none

Concept ACK

wallet: Fix relative path backup during migration. #32273

Steps to reproduce on master

Code Coverage & Benchmarks

Reviews

Conflicts