wallet: Fix listwalletdir listing of migrated default wallets and generated backup files

achow101 commented at 9:54 pm on June 10, 2024: member

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.

DrahtBot commented at 9:54 pm on June 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, fjahr, glozow
Stale ACK	Eunovo

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:

#bitcoin-core/gui/824 (Migrate legacy wallets that are not loaded 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.

DrahtBot added the label Wallet on Jun 10, 2024

Eunovo commented at 12:46 pm on June 19, 2024: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/30265/commits/0ce7c6294bf58142de55372d6448416a73f4caa5 I ran the wallet_migration test locally and also prepared a wallets directory with a default wallet file, a backup file and a legacy wallet file for manual testing. Running listwalletdir on master returned the following output

0{
1  "wallets": [
2    {
3      "name": "boblegacy_1718798800.legacy.bak"
4    },
5    {
6      "name": "boblegacy"
7    }
8  ]
9}

And listwalletdir on https://github.com/bitcoin/bitcoin/pull/30265/commits/0ce7c6294bf58142de55372d6448416a73f4caa5 returned this

0{
1  "wallets": [
2    {
3      "name": "boblegacy"
4    },
5    {
6      "name": ""
7    }
8  ]
9}

The no-name wallet is displayed and the backup wallet file is hidden as expected.

achow101 added this to the milestone 28.0 on Aug 8, 2024

in test/functional/wallet_migration.py:550 in a700b303fc outdated

546+        backup_path = self.nodes[0].wallets_path / backup_filename
547         assert backup_path.exists()
548         assert_equal(str(backup_path), res['backup_path'])
549+        assert {"name": backup_filename} not in walletdir_list["wallets"]
550+
551+        wallet.setmocktime(0)

furszy commented at 6:00 pm on August 9, 2024:

this line does not seems to be related to a700b303fcba7915f.

achow101 commented at 7:55 pm on August 9, 2024:

Removed

wallet: Ignore .bak files when listing wallet files

Migration creates backup files in the wallet directory with .bak as the
extension. This pollutes the output of listwalletdir with backup files
that most users should not need to care about.

3ddbdd1815

wallet: List sqlite wallets with empty string name

Although it is not explicitly possible to create a default wallet with
descriptors, it is possible to migrate a default wallet and have it end
up being a default wallet with descriptors. These wallets should be
listed by ListDatabases so that it appears in wallet directory listings
to avoid user confusion.

6b2dcba076

achow101 force-pushed on Aug 9, 2024

furszy commented at 10:12 pm on August 9, 2024: member

Code ACK 6b2dcba076

fjahr commented at 12:04 pm on August 11, 2024: contributor

Code review ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d

Also verified manually that the added tests do cover the change of behavior.

glozow commented at 2:37 pm on August 12, 2024: member

ACK 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d

Tested by creating legacy default and non-default wallets, then migrating, then using listwalletdir. I can see the default wallet is listed as a .bak on master, then disappears on 3ddbdd1815c676a88345b3b0e55a551d2a569e28, then is listed with its name on 6b2dcba07670f04f32c0dc3a2c86fd805c85f12d.

glozow merged this on Aug 12, 2024

glozow closed this on Aug 12, 2024

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

Code Coverage

Reviews

Conflicts