Merge 3474524ee5 into 66aa6a47bd

7c123c08dd  miner: add package feerate vector to CBlockTemplate (ismaelsadeeq)

Pull request description:

  This PR enables `BlockAssembler` to add all selected packages' fee and virtual size to a vector, and then return the vector as a member of `CBlockTemplate` struct.

  This PR is the first step in the https://github.com/bitcoin/bitcoin/issues/30392 project.

  The packages' vsize and fee are used in #30157 to select a percentile fee rate of the top block in the mempool.

ACKs for top commit:
  rkrux:
    tACK 7c123c08dd
  ryanofsky:
    Code review ACK 7c123c08dd. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.
  glozow:
    reACK 7c123c08dd

Tree-SHA512: 767b0b3d4273cf1589fd2068d729a66c7414c0f9574b15989fbe293f8c85cd6c641dd783cde55bfabab32cd047d7d8a071d6897b06ed4295c0d071e588de0861

src/node/miner.cpp

@@ -421,6 +421,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
         }
 
         ++nPackagesSelected;
+        pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast<int32_t>(packageSize));
 
         // Update transactions that depend on each of these
         nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);

src/node/miner.h

@@ -10,6 +10,7 @@
 #include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
+#include <util/feefrac.h>
 
 #include <memory>
 #include <optional>
@@ -39,6 +40,9 @@ struct CBlockTemplate
     std::vector<CAmount> vTxFees;
     std::vector<int64_t> vTxSigOpsCost;
     std::vector<unsigned char> vchCoinbaseCommitment;
+    /* A vector of package fee rates, ordered by the sequence in which
+     * packages are selected for inclusion in the block template.*/
+    std::vector<FeeFrac> m_package_feerates;
 };
 
 // Container for tracking updates to ancestor feerate as we include (parent)

src/test/miner_tests.cpp

@@ -16,6 +16,7 @@
 #include <txmempool.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/feefrac.h>
 #include <util/strencodings.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -25,6 +26,7 @@
 #include <test/util/setup_common.h>
 
 #include <memory>
+#include <vector>
 
 #include <boost/test/unit_test.hpp>
 
@@ -123,19 +125,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout[0].nValue = 5000000000LL - 1000;
     // This tx has a low fee: 1000 satoshis
     Txid hashParentTx = tx.GetHash(); // save this txid for later use
-    AddToMempool(tx_mempool, entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto parent_tx{entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, parent_tx);
 
     // This tx has a medium fee: 10000 satoshis
     tx.vin[0].prevout.hash = txFirst[1]->GetHash();
     tx.vout[0].nValue = 5000000000LL - 10000;
     Txid hashMediumFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto medium_fee_tx{entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, medium_fee_tx);
 
     // This tx has a high fee, but depends on the first transaction
     tx.vin[0].prevout.hash = hashParentTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee
     Txid hashHighFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
+    const auto high_fee_tx{entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx)};
+    AddToMempool(tx_mempool, high_fee_tx);
 
     std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
     BOOST_REQUIRE(block_template);
@@ -145,6 +150,21 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx);
     BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx);
 
+    // Test the inclusion of package feerates in the block template and ensure they are sequential.
+    const auto block_package_feerates = BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}.CreateNewBlock()->m_package_feerates;
+    BOOST_CHECK(block_package_feerates.size() == 2);
+
+    // parent_tx and high_fee_tx are added to the block as a package.
+    const auto combined_txs_fee = parent_tx.GetFee() + high_fee_tx.GetFee();
+    const auto combined_txs_size = parent_tx.GetTxSize() + high_fee_tx.GetTxSize();
+    FeeFrac package_feefrac{combined_txs_fee, combined_txs_size};
+    // The package should be added first.
+    BOOST_CHECK(block_package_feerates[0] == package_feefrac);
+
+    // The medium_fee_tx should be added next.
+    FeeFrac medium_tx_feefrac{medium_fee_tx.GetFee(), medium_fee_tx.GetTxSize()};
+    BOOST_CHECK(block_package_feerates[1] == medium_tx_feefrac);
+
     // Test that a package below the block min tx fee doesn't get included
     tx.vin[0].prevout.hash = hashHighFeeTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee

src/wallet/scriptpubkeyman.cpp

@@ -591,7 +591,7 @@ std::unique_ptr<SigningProvider> LegacyDataSPKM::GetSolvingProvider(const CScrip
     return std::make_unique<LegacySigningProvider>(*this);
 }
 
-bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)
+bool LegacyDataSPKM::CanProvide(const CScript& script, SignatureData& sigdata)
 {
     IsMineResult ismine = IsMineInner(*this, script, IsMineSigVersion::TOP, /* recurse_scripthash= */ false);
     if (ismine == IsMineResult::SPENDABLE || ismine == IsMineResult::WATCH_ONLY) {
@@ -1700,61 +1700,61 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
     return set_address;
 }
 
-std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetScriptPubKeys() const
+std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetCandidateScriptPubKeys() const
 {
     LOCK(cs_KeyStore);
+    std::unordered_set<CScript, SaltedSipHasher> candidate_spks;
+
+    // For every private key in the wallet, there should be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH
+    const auto& add_pubkey = [&candidate_spks](const CPubKey& pub) -> void {
+        candidate_spks.insert(GetScriptForRawPubKey(pub));
+        candidate_spks.insert(GetScriptForDestination(PKHash(pub)));
+
+        CScript wpkh = GetScriptForDestination(WitnessV0KeyHash(pub));
+        candidate_spks.insert(wpkh);
+        candidate_spks.insert(GetScriptForDestination(ScriptHash(wpkh)));
+    };
+    for (const auto& [_, key] : mapKeys) {
+        add_pubkey(key.GetPubKey());
+    }
+    for (const auto& [_, ckeypair] : mapCryptedKeys) {
+        add_pubkey(ckeypair.first);
+    }
+
+    // mapScripts contains all redeemScripts and witnessScripts. Therefore each script in it has
+    // itself, P2SH, P2WSH, and P2SH-P2WSH as a candidate.
+    const auto& add_script = [&candidate_spks](const CScript& script) -> void {
+        candidate_spks.insert(script);
+        candidate_spks.insert(GetScriptForDestination(ScriptHash(script)));
+
+        CScript wsh = GetScriptForDestination(WitnessV0ScriptHash(script));
+        candidate_spks.insert(wsh);
+        candidate_spks.insert(GetScriptForDestination(ScriptHash(wsh)));
+    };
+    for (const auto& [_, script] : mapScripts) {
+        add_script(script);
+    }
+
+    // Although setWatchOnly should only contain output scripts, we will also include each script's
+    // P2SH, P2WSH, and P2SH-P2WSH as a precaution.
+    for (const auto& script : setWatchOnly) {
+        add_script(script);
+    }
+
+    return candidate_spks;
+}
+
+std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetScriptPubKeys() const
+{
+    // Run IsMine() on each candidate output script. Any script that is not ISMINE_NO is an output
+    // script to return
     std::unordered_set<CScript, SaltedSipHasher> spks;
-
-    // All keys have at least P2PK and P2PKH
-    for (const auto& key_pair : mapKeys) {
-        const CPubKey& pub = key_pair.second.GetPubKey();
-        spks.insert(GetScriptForRawPubKey(pub));
-        spks.insert(GetScriptForDestination(PKHash(pub)));
-    }
-    for (const auto& key_pair : mapCryptedKeys) {
-        const CPubKey& pub = key_pair.second.first;
-        spks.insert(GetScriptForRawPubKey(pub));
-        spks.insert(GetScriptForDestination(PKHash(pub)));
-    }
-
-    // For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.
-    // The watchonly ones will be in setWatchOnly which we deal with later
-    // For all keys, if they have segwit scripts, those scripts will end up in mapScripts
-    for (const auto& script_pair : mapScripts) {
-        const CScript& script = script_pair.second;
-        if (IsMine(script) == ISMINE_SPENDABLE) {
-            // Add ScriptHash for scripts that are not already P2SH
-            if (!script.IsPayToScriptHash()) {
-                spks.insert(GetScriptForDestination(ScriptHash(script)));
-            }
-            // For segwit scripts, we only consider them spendable if we have the segwit spk
-            int wit_ver = -1;
-            std::vector<unsigned char> witprog;
-            if (script.IsWitnessProgram(wit_ver, witprog) && wit_ver == 0) {
-                spks.insert(script);
-            }
-        } else {
-            // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH
-            // So check the P2SH of a multisig to see if we should insert it
-            std::vector<std::vector<unsigned char>> sols;
-            TxoutType type = Solver(script, sols);
-            if (type == TxoutType::MULTISIG) {
-                CScript ms_spk = GetScriptForDestination(ScriptHash(script));
-                if (IsMine(ms_spk) != ISMINE_NO) {
-                    spks.insert(ms_spk);
-                }
-            }
+    for (const CScript& script : GetCandidateScriptPubKeys()) {
+        if (IsMine(script) != ISMINE_NO) {
+            spks.insert(script);
         }
     }
 
-    // All watchonly scripts are raw
-    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;
 }
 
@@ -1925,6 +1925,19 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
 
         // InferDescriptor as that will get us all the solving info if it is there
         std::unique_ptr<Descriptor> desc = InferDescriptor(spk, *GetSolvingProvider(spk));
+
+        // Past bugs in InferDescriptor has caused it to create descriptors which cannot be re-parsed
+        // Re-parse the descriptors to detect that, and skip any that do not parse.
+        {
+            std::string desc_str = desc->ToString();
+            FlatSigningProvider parsed_keys;
+            std::string parse_error;
+            std::vector<std::unique_ptr<Descriptor>> parsed_descs = Parse(desc_str, parsed_keys, parse_error, false);
+            if (parsed_descs.empty()) {
+                continue;
+            }
+        }
+
         // Get the private keys for this descriptor
         std::vector<CScript> scripts;
         FlatSigningProvider keys;
@@ -1974,57 +1987,51 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
         }
     }
 
-    // Multisigs are special. They don't show up as ISMINE_SPENDABLE unless they are in a P2SH
-    // So we have to check if any of our scripts are a multisig and if so, add the P2SH
-    for (const auto& script_pair : mapScripts) {
-        const CScript script = script_pair.second;
+    // Make sure that we have accounted for all scriptPubKeys
+    assert(spks.size() == 0);
+
+    // Legacy wallets can also contains scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
+    // but can provide script data to a PSBT spending them. These "solvable" output scripts will need to
+    // be put into the separate "solvables" wallet.
+    // These can be detected by going through the entire candidate output scripts, finding the ISMINE_NO scripts,
+    // and checking CanProvide() which will dummy sign.
+    for (const CScript& script : GetCandidateScriptPubKeys()) {
+        // Since we only care about P2SH, P2WSH, and P2SH-P2WSH, filter out any scripts that are not those
+        if (!script.IsPayToScriptHash() && !script.IsPayToWitnessScriptHash()) {
+            continue;
+        }
+        if (IsMine(script) != ISMINE_NO) {
+            continue;
+        }
+        SignatureData dummy_sigdata;
+        if (!CanProvide(script, dummy_sigdata)) {
+            continue;
+        }
 
         // Get birthdate from script meta
         uint64_t creation_time = 0;
-        const auto& it = m_script_metadata.find(CScriptID(script));
-        if (it != m_script_metadata.end()) {
-            creation_time = it->second.nCreateTime;
+        const auto& mit = m_script_metadata.find(CScriptID(script));
+        if (mit != m_script_metadata.end()) {
+            creation_time = mit->second.nCreateTime;
         }
 
-        std::vector<std::vector<unsigned char>> sols;
-        TxoutType type = Solver(script, sols);
-        if (type == TxoutType::MULTISIG) {
-            CScript sh_spk = GetScriptForDestination(ScriptHash(script));
-            CTxDestination witdest = WitnessV0ScriptHash(script);
-            CScript witprog = GetScriptForDestination(witdest);
-            CScript sh_wsh_spk = GetScriptForDestination(ScriptHash(witprog));
+        // InferDescriptor as that will get us all the solving info if it is there
+        std::unique_ptr<Descriptor> desc = InferDescriptor(script, *GetSolvingProvider(script));
 
-            // We only want the multisigs that we have not already seen, i.e. they are not watchonly and not spendable
-            // For P2SH, a multisig is not ISMINE_NO when:
-            // * All keys are in the wallet
-            // * The multisig itself is watch only
-            // * The P2SH is watch only
-            // For P2SH-P2WSH, if the script is in the wallet, then it will have the same conditions as P2SH.
-            // For P2WSH, a multisig is not ISMINE_NO when, other than the P2SH conditions:
-            // * The P2WSH script is in the wallet and it is being watched
-            std::vector<std::vector<unsigned char>> keys(sols.begin() + 1, sols.begin() + sols.size() - 1);
-            if (HaveWatchOnly(sh_spk) || HaveWatchOnly(script) || HaveKeys(keys, *this) || (HaveCScript(CScriptID(witprog)) && HaveWatchOnly(witprog))) {
-                // The above emulates IsMine for these 3 scriptPubKeys, so double check that by running IsMine
-                assert(IsMine(sh_spk) != ISMINE_NO || IsMine(witprog) != ISMINE_NO || IsMine(sh_wsh_spk) != ISMINE_NO);
+        // Past bugs in InferDescriptor has caused it to create descriptors which cannot be re-parsed
+        // Re-parse the descriptors to detect that, and skip any that do not parse.
+        {
+            std::string desc_str = desc->ToString();
+            FlatSigningProvider parsed_keys;
+            std::string parse_error;
+            std::vector<std::unique_ptr<Descriptor>> parsed_descs = Parse(desc_str, parsed_keys, parse_error, false);
+            if (parsed_descs.empty()) {
                 continue;
             }
-            assert(IsMine(sh_spk) == ISMINE_NO && IsMine(witprog) == ISMINE_NO && IsMine(sh_wsh_spk) == ISMINE_NO);
-
-            std::unique_ptr<Descriptor> sh_desc = InferDescriptor(sh_spk, *GetSolvingProvider(sh_spk));
-            out.solvable_descs.emplace_back(sh_desc->ToString(), creation_time);
-
-            const auto desc = InferDescriptor(witprog, *this);
-            if (desc->IsSolvable()) {
-                std::unique_ptr<Descriptor> wsh_desc = InferDescriptor(witprog, *GetSolvingProvider(witprog));
-                out.solvable_descs.emplace_back(wsh_desc->ToString(), creation_time);
-                std::unique_ptr<Descriptor> sh_wsh_desc = InferDescriptor(sh_wsh_spk, *GetSolvingProvider(sh_wsh_spk));
-                out.solvable_descs.emplace_back(sh_wsh_desc->ToString(), creation_time);
-            }
         }
-    }
 
-    // Make sure that we have accounted for all scriptPubKeys
-    assert(spks.size() == 0);
+        out.solvable_descs.emplace_back(desc->ToString(), creation_time);
+    }
 
     // Finalize transaction
     if (!batch.TxnCommit()) {

src/wallet/scriptpubkeyman.h

@@ -302,6 +302,9 @@ protected:
     virtual bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);
     bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
 
+    // Helper function to retrieve a set of all output scripts that may be relevant to this LegacyDataSPKM
+    // Used only in migration.
+    std::unordered_set<CScript, SaltedSipHasher> GetCandidateScriptPubKeys() const;
 public:
     using ScriptPubKeyMan::ScriptPubKeyMan;
 
@@ -318,6 +321,7 @@ public:
     uint256 GetID() const override { return uint256::ONE; }
     // TODO: Remove IsMine when deleting LegacyScriptPubKeyMan
     isminetype IsMine(const CScript& script) const override;
+    bool CanProvide(const CScript& script, SignatureData& sigdata) override;
 
     // FillableSigningProvider overrides
     bool HaveKey(const CKeyID &address) const override;
@@ -486,8 +490,6 @@ public:
 
     bool CanGetAddresses(bool internal = false) const override;
 
-    bool CanProvide(const CScript& script, SignatureData& sigdata) override;
-
     bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, bilingual_str>& input_errors) const override;
     SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
     std::optional<common::PSBTError> FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override;

test/functional/wallet_migration.py

@@ -10,9 +10,10 @@ import struct
 import time
 
 from test_framework.address import (
-    script_to_p2sh,
     key_to_p2pkh,
     key_to_p2wpkh,
+    script_to_p2sh,
+    script_to_p2wsh,
 )
 from test_framework.bdb import BTREE_MAGIC
 from test_framework.descriptors import descsum_create
@@ -24,6 +25,7 @@ from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script,
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    find_vout_for_address,
     sha256sum_file,
 )
 from test_framework.wallet_util import (
@@ -102,6 +104,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         # Reload to force write that record
         self.old_node.unloadwallet(wallet_name)
         self.old_node.loadwallet(wallet_name)
+        assert_equal(self.old_node.get_wallet_rpc(wallet_name).getwalletinfo()["descriptors"], False)
         # Now unload so we can copy it to the master node for the migration test
         self.old_node.unloadwallet(wallet_name)
         if wallet_name == "":
@@ -111,7 +114,9 @@ class WalletMigrationTest(BitcoinTestFramework):
         # Migrate, checking that rescan does not occur
         with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
             migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
-        return migrate_info, self.master_node.get_wallet_rpc(wallet_name)
+        rpc = self.master_node.get_wallet_rpc(wallet_name)
+        assert_equal(rpc.getwalletinfo()["descriptors"], True)
+        return migrate_info, rpc
 
     def test_basic(self):
         default = self.master_node.get_wallet_rpc(self.default_wallet_name)
@@ -1060,6 +1065,289 @@ class WalletMigrationTest(BitcoinTestFramework):
         assert_equal(expected_descs, migrated_desc)
         wo_wallet.unloadwallet()
 
+    def test_p2wsh(self):
+        self.log.info("Test that non-multisig P2WSH output scripts are migrated")
+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
+
+        wallet = self.create_legacy_wallet("p2wsh")
+
+        # Craft wsh(pkh(key))
+        pubkey = wallet.getaddressinfo(wallet.getnewaddress())["pubkey"]
+        pkh_script = key_to_p2pkh_script(pubkey).hex()
+        wsh_pkh_script = script_to_p2wsh_script(pkh_script).hex()
+        wsh_pkh_addr = script_to_p2wsh(pkh_script)
+
+        wallet.importaddress(address=pkh_script, p2sh=False)
+        wallet.importaddress(address=wsh_pkh_script, p2sh=False)
+
+        def_wallet.sendtoaddress(wsh_pkh_addr, 5)
+        self.generate(self.nodes[0], 6)
+        assert_equal(wallet.getbalances()['mine']['trusted'], 5)
+
+        _, wallet = self.migrate_and_get_rpc("p2wsh")
+
+        assert_equal(wallet.getbalances()['mine']['trusted'], 5)
+        addr_info = wallet.getaddressinfo(wsh_pkh_addr)
+        assert_equal(addr_info["ismine"], True)
+        assert_equal(addr_info["iswatchonly"], False)
+        assert_equal(addr_info["solvable"], True)
+
+        wallet.unloadwallet()
+
+    def test_disallowed_p2wsh(self):
+        self.log.info("Test that P2WSH output scripts with invalid witnessScripts are not migrated and do not cause migration failure")
+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
+
+        wallet = self.create_legacy_wallet("invalid_p2wsh")
+
+        invalid_addrs = []
+
+        # For a P2WSH output script stored in the legacy wallet's mapScripts, both the native P2WSH
+        # and the P2SH-P2WSH are detected by IsMine. We need to verify that descriptors for both
+        # output scripts are added to the resulting descriptor wallet.
+        # However, this cannot be done using a multisig as wallet migration treats multisigs specially.
+        # Instead, this is tested by importing a wsh(pkh()) script. But importing this directly will
+        # insert the wsh() into setWatchOnly which means that the setWatchOnly migration ends up handling
+        # this case, which we do not want.
+        # In order to get the wsh(pkh()) into only mapScripts and not setWatchOnly, we need to utilize
+        # importmulti and wrap the wsh(pkh()) inside of a sh(). This will insert the sh(wsh(pkh())) into
+        # setWatchOnly but not the wsh(pkh()).
+        # Furthermore, migration should not migrate the wsh(pkh()) if the key is uncompressed.
+        comp_wif, comp_pubkey = generate_keypair(compressed=True, wif=True)
+        comp_pkh_script = key_to_p2pkh_script(comp_pubkey).hex()
+        comp_wsh_pkh_script = script_to_p2wsh_script(comp_pkh_script).hex()
+        comp_sh_wsh_pkh_script = script_to_p2sh_script(comp_wsh_pkh_script).hex()
+        comp_wsh_pkh_addr = script_to_p2wsh(comp_pkh_script)
+
+        uncomp_wif, uncomp_pubkey = generate_keypair(compressed=False, wif=True)
+        uncomp_pkh_script = key_to_p2pkh_script(uncomp_pubkey).hex()
+        uncomp_wsh_pkh_script = script_to_p2wsh_script(uncomp_pkh_script).hex()
+        uncomp_sh_wsh_pkh_script = script_to_p2sh_script(uncomp_wsh_pkh_script).hex()
+        uncomp_wsh_pkh_addr = script_to_p2wsh(uncomp_pkh_script)
+        invalid_addrs.append(uncomp_wsh_pkh_addr)
+
+        import_res = wallet.importmulti([
+            {
+                "scriptPubKey": comp_sh_wsh_pkh_script,
+                "timestamp": "now",
+                "redeemscript": comp_wsh_pkh_script,
+                "witnessscript": comp_pkh_script,
+                "keys": [
+                    comp_wif,
+                ],
+            },
+            {
+                "scriptPubKey": uncomp_sh_wsh_pkh_script,
+                "timestamp": "now",
+                "redeemscript": uncomp_wsh_pkh_script,
+                "witnessscript": uncomp_pkh_script,
+                "keys": [
+                    uncomp_wif,
+                ],
+            },
+        ])
+        assert_equal(import_res[0]["success"], True)
+        assert_equal(import_res[1]["success"], True)
+
+        # Create a wsh(sh(pkh())) - P2SH inside of P2WSH is invalid
+        comp_sh_pkh_script = script_to_p2sh_script(comp_pkh_script).hex()
+        wsh_sh_pkh_script = script_to_p2wsh_script(comp_sh_pkh_script).hex()
+        wsh_sh_pkh_addr = script_to_p2wsh(comp_sh_pkh_script)
+        invalid_addrs.append(wsh_sh_pkh_addr)
+
+        # Import wsh(sh(pkh()))
+        wallet.importaddress(address=comp_sh_pkh_script, p2sh=False)
+        wallet.importaddress(address=wsh_sh_pkh_script, p2sh=False)
+
+        # Create a wsh(wsh(pkh())) - P2WSH inside of P2WSH is invalid
+        wsh_wsh_pkh_script = script_to_p2wsh_script(comp_wsh_pkh_script).hex()
+        wsh_wsh_pkh_addr = script_to_p2wsh(comp_wsh_pkh_script)
+        invalid_addrs.append(wsh_wsh_pkh_addr)
+
+        # Import wsh(wsh(pkh()))
+        wallet.importaddress(address=wsh_wsh_pkh_script, p2sh=False)
+
+        # The wsh(pkh()) with a compressed key is always valid, so we should see that the wallet detects it as ismine, not
+        # watchonly, and can provide us information about the witnessScript via "embedded"
+        comp_wsh_pkh_addr_info = wallet.getaddressinfo(comp_wsh_pkh_addr)
+        assert_equal(comp_wsh_pkh_addr_info["ismine"], True)
+        assert_equal(comp_wsh_pkh_addr_info["iswatchonly"], False)
+        assert "embedded" in comp_wsh_pkh_addr_info
+
+        # The invalid addresses are invalid, so the legcy wallet should not detect them as ismine,
+        # nor consider them watchonly. However, because the legacy wallet has the witnessScripts/redeemScripts,
+        # we should see information about those in "embedded"
+        for addr in invalid_addrs:
+            addr_info = wallet.getaddressinfo(addr)
+            assert_equal(addr_info["ismine"], False)
+            assert_equal(addr_info["iswatchonly"], False)
+            assert "embedded" in addr_info
+
+        # Fund those output scripts
+        def_wallet.send([{comp_wsh_pkh_addr: 1}] + [{k: i + 1} for i, k in enumerate(invalid_addrs)])
+        self.generate(self.nodes[0], 6)
+        assert_equal(wallet.getbalances()["mine"]["trusted"], 1)
+
+        _, wallet = self.migrate_and_get_rpc("invalid_p2wsh")
+
+        assert_equal(wallet.getbalances()["mine"]["trusted"], 1)
+
+        # After migration, the wsh(pkh()) with a compressed key is still valid and the descriptor wallet will have
+        # information about the witnessScript
+        comp_wsh_pkh_addr_info = wallet.getaddressinfo(comp_wsh_pkh_addr)
+        assert_equal(comp_wsh_pkh_addr_info["ismine"], True)
+        assert_equal(comp_wsh_pkh_addr_info["iswatchonly"], False)
+        assert "embedded" in comp_wsh_pkh_addr_info
+
+        # After migration, the invalid addresses should still not be detected as ismine and not watchonly.
+        # The descriptor wallet should not have migrated these at all, so there should additionally be no
+        # information in "embedded" about the witnessScripts/redeemScripts.
+        for addr in invalid_addrs:
+            addr_info = wallet.getaddressinfo(addr)
+            assert_equal(addr_info["ismine"], False)
+            assert_equal(addr_info["iswatchonly"], False)
+            assert "embedded" not in addr_info
+
+        wallet.unloadwallet()
+
+    def test_miniscript(self):
+        # It turns out that due to how signing logic works, legacy wallets that have valid miniscript witnessScripts
+        # and the private keys for them can still sign and spend them, even though output scripts involving them
+        # as a witnessScript would not be detected as ISMINE_SPENDABLE.
+        self.log.info("Test migration of a legacy wallet containing miniscript")
+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
+        wallet = self.create_legacy_wallet("miniscript")
+
+        privkey, _ = generate_keypair(compressed=True, wif=True)
+
+        # Make a descriptor where we only have some of the keys. This will be migrated to the watchonly wallet.
+        some_keys_priv_desc = descsum_create(f"wsh(or_b(pk({privkey}),s:pk(029ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0)))")
+        some_keys_addr = self.master_node.deriveaddresses(some_keys_priv_desc)[0]
+
+        # Make a descriptor where we have all of the keys. This will stay in the migrated wallet
+        all_keys_priv_desc = descsum_create(f"wsh(and_v(v:pk({privkey}),1))")
+        all_keys_addr = self.master_node.deriveaddresses(all_keys_priv_desc)[0]
+
+        imp = wallet.importmulti([
+            {
+                "desc": some_keys_priv_desc,
+                "timestamp": "now",
+            },
+            {
+                "desc": all_keys_priv_desc,
+                "timestamp": "now",
+            }
+        ])
+        assert_equal(imp[0]["success"], True)
+        assert_equal(imp[1]["success"], True)
+
+        def_wallet.sendtoaddress(some_keys_addr, 1)
+        def_wallet.sendtoaddress(all_keys_addr, 1)
+        self.generate(self.master_node, 6)
+        # Double check that the miniscript can be spent by the legacy wallet
+        send_res = wallet.send(outputs=[{some_keys_addr: 1},{all_keys_addr: 0.75}], include_watching=True, change_address=def_wallet.getnewaddress())
+        assert_equal(send_res["complete"], True)
+        self.generate(self.old_node, 6)
+        assert_equal(wallet.getbalances()["watchonly"]["trusted"], 1.75)
+
+        res, wallet = self.migrate_and_get_rpc("miniscript")
+
+        # The miniscript with all keys should be in the migrated wallet
+        assert_equal(wallet.getbalances()["mine"], {"trusted": 0.75, "untrusted_pending": 0, "immature": 0})
+        assert_equal(wallet.getaddressinfo(all_keys_addr)["ismine"], True)
+        assert_equal(wallet.getaddressinfo(some_keys_addr)["ismine"], False)
+
+        # The miniscript with some keys should be in the watchonly wallet
+        assert "miniscript_watchonly" in self.master_node.listwallets()
+        watchonly = self.master_node.get_wallet_rpc("miniscript_watchonly")
+        assert_equal(watchonly.getbalances()["mine"], {"trusted": 1, "untrusted_pending": 0, "immature": 0})
+        assert_equal(watchonly.getaddressinfo(some_keys_addr)["ismine"], True)
+        assert_equal(watchonly.getaddressinfo(all_keys_addr)["ismine"], False)
+
+    def test_taproot(self):
+        # It turns out that due to how signing logic works, legacy wallets that have the private key for a Taproot
+        # output key will be able to sign and spend those scripts, even though they would not be detected as ISMINE_SPENDABLE.
+        self.log.info("Test migration of Taproot scripts")
+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
+        wallet = self.create_legacy_wallet("taproot")
+
+        privkey, _ = generate_keypair(compressed=True, wif=True)
+
+        rawtr_desc = descsum_create(f"rawtr({privkey})")
+        rawtr_addr = self.master_node.deriveaddresses(rawtr_desc)[0]
+        rawtr_spk = self.master_node.validateaddress(rawtr_addr)["scriptPubKey"]
+        tr_desc = descsum_create(f"tr({privkey})")
+        tr_addr = self.master_node.deriveaddresses(tr_desc)[0]
+        tr_spk = self.master_node.validateaddress(tr_addr)["scriptPubKey"]
+        tr_script_desc = descsum_create(f"tr(9ffbe722b147f3035c87cb1c60b9a5947dd49c774cc31e94773478711a929ac0,pk({privkey}))")
+        tr_script_addr = self.master_node.deriveaddresses(tr_script_desc)[0]
+        tr_script_spk = self.master_node.validateaddress(tr_script_addr)["scriptPubKey"]
+
+        wallet.importaddress(rawtr_spk)
+        wallet.importaddress(tr_spk)
+        wallet.importaddress(tr_script_spk)
+        wallet.importprivkey(privkey)
+
+        txid = def_wallet.send([{rawtr_addr: 1},{tr_addr: 2}, {tr_script_addr: 3}])["txid"]
+        rawtr_vout = find_vout_for_address(self.master_node, txid, rawtr_addr)
+        tr_vout = find_vout_for_address(self.master_node, txid, tr_addr)
+        tr_script_vout = find_vout_for_address(self.master_node, txid, tr_script_addr)
+        self.generate(self.master_node, 6)
+
+        # Double check that the rawtr can be spent by the legacy wallet
+        send_res = wallet.send(outputs=[{rawtr_addr: 0.5}], include_watching=True, change_address=def_wallet.getnewaddress(), inputs=[{"txid": txid, "vout": rawtr_vout}])
+        assert_equal(send_res["complete"], True)
+        self.generate(self.old_node, 6)
+        assert_equal(wallet.getbalances()["watchonly"]["trusted"], 5.5)
+
+        # Check that the tr() cannot be spent by the legacy wallet
+        send_res = wallet.send(outputs=[{def_wallet.getnewaddress(): 4}], include_watching=True, inputs=[{"txid": txid, "vout": tr_vout}, {"txid": txid, "vout": tr_script_vout}])
+        assert_equal(send_res["complete"], False)
+
+        res, wallet = self.migrate_and_get_rpc("taproot")
+
+        # The rawtr should be migrated
+        assert_equal(wallet.getbalances()["mine"], {"trusted": 0.5, "untrusted_pending": 0, "immature": 0})
+        assert_equal(wallet.getaddressinfo(rawtr_addr)["ismine"], True)
+        assert_equal(wallet.getaddressinfo(tr_addr)["ismine"], False)
+        assert_equal(wallet.getaddressinfo(tr_script_addr)["ismine"], False)
+
+        # The miniscript with some keys should be in the watchonly wallet
+        assert "taproot_watchonly" in self.master_node.listwallets()
+        watchonly = self.master_node.get_wallet_rpc("taproot_watchonly")
+        assert_equal(watchonly.getbalances()["mine"], {"trusted": 5, "untrusted_pending": 0, "immature": 0})
+        assert_equal(watchonly.getaddressinfo(rawtr_addr)["ismine"], False)
+        assert_equal(watchonly.getaddressinfo(tr_addr)["ismine"], True)
+        assert_equal(watchonly.getaddressinfo(tr_script_addr)["ismine"], True)
+
+    def test_solvable_no_privs(self):
+        self.log.info("Test migrating a multisig that we do not have any private keys for")
+        wallet = self.create_legacy_wallet("multisig_noprivs")
+
+        privkey, pubkey = generate_keypair(compressed=True, wif=True)
+
+        add_ms_res = wallet.addmultisigaddress(nrequired=1, keys=[pubkey.hex()])
+        addr = add_ms_res["address"]
+
+        # The multisig address should be ISMINE_NO but we should have the script info
+        addr_info = wallet.getaddressinfo(addr)
+        assert_equal(addr_info["ismine"], False)
+        assert "hex" in addr_info
+
+        migrate_res, wallet = self.migrate_and_get_rpc("multisig_noprivs")
+        assert_equal(migrate_res["solvables_name"], "multisig_noprivs_solvables")
+        solvables = self.master_node.get_wallet_rpc(migrate_res["solvables_name"])
+
+        # The multisig should not be in the spendable wallet
+        addr_info = wallet.getaddressinfo(addr)
+        assert_equal(addr_info["ismine"], False)
+        assert "hex" not in addr_info
+
+        # The multisig address should be in the solvables wallet
+        addr_info = solvables.getaddressinfo(addr)
+        assert_equal(addr_info["ismine"], True)
+        assert "hex" in addr_info
+
     def run_test(self):
         self.master_node = self.nodes[0]
         self.old_node = self.nodes[1]
@@ -1087,7 +1375,11 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_blank()
         self.test_migrate_simple_watch_only()
         self.test_manual_keys_import()
-
+        self.test_p2wsh()
+        self.test_disallowed_p2wsh()
+        self.test_miniscript()
+        self.test_taproot()
+        self.test_solvable_no_privs()
 
 if __name__ == '__main__':
     WalletMigrationTest(__file__).main()