ToIntegral instead of ParseInt64
#30577
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
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))};
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
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.
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?
0diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
1index c99a4594ce..b155adff85 100644
2--- a/src/test/miniscript_tests.cpp
3+++ b/src/test/miniscript_tests.cpp
4@@ -698,6 +698,11 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
5 BOOST_CHECK(ms_ins && ms_ins->IsValid() && !ms_ins->IsSane());
6 const auto insane_sub = ms_ins->FindInsaneSub();
7 BOOST_CHECK(insane_sub && *insane_sub->ToString(wsh_converter) == "and_b(after(1),a:after(1000000000))");
8+ // Numbers can't be prefixed by a sign.
9+ BOOST_CHECK(!miniscript::FromString("after(-1)", wsh_converter));
10+ BOOST_CHECK(!miniscript::FromString("after(+1)", wsh_converter));
11+ BOOST_CHECK(!miniscript::FromString("thresh(-1,pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204))", wsh_converter));
12+ BOOST_CHECK(!miniscript::FromString("multi(+1,03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204)", wsh_converter));
13
14 // Timelock tests
15 Test("after(100)", "?", "?", TESTMODE_VALID | TESTMODE_NONMAL); // only heightlock
