Merge bitcoin/bitcoin#30577: miniscript: Use ToIntegral instead of ParseInt64

6714276 miniscript: Use `ToIntegral` instead of `ParseInt64` (brunoerg)

Pull request description:

  Currently, miniscript code uses `ParseInt64` function for `after`, `older`, `multi` and `thresh` fragments. It means that a leading `+` or whitespace, among other things, are accepted into the fragments. However, these cases are not useful and cause Bitcoin Core to behave differently compared to other miniscript implementations (see bitcoinfuzz/bitcoinfuzz#34). This PR fixes it.

ACKs for top commit:
  achow101:
    ACK 6714276
  tdb3:
    cr ACK 6714276
  danielabrozzoni:
    tACK 6714276
  darosior:
    utACK 6714276

Tree-SHA512: d9eeb93f380f346d636513eeaf26865285e7b0907b8ed258fe1e02153a9eb69d484c82180eb1c78b0ed77ad5f0e5b244be6672c2f890b1d9fddc9e844bee6dde

Author: achow101

src/script/miniscript.h

@@ -1793,8 +1793,9 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
         // Get threshold
         int next_comma = FindNextChar(in, ',');
         if (next_comma < 1) return false;
-        int64_t k;
-        if (!ParseInt64(std::string(in.begin(), in.begin() + next_comma), &k)) return false;
+        const auto k_to_integral{ToIntegral<int64_t>(std::string_view(in.begin(), next_comma))};
+        if (!k_to_integral.has_value()) return false;
+        const int64_t k{k_to_integral.value()};
         in = in.subspan(next_comma + 1);
         // Get keys. It is compatible for both compressed and x-only keys.
         std::vector<Key> keys;
@@ -1948,35 +1949,33 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
             } else if (Const("after(", in)) {
                 int arg_size = FindNextChar(in, ')');
                 if (arg_size < 1) return {};
-                int64_t num;
-                if (!ParseInt64(std::string(in.begin(), in.begin() + arg_size), &num)) return {};
-                if (num < 1 || num >= 0x80000000L) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, num));
+                const auto num{ToIntegral<int64_t>(std::string_view(in.begin(), arg_size))};
+                if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
+                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num));
                 in = in.subspan(arg_size + 1);
-                script_size += 1 + (num > 16) + (num > 0x7f) + (num > 0x7fff) + (num > 0x7fffff);
+                script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
             } else if (Const("older(", in)) {
                 int arg_size = FindNextChar(in, ')');
                 if (arg_size < 1) return {};
-                int64_t num;
-                if (!ParseInt64(std::string(in.begin(), in.begin() + arg_size), &num)) return {};
-                if (num < 1 || num >= 0x80000000L) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, num));
+                const auto num{ToIntegral<int64_t>(std::string_view(in.begin(), arg_size))};
+                if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
+                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num));
                 in = in.subspan(arg_size + 1);
-                script_size += 1 + (num > 16) + (num > 0x7f) + (num > 0x7fff) + (num > 0x7fffff);
+                script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
             } else if (Const("multi(", in)) {
                 if (!parse_multi_exp(in, /* is_multi_a = */false)) return {};
             } else if (Const("multi_a(", in)) {
                 if (!parse_multi_exp(in, /* is_multi_a = */true)) return {};
             } else if (Const("thresh(", in)) {
                 int next_comma = FindNextChar(in, ',');
                 if (next_comma < 1) return {};
-                if (!ParseInt64(std::string(in.begin(), in.begin() + next_comma), &k)) return {};
-                if (k < 1) return {};
+                const auto k{ToIntegral<int64_t>(std::string_view(in.begin(), next_comma))};
+                if (!k.has_value() || *k < 1) return {};
                 in = in.subspan(next_comma + 1);
                 // n = 1 here because we read the first WRAPPED_EXPR before reaching THRESH
-                to_parse.emplace_back(ParseContext::THRESH, 1, k);
+                to_parse.emplace_back(ParseContext::THRESH, 1, *k);
                 to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1);
-                script_size += 2 + (k > 16) + (k > 0x7f) + (k > 0x7fff) + (k > 0x7fffff);
+                script_size += 2 + (*k > 16) + (*k > 0x7f) + (*k > 0x7fff) + (*k > 0x7fffff);
             } else if (Const("andor(", in)) {
                 to_parse.emplace_back(ParseContext::ANDOR, -1, -1);
                 to_parse.emplace_back(ParseContext::CLOSE_BRACKET, -1, -1);

src/test/miniscript_tests.cpp

@@ -699,6 +699,12 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     const auto insane_sub = ms_ins->FindInsaneSub();
     BOOST_CHECK(insane_sub && *insane_sub->ToString(wsh_converter) == "and_b(after(1),a:after(1000000000))");
 
+    // Numbers can't be prefixed by a sign.
+    BOOST_CHECK(!miniscript::FromString("after(-1)", wsh_converter));
+    BOOST_CHECK(!miniscript::FromString("after(+1)", wsh_converter));
+    BOOST_CHECK(!miniscript::FromString("thresh(-1,pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204))", wsh_converter));
+    BOOST_CHECK(!miniscript::FromString("multi(+1,03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204)", wsh_converter));
+
     // Timelock tests
     Test("after(100)", "?", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only heightlock
     Test("after(1000000000)", "?", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only timelock