miniscript: Use `ToIntegral` instead of `ParseInt64` #30577

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2024-07-miniscript-tointegral changing 2 files +21 −16

brunoerg commented at 9:29 PM on August 2, 2024: contributor

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 https://github.com/brunoerg/bitcoinfuzz/issues/34). This PR fixes it.
DrahtBot commented at 9:29 PM on August 2, 2024: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK tdb3, danielabrozzoni, darosior, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Descriptors on Aug 2, 2024

Type	Reviewers
ACK	tdb3, danielabrozzoni, darosior, achow101

in src/script/miniscript.h:1952 in 95ef8a2e52 outdated

1948 | @@ -1948,35 +1949,33 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
1949 |              } else if (Const("after(", in)) {
1950 |                  int arg_size = FindNextChar(in, ')');
1951 |                  if (arg_size < 1) return {};
1952 | -                int64_t num;
1953 | -                if (!ParseInt64(std::string(in.begin(), in.begin() + arg_size), &num)) return {};
1954 | -                if (num < 1 || num >= 0x80000000L) return {};
1955 | -                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, num));
1956 | +                const auto num{ToIntegral<int64_t>(std::string(in.begin(), in.begin() + arg_size))};

maflcko commented at 9:15 AM on August 3, 2024:

nit: A string_view should be enough and avoid a string copy to insert an unused null terminator?

tdb3 commented at 1:42 AM on August 5, 2024:

Yes, seems more efficient to use string_view here. Same for the other ToIntegral() calls (useful string_view constructors may help).

https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view

brunoerg commented at 11:19 AM on August 5, 2024:

Yes, I'll address it.

brunoerg commented at 11:20 AM on August 5, 2024:

Yes, I'll address it.

maflcko commented at 9:16 AM on August 3, 2024: member

I couldn't find a test vector in https://github.com/bitcoin/bips/blob/master/bip-0379.md#test-vectors, but it would be good to add one in this pull request at least?
tdb3 commented at 2:01 AM on August 5, 2024: contributor

Approach ACK Nice. Seems to make things safer.

Looks like after, older, multi, and thresh are exercised in the existing test/functional/wallet_miniscript.py.

I agree with maflko's comment about using string_view over string.

darosior commented at 9:02 AM on August 5, 2024: member

Concept ACK. Good catch, again.

This is not a breaking change for any hypothetical user who'd have imported such a descriptor to their wallet, since we use our own serialization to derive the descriptor id and store in the wallet database.

This is a minor breaking change of the RPC interface. But it's fine because it's corrective: i don't think anybody would be relying on this incorrect behaviour as it serves no additional purpose and there exists no compiler or software library that would produce such descriptors.

Let's cover those in the misc unit test?

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index c99a4594ce..b155adff85 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -698,6 +698,11 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     BOOST_CHECK(ms_ins && ms_ins->IsValid() && !ms_ins->IsSane());
     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

miniscript: Use `ToIntegral` instead of `ParseInt64` 6714276d72
brunoerg force-pushed on Aug 5, 2024
brunoerg commented at 11:36 AM on August 5, 2024: contributor

Thanks @maflcko, @tdb3 and @darosior.

Force-pushed. Changed that to use string_view and added the tests suggested by @darosior.
tdb3 approved
tdb3 commented at 1:41 PM on August 5, 2024: contributor

cr ACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc Ran unit/functionals locally (passed).
DrahtBot requested review from darosior on Aug 5, 2024
brunoerg requested review from maflcko on Aug 5, 2024
danielabrozzoni approved
danielabrozzoni commented at 7:19 PM on August 5, 2024: contributor

tACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
darosior commented at 9:23 AM on August 6, 2024: member

utACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
achow101 commented at 12:00 AM on August 7, 2024: member

ACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
achow101 merged this on Aug 7, 2024
achow101 closed this on Aug 7, 2024
achow101 referenced this in commit 51d76634fb on Apr 29, 2025
bitcoin locked this on Aug 7, 2025

Contributors

Review Requested
maflcko

Labels

Descriptors

Linked (view graph)

#32365 descriptors: Reject + sign while parsing unsigned

miniscript: Use `ToIntegral` instead of `ParseInt64` #30577

Code Coverage

Reviews