refactor expression parsing and checksum checking #773
Conversation
This file contains hidden or 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
This creates a new checksum::Error type. For now it sticks it into the giant global error enum, but we will fold it into an expression-parsing enum in the following commits. This eliminates several instances of the Error::BadDescriptor variant, which is a meaningless stringly-typed variant that we'd like to eliminate.
This is really useful for testing.
When we convert the parser to be non-recursive, it will be simpler if we
first do a pass to confirm that the expression is well-formed (all
parens match and are balanced, commas in the right place, etc.). After
that we can assume the string is well-formed and write an algorithm that
basically just translates it into a data structure.
Conveniently, because this check is independent of the parsing
algorithm, we can add it to the existing code as a pre-check.
Since all errors during parsing should occur during the pre-check, we
can then turn the existing stringly-typed errors into unreachable!()s.
By fuzzing the resulting code we can guarantee that our new pre-check
is at least as strict as the existing parser.
In a later PR we will generalize the pre-check a bit so that it treats
*both* () and {} as parentheses, and markes tree nodes based on which
parens are used. But until we change the Taproot parser (which is is an
ad-hoc mess of combination manual string parsing and tree parsing, and
expects tapleaves to show up as the "name"s of childless nodes in a tree
of {} combinators) we can't effectively do this.
There is a new unit test `parse_tree_taproot` that will track this
progress.
Right now our character validation logic is spread all over the place. We have `expression::check_valid_chars` and `checksum::verify_checksum` and also `checksum::Engine::input` which all do the same checks, sharing the public array `expression::VALID_CHARS` which is arguably an implementation detail and shouldn't be exposed. In fact, whenever we parse any Miniscript object (a script, a descriptor, a policy of either type), we are first validating the checksum and then parsing into a tree, and our tree-parsing re-does character validation that was already done as part of the checksum check. This commit moves the checksum error logic into expression, where it belongs, and folds the checksum error variant into the expression parsing error, where it belongs (removing it from the mega-enum at the root). However it leaves the Taproot parsing alone, which is still a bit of a mess and will be addressed in a later PR. Benchmarks show that this significantly slows down expression parsing (as expected, since it is now doing checksumming) but significantly speeds up descriptor parsing (which is no longer doing multiple checks for character validity).
Member
Author
Benchmarks on masterBenchmarks on this PR |
sanket1729
approved these changes
Nov 13, 2024
Member
sanket1729
left a comment
sanket1729
left a comment
There was a problem hiding this comment.
ACK eb4f8c4
I love how this is shaping out to be. Thanks for detailed benchmarks.
| let expected = eng.checksum_chars(); | ||
| let mut actual = ['_'; CHECKSUM_LENGTH]; | ||
| for (act, ch) in actual.iter_mut().zip(checksum_str.chars()) { | ||
| *act = ch; |
Member
There was a problem hiding this comment.
From chatgpt; with MSRV 1.63; you can avoid the mut and creating the actual with all '_' as
let mut iter = checksum_str.chars()
let actual: [char; CHECKSUM_LENGTH] = core::array::from_fn(|_| iter.next().expect("Len checked eariler))
Member
Author
There was a problem hiding this comment.
Nice! I will add a commit to my next PR doing this. I'm thrilled to have from_fn now.
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
…nd checksum checking
eb4f8c4dce3c45d6e510075a63d2115f7198f7b4 expression: move checksum validation into expression parsing (Andrew Poelstra)
bb2e658cabe272e84270a0f30ce28ae0b81e9df2 expression: add explicit well-formedness check to parser (Andrew Poelstra)
9b03043a9e2f73f33188a541881e06404fd30a86 expression: add some unit tests (Andrew Poelstra)
ad85f92fb92cac9beb216f9a29d75ba165264edb expression: implement PartialEq/Eq for trees (Andrew Poelstra)
b0508d864c235fdf4fc03d24aca69acfab8d66e3 checksum: clean up errors and error paths (Andrew Poelstra)
Pull request description:
As a step toward rewriting the expression parser to be non-recursive, add a pre-parsing well-formedness check, which verifies that an expression is well-formed, uses only the correct character set, and that the checksum (if present) is valid.
Along the way, replace the error types returned from the `expression` module with a new more-precise one which can identify the location of parse errors (and identify what the error was in a more correct way). This improves our error reporting and drops many instances of the stringly-typed `BadDescriptor` error.
There is currently a bunch of special logic for Taproot descriptors which have the extra characters `{` and `}`. To the extent possible, this PR doesn't touch that logic. It will be addressed in a later PR.
The benchmarks show a slight slowdown since we essentially added new validation logic without removing the old logic. Later PRs will improve things.
ACKs for top commit:
sanket1729:
ACK eb4f8c4dce3c45d6e510075a63d2115f7198f7b4
Tree-SHA512: cb972e9683aca51f8d18ab61521af8606f47bd1d0bc37dd1ed085101dbc4dd69b382eb05e8e21d2856ac68ebe7d2ca7879ca8a0692dacbea0b22b7b10c9ef987
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
As a step toward rewriting the expression parser to be non-recursive, add a pre-parsing well-formedness check, which verifies that an expression is well-formed, uses only the correct character set, and that the checksum (if present) is valid.
Along the way, replace the error types returned from the
expressionmodule with a new more-precise one which can identify the location of parse errors (and identify what the error was in a more correct way). This improves our error reporting and drops many instances of the stringly-typedBadDescriptorerror.There is currently a bunch of special logic for Taproot descriptors which have the extra characters
{and}. To the extent possible, this PR doesn't touch that logic. It will be addressed in a later PR.The benchmarks show a slight slowdown since we essentially added new validation logic without removing the old logic. Later PRs will improve things.