Remove a bit of validation-related cruft #871
Conversation
This is a weird function. Its role is to double-check a witness after it has been produced to ensure that it falls within limits. If you are working with a sane miniscript this is impossible to fail, and if you're working with an insane miniscript then the checks are too strong -- it verifies standardness rules rather than consensus. This was introduced in rust-bitcoin#189 without much discussion. It had no tests then and has no tests now -- everything continues to pass. I am removing it for now. I will later replace it with an API where you call Plan::validate if you want to do checks like this. If the user wants to do something oddball like "parse a miniscript without checking satisfaction limits, then produce a satisfaction and see if the result is nonetheless within limits" the workflow will be: 1. Parse an insane Miniscript 2. Produce a Plan 3. Run Plan::validate with tighter validation params than were used in step 1. This gives the user a fair bit more flexibility. It is an awkward and hard to discover workflow, but IMO it's better than we have now.
|
@sanket1729 this one should be easy to review -- the diff is big but it's mostly about updating test cases. The actual code changes are to remove obscure functionality that I doubt any user has ever (intentionally) used. Since it removes the |
|
The next PR will be about making satisfaction non-recursive, which is a big job and I don't want to block 13.0 on. (And it is closely related to #868 which requires API changes we haven't quite finalized, so I don't want to do it right before a release even if it were technically easy.) |
…nsensus Currently we have decode() and decode_insane() (which were both called parse_* before PR rust-bitcoin#845). In practice though, throughout the codebase we see that when decoding from script we want to relax more than sanity. In particular, we frequently with ExtParams::allow_all, which beyond relaxing the sanity rules, also allows raw pubkeyhashes. We do this in the script interpreter and in PSBT, on the basis that once we're working with a script, we should just deal with whatever the library is capable of dealing with. (Is this the right decision? IMO yes. It could be argued. But regardless, it's the decision we've made for the past many versions of rust-miniscript.) I also propose to backport this with deprecation, telling users that if they really want parse_insane, they now need to call decode_with_ext and specify ExtParams::insane. But if their goal was "parse anything that might be allowed", they actually want decode_consensus. There is now an asymmetry between parsing from string and parsing from script: with strings we have parse() and parse_insane(). With script we have decode() and decode_consensus(). I think this is correct, and reflects the fact that somebody "breaking the rules" with a string is likely trying to use syntactically valid Miniscripts without the library whining at him, while somebody "breaking the rules" with a Script is probably trying to deal with some immutable on-chain thing and wants the library to work if at-all possible. Right now the distinction is simply "do we support raw pkh or not" but I think we could accept more-or-less arbitrary extensions to Miniscript in this library that worked the same way (allowed when decoded from script but disallowed from strings since there's no serialization).
apoelstra
force-pushed
the
2025-10/validation-cruft
branch
from
553c2ae to
bd8fca6
Compare
|
Locally I see the error which is about using [] around backticks in a link to a md document. This has been wrong since #565. I think it's a new nightly compiler thing to notice it, so it shouldn't affect CI, so I'm not gonna bother fixing it for now. |
|
FWIW this seems sane to me (pun intended). |
There was a problem hiding this comment.
ACK bd8fca6
There is now an asymmetry between parsing from string and parsing from
script: with strings we have parse() and parse_insane(). With script we
have decode() and decode_consensus(). I think this is correct, and
reflects the fact that somebody "breaking the rules" with a string is
likely trying to use syntactically valid Miniscripts without the library
whining at him, while somebody "breaking the rules" with a Script is
probably trying to deal with some immutable on-chain thing and wants the
library to work if at-all possible
Makes sense to me.
3 participants
This PR contains two mostly-unrelated commits.
The first removes the
Ctx::check_witnessfunction, which is a no-op for the overwhelmingly common case that users are working with sane Miniscripts, and probably wrong in uncommon cases.The second one removes the
decode_insanemethod, replacing it withdecode_consensuswhich allows much more stuff. The premise here is that "insane" Miniscripts are about allowing well-formed scripts that might not be analyzable because they violate some sanity rules, while "consensus" Miniscripts are anything the library can parse, no matter how half-baked or incomplete it is. When parsing strings we want "insane", when parsing on-chain scripts we want "consensus". The difference, for now, is that "consensus" Miniscripts can have raw pkhs while "insane" ones cannot.More details in the commit messages.