bitcoin

mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-19 14:53:43 +01:00

Files

Ava Chow 91058877ff Merge bitcoin/bitcoin#32273 : wallet: Fix relative path backup during migration.

76fe0e59ec test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fe test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d36437 test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c90 wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6 test: wallet: Check direct file backup name. (David Gumberg)

Pull request description:

  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:

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

  9fa5480fc4/src/wallet/wallet.cpp (L4254-L4268)

  ##### Steps to reproduce on master
  Build and run `bitcoind` with legacy wallet creation enabled:
  ```bash
  $ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
  $ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb
  ```

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

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

  Try to migrate the wallet:
  ```bash
  $ ./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.

ACKs for top commit:
  pablomartin4btc:
    tACK 76fe0e59ec
  achow101:
    ACK 76fe0e59ec
  ryanofsky:
    Code review ACK 76fe0e59ec. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.

Tree-SHA512: 5cf6ed9f44ac7d204e4e9854edd3fb9b43812e930f76343b142b3c19df3de2ae5ca1548d4a8d26226d537bca231e3a50b3ff0d963c200303fb761f2b4eb3f0d9