Repository: bitcoin/bitcoin
File: src/wallet/scriptpubkeyman.cpp (C++)

Code before:

        // Make the descriptor string with private keys
        std::string desc_str;
        bool watchonly = !desc->ToPrivateString(*this, desc_str);
        if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
            out.watch_descs.emplace_back(desc->ToString(), creation_time);

            // Get the scriptPubKeys without writing this to the wallet
            FlatSigningProvider provider;
            desc->Expand(0, provider, desc_spks, provider);
        } else {
            // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
            WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
            auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
            for (const auto& keyid : privkeyids) {
                CKey key;
                if (!GetKey(keyid, key)) {
                    continue;
                }
                WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
            }
            desc_spk_man->TopUpWithDB(batch);
            auto desc_spks_set = desc_spk_man->GetScriptPubKeys();
            desc_spks.insert(desc_spks.end(), desc_spks_set.begin(), desc_spks_set.end());

            out.desc_spkms.push_back(std::move(desc_spk_man));
        }

        // Remove the scriptPubKeys from our current set
        for (const CScript& desc_spk : desc_spks) {
            auto del_it = spks.find(desc_spk);
            assert(del_it != spks.end());
            assert(IsMine(desc_spk));
            it = spks.erase(del_it);
        }
    }

    // Make sure that we have accounted for all scriptPubKeys
    if (!Assume(spks.empty())) {
        LogError("%s", STR_INTERNAL_BUG("Error: Some output scripts were not migrated."));
        return std::nullopt;
    }

    // Legacy wallets can also contain 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 not IsMine 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()) {

Review comment (bug):
In 2551fdfdd662dfb4077c3059b437acf56bdbaf5b "wallet/migration: use count private key count to identify watchonly descs"

This evaluation of `watchonly` at a barebones level looks great to me! Though maybe we can put this inside a `IsWatchOnly` function in the Descriptor{Impl} class? Reading all these details in the migration flow might be distracting, and moreover this generic function might have its own use cases separate from this later. Following patch builds and tests fine on my system. I have added few minor renaming changes as well. 

<details open>
<summary>Descriptor IsWatchOnly()</summary>

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index bd85ddd9e2..bf67b8c43e 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -646,6 +646,31 @@ public:
         return false;
     }
 
+    bool IsWatchOnly(const SigningProvider& arg) const override
+    {
+        size_t privkey_count{0};
+        std::set<CPubKey> pubkeys;
+        std::set<CExtPubKey> ext_pubkeys;
+        this->GetPubKeys(pubkeys, ext_pubkeys);
+        auto output_type{this->GetOutputType()};
+        for (auto pubkey : pubkeys) {
+            CKey key;
+            if (arg.GetKey(pubkey.GetID(), key)) {
+                privkey_count += 1;
+            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && arg.GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {
+                privkey_count += 1;
+            }
+        }
+        for (auto extpubkey : ext_pubkeys) {
+            CKey dummy;
+            if (arg.GetKey(extpubkey.pubkey.GetID(), dummy)) {
+                privkey_count += 1;
+            }
+        }
+        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
+        return (privkey_count == 0 || pubkey_count > privkey_count);
+    }
+
     // NOLINTNEXTLINE(misc-no-recursion)
     virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, bool* has_priv_key, const DescriptorCache* cache = nullptr) const
     {
diff --git a/src/script/descriptor.h b/src/script/descriptor.h
index 09e9840645..f2e40b7f7a 100644
--- a/src/script/descriptor.h
+++ b/src/script/descriptor.h
@@ -111,6 +111,9 @@ struct Descriptor {
     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
     virtual bool IsSingleType() const = 0;
 
+    /** Whether this descriptor doesn't contain any private key material */
+    virtual bool IsWatchOnly(const SigningProvider& provider) const = 0;
+
     /** Convert the descriptor to a private string. This uses public keys if the relevant private keys are not in the SigningProvider.
      *  If none of the relevant private keys are available, the output string in the "out" parameter will not contain any private key information,
      *  and this function will return "false".
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 117fdd3a5e..e403bf3bbe 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -721,27 +721,7 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
 
         std::vector<CScript> desc_spks;
 
-        size_t privkey_count{0};
-        std::set<CPubKey> pubkeys;
-        std::set<CExtPubKey> ext_pubkeys;
-        desc->GetPubKeys(pubkeys, ext_pubkeys);
-        auto output_type{desc->GetOutputType()};
-        for (auto pubkey : pubkeys) {
-            CKey key;
-            if (GetKey(pubkey.GetID(), key)) {
-                privkey_count += 1;
-            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {
-                privkey_count += 1;
-            }
-        }
-        for (auto extpubkey : ext_pubkeys) {
-            CKey key;
-            if (GetKey(extpubkey.pubkey.GetID(), key)) {
-                privkey_count += 1;
-            }
-        }
-        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
-        bool watchonly{privkey_count == 0 || pubkey_count > privkey_count};
+        bool watchonly = desc->IsWatchOnly(*this);
         if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
             out.watch_descs.emplace_back(desc->ToString(), creation_time);
 
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
index 0c69849d0b..29f174be81 100644
--- a/src/wallet/test/walletload_tests.cpp
+++ b/src/wallet/test/walletload_tests.cpp
@@ -26,6 +26,7 @@ public:
     bool IsRange() const override { return false; }
     bool IsSolvable() const override { return false; }
     bool IsSingleType() const override { return true; }
+    bool IsWatchOnly(const SigningProvider&) const override { return false; }
     bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; }
     bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; }
     bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; };

```
</details>

Code after:

        // If we can't provide all private keys for this inferred descriptor,
        // but this wallet is not watch-only, migrate it to the watch-only wallet.
        if (!desc->HavePrivateKeys(*this) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
            out.watch_descs.emplace_back(desc->ToString(), creation_time);

            // Get the scriptPubKeys without writing this to the wallet
            FlatSigningProvider provider;
            desc->Expand(0, provider, desc_spks, provider);
        } else {
            // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
            WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
            auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
            for (const auto& keyid : privkeyids) {
                CKey key;
                if (!GetKey(keyid, key)) {
                    continue;
                }
                WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
            }
            desc_spk_man->TopUpWithDB(batch);
            auto desc_spks_set = desc_spk_man->GetScriptPubKeys();
            desc_spks.insert(desc_spks.end(), desc_spks_set.begin(), desc_spks_set.end());

            out.desc_spkms.push_back(std::move(desc_spk_man));
        }

        // Remove the scriptPubKeys from our current set
        for (const CScript& desc_spk : desc_spks) {
            auto del_it = spks.find(desc_spk);
            assert(del_it != spks.end());
            assert(IsMine(desc_spk));
            it = spks.erase(del_it);
        }
    }

    // Make sure that we have accounted for all scriptPubKeys
    if (!Assume(spks.empty())) {
        LogPrintf("%s\n", STR_INTERNAL_BUG("Error: Some output scripts were not migrated.\n"));
        return std::nullopt;
    }

    // Legacy wallets can also contain 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 not IsMine 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;