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’. 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:

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.

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

The solution implemented here is to reuse the path parsing logic used during wallet loading in CWallet::GetWalletPath():

https://github.com/bitcoin/bitcoin/blob/fad80d14175ba3a8568ee074ac7e9a1a709db173/src/wallet/wallet.cpp#L4234-L4243

And to split up the handling of direct file wallets and normal wallets:

https://github.com/bitcoin/bitcoin/blob/fad80d14175ba3a8568ee074ac7e9a1a709db173/src/wallet/wallet.cpp#L4245-L4261

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()));

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	pseudoramdom, ryanofsky, pablomartin4btc

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:

#31423 (wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet by furszy)

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

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.

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

davidgumberg force-pushed on Apr 17, 2025

in test/functional/wallet_migration.py:1085 in 6b4b7ac211 outdated

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:615 in 6b4b7ac211 outdated

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.

davidgumberg commented at 7:56 pm on April 23, 2025:

Rather than explaining the line below, I added this comment to add context that I think isn’t immediately available from the test, namely what the (obscure in both use and name) listwalletdir rpc does, I think your suggested comment would repeat what the test is asserting.

in test/functional/wallet_migration.py:608 in 6b4b7ac211 outdated

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.")

davidgumberg commented at 8:16 pm on April 23, 2025:

Unlike, assert_not_equal, assert_equal does not support passing an error message. Re: the conceptual change; while it might have some advantages over the style currently used, as far as I can tell it is almost never the case that we log the “big picture” of what went wrong in a functional test, rather than details about the exact comparison or check that failed.

in test/functional/wallet_migration.py:658 in 6b4b7ac211 outdated

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.

davidgumberg commented at 8:36 pm on April 23, 2025:

We are already asserting the transaction id exists in the migrated wallet, and that the wallet has the correct balance, I’m not totally opposed to checking the tx amount as opposed to the wallet balance, but I’m not sure that this change really does add any guarantees, or help with debugging when the test fails, so I will err on the side of keeping this as-is so it parallels test_unloaded_by_path above, although after testing, I realized that the assert I had here does not do anything, since if gettransaction fails, it results in a JSONRPCException.

in test/functional/wallet_migration.py:1078 in 6b4b7ac211 outdated

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:1051 in 6b4b7ac211 outdated

 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")

davidgumberg commented at 8:38 pm on April 23, 2025:

See #32273 (review).

in test/functional/wallet_migration.py:1086 in 6b4b7ac211 outdated

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)

davidgumberg commented at 8:42 pm on April 23, 2025:

If open fails an exception will be thrown and the block below it will not be executed.

https://docs.python.org/3/reference/compound_stmts.html#with

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

Concept ACK

ryanofsky approved

ryanofsky commented at 4:48 pm on April 21, 2025: contributor

Code review ACK 6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97. Main change is expansion of test to cover restoring from the backup.

IIUC, PR description seems helpful for describing part of the problem, but contains inaccuracies and would be good to revise to avoid being misleading. Here are things I might add or correct:

PR describes there being a problems with relative paths, but not all relative paths have a problem, and absolute paths have problems too. Things only currently work correctly when wallet names contains no slashes (e.g. mywallet). Whenever a wallet name has slashes, there are bugs which this PR fixes (the bugs are different depending on whether the path is absolute or relative).
If wallet name is relative and has slashes like my/wallet there is a bug currently because migrate code tries to write a backup file inside the wallet directory at a path like my/wallet/my/wallet_123.bak and this fails when the backup file can’t be written at a nonexistent subdirectory. This PR fixes that problem by dropping extra path components so the backup is saved at my/wallet/wallet_123.bak
If wallet name is absolute and has slashes like /my/wallet there is a bug currently because instead of the backup being written inside the wallet directory at a location like /my/wallet/wallet_123.bak the backup is written outside the wallet directory, next to it, at a location like /my/wallet_123.bak. The PR fixes that problem by always writing the backup file inside the wallet directory.
If wallet name is relative and contains .. there can be a combination of both problems: an attempt to write the backup file outside the wallet directory can happen, and the write can fail because the destination directory probably won’t exist.

Also, I think I’d suggest dropping “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.” because it increases vagueness and uncertainty in the PR description and I don’t actually think it is true. Outside of backup and restore code wallet names are treated like identifiers, not like paths, so I don’t think we should expect to see problems outside of backup & restore code.

Overall changes look good though. Just trying to reconcile my understanding of the problem with the way it’s described in the PR.

DrahtBot requested review from shahsb on Apr 21, 2025

achow101 commented at 9:30 pm on April 21, 2025: member

I think there’s one more wrinkle to this issue: wallets outside of the wallets directory are probably stored where the wallet file is the name of the wallet, rather than the directory being the name, and containing a wallet.dat. An example of something similar to this is test_direct_file test case, although in that case the file is still in the wallets directory. In fact, I think this PR actually “breaks” that test case as the backup file is no longer named as we expect it (although the test does not check that, but it probably should).

I believe this PR would be incorrect for such wallets since it uses the parent directory’s name for the backup, rather than the wallet file’s name. For such wallets, the name of the parent directory would probably be something unrelated to the wallet file’s name. If there are multiple wallet files in the same directory, this would also be problematic.

luke-jr changes_requested

luke-jr commented at 10:05 pm on April 21, 2025: member

Won’t this turn /home/user/arbitrarywalletname.db into /home/user/user_%d.legacy.bak ? Seems incorrect?

ryanofsky commented at 11:21 pm on April 21, 2025: contributor

I was under impression that the only wallet paths that could be loaded are:

https://github.com/bitcoin/bitcoin/blob/06439a14c884d7f81f331646ad361e88b3037a51/src/wallet/wallet.cpp#L2974-L2977

So it would not be possible to load a bare db outside -walletdir. And if the migration code was backing up a bare db file inside -walletdir like arbitrarywalletname.db it would just be backed up as arbitrarywalletname.db_1223.bak

achow101 commented at 0:44 am on April 22, 2025: member

So it would not be possible to load a bare db outside -walletdir.

Good point

And if the migration code was backing up a bare db file inside -walletdir like arbitrarywalletname.db it would just be backed up as arbitrarywalletname.db_1223.bak

This PR currently breaks that. Although test_direct_file doesn’t currently fail, adding a check for the wallet’s name in the backup path does make it fail, e.g.

 0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
 1index c467d6ad36d..90088db34e7 100755
 2--- a/test/functional/wallet_migration.py
 3+++ b/test/functional/wallet_migration.py
 4@@ -568,7 +568,8 @@ class WalletMigrationTest(BitcoinTestFramework):
 5         )
 6         assert (self.master_node.wallets_path / "plainfile").is_file()
 7 
 8-        self.master_node.migratewallet("plainfile")
 9+        migrate_res = self.master_node.migratewallet("plainfile")
10+        assert "plainfile" in os.path.basename(migrate_res["backup_path"])
11         wallet = self.master_node.get_wallet_rpc("plainfile")
12         info = wallet.getwalletinfo()
13         assert_equal(info["descriptors"], True)

ryanofsky commented at 1:19 pm on April 22, 2025: contributor

Although test_direct_file doesn’t currently fail, adding a check for the wallet’s name in the backup path does make it fail, e.g.

That’s a good point. Instead of backing up plainfile as plainfile_123.legacy.bak 47174476c04a7f1138fbd32eb0c9c38e85946393 backs it up as wallets_123.legacy.bak because it assumes the name of the parent directory is the name of the wallet. Following would seem to be a good fix:

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -4499,13 +4499,14 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 3         return util::Error{_("Error: This wallet is already a descriptor wallet")};
 4     }
 5 
 6-    // Make a backup of the DB in the wallet's directory with a unique filename using the wallet
 7-    // name and current timestamp. If not the default wallet, the name of the folder containing
 8-    // wallet data is used as the wallet's name, since its full name may contain slashes.
 9-    fs::path legacy_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path();
10-    std::string legacy_wallet_dir_name = fs::PathToString(legacy_wallet_dir.filename());
11-    fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : legacy_wallet_dir_name), GetTime()));
12-    fs::path backup_path = legacy_wallet_dir / backup_filename;
13+    // Make a backup of the DB in the wallet's directory with a unique filename
14+    // using the wallet name and current timestamp. The backup filename is based
15+    // on walletname but expanded to "default_wallet" if it is empty and only
16+    // the final filename portion is used if contains slashes.
17+    fs::path legacy_wallet_dir = fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path();
18+    std::string backup_prefix = fs::PathToString(fs::PathFromString(wallet_name).filename());
19+    fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (backup_prefix.empty() ? "default_wallet" : backup_prefix), GetTime()));
20+    fs::path backup_path = fsbridge::AbsPathJoin(legacy_wallet_dir, backup_filename);
21     if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) {
22         if (was_loaded) {
23             reload_wallet(local_wallet);

It also drops the incorrect and unnecessary fs::absolute call since CWD should be irrelevant here.

DrahtBot added the label Needs rebase on Apr 25, 2025

pablomartin4btc commented at 11:15 pm on June 26, 2025: member

tACK 6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97

I was able to reproduce the issue on master. As a detail, the CLI outputs “Unable to make a backup of your wallet” when the backup fails—might be worth mentioning that in the PR description.

This branch fixes the problem as expected (checked both relatives .../../ and a/b/c).

Note that legacy wallet creation is no longer possible on master, so it might be helpful to update the reproduction steps. One option could be to use a previous release (e.g., v28.0 - ./releases/v28.0/bin/bitcoind) via previously ./test/get_previous_releases.py -b.

https://github.com/bitcoin/bitcoin/commit/47174476c04a7f1138fbd32eb0c9c38e85946393 backs it up as wallets_123.legacy.bak

I’ve verified that the above needs to be fixed for plainfile wallets migration.

in test/functional/wallet_migration.py:1091 in 6b4b7ac211 outdated

1036+            data = f.read(16)
1037+            _, _, magic = struct.unpack("QII", data)
1038+            assert_equal(magic, BTREE_MAGIC)
1039+
1040+
1041     def test_blank(self):

pablomartin4btc commented at 7:18 pm on June 30, 2025:

nit: extra line here?

0            assert_equal(magic, BTREE_MAGIC)
1
2    def test_blank(self):

davidgumberg commented at 0:11 am on July 1, 2025:

Fixed, thanks.

davidgumberg force-pushed on Jul 1, 2025

DrahtBot removed the label Needs rebase on Jul 1, 2025

DrahtBot added the label CI failed on Jul 1, 2025

DrahtBot commented at 1:08 am on July 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/45091596520 LLM reason (✨ experimental): The CI failure is caused by an assertion failure related to fs::path in the bench_sanity_check test, leading to a subprocess abort.

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 force-pushed on Jul 1, 2025

DrahtBot removed the label CI failed on Jul 1, 2025

test: wallet: Check direct file backup name.

This check ensures that when migrating a legacy wallet with a direct
filename, the backup file is named as expected.

Co-authored-by: Ava Chow <github@achow101.com>

9a84710f20

wallet: Fix migration of wallets with pathnames.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

15583b8c35

test: Migrating wallet with `../` in path. 553507f086

test: Migration fail recovery w/ `../` in path ab7325cccb

test: Migration of a wallet ending in `/` e48bf2d845

test: Wallet name ending in relative specifier. fad80d1417

davidgumberg force-pushed on Jul 1, 2025

davidgumberg commented at 7:17 am on July 1, 2025: contributor

Won’t this turn /home/user/arbitrarywalletname.db into /home/user/user_%d.legacy.bak ? Seems incorrect?

This PR currently breaks that. Although test_direct_file doesn’t currently fail, adding a check for the wallet’s name in the backup path does make it fail, e.g.

[…] @luke-jr @achow101 Thanks for catching this, I’ve added the test case @achow101 suggested for this in 5cd3e2794d78848e7040e.

@ryanofsky

Following would seem to be a good fix:

[…]

Thanks, I’ve used parts of this suggestion but with some changes:

backup_prefix = wallet_name.filename() does not work when the wallet name ends in relative specifiers, since “..” will be the backup_prefix, (test case: fad80d14175ba3a8568ee074ac7e9a1a709db173). It also doesn’t work when the wallet name ends in a /, since filename() will return "" in that case. (https://en.cppreference.com/w/cpp/filesystem/path/filename) (test: https://github.com/bitcoin/bitcoin/commit/e48bf2d84502e0c61558eac5fd02d0a7fcbc5065)
Absolute paths are needed here since what WalletDatabase::Backup() does with the path you pass it is an implementation detail, e.g. in the case of SQLiteDatabase::Backup(), the path gets passed off to the sqlite api.

There are also some subtle and likely unimportant edge cases that were not being handled during migration, e.g. disallowing the use of symbolic links to direct files in migrations, so the solution I’ve used is to reuse the path parsing logic used during wallet loading in CWallet::GetWalletPath():

https://github.com/bitcoin/bitcoin/blob/fad80d14175ba3a8568ee074ac7e9a1a709db173/src/wallet/wallet.cpp#L4234-L4243

And to split up the handling of direct file wallets and normal wallets:

https://github.com/bitcoin/bitcoin/blob/fad80d14175ba3a8568ee074ac7e9a1a709db173/src/wallet/wallet.cpp#L4245-L4261

ryanofsky commented at 2:12 pm on July 1, 2025: contributor

Thanks, I’ve used parts of this suggestion but with some changes

Thanks for following up, and it’s a good observation that calling filename() on an fs::path like I suggested doesn’t always always return a usable file prefix, since it can return odd fragments like “..” and doesn’t handle trailing slashes.

But I feel like it would still be straightforward to address the problem of turning a wallet name into a safe prefix by making a replacement like:

0-    std::string backup_prefix = fs::PathToString(fs::PathFromString(wallet_name).filename());
1+    std::string backup_prefix = [&] {
2+        // Normalize any .. components
3+        fs::path p{fs::PathFromString(wallet_name).lexically_normal()};
4+        // Drop trailing slash if any.
5+        if (p.filename().empty()) p = p.parent_path();
6+        // Return last path component
7+        return fs::PathToString(p.filename());
8+    }();

More generally I have some concerns about the implementation in 15583b8c35014b59f4f1ea21522a1509b30cf221:

I feel like it doesn’t make sense to store backups of a wallet inside the wallet being backed up. The approach I suggested of just backing up all wallets as {prefix}_{timestamp}.legacy.bak files in the top-level wallets directory seems like it should make the backup files easier to find, identify, and clean up.

By contrast the approach in 15583b8c35014b59f4f1ea21522a1509b30cf221 seems more complicated and inconsistent, creating backup files for file wallets as suggested, but creating backups for directory wallets by putting the backup files inside the wallet directories. I think this is not good because it is inconsistent, and will make the backup files harder to find, and it moves away from the concept that 1 directory = 1 wallet to one where 1 directory = (1 wallet + backups of itself).
Am also concerned that the implementation in 15583b8c35014b59f4f1ea21522a1509b30cf221 is fragile, accessing the filesystem with symlink_status and weakly_canonical where the should be no need to access the filesystem and the only job that actually needs to be done is turning the wallet name into a prefix string. Accessing the filesystem makes things more complicated and unpredictable (even introducing a util::Error case) than I think is ideal.

Update: Should note these are just initial thoughts and not a full review. 15583b8c35014b59f4f1ea21522a1509b30cf221 is just different than what I was expecting and may have advantages I’m not seeing or haven’t thought of.

achow101 commented at 8:32 pm on July 1, 2025: member

The approach I suggested of just backing up all wallets as {prefix}_{timestamp}.legacy.bak files in the top-level wallets directory seems like it should make the backup files easier to find, identify, and clean up.

That seems reasonable to me.

wallet: Fix relative path backup during migration. #32273

Steps to reproduce on master

Code Coverage & Benchmarks

Reviews

Conflicts