wallet: migration, don't create spendable wallet from a watch-only legacy wallet

furszy commented at 5:59 pm on December 4, 2024: member

Currently, the migration process creates a brand-new descriptor wallet with no connection to the user’s legacy wallet when the legacy wallet lacks key material and contains only watch-only scripts. This behavior is not aligned with user expectations. If the legacy wallet contains only watch-only scripts, the migration process should only generate a watch-only wallet instead.

TODO List:

Explain that migratewallet renames the watch-only after migration, and also that the wallet will not have keys enabled.

DrahtBot commented at 11:11 am on December 5, 2024: 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/31423.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors, pablomartin4btc

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:

#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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 CI failed on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

DrahtBot added the label Wallet on Dec 5, 2024

DrahtBot added the label CI failed on Dec 7, 2024

in src/wallet/wallet.cpp:4123 in 37e3cab479 outdated

4118@@ -4112,8 +4119,10 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
4119         if (data.master_key.key.IsValid()) {
4120             SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key);
4121         } else {
4122-            // Setup with a new seed if we don't.
4123-            SetupOwnDescriptorScriptPubKeyMans(local_wallet_batch);
4124+            // Setup with a new seed, only if we are actually migrating any spendable descriptor
4125+            if (!empty_local_wallet) {

achow101 commented at 8:35 pm on December 9, 2024:

In 37e3cab479080db9a3ac10f9b9b5c7f6c0e07882 “wallet: migration, don’t create spendable wallet from a watch-only legacy wallet”

This check can go above next to the !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS).

furszy commented at 6:20 pm on December 10, 2024:

sure, done as suggested. Also added an extra !data->master_key.key.IsValid() to the empty_local_wallet definition.

DrahtBot added the label Needs rebase on Dec 9, 2024

in test/functional/wallet_migration.py:1032 in 37e3cab479 outdated

1027+        res = wallet.migratewallet()
1028+        wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name'])
1029+        assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.get_bytes().hex()})'))
1030+        # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
1031+        assert_equal('bare_p2pk_watchonly', res['wallet_name'])
1032+        assert "bare_p2pk" not in self.nodes[0].listwallets()

achow101 commented at 8:55 pm on December 9, 2024:

In 37e3cab479080db9a3ac10f9b9b5c7f6c0e07882 “wallet: migration, don’t create spendable wallet from a watch-only legacy wallet”

Should also check that the wallet does not appear in listwalletdir:

0assert "bare_p2pk" not in [w["name"] for w in self.nodes[0].listwalletdir()["wallets"]]

This causes the test to fail. Examining the datadir after the test shows us that the wallet directory is still there, and the wallet.dat file is still intact and valid, although missing descriptor records.

The spendable wallet should be deleted if it is empty, although this needs to be done carefully to avoid deleting the backup file that is stored in the wallet directory.

furszy commented at 6:23 pm on December 10, 2024:

This causes the test to fail. Examining the datadir after the test shows us that the wallet directory is still there, and the wallet.dat file is still intact and valid, although missing descriptor records.

Good catch. The wallet is actually entirely empty. There’s neither address book records nor any transaction. We can safely remove it 👍🏼 .

furszy force-pushed on Dec 10, 2024

DrahtBot removed the label Needs rebase on Dec 10, 2024

DrahtBot removed the label CI failed on Dec 10, 2024

achow101 added the label Needs backport (28.x) on Dec 11, 2024

in src/wallet/wallet.cpp:4545 in 8aab56772c outdated

4544+            res.wallet = local_wallet;
4545+            res.wallet_name = wallet_name;
4546+        } else {
4547+            // At this point, the main wallet is empty after migration, meaning it has no records.
4548+            // Therefore, we can safely remove it.
4549+            fs::path path_to_remove = fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path();

achow101 commented at 5:18 pm on December 11, 2024:

This also deletes the backup file.

furszy commented at 6:17 pm on December 11, 2024:

Upps, corrected. Thanks. Was interesting to know that we have no backup file existence general check. Added one now.

in test/functional/wallet_migration.py:1033 in 8aab56772c outdated

1017@@ -1018,6 +1018,12 @@ def test_migrate_simple_watch_only(self):
1018         res, _ = self.migrate_and_get_rpc("bare_p2pk")
1019         wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name'])
1020         assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})'))
1021+
1022+        # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
1023+        assert_equal('bare_p2pk_watchonly', res['wallet_name'])
1024+        assert "bare_p2pk" not in self.master_node.listwallets()
1025+        assert "bare_p2pk" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]]

achow101 commented at 5:19 pm on December 11, 2024:

Should also check that the backup file is still present.

furszy commented at 6:17 pm on December 11, 2024:

Done. Thx.

furszy force-pushed on Dec 11, 2024

achow101 removed the label Needs backport (28.x) on Dec 13, 2024

in src/wallet/sqlite.h:175 in 788bb48f67 outdated

170+    std::vector<fs::path> Files() override
171+    {
172+        std::vector<fs::path> files;
173+        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
174+        files.emplace_back(m_dir_path / "database");
175+        files.emplace_back(m_dir_path / "db.log");

achow101 commented at 7:46 pm on December 30, 2024:

In 788bb48f6712c39995831020dc36297f88c42f71 “wallet: introduce method to return all db created files”

These are created only by BDB.

SQLite makes only wallet.dat and wallet.dat-journal.

furszy commented at 4:17 pm on January 22, 2025:

Fixed. Thanks.

Sjors commented at 12:53 pm on January 21, 2025: member

Concept ACK

Can this go in parallel with #31495 or better to wait for it?

furszy force-pushed on Jan 22, 2025

furszy commented at 4:21 pm on January 22, 2025: member

Can this go in parallel with #31495 or better to wait for it?

Can go in parallel. There shouldn’t be any conflict between these two.

DrahtBot added the label Needs rebase on Feb 13, 2025

refactor: remove sqlite dir path back-and-forth conversion 57694818bb

wallet: introduce method to return all db created files a48d81047e

wallet: migration, don't create spendable wallet from a watch-only legacy wallet

Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.

This commit implements this behavior.

33b170721d

furszy force-pushed on Feb 14, 2025

DrahtBot removed the label Needs rebase on Feb 14, 2025

pablomartin4btc commented at 10:57 am on March 4, 2025: member

Concept ACK

nit (top PR’s description & last commit body - perhaps it’s obvious but still):

If the legacy wallet contains only watch-only scripts, the migration process should only generate a watch-only descriptor wallet instead.

(I’ll test and review soon)

in src/wallet/sqlite.cpp:120 in 33b170721d

117 {
118     {
119         LOCK(g_sqlite_mutex);
120         LogPrintf("Using SQLite Version %s\n", SQLiteDatabaseVersion());
121-        LogPrintf("Using wallet %s\n", m_dir_path);
122+        LogPrintf("Using wallet %s\n", fs::PathToString(m_dir_path));

pablomartin4btc commented at 1:46 pm on March 18, 2025:

Any reasons why you wouldn’t use std::filesystem::path::string instead (same for other instances):

0        LogPrintf("Using wallet %s\n", m_dir_path.string());

in src/wallet/wallet.cpp:4096 in 33b170721d

4091@@ -4092,6 +4092,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
4092         return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
4093     }
4094 
4095+    // When the legacy wallet has no spendable scripts, it will be replaced by the watch-only/solvable wallets.
4096+    bool empty_local_wallet = data.desc_spkms.empty() && !data.master_key.key.IsValid();

pablomartin4btc commented at 3:54 pm on March 18, 2025:

nit (to add a bit of clarity - I also think this CWallet::ApplyMigrationData should be refactored, at least extracting some behaviour into functions to begin with if possible):

0    bool is_local_wallet_empty = data.desc_spkms.empty() && !data.master_key.key.IsValid();

pablomartin4btc commented at 5:46 pm on March 25, 2025: member

I see the changes done on test/functional/wallet_migration.py applied on master makes the test to fail, validating its purpose, however when I test this branch manually (on both bitcoin-qt and bitcoind + cli) I got a bad_function_call (returning from here) when I create a “blank” legacy wallet, import an address (watch-only) and run the migratewallet to it (both wallets are being created, but not loaded, I see the details from the wo look correct with getwalletinfo but not the “spendable/ main”, which when I load it has "format": "sqlite", and "descriptors": false,).

On the other hand, in master, when I create a blank legacy wallet with the imported address, the migratewallet process creates both, the “main” wallet (called like that in the source code) and the wo one (watch-only), and if I run getnewaddress on the main wallet I got an address, when it was originally blank with no keys (before migration I could get “Error: This wallet has no available keys (code -4)”). is the case you are trying to solve here?

wallet: migration, don’t create spendable wallet from a watch-only legacy wallet #31423

Code Coverage & Benchmarks

Reviews

Conflicts