miniscript: replace ExtParams, Ctx::check_* methods, and more, with ValidationParams#991
Open
apoelstra wants to merge 6 commits into
Open
miniscript: replace ExtParams, Ctx::check_* methods, and more, with ValidationParams#991apoelstra wants to merge 6 commits into
ExtParams, Ctx::check_* methods, and more, with ValidationParams#991apoelstra wants to merge 6 commits into
Conversation
We have two error types in the policy module, LiftError and PolicyError. These both cover "duplicate pubkeys" and "mixed timelocks". (Before the previous commit, PolicyError also covered non-binary ands and ors.) These both can now be covered by ValidationError, which reduces the total number of error types in the library and also gets rid of two variants in the top-level Error enum, which I am slowly trying to eliminate. However, there is some changed behavior: 1. When parsing concrete policies we now check for duplicate pubkeys. Previously we only checked for mixed timelocks, and checked for duplicate pubkeys at compile time. I could've gone either way in resolving this inconsistency and chose to go in the direction of "enforce sanity when parsing". 2. When *compiling* concrete policies we don't do validation checks anymore. The premise is that you shouldn't be able to construct an invalid policy, so there's no need to check again before compiling. (Of course, with the existing public `enum` type you can directly make bad policies; we will close this loophole later.) A couple unit tests needed to be changed since they assumed you could parse such policies but not compile them. One unit test, compile_q, bizarrely calls the `best_t` function directly, thereby bypassing a duplicate key check that otherwise would've applied. In this one I just removed the duplicate keys. I don't think they were there on purpose. By the way, for Taproot scripts it's not clear that "no duplicate keys at all" is the correct thing to validate. We probably want to check "no duplicate keys within a single Tapleaf". But we can deal with that later. For now let's retain the existing behavior (no duplicate pubkeys anywhere). 3. You can no longer lift miniscripts with duplicate keys. (I am pretty sure this was just always wrong; see the `duplicate_keys` unit test which I modified which had a comment saying it was "unclear" whether we had intended to allow this behavior.
Update sortedmult, pkh, wpkh to the new validation framework. These constructors now return a ValidationError rather than an Error, which is a more precise type. The constructors also actually do validation; in particular sortedmulti now does a duplicate pubkey check where it didn't use to. The theme over these next several commits is going to be "we now do checks where we didn't use to". It may be that we get pushback from users over this. In that case we need to adapt somehow but at least we can do it in a principled way, rather than the current "there are a ton of different ways to check and they're all randomly half implemented".
This grammar is weird and hard to understand.
This is another big commit that has a bunch of behavior changes of the form "now we enforce sanity rules where we didn't use to". This one lets us delete a huge chunk of context.rs which is nice. This eliminates the NonStandardBareScript error variant, but no tests fail and I didn't replace the functionality. If I open a PR with this commit in this state, we need to file an issue to restore this and to add tests for it.
This is superceded by the validation error. For some reason triggers rustfmt to reflow the MaxRecursiveDepthExceeded variant. That will also be deleted in a later commit.
This is a big commit but it's almost all red. This results in a bunch of error messages changing since we now fail due to ValidationParams rather than due to Context::check_*. One observable performance change is that we parse entire Miniscripts before doing bounds checks. (This means that the error messages for exceeded limits are now more accurate, though they will probably revert when we do some optimization passes.) There is a max recursion check in Expression::from_str and another one in Miniscript::from_ast that I have left alone, for now, because of this. The recursion check needs to fire immediately before constructing any object that exceeds the maximum depth. Later we will add a call to validate_non_top_level to Miniscript::from_ast. This will let us drop the maximum depth check, but it will require an API change because the caller will need to provide validation parameters. So we leave it be for now.
trevarj
reviewed
Jun 22, 2026
Comment on lines
+50
to
+51
| // The context checks will be carried out inside new function for | ||
| // sortedMultiVec |
Contributor
Member
Author
There was a problem hiding this comment.
Ah, yes, this is a rebase problem after #915. An old version of this PR had code to update SortedMultiVec but I think I dropped that commit. I'll track it down.
| // Malleability is only a top-level check since you can fix malleability | ||
| // in some cases by adding wrappers. | ||
| if !params.allow_malleability && !self.is_non_malleable() { | ||
| println!("{:?}", self); |
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
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.
This series of commits starts a couple of parallel tracks:
ScriptContextError,LiftErrorandPolicyErrorare merged intoValidationError, since they all represent essentially the same kinds of errors (except the previous error types were unequal and overlapping, and it was basically random which checks were done where)Errorare removed and not replaced; many methods now returnValidationErroror another more-specific type rather thanErrorDescriptorconstructors; many previously-legal things (likewsh(1)which is an anyone-can-spend and should be forbidden by sanity rules) are no longer legal according to the library.Will conflict with #988. Just pick one and review it; I'll rebase.