Introduce ValidationParams structure; use in Miniscript constructors#957
Open
apoelstra wants to merge 6 commits into
Open
Introduce ValidationParams structure; use in Miniscript constructors#957apoelstra wants to merge 6 commits into
ValidationParams structure; use in Miniscript constructors#957apoelstra wants to merge 6 commits into
Conversation
Member
Author
|
Happy to rebase on #944 if that goes in first. I don't think they'll conflict significantly. |
apoelstra
force-pushed
the
2026-05/validation-params-1
branch
from
f6299e2 to
9a6938d
Compare
This introduces the ValidationParams type but does not use it anywhere. As you can see, this is a composite of the checks in ExtParams and the checks in miniscript::analyzable and the checks in miniscript::context. In this commit we add a few associated constants to ValidationParams: * MAX means "allow the maximum amount of stuff" and literally just turns off every single validation check. This is rarely useful by itself but it is frequently useful to say "do this one check" by constructing a ValidationParams with that check turned on and `..MAX` for everything else. The struct is #[non_exhaustive] so users cannot construct it without doing something like this. * (There is no MIN because I can't think of a use for it.) * CONSENSUS is MAX except enables the rules that are enforced by consensus across all contexts. Previously we used the term "insane" for this. * SANE is CONSENSUS with additional policy limits and Miniscript-specific rules, e.g. banning duplicate public keys. **One important change:** our CONSENSUS constant is going to replace all usage of "insane", but CONSENSUS allows raw pkhs while "insane" did not. It's hard for me to imagine a case where somebody would even notice this, but I'm highlighting it because it's an intentional change to try to make our validation rulesets match their names better. My feeling is that "consensus" means "handle anything allowed by consensus", including extensions like rawpkhs that exist only in rust-miniscript. In the next commit, we will add MAX/CONSENSUS/SANE associated constants to each context object, by starting with the global ones and then turning on the additional rules that apply to each context. The eventual goal is that we remove the existing analyzable/ExtParams/Ctx stuff, drop Ctx as a type parameter on the Miniscript type, and then the Ctx trait becomes basically a nice way to access constants. I am keeping the term "sane" throughout these PRs, though a better term might be "default", since the sane rules are the ones that you get if you use the "normal" parse/decode/from_str methods. We can do this once the dust settles a bit. A natural question, if you are familiar with the codebase, is "why bring in a new struct instead of just extending ExtParams". Well, I could have, but I hate the name of the struct and the names of all the fields. And since I'm causing a lot of API breakage I might as well take the opportunity to improve the naming.
apoelstra
force-pushed
the
2026-05/validation-params-1
branch
from
9a6938d to
fc9129f
Compare
Member
Author
|
CI is failing because the policy size limits for |
apoelstra
force-pushed
the
2026-05/validation-params-1
branch
from
fc9129f to
6bc4d80
Compare
We are going to move all of the logic currently ad-hoc spread across the various "global"/"local" "consensus"/"policy" checks and other related checks into a single validate() method implemented on Miniscript, Policy, Semantic and possibly other types. This validate() method will have an understanding of which checks are "local" (i.e. computable from a single node) and which are "global" (i.e. require looking at the whole script). This distinction is not meaningful to an end user and it's hard for even developers to keep track of. For a user, all the checks are just checks, and their knob belongs in the big ValidationParams structure.
This is a fairly simple commit and maybe should be squashed into the previous one. It still does not actually use any of the functionality that is being introduced. (Trivia: in a much earlier version of this commit, I added a validation_params field to the Miniscript type, with the idea that the validation parameters would be set at the terminals (to e.g. Segwitv0::SANE) and then propagated upward, so that it would be impossible to construct a Miniscript fragment without having the validation parameters set. However, it turned out that this is not really workable, because many of our parameters (notably allow_sigless_branch and allow_non_b) are "top level only" checks. If they are set on terminals then they will trigger incorrectly. So they must be unset, and then they won't propagate to the top level. One solution to this could be to separate out the Miniscript type into two: a recursive "fragment" type used during construction and then a "toplevel" type which the user actually uses. We have done this for `Expression` for example. I believe we will eventually get to this point in a later PR series in which we eliminate recursion from Miniscript and the two Policy types. But we're not there now, and we can only change one thing at a time without getting overwhelmed.)
This moves us toward dropping ExtData.
This moves us toward being able to drop ExtParams. It also eliminates the NonTopLevel error variant, which is now ValidationError::NonBase and controllable via ValidationParams::allow_non_b. Also tweaks the "hack" in Tr::from_tree to be more sensible, though it's still a hack. A later commit will change the FromTree trait to incorporate validation and then the inconsistency this hack reflects will go away. Leaves the from_str_insane method, with cleaned up doccomment. Probably we should deprecate this since I don't like the term "insane", but we can do that in a followup if that's what we want to do.
Well, there are sanity checks in the type system (which are private and make sense) and a sanity_check method in the PSBT module that I didn't look at. But the rest are gone now. These should really never have existed. At this point in this commit series we are pretty close to a point where they serve no purpose at all; miniscripts constructed with from_ast, from_str and decode are all sanity-checked at construction with the same set of checks. We haven't yet attempted to apply these validation parameters to either of the policy types, or gone through all the non-miniscript descriptor types, but we'll get there. The next series of PRs are going to overhaul all the error types in the library based on this validation framework. I considered deprecating these but we are also deleting the AnalysisError error that it returns. We could reintroduce them with a return type like Result<(), Infallible>, which in a decent language would be non-breaking, but Rust is indecent and this wouldn't really help callers. Also deletes AnalysisError, ExtParams, and the Analysis variant of the top-level Error enum!
apoelstra
force-pushed
the
2026-05/validation-params-1
branch
from
6bc4d80 to
f7756f5
Compare
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
1 participant
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 begins the process of overhauling the validation framework of this library. It introduces a
ValidationParamsstruct which is a huge struct containing knobs for every single validation check that this library does. The goal is to centralize all these knobs, document them, apply them consistently, and make it possible for the user to choose any set of them.Currently validation is spread across a variety of
sanity_checkmethods, inconsistently applied in various constructors forMiniscriptandDescriptor, and only configurable in some ad-hoc inconsistent ways via methods likefrom_str_insane. There is no place where these are all listed; the closest thing is theCtxtype parameter, which has a bunch of methods likecheck_local_policy_validitywhich have inscrutable names and are all implemented by calling each other in slightly-different ways.In short, it's a mess.
As a side-effect of this PR, we introduce some new validation errors which let us eliminate two variants from the top-level
Errormega-enum. We also delete theExtParamsstruct, which was similar in spirit toValidationParams, except that it was incomplete and all its names sucked.This PR also introduces a soft-deprecation of the term "insane", which was used to mean "validate a bunch of rules, but not the ones that we decided are the boundary between a Miniscript that is 'sane' and not". We now use the term "consensus", i.e., "validate the consensus rules but no additional rules". Note that the new notion allows raw pkh fragments while the old one did not. I am open to reverting this change, but my feeling is that nobody will notice because nobody is pushing the boundaries of any of these validation rules. (If they were, they'd be filing a lot more bugs.)