In commit "wallet: Fix migration of wallets with pathnames." (70f1c99)

Note: This line of code was broken before this commit when wallet_name contained slashes. It would either cause the BackupWallet call below to fail, or to write backup files to unexpected places, instead of writing them to -walletdir as intended after the previous commit.

In commit "wallet: Fix migration of wallets with pathnames." (70f1c99)

IMO, it's confusing for this comment to distinguish between the case when the wallet is a directory and the case when it's a file, when there is no distinction in the code below. The code below is always returning the last path component no matter what kind of path is provided.

In commit "wallet: Fix migration of wallets with pathnames." (70f1c99)

This looks good, but just for comparison here is an updated version of the suggestion from: #32273 (review)

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4231,14 +4231,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 
     // Make a backup of the DB in the wallet's directory with a unique filename
     // using the wallet name and current timestamp. The backup filename is based
-    // on the name of the parent directory containing the wallet data in most
-    // cases, but in the case where the wallet name is a path to a data file,
-    // the name of the data file is used, and in the case where the wallet name
-    // is blank, "default_wallet" is used.
-    const std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
-        // fs::weakly_canonical resolves relative specifiers and remove trailing slashes.
-        const auto legacy_wallet_path = fs::weakly_canonical(GetWalletDir() / fs::PathFromString(wallet_name));
-        return fs::PathToString(legacy_wallet_path.filename());
+    // on walletname but expanded to "default_wallet" if it is empty and only
+    // the final filename portion is used if contains slashes.
+    std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
+        // Normalize any .. components
+        fs::path p{fs::PathFromString(wallet_name).lexically_normal()};
+        // Drop trailing slash if any.
+        if (p.filename().empty()) p = p.parent_path();
+        // Return last path component
+        return fs::PathToString(p.filename());
     }();
 
     fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", backup_prefix, GetTime()));

IMO, the suggestion is safer because it just uses string manipulation to choose a backup file prefix, avoiding the complexity of GetWalletDir() and unnecessary filesystem access done by weakly_canonical.

I also think the suggestion is better because it chooses the backup prefix based on the wallet name, not based on the last component of the symlink destination path if the wallet is a symlink.

But current code is fine and a clear improvement over the status quo.

addressing this in #33122

re: #32273 (comment)

addressing this in #33122

Note this change is now dropped from #33122, but could be good to revisit at some point (I think it's still a good idea).

I can't recall if this was discussed elsewhere, but in some cases when the path ends in a relative specifier the parent directory name can't be recovered without filesystem access: e.g. the wallet name is ../, I think filesystem access might be necessary here.

It was discussed, see #32273 (review):

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

More generally, putting this specific suggestion aside, hopefully it is clear that turning a wallet name into a safe backup filename just requires string manipulation, and does not require accessing the filesystem, resolving symlinks, or calling GetWalletDir().

I agree, maybe I had too strict of an idea of what the expectation would be for the destination of the backup: if the only goal is that the backup filename be safe and likely to work, then lexically_normal() or even current_time_1234.legacy.bak in the datadir works perfectly fine, and satisfying those requirements is probably good enough for the migration backup, but I'm still not sure I understand what is wrong with using the filesystem here.

In commit "wallet: migration: Make backup in walletdir" (f6ee59b)

Notes about this change (mostly for myself to avoid getting confused later)

• This commit changes location of wallet backups created during migrations when the wallets being migrated are directories. Instead of placing backups inside those directories, backups are placed in the top level -walletdir (<datadir>/wallets) so they are easier to find and don't need to copied around if migration fails. Currently two extra copies are necessary because if migration fails the wallet directories are deleted and recreated.

• This commit does not change location of wallet backups created during migrations when the wallets being migrated are files, since they were always saved in -walletdir. But I think this commit might prevent errors if migration fails in that case, because backup_path and temp_backup_location would point to the same path in that case, and copying a file on top of itself could be an error.

• Some reasons for wanting to store backups in the top-level directory wallets directory instead of inside individual wallet directories are mentioned in wallet: Fix relative path backup during migration. #32273 (comment). (Though at the time I wrote that comment I was under the mistaken impression that storing backups inside wallets was newly added behavior and did not realize it happened previously.)