wallet: Fix relative path backup during migration. #32273

pull davidgumberg wants to merge 3 commits into bitcoin:master from davidgumberg:4-14-25-relative-migration changing 2 files +122 −4
  1. davidgumberg commented at 3:32 am on April 15, 2025: contributor

    Support for wallets outside of the default wallet directory was added in #11687, and these external wallets can be specified with paths relative to the wallet directory, e.g. bitcoin-cli loadwallet ../../mywallet. In the RPC commands, there is no distinction between a wallet’s ’name’ and a wallet’s ‘path’. I suspect this may not be the only bug like this, since everywhere that a wallet’s name appears in code, that name might be a path, and that path may contain relative specifiers, and the consequences of that are not always obvious, and test coverage for wallets with relative paths seems like it could be improved.

    This PR fixes an issue with wallet backup during migration where the wallet’s ’name’ is used in the backup filename. This goes south when that filename is appended (/=) to the directory where we want to put the file and the wallet’s ’name’ actually gets treated as a path:

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

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

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

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

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

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

    Steps to reproduce on master

    Build and run bitcoind with legacy wallet creation enabled:

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

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

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

    Try to migrate the wallet:

    0$ ./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="../../../myrelativewallet"

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

  2. 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 ryanofsky
    Concept ACK shahsb
    Stale ACK pseudoramdom

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32349 (test: Increase stack size for “Debug” builds with MSVC by hebasto)
    • #31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)

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

  3. DrahtBot added the label Wallet on Apr 15, 2025
  4. 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.
  5. ryanofsky approved
  6. 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.

  7. davidgumberg force-pushed on Apr 15, 2025
  8. wallet: Fix relative path backup during migration. 47174476c0
  9. davidgumberg force-pushed on Apr 15, 2025
  10. ryanofsky approved
  11. 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.

  12. 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.
  13. test: Migrating wallet with `../` in path. c6d439df6f
  14. davidgumberg force-pushed on Apr 16, 2025
  15. davidgumberg force-pushed on Apr 16, 2025
  16. DrahtBot added the label CI failed on Apr 16, 2025
  17. 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.

  18. 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
  19. 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
  20. DrahtBot requested review from ryanofsky on Apr 17, 2025
  21. test: Migration fail recovery w/ `../` in path 6b4b7ac211
  22. davidgumberg force-pushed on Apr 17, 2025
  23. in test/functional/wallet_migration.py:1034 in 6b4b7ac211 
    1029+        assert os.path.exists(solvables_path)
1030+        new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
1031+        assert_equal(original_shasum, new_shasum)
1032+
1033+        # Check the wallet we tried to migrate is still BDB
1034+        datfile = os.path.join(absolute_path, "wallet.dat")
    davidgumberg commented at 6:32 am on April 17, 2025:
    Writing as a review comment since I didn’t think a code comment was warranted here, the reason for putting this on a separate line even though it is only used once is to avoid the wrath of the slightly imprecise python-utf8-encoding lint check which otherwise won’t catch that the file is being opened in binary mode.
  24. DrahtBot removed the label CI failed on Apr 17, 2025
  25. in test/functional/wallet_migration.py:549 in 6b4b7ac211 
    544+        wallet_name = "relative"
545+        absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name))
546+        relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path)
547+
548+        wallet = self.create_legacy_wallet(relative_name)
549+        # listwalletdirs only returns wallets in the wallet directory
    shahsb commented at 4:22 am on April 18, 2025:

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

    0# Wallet created using relative path won't reside in the default wallet dir,
1# hence it should not appear in listwalletdir results.
    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.
  26. in test/functional/wallet_migration.py:542 in 6b4b7ac211 
    537+
538+        # Get the nearest common path of both nodes' wallet paths.
539+        common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
540+
541+        # This test assumes that the relative path from each wallet directory to the common path is identical.
542+        assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))
    shahsb commented at 4:24 am on April 18, 2025:

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

    0assert_equal(..., ..., "Relative paths from wallet dirs to common parent must be identical for this test.")
    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.
  27. in test/functional/wallet_migration.py:592 in 6b4b7ac211 
    587+        assert {"name": backup_filename} not in walletdirlist["wallets"]
588+
589+        # Check that old node can restore from the backup.
590+        self.old_node.restorewallet("relative_restored", migrate_res['backup_path'])
591+        wallet = self.old_node.get_wallet_rpc("relative_restored")
592+        assert wallet.gettransaction(txid)
    shahsb commented at 4:31 am on April 18, 2025:

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

    For example:

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

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

    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.
  28. in test/functional/wallet_migration.py:1027 in 6b4b7ac211 
    1022+        self.old_node.unloadwallet(relative_name)
1023+        assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, relative_name)
1024+
1025+        assert all(wallet not in self.master_node.listwallets() for wallet in [f"{wallet_name}", f"{wallet_name}_watchonly", f"{wallet_name}_solvables"])
1026+
1027+        assert not (self.master_node.wallets_path / "failed_watchonly").exists()
    shahsb commented at 4:36 am on April 18, 2025:

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

    0# No 'failed_watchonly' directory should exist because migration cleanup should be complete
  29. in test/functional/wallet_migration.py:1000 in 6b4b7ac211 
     995+
 996+        # Get the nearest common path of both nodes' wallet paths.
 997+        common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path])
 998+
 999+        # This test assumes that the relative path from each wallet directory to the common path is identical.
1000+        assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path))
    shahsb commented at 4:38 am on April 18, 2025:

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

    0assert_equal(..., ..., "Test requires that both wallet paths have the same relative path to the common parent")
    davidgumberg commented at 8:38 pm on April 23, 2025:
    See #32273 (review).
  30. in test/functional/wallet_migration.py:1035 in 6b4b7ac211 
    1030+        new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
1031+        assert_equal(original_shasum, new_shasum)
1032+
1033+        # Check the wallet we tried to migrate is still BDB
1034+        datfile = os.path.join(absolute_path, "wallet.dat")
1035+        with open(datfile, "rb") as f:
    shahsb commented at 4:41 am on April 18, 2025:
    We should first validate that file is present at the specified location and accessible and then only try to open and validate other things
    shahsb commented at 4:43 am on April 18, 2025:

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

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

    and then:

    0assert is_bdb_wallet(datfile)
    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

  31. shahsb commented at 4:44 am on April 18, 2025: none
    Concept ACK
  32. ryanofsky approved
  33. 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:

    1. 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).
    2. 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
    3. 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.
    4. 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.

  34. DrahtBot requested review from shahsb on Apr 21, 2025
  35. 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.

  36. luke-jr changes_requested
  37. 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?
  38. 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

  39. 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)
  40. 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.

  41. DrahtBot added the label Needs rebase on Apr 25, 2025
  42. DrahtBot commented at 1:02 pm on April 25, 2025: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.


davidgumberg DrahtBot ryanofsky achow101 pseudoramdom shahsb luke-jr


shahsb

Labels
Wallet Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-19 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me