mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-19 05:45:05 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#30265: wallet: Fix listwalletdir listing of migrated default wallets and generated backup files

Browse Source 
6b2dcba076 wallet: List sqlite wallets with empty string name (Ava Chow)
3ddbdd1815 wallet: Ignore .bak files when listing wallet files (Ava Chow)

Pull request description:

  When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so change `listwalletdir` to include these wallets.

  Additionally, the migration of the default wallet, and of any plain wallet files in the walletdir, produces a backup file in the walletdir itself. Since these backups are a BDB file, `listwalletdir` will detect them as being another wallet that we could open, but this is erroneous and could lead to confusion and potentially funds loss if both the backup and the migrated wallet are in use simultaneously. To reduce the likelihood of this issue, don't list these wallets in `listwalletdir`.

  ***

  Possibly we could have more stringent checks on loading to resolve these issues, but I'm concerned that that will just confuse users and gratuitously break things that already worked.

  Since the original intent was to disallow default wallets for sqlite/descriptors, a possible alternative would be to prevent people from loading such wallets and change migration to rename those wallets. However, given that this behavior with migrating default wallets has existed since default wallet migration was fixed, I think that making such a change would be confusing and break things for no good reason. Although perhaps we should still do the renaming.

  For the backups, we could also change loading to refuse to load any wallet named with `.bak` (or `.legacy.bak`) as such wallets can still be loaded by giving the path to them directly, which some users may do to "restore" the backup. However restricting what can be loaded based on filename seems a little heavyhanded. It wouldn't be funds loss though since the correct way to restore the backup is with `restorewallet`.

ACKs for top commit:
  fjahr:
    Code review ACK 6b2dcba076
  furszy:
    Code ACK  6b2dcba076
  glozow:
    ACK 6b2dcba076

Tree-SHA512: 0b033f6ed55830f8a054afea3fb2cf1fa82a94040053ebfaf123bda36c99f45d3f01a2aec4ed02fed9c61bb3d320b047ed892d7f6644b5a356a7bc5974b10cff
This commit is contained in:
glozow 2024-08-12 15:39:06 +01:00
commit b21ba081be
No known key found for this signature in database
GPG Key ID: BA03F4DBE0C63FB4
2 changed files with 8 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/wallet/db.cpp
View File

@ -42,12 +42,12 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
(IsBDBFile(BDBDataFile(it->path())) || IsSQLiteFile(SQLiteDataFile(it->path())))) {
// Found a directory which contains wallet.dat btree file, add it as a wallet.
paths.emplace_back(path);
} else if (it.depth() == 0 && it->symlink_status().type() == fs::file_type::regular && IsBDBFile(it->path())) {
} else if (it.depth() == 0 && it->symlink_status().type() == fs::file_type::regular && it->path().extension() != ".bak") {
if (it->path().filename() == "wallet.dat") {
// Found top-level wallet.dat btree file, add top level directory ""
// as a wallet.
paths.emplace_back();
} else {
} else if (IsBDBFile(it->path())) {
// Found top-level btree file not called wallet.dat. Current bitcoin
// software will never create these files but will allow them to be
// opened in a shared database environment for backwards compatibility.

7
test/functional/wallet_migration.py
View File

@ -548,10 +548,15 @@ class WalletMigrationTest(BitcoinTestFramework):
assert_equal(info["descriptors"], True)
assert_equal(info["format"], "sqlite")
walletdir_list = wallet.listwalletdir()
assert {"name": info["walletname"]} in walletdir_list["wallets"]
# Check backup existence and its non-empty wallet filename
backup_path = self.nodes[0].wallets_path / f'default_wallet_{curr_time}.legacy.bak'
backup_filename = f"default_wallet_{curr_time}.legacy.bak"
backup_path = self.nodes[0].wallets_path / backup_filename
assert backup_path.exists()
assert_equal(str(backup_path), res['backup_path'])
assert {"name": backup_filename} not in walletdir_list["wallets"]
def test_direct_file(self):
self.log.info("Test migration of a wallet that is not in a wallet directory")