Merge bitcoin/bitcoin#27997: Descriptors: rule out unspendable miniscript descriptors

c7db88af71b3204171f33399aa4f33b40a4f7cd9 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec7431c286139b76f8759719a99a8551 qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c9759a7a582c5c86fdbfa5ea99cb7bb16 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b53006d74d11f3cf9d7a9dc7ff2c39e miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)

Pull request description:

  `IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.

  This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).

ACKs for top commit:
  sipa:
    utACK c7db88af71b3204171f33399aa4f33b40a4f7cd9
  achow101:
    ACK c7db88af71b3204171f33399aa4f33b40a4f7cd9

Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5

2 files changed, 27 insertions, 6 deletions

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index dff6429259..f57f167e5d 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -15,6 +15,7 @@
 #include <common/args.h>
 #include <span.h>
 #include <util/bip32.h>
+#include <util/check.h>
 #include <util/spanparsing.h>
 #include <util/strencodings.h>
 #include <util/vector.h>
@@ -1553,14 +1554,14 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
                 error = std::move(parser.m_key_parsing_error);
                 return nullptr;
             }
-            if (!node->IsSane()) {
+            if (!node->IsSane() || node->IsNotSatisfiable()) {
                 // Try to find the first insane sub for better error reporting.
                 auto insane_node = node.get();
                 if (const auto sub = node->FindInsaneSub()) insane_node = sub;
                 if (const auto str = insane_node->ToString(parser)) error = *str;
                 if (!insane_node->IsValid()) {
                     error += " is invalid";
-                } else {
+                } else if (!node->IsSane()) {
                     error += " is not sane";
                     if (!insane_node->IsNonMalleable()) {
                         error += ": malleable witnesses exist";
@@ -1573,9 +1574,14 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
                     } else if (!insane_node->ValidSatisfactions()) {
                         error += ": needs witnesses that may exceed resource limits";
                     }
+                } else {
+                    error += " is not satisfiable";
                 }
                 return nullptr;
             }
+            // A signature check is required for a miniscript to be sane. Therefore no sane miniscript
+            // may have an empty list of public keys.
+            CHECK_NONFATAL(!parser.m_keys.empty());
             return std::make_unique<MiniscriptDescriptor>(std::move(parser.m_keys), std::move(node));
         }
     }
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 7c1a87a7dc..b58740a125 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1134,20 +1134,35 @@ public:
     size_t ScriptSize() const { return scriptlen; }
 
     //! Return the maximum number of ops needed to satisfy this script non-malleably.
-    uint32_t GetOps() const { return ops.count + ops.sat.value; }
+    std::optional<uint32_t> GetOps() const {
+        if (!ops.sat.valid) return {};
+        return ops.count + ops.sat.value;
+    }
 
     //! Return the number of ops in the script (not counting the dynamic ones that depend on execution).
     uint32_t GetStaticOps() const { return ops.count; }
 
     //! Check the ops limit of this script against the consensus limit.
-    bool CheckOpsLimit() const { return GetOps() <= MAX_OPS_PER_SCRIPT; }
+    bool CheckOpsLimit() const {
+        if (const auto ops = GetOps()) return *ops <= MAX_OPS_PER_SCRIPT;
+        return true;
+    }
 
     /** Return the maximum number of stack elements needed to satisfy this script non-malleably, including
      * the script push. */
-    uint32_t GetStackSize() const { return ss.sat.value + 1; }
+    std::optional<uint32_t> GetStackSize() const {
+        if (!ss.sat.valid) return {};
+        return ss.sat.value + 1;
+    }
 
     //! Check the maximum stack size for this script against the policy limit.
-    bool CheckStackSize() const { return GetStackSize() - 1 <= MAX_STANDARD_P2WSH_STACK_ITEMS; }
+    bool CheckStackSize() const {
+        if (const auto ss = GetStackSize()) return *ss - 1 <= MAX_STANDARD_P2WSH_STACK_ITEMS;
+        return true;
+    }
+
+    //! Whether no satisfaction exists for this node.
+    bool IsNotSatisfiable() const { return !GetStackSize(); }
 
     //! Return the expression type.
     Type GetType() const { return typ; }