Make Parse function non-recursive #68
Conversation
meshcollider
force-pushed
the
202109_nonrecursive_parse_alone
branch
from
3fe3304 to
8e191ad
Compare
|
Rebased and addressed @darosior's comment |
|
Sorry, my suggestion is outdated now. Let's wait until we settled on what should be in |
|
Rebase? |
meshcollider
force-pushed
the
202109_nonrecursive_parse_alone
branch
from
8a552e9 to
0d3594a
Compare
|
Rebased 🙂 |
|
Can I haz rerebase? |
meshcollider
force-pushed
the
202109_nonrecursive_parse_alone
branch
3 times, most recently
from
e4cf81a to
8196fb0
Compare
|
Rebased |
meshcollider
force-pushed
the
202109_nonrecursive_parse_alone
branch
from
8196fb0 to
dcaf9ac
Compare
|
Done! |
|
Added a commit to just consume the input span directly rather than working with the iterator, and used the spanparsing::Const function rather than the custom StartsWith. Still passes all the tests but apologies for adding more to review. |
meshcollider
force-pushed
the
202109_nonrecursive_parse_alone
branch
from
7e0d13b to
60879af
Compare
|
Updated |
| } else if (Const("pk(", in)) { | ||
| Key key; | ||
| int key_size = FindNextChar(in, ')'); | ||
| if (key_size < 1) return {}; |
There was a problem hiding this comment.
I think this is technically making the rules somewhat stricter, in that before it it was possible to have a ctx that accepted empty strings as key expressions, while now they're unconditionally rejected. I don't think that's a problem, though.
|
utACK 60879af |
|
The changes here are now active on http://bitcoin.sipa.be/miniscript/. |
There was a problem hiding this comment.
Apart from not checking the bounds on thresh(), the code looks good to me.
I've been fuzzing the new parsing function using a modified parse_descriptor fuzzing target overnight, with a corpus expanded using the previous parse function. So far i did not find any crash in parsing nor failure to roundtrip introduced here.
(I seeded the corpus with valid Miniscripts, which might explain why the fuzzer didn't find the thresh() regression..)
| } 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 {}; | ||
| 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::WRAPPED_EXPR, -1, -1); |
There was a problem hiding this comment.
Hmm looks like you would accept a k = 0?
| // Children are constructed in reverse order, so iterate from end to beginning | ||
| std::vector<NodeRef<Key>> subs; | ||
| for (int i = 0; i < n; ++i) { | ||
| subs.push_back(std::move(constructed.back())); | ||
| constructed.pop_back(); | ||
| } | ||
| std::reverse(subs.begin(), subs.end()); | ||
| constructed.push_back(MakeNodeRef<Key>(NodeType::THRESH, std::move(subs), k)); |
There was a problem hiding this comment.
You need to check that k <= subs.size().
| auto ret = Parse<typename Ctx::Key>(span, ctx); | ||
| if (!ret) return {}; |
There was a problem hiding this comment.
nit:
auto ret = Parse<typename Ctx::Key>(span, ctx);
if (!ret) return {};
return ret;Could just be
return Parse<typename Ctx::Key>(span, ctx);
Follow-on from #66
This makes Parse non-recursive too.
disclaimer: I haven't thoroughly tested this since rebasing so using the grammar again to test this thoroughly would be great.