Rewrite expression parser to be non-recursive #775
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apoelstra
force-pushed
the
2024-11--expression-2
branch
from
3a20537
to
6a8d687
Compare
|
Will run benchmarks once my server is idle. |
Benchmarks on masterBenchmarks with this PRAs you can see, again there is a performance regression, this time (I think) because we have converted a bunch of stack-based allocations into heap-based allocations. But on my local branch that includes the next several PRs, these numbers do improve (I swear). They roughly match the "master" numbers from #773 for small benchmarks, and significantly beat the "master" numbers for large benchmarks. |
|
There is a fuzztest failing on master, which I confirmed passes with this PR. |
sanket1729
approved these changes
Nov 23, 2024
There was a problem hiding this comment.
Awesome work adding the position to the Tree structure.
We can improve the "Unexpected("(1 args) while parsing Miniscript")" with the exact position where the parsing failed :O . Huge UX improvment
This includes the stringly-typed
BadDescriptor:).
Nice work removing this finally.
| @@ -64,6 +64,9 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> { | |||
| Pk: FromStr, | |||
| <Pk as FromStr>::Err: fmt::Display, | |||
| { | |||
| tree.verify_toplevel("sortedmulti", 1..) | |||
There was a problem hiding this comment.
Nice catch. This was missing previously
sanket1729
reviewed
Nov 23, 2024
There was a problem hiding this comment.
The only subtle/nonobvious thing maybe is that in Miniscript::from_tree
I added a call to verify_no_curly_braces which does a recursive check
for curly-braces. None of the other checks are recursive (nor do they
need to be).
From commit message of c8aa7d2
Where is the recursion?
|
Tried to do my best careful review. But we do need fuzzing on this to gain more confidence |
sanket1729
previously approved these changes
Nov 23, 2024
I have a fuzztest that runs against the version of 0.12 from #735. I'll publish the fuzz test once that is merged. Also I will check on the commit message that mentions recursion -- may be the commit message is just stale. |
Along the way, fix a bunch of other instances of "manual" slice construction that can be done with try_from or from_fn. Instances where the closure to `from_fn` would be several lines long, I left alone, but we could revisit those later. Fixes rust-bitcoin#774
This commit should yield no observable changes.
|
Yeah, the commit message is stale. It predates my use of |
When we change the expression parser to start parsing both ()s and {}s
at once, we will need to know the parenthesis type. To return nice
errors we also need to store some position information in the Tree type.
Adding these new fields (which need to be pub to make them accessible
from descriptor/tr.rs, but which we will later encapsulate better) is
mechanical and pretty noisy, so we do it in its own commit to reduce the
size of the real "fix Taproot parsing" commit.
In the next commit we will change the expression parser to parse both
{}s and ()s as parentheses, no longer distinguishing between a "taproot"
and "non-taproot" mode. This means that all non-Taproot descriptors need
to check that curly-brace {} expressions do not appear.
While we are adding this check, we can replace the existing checks for
things like "does this start with wsh and have exactly one child" with
an encapsulated function with strongly-typed errors. This gets rid of a
couple of Error::Unexpected instances.
We change one error output (if you pass no children to a sortedmulti).
The old text is nonsensical and the new text is explicit about what is
wrong.
This change is pretty-much mechanical, though unfortunately these are
all "manual" calls to validation functions, and if I missed any, the
compiler won't give us any help in noticing. But there aren't too many.
Anyway, later on I will write a fuzz test which checks that we have not
changed the set of parseable descriptors (using normal keys, not Strings
or anything that might have braces in them, which we know we broke) and
that should catch any mistakes.
Also, similar to the last commit, this one doesn't really "do" anything
because it's still impossible to parse trees with mixed brace styles.
But in the next one, it will be possible, and we will be glad to have
moved a bunch of the diff into these prepatory commits.
We will use this error type in the next commit. For now we will continue to use the giant global Error enum, so we add a variant to hold the new `ParseError`. But eventually `ParseError` will become a top-level error and `Error` will go away.
The `expression::Tree` type now parses expression trees containing both
{} and () as children. It marks which is which, and it is the
responsibility of FromTree implementors to make sure that the correct
style is used.
This eliminates a ton of ad-hoc string parsing code from
descriptor/tr.rs, including 8 instances of Error::Unexpected. It is also
the first change that preserves (sorta) a pubkey parsing error rather
than converting it to a string.
Because of rust-bitcoin#734
this inserts a call to `Descriptor::sanity_check` when parsing a string,
specifically for Taproot descriptors. This is weird and wrong but we
want to address it separately from this PR, whose goal is to preserve
all behavior.
The correct way to build a Error::ParseTree is to make an Error::Parse containing a ParseError::Tree. Eventually when we get rid of the top-level Error enum this will be more sensible, and we will be able to use ? instead of this awful `.map_err(From::from).map_err(Error::parse)` construction. This commit rightfully belongs as part of the previous commit, but it's noisy and mechanical so I separated it out.
This includes the stringly-typed `BadDescriptor` :).
apoelstra
force-pushed
the
2024-11--expression-2
branch
from
6a8d687
to
75f400f
Compare
|
Addressed your comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does several things but the only big commit is "unify Taproot and non-Taproot parsing" which replaces the ad-hoc logic in
Tr::from_strto handle Taproot parsing with unified expression-parsing logic.In addition to this, we also:
errormodule whose types will eventually replace the top-levelErrorenumBadDescriptor(it does not get rid ofUnexpectedwhich is also a stringly-typed catch-all error, but we will..)