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

1. 
   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.
2. 
   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. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31495 (wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases by achow101)
   • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
   • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
   • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

   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.

3. DrahtBot added the label CI failed on Dec 5, 2024
4. DrahtBot removed the label CI failed on Dec 5, 2024
5. DrahtBot added the label Wallet on Dec 5, 2024
6. DrahtBot added the label CI failed on Dec 7, 2024
7. 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.
8. DrahtBot added the label Needs rebase on Dec 9, 2024
9. 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 👍🏼 .

10. furszy force-pushed on Dec 10, 2024
11. DrahtBot removed the label Needs rebase on Dec 10, 2024
12. DrahtBot removed the label CI failed on Dec 10, 2024
13. achow101 added the label Needs backport (28.x) on Dec 11, 2024
14. 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.
15. in test/functional/wallet_migration.py:1027 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.
16. refactor: remove sqlite dir path back-and-forth conversion 81da08929f
17. wallet: introduce method to return all db created files 788bb48f67
18. furszy force-pushed on Dec 11, 2024
19. 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.

    8eb2736de4
20. furszy force-pushed on Dec 11, 2024
21. achow101 removed the label Needs backport (28.x) on Dec 13, 2024

Contributors
furszy DrahtBot achow101

Labels
Wallet