mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-02-02 11:41:18 +00:00
91058877ff
76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77 test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fef08d9f981eac45841ef2df7444031b test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80d69ed984af685bd9d2a43268009fb6 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d364376907c10b9baa3c6f4d72e3f1881abc test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c901de64d6ccea793b7e267e20dfa49cf wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6772730e72e17fc68c99feea09b4d29 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 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
achow101:
ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
ryanofsky:
Code review ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77. 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