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

https://github.com/bitcoin/bitcoin/blob/9fa5480fc4c46fa11345c45fbe4dd21c127e3e05/src/wallet/wallet.cpp#L4254-L4268

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
ACK	pablomartin4btc
Concept ACK	shahsb
Stale ACK	pseudoramdom, ryanofsky

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:

#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33031 (wallet: Set descriptor cache upgraded flag for migrated wallets 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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

remove -> removes [subject “fs::weakly_canonical” is singular, so the verb should be “removes”]
exist -> exists [“the backup path” is singular, so it “exists”]

drahtbot_id_4_m

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:1076 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:580 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:573 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:613 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:1072 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:1042 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:1077 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:1082 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

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.

DrahtBot added the label Needs rebase on Jul 2, 2025

davidgumberg force-pushed on Jul 2, 2025

davidgumberg commented at 10:10 pm on July 2, 2025: contributor

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.

That seems reasonable to me.

Agreed, I’ve added https://github.com/bitcoin/bitcoin/pull/32273/commits/75df71398dc4feefaa6f6ee611e41c5daa8ebc2b which moves the backup from inside the wallet directory to being in the node’s -walletdir.

Am also concerned that the implementation in https://github.com/bitcoin/bitcoin/commit/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.

Unfortunately, I think knowing the name of the wallet’s parent folder requires filesystem access when arbitrary relative paths are permitted. lexically_normal() won’t work in test cases like 8e20619341b465e30bdbfc22e6be515fb2ae2e6f where the wallet is specified by a relative path that ascends more than it descends, or e.g. the wallet named ../.

Accessing the filesystem makes things more complicated and unpredictable (even introducing a util::Error case) than I think is ideal.

I think using GetWalletPath() and checking the Util::error case here is just a bit bit of belt and suspenders to ensure no wallet names that are not permitted elsewhere are permitted in migration, but in practice, bad names get caught earlier on in migration when the legacy wallet is loaded, so this is currently unreachable.

This code could just as well be:

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 6c94bd807c..ac1b37d262 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -4257,14 +4257,11 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 5     // cases, but in the case where the wallet name is a path to a data file,
 6     // the name of the data file is used, and in the case where the wallet name
 7     // is blank, "default_wallet" is used.
 8-    auto legacy_wallet_path = GetWalletPath(wallet_name);
 9-    if (!legacy_wallet_path) {
10-        return util::Error{util::ErrorString(legacy_wallet_path)};
11-    }
12+    const fs::path legacy_wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_name));
13
14     // Backwards-compatibility logic for migrating wallets whose names are
15     // paths to data files directly rather than directories.
16-    bool is_direct_file = fs::symlink_status(*legacy_wallet_path).type() == fs::file_type::regular;
17+    bool is_direct_file = fs::symlink_status(legacy_wallet_path).type() == fs::file_type::regular;

This is verbatim what GetWalletPath() does, minus handing out errors for disallowed paths, which I felt off about omitting, but in practice, it’s a no-op today because of the earlier checks.

davidgumberg force-pushed on Jul 2, 2025

davidgumberg commented at 11:04 pm on July 2, 2025: contributor

Rebased because of spurious CI failure fixed by #32859.

DrahtBot removed the label Needs rebase on Jul 2, 2025

in test/functional/wallet_migration.py:154 in c38297c741 outdated

147@@ -148,7 +148,10 @@ def test_basic(self):
148         assert_equal(old_change_addr_info["hdkeypath"], "m/0'/1'/0'")
149 
150         # Note: migration could take a while.
151-        _, basic0 = self.migrate_and_get_rpc("basic0")
152+        res0, basic0 = self.migrate_and_get_rpc("basic0")
153+        backup_path = self.master_node.wallets_path / f"basic0_{int(time.time())}.legacy.bak"
154+        assert backup_path.exists()
155+        assert_equal(str(backup_path), res0['backup_path'])

furszy commented at 0:16 am on July 3, 2025:

nit: we are already doing the backup file existence check at the end of migrate_and_get_rpc(), what if you include this extra check there too?

davidgumberg commented at 10:40 pm on July 3, 2025:

Thanks for this suggestion, I’ve implemented this approach in https://github.com/bitcoin/bitcoin/pull/32273/commits/55e60bd9c76ea74a108cd5c7275e26554b822b4c, and I’ve reused migrate_and_get_rpc() in the other tests added in this PR (except test_failed_migration_cleanup_relative_path() where it’s not feasible)

in test/functional/wallet_migration.py:573 in 499099320b outdated

568+        wallet.unloadwallet()
569+
570+        wallet_path = f"{self.old_node.wallets_path}/{wallet_name}"
571+        migrate_res = self.master_node.migratewallet(wallet_path)
572+        expected_backup_path = self.master_node.wallets_path / f"{final_dir}_{int(time.time())}.legacy.bak"
573+        os.path.exists(expected_backup_path)

ryanofsky commented at 3:15 pm on July 3, 2025:

In commit “test: Migration of a wallet ending in /” (499099320b138c3d895d3ad8a622a08e404008d6)

This fails on my system because the test is racy. By the time backup is created time.time() is higher than the backup time.

Also the os.path.exists() return value is discarded here so this doesn’t actually check anything.

Here are the changes I made to fix these problems. (Another approach might be to set a mock time instead)

 0--- a/test/functional/wallet_migration.py
 1+++ b/test/functional/wallet_migration.py
 2@@ -5,6 +5,7 @@
 3 """Test Migrating a wallet from legacy to descriptor."""
 4 import os.path
 5 import random
 6+import re
 7 import shutil
 8 import struct
 9 import time
10@@ -23,6 +24,7 @@ from test_framework.script import hash160
11 from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script, script_to_p2sh_script, script_to_p2wsh_script
12 from test_framework.util import (
13     assert_equal,
14+    assert_not_equal,
15     assert_raises_rpc_error,
16     find_vout_for_address,
17     sha256sum_file,
18@@ -567,9 +569,8 @@ class WalletMigrationTest(BitcoinTestFramework):
19 
20         wallet_ending_in_rel_path = f"{self.old_node.wallets_path}/{wallet_ending_in_relative}"
21         migrate_res = self.master_node.migratewallet(wallet_ending_in_rel_path)
22-        expected_backup_path = self.master_node.wallets_path / f"ends_{int(time.time())}.legacy.bak"
23-        os.path.exists(expected_backup_path)
24-        assert_equal(f"{expected_backup_path}", migrate_res['backup_path'])
25+        assert_not_equal(re.fullmatch(re.escape(str(self.master_node.wallets_path / "ends")) + r"_\d+\.legacy\.bak", migrate_res['backup_path']), None)
26+        assert_equal(os.path.exists(migrate_res['backup_path']), True)
27 
28         wallet = self.master_node.get_wallet_rpc(wallet_ending_in_rel_path)
29         info = wallet.getwalletinfo()
30@@ -598,9 +599,8 @@ class WalletMigrationTest(BitcoinTestFramework):
31 
32         wallet_path = f"{self.old_node.wallets_path}/{wallet_name}"
33         migrate_res = self.master_node.migratewallet(wallet_path)
34-        expected_backup_path = self.master_node.wallets_path / f"{final_dir}_{int(time.time())}.legacy.bak"
35-        os.path.exists(expected_backup_path)
36-        assert_equal(f"{expected_backup_path}", migrate_res['backup_path'])
37+        assert_not_equal(re.fullmatch(re.escape(str(self.master_node.wallets_path / final_dir)) + r"_\d+\.legacy\.bak", migrate_res['backup_path']), None)
38+        assert_equal(os.path.exists(migrate_res['backup_path']), True)
39 
40         wallet = self.master_node.get_wallet_rpc(wallet_path)
41

davidgumberg commented at 10:43 pm on July 3, 2025:

As suggested by @furszy above I moved this logic to migrate_and_get_rpc() in https://github.com/bitcoin/bitcoin/commit/55e60bd9c76ea74a108cd5c7275e26554b822b4c:

https://github.com/bitcoin/bitcoin/blob/6b2e3fbf64b19b6065768ef85155f986859fcfb9/test/functional/wallet_migration.py#L125-L147

And I’ve reused migrate_and_get_rpc() in the tests added in this PR.

ryanofsky commented at 4:04 pm on July 3, 2025: contributor

re: #32273 (comment)

Sorry I didn’t post my complete suggestion before. Here is the change I would suggest based on the current PR (2e8a21a43a3c321c9f3a8c75024d036165658225):

 0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -4253,30 +4253,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 3 
 4     // Make a backup of the DB in the wallet's directory with a unique filename
 5     // using the wallet name and current timestamp. The backup filename is based
 6-    // on the name of the parent directory containing the wallet data in most
 7-    // cases, but in the case where the wallet name is a path to a data file,
 8-    // the name of the data file is used, and in the case where the wallet name
 9-    // is blank, "default_wallet" is used.
10-    auto legacy_wallet_path = GetWalletPath(wallet_name);
11-    if (!legacy_wallet_path) {
12-        return util::Error{util::ErrorString(legacy_wallet_path)};
13-    }
14-
15-    // Backwards-compatibility logic for migrating wallets whose names are
16-    // paths to data files directly rather than directories.
17-    bool is_direct_file = fs::symlink_status(*legacy_wallet_path).type() == fs::file_type::regular;
18-    std::string backup_prefix;
19-    if (wallet_name.empty()) {
20-        backup_prefix = "default_wallet";
21-    } else if (is_direct_file) {
22-        // We use filename() to strip slashes in the direct file name.
23-        backup_prefix = fs::PathToString(fs::PathFromString(wallet_name).filename());
24-    } else {
25-        // The canonical form resolves relative specifiers in the
26-        // wallet's path, useful for the backup prefix.
27-        auto legacy_wallet_dir = fs::weakly_canonical(*legacy_wallet_path);
28-        backup_prefix = fs::PathToString(legacy_wallet_dir.filename());
29-    }
30+    // on walletname but expanded to "default_wallet" if it is empty and only
31+    // the final filename portion is used if contains slashes.
32+    std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
33+        // Normalize any .. components
34+        fs::path p{fs::PathFromString(wallet_name).lexically_normal()};
35+        // Drop trailing slash if any.
36+        if (p.filename().empty()) p = p.parent_path();
37+        // Return last path component
38+        return fs::PathToString(p.filename());
39+    }();
40     fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", backup_prefix, GetTime()));
41     fs::path backup_path = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
42     if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) {

There isn’t a reason I can see to access the filesystem just to convert a path into a backup prefix. IMO doing this makes the code unnecessarily fragile and complicated. I also don’t think it’s good to add a call to the GetWalletPath() function which is complicated enough to understand in the context of wallet loading, and shouldn’t be responsible for supporting wallet migration code as well.

I don’t think belt and suspenders checks are a great idea in general, but even when they are appropriate I think they need to be labeled with “// This condition is never expected to trigger” or similar comments to avoid confusing people reading the code and trying understand why the checks are there.

Agreed, I’ve added 75df713 which moves the backup from inside the wallet directory to being in the node’s -walletdir.

Nice! I actually misinterpreted your commit and didn’t realize storing backups inside wallet directories was actually previous behavior, but I think changing this does make sense and it is good to be able to remove the extra copy_file calls in the code.

Unfortunately, I think knowing the name of the wallet’s parent folder requires filesystem access when arbitrary relative paths are permitted. lexically_normal() won’t work in test cases like 8e20619 where the wallet is specified by a relative path that ascends more than it descends, or e.g. the wallet named ../.

I don’t think this is true. If a wallet ascends more than it descends like ../../../../mywallet, the suggested change would use “mywallet” as the prefix. If the path contains . components or internal .., lexically_normal will strip them out. If the path contains trailing slashes, the suggested code strips them out to. If the path is literally just .. or ../.. or similar even that is fine, producing backups files like .._1234.legacy.bak

This code could just as well be: […]

Yes, and I do think that change would be a clear simplification with no downsides. But I think it would be good to simplify even further and not resolve symlinks, not check to see if wallets are files or directories. None of this seems very helpful for choosing a good backup prefix.

davidgumberg force-pushed on Jul 3, 2025

davidgumberg commented at 10:52 pm on July 3, 2025: contributor

I don’t think belt and suspenders checks are a great idea in general, but even when they are appropriate I think they need to be labeled with “// This condition is never expected to trigger” or similar comments to avoid confusing people reading the code and trying understand why the checks are there.

OK, in lieu of an assert/assume, I’ve gone to the simplified form suggested, and I’ve also used the lambda+ternary over if+else form as you suggested. I’ve also taken advantage of the fact that direct files will never have slashes in their names, so we can use the same logic for direct files and paths in picking a prefix:

https://github.com/bitcoin/bitcoin/blob/9fa5480fc4c46fa11345c45fbe4dd21c127e3e05/src/wallet/wallet.cpp#L4260-L4267

But, I have retained the use of fs::weakly_canonical, since:

If the path is literally just .. or ../.. or similar even that is fine, producing backups files like .._1234.legacy.bak

While better than master, I don’t think this is the right behavior, and I’m not sure I follow why it’s fragile or complicated for us to ask the filesystem to resolve the canonical names of paths, and it’s the only way to preserve what seems to me the ideal behavior of naming the wallet backup after the name of the wallet directory it comes from.

in test/functional/wallet_migration.py:1034 in e7fec27359 outdated

1029+        self.old_node.unloadwallet(relative_name)
1030+        assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, relative_name)
1031+
1032+        assert all(wallet not in self.master_node.listwallets() for wallet in [f"{wallet_name}", f"{wallet_name}_watchonly", f"{wallet_name}_solvables"])
1033+
1034+        assert not (self.master_node.wallets_path / "failed_watchonly").exists()

achow101 commented at 0:13 am on July 4, 2025:

In e7fec273593d825e0e7dbec46a026aacc58c479f “test: Migration fail recovery w/ ../ in path”

I think failed_watchonly is the wrong wallet name. Probably best to do f"{wallet_name}_watchonly"

davidgumberg commented at 0:31 am on July 4, 2025:

Thanks for catching this, I’ve applied your fix in f67d6fe811

in test/functional/wallet_migration.py:634 in 146b133697 outdated

630+        addr = wallet.getnewaddress()
631+        txid = default.sendtoaddress(addr, 1)
632+        self.generate(self.master_node, 1)
633+        bals = wallet.getbalances()
634+
635+        _, wallet = self.migrate_and_get_rpc(wallet_name)

achow101 commented at 0:15 am on July 4, 2025:

In 146b133697f07c8dda004d60087505b2e1ec14b9 “test: Migration of a wallet ending in /”

Could add a check for the expected backup path.

davidgumberg commented at 0:26 am on July 4, 2025:

I add this check to migrate_and_get_rpc() in 55e60bd9c76:

https://github.com/bitcoin/bitcoin/blob/9fa5480fc4c46fa11345c45fbe4dd21c127e3e05/test/functional/wallet_migration.py#L138-L146

in test/functional/wallet_migration.py:654 in 9fa5480fc4 outdated

650+        self.generate(self.master_node, 1)
651+        bals = wallet.getbalances()
652+
653+        _, wallet = self.migrate_and_get_rpc(wallet_ending_in_relative)
654+
655+        assert wallet.gettransaction(txid)

achow101 commented at 0:15 am on July 4, 2025:

In 9fa5480fc4c46fa11345c45fbe4dd21c127e3e05 “test: Migration of a wallet ending in ../”

Could add a check for the expected backup path.

davidgumberg commented at 0:27 am on July 4, 2025:

See above: #32273 (review)

davidgumberg force-pushed on Jul 4, 2025

DrahtBot added the label Needs rebase on Jul 10, 2025

in test/functional/wallet_migration.py:589 in 5ab42b413f outdated

584@@ -585,7 +585,8 @@ def test_direct_file(self):
585         )
586         assert (self.master_node.wallets_path / "plainfile").is_file()
587 
588-        self.master_node.migratewallet("plainfile")
589+        migrate_res = self.master_node.migratewallet("plainfile")
590+        assert_equal(f"plainfile_{int(time.time())}.legacy.bak", os.path.basename(migrate_res["backup_path"]))

pablomartin4btc commented at 2:24 pm on July 11, 2025:

Maybe you can use mocktime here for consistency?

 0--- a/test/functional/wallet_migration.py
 1+++ b/test/functional/wallet_migration.py
 2@@ -585,8 +585,10 @@ class WalletMigrationTest(BitcoinTestFramework):
 3         )
 4         assert (self.master_node.wallets_path / "plainfile").is_file()
 5 
 6+        mocked_time = int(time.time())
 7+        self.master_node.setmocktime(mocked_time)
 8         migrate_res = self.master_node.migratewallet("plainfile")
 9-        assert_equal(f"plainfile_{int(time.time())}.legacy.bak", os.path.basename(migrate_res["backup_path"]))
10+        assert_equal(f"plainfile_{mocked_time}.legacy.bak", os.path.basename(migrate_res["backup_path"]))

davidgumberg commented at 0:51 am on July 17, 2025:

Thank you, fixed!

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>

e22c3599c6

wallet: migration: Make backup in walletdir f6ee59b6e2

wallet: Fix migration of wallets with pathnames.

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

70f1c99c90

test: Migration of a wallet with `../` in path. 63c6d36437

test: Migration fail recovery w/ `../` in path 41faef5f80

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

test: Migration of a wallet ending in `../` 76fe0e59ec

davidgumberg force-pushed on Jul 17, 2025

davidgumberg commented at 0:51 am on July 17, 2025: contributor

Rebased to address @pablomartin4btc’s feedback and merge conflict.

DrahtBot removed the label Needs rebase on Jul 17, 2025

pablomartin4btc commented at 1:59 am on July 24, 2025: member

tACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77

I’ve re-tested the relative path cases and the plainfile issue has been fixed.

Regarding the usage of fs::weakly_canonical vs lexically_normal(), I agree with @ryanofsky, I think that resource cost-wise, to “access” the filesystem in order to validate the name that we’ll use for the backup file that it’ll be written and validated then (its existence/ permission and so on). If there’s something that lexically_normal() can’t do right comparing to what fs::weakly_canonical does, I think we should go around that logic (if possible).

e.g. fs::path("/foo/./bar/../baz").lexically_normal() → "/foo/baz" (even if /foo/baz doesn’t exist — this will still work).

In any case, are we logging the before & after applying this parsing (using fs::weakly_canonical or lexically_normal())? Also perhaps we should document some examples somewhere so the users can find out why something they tried didn’t meet their expectations (was accidentally stored somewhere else or with a different name).

DrahtBot requested review from ryanofsky on Jul 24, 2025

wallet: Fix relative path backup during migration. #32273

Steps to reproduce on master

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)