miniscript: Use ToIntegral instead of ParseInt64
#30577
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
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
left a comment
tdb3
left a comment
There was a problem hiding this comment.
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
left a comment
darosior
left a comment
There was a problem hiding this comment.
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
brunoerg
force-pushed
the
2024-07-miniscript-tointegral
branch
from
95ef8a2 to
6714276
Compare
|
ACK 6714276 |
fa655da test: [refactor] Use ToIntegral in CheckInferDescriptor (MarcoFalke) fa55dd0 descriptors: Reject + sign when parsing multi threshold (MarcoFalke) fa6f77e descriptors: Reject + sign in ParseKeyPathNum (MarcoFalke) Pull request description: As a follow-up to #30577, reject `+` for unsigned values in key-path parsing and multi threshold parsing as well. Both of those are using unsigned, and Bitcoin Core would never serialize a descriptor string with a stray `+`. Accepting stray `+` signs could lead to checksum mismatches, or other incompatibilities later on. Just like #30577, both changes are breaking changes on the RPC interface, but hopefully no one should be relying on this behavior in production. Similarly, both changes should be fine for the wallet, because it normalizes the strings on import, see #30577 (review). ACKs for top commit: achow101: ACK fa655da brunoerg: code review ACK fa655da janb84: tACK [fa655da](fa655da) Tree-SHA512: d0c7262a167f7ba98b44ed8bf49ff4c15a1eb647cbac39a59b83c7cc379903c24dae3996e5f557497eb08e16d7121417916147058d97bdf168cd6eada57dceef
7 participants
Currently, miniscript code uses
ParseInt64function forafter,older,multiandthreshfragments. 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.