Merge bitcoin/bitcoin#28125: wallet: bugfix, disallow migration of invalid scripts

8e7e3e6 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a23 wallet: disallow migration of invalid or not-watched scripts (furszy)

Pull request description:

  Fixing #28057.

  The legacy wallet allows to import any raw script (#28126), without
  checking if it was valid or not. Appending it to the watch-only set.

  This causes a crash in the migration process because we are only
  expecting to find valid scripts inside the legacy spkm.

  These stored scripts internally map to `ISMINE_NO` (same as if they
  weren't stored at all..).

  So we need to check for these special case, and take into account that
  the legacy spkm could be storing invalid not watched scripts.

  Which, in code words, means `IsMineInner()` returning
  `IsMineResult::INVALID` for them.

  Note:
  To verify this, can run the test commit on top of master.
  `wallet_migration.py` will crash without the bugfix commit.

ACKs for top commit:
  achow101:
    ACK 8e7e3e6

Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7

Author: achow101

src/wallet/scriptpubkeyman.cpp

@@ -1716,8 +1716,23 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetScriptPub
     }
 
     // All watchonly scripts are raw
-    spks.insert(setWatchOnly.begin(), setWatchOnly.end());
+    for (const CScript& script : setWatchOnly) {
+        // As the legacy wallet allowed to import any script, we need to verify the validity here.
+        // LegacyScriptPubKeyMan::IsMine() return 'ISMINE_NO' for invalid or not watched scripts (IsMineResult::INVALID or IsMineResult::NO).
+        // e.g. a "sh(sh(pkh()))" which legacy wallets allowed to import!.
+        if (IsMine(script) != ISMINE_NO) spks.insert(script);
+    }
+
+    return spks;
+}
 
+std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const
+{
+    LOCK(cs_KeyStore);
+    std::unordered_set<CScript, SaltedSipHasher> spks;
+    for (const CScript& script : setWatchOnly) {
+        if (IsMine(script) == ISMINE_NO) spks.insert(script);
+    }
     return spks;
 }
 

src/wallet/scriptpubkeyman.h

@@ -525,6 +525,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     std::set<CKeyID> GetKeys() const override;
     std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const override;
 
+    /**
+     * Retrieves scripts that were imported by bugs into the legacy spkm and are
+     * simply invalid, such as a sh(sh(pkh())) script, or not watched.
+     */
+    std::unordered_set<CScript, SaltedSipHasher> GetNotMineScriptPubKeys() const;
+
     /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan.
      * Does not modify this ScriptPubKeyMan. */
     std::optional<MigrationData> MigrateToDescriptor();

src/wallet/wallet.cpp

@@ -3860,6 +3860,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
         return false;
     }
 
+    // Get all invalid or non-watched scripts that will not be migrated
+    std::set<CTxDestination> not_migrated_dests;
+    for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) {
+        CTxDestination dest;
+        if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest);
+    }
+
     for (auto& desc_spkm : data.desc_spkms) {
         if (m_spk_managers.count(desc_spkm->GetID()) > 0) {
             error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.");
@@ -3966,6 +3973,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                         continue;
                     }
                 }
+
+                // Skip invalid/non-watched scripts that will not be migrated
+                if (not_migrated_dests.count(addr_pair.first) > 0) {
+                    dests_to_delete.push_back(addr_pair.first);
+                    continue;
+                }
+
                 // Not ours, not in watchonly wallet, and not in solvable
                 error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets");
                 return false;

test/functional/wallet_migration.py

@@ -6,6 +6,7 @@
 
 import random
 import shutil
+from test_framework.address import script_to_p2sh
 from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import COIN, CTransaction, CTxOut
@@ -683,6 +684,21 @@ def send_to_script(script, amount):
         wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False)
         assert_equal(wallet.getbalances()['watchonly']['trusted'], 5)
 
+        # Import sh(pkh()) script, by using importaddress(), with the p2sh flag enabled.
+        # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet.
+        # The migration process must skip the invalid scripts and the addressbook records linked to them.
+        # They are not being watched by the current wallet, nor should be watched by the migrated one.
+        label_sh_pkh = "raw_sh_pkh"
+        script_pkh = key_to_p2pkh_script(df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"])
+        script_sh_pkh = script_to_p2sh_script(script_pkh)
+        addy_script_sh_pkh = script_to_p2sh(script_pkh)  # valid script address
+        addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh)  # invalid script address
+
+        # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'.
+        #       Both of them will be stored with the same addressbook label. And only the latter one should
+        #       be discarded during migration. The first one must be migrated.
+        wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True)
+
         # Migrate wallet and re-check balance
         info_migration = wallet.migratewallet()
         wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"])
@@ -692,6 +708,20 @@ def send_to_script(script, amount):
         # The watch-only scripts are no longer part of the main wallet
         assert_equal(wallet.getbalances()['mine']['trusted'], 0)
 
+        # The invalid sh(sh(pk())) script label must not be part of the main wallet anymore
+        assert label_sh_pkh not in wallet.listlabels()
+        # But, the standard sh(pkh()) script should be part of the watch-only wallet.
+        addrs_by_label = wallet_wo.getaddressesbylabel(label_sh_pkh)
+        assert addy_script_sh_pkh in addrs_by_label
+        assert addy_script_double_sh_pkh not in addrs_by_label
+
+        # Also, the watch-only wallet should have the descriptor for the standard sh(pkh())
+        desc = descsum_create(f"addr({addy_script_sh_pkh})")
+        assert next(it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc)
+        # And doesn't have a descriptor for the invalid one
+        desc_invalid = descsum_create(f"addr({addy_script_double_sh_pkh})")
+        assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None)
+
         # Just in case, also verify wallet restart
         self.nodes[0].unloadwallet(info_migration["watchonly_name"])
         self.nodes[0].loadwallet(info_migration["watchonly_name"])