From 9320f368dd08d6e4da22b8bddfa3ae741efe7f1d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 10 May 2025 13:01:14 +0000 Subject: [PATCH 1/6] miniscript: introduce ValidationParams type 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. --- src/lib.rs | 6 + src/validation.rs | 405 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 411 insertions(+) create mode 100644 src/validation.rs diff --git a/src/lib.rs b/src/lib.rs index cbed64e51..2591b062b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -124,6 +124,7 @@ pub mod plan; pub mod policy; mod primitives; pub mod psbt; +mod validation; #[cfg(test)] mod test_utils; @@ -147,6 +148,7 @@ use crate::prelude::*; pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError}; pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; pub use crate::primitives::threshold::{Threshold, ThresholdError}; +pub use crate::validation::{Error as ValidationError, ValidationParams}; /// Trait representing a key which can be converted to a hash type. pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { @@ -492,6 +494,8 @@ pub enum Error { ParseThreshold(ParseThresholdError), /// Invalid expression tree. Parse(ParseError), + /// Validation of a script failed. + Validation(ValidationError), } #[doc(hidden)] // will be removed when we remove Error @@ -547,6 +551,7 @@ impl fmt::Display for Error { Error::Threshold(ref e) => e.fmt(f), Error::ParseThreshold(ref e) => e.fmt(f), Error::Parse(ref e) => e.fmt(f), + Error::Validation(ref e) => e.fmt(f), } } } @@ -588,6 +593,7 @@ impl std::error::Error for Error { Threshold(e) => Some(e), ParseThreshold(e) => Some(e), Parse(e) => Some(e), + Validation(e) => Some(e), } } } diff --git a/src/validation.rs b/src/validation.rs new file mode 100644 index 000000000..177a208a1 --- /dev/null +++ b/src/validation.rs @@ -0,0 +1,405 @@ +// SPDX-License-Identifier: CC0-1.0 + +//! Validation Parameters +//! +//! Settings for determining what a "valid" Miniscript is. + +use core::fmt; +#[cfg(feature = "std")] +use std::error; + +use crate::prelude::{String, ToString as _}; + +/// Validation Parameters +#[non_exhaustive] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ValidationParams { + /// Allow compressed 33-byte keys. + pub allow_compressed_keys: bool, + /// Allow duplicate public keys in the script. + pub allow_duplicate_keys: bool, + /// Allow the `d:` fragment. (Disallowed pre-segwit because minimality is not enforced.) + pub allow_dup_if: bool, + /// Allow scripts with 3rd-party malleability vectors. + pub allow_malleability: bool, + /// Allow the `multi` fragment. (This allowed only pre-Taproot.) + pub allow_multi: bool, + /// Allow the `multi_a` fragment. (This allowed only post-Taproot.) + pub allow_multi_a: bool, + /// Allow mixing of height-based and time-based timelocks in a given script. + pub allow_mixed_time_locks: bool, + /// Allow the `or_i` fragment. (Disallowed pre-segwit because minimality is not enforced.) + pub allow_or_i: bool, + /// Allow the `expr_raw_pkh` fragment. + pub allow_raw_pkh: bool, + /// Allow branches without any signatures (this property is referred to as `hassig` + /// in the Miniscript specification, and in very old documentation as "safety"). + pub allow_sigless_branch: bool, + /// Allow scripts that don't have the "B" type, i.e. which are not valid scripts + /// on their own. + pub allow_non_b: bool, + /// Allow 65-byte uncompressed and hybrid keys. + pub allow_uncompressed_keys: bool, + /// Allow unsatisfiable programs. + pub allow_unsatisfiable: bool, + /// Allow 32-byte x-only keys. + pub allow_x_only_keys: bool, + /// Allow multipath keys with inconsistent lengths. + pub allow_inconsistent_multipath_keys: bool, + /// Maximum number of non-push opcodes executed in any branch. + pub max_opcode_count: usize, + /// Maximum size of the encoded script. + /// + /// Depending on context, a Miniscript may be embedded in a Taproot leaf, a segwit + /// witness script, a P2SH redeem script, a bare `scriptPubKey`, or possibly some + /// future thing. This limit describes how many bytes it may be encoded as, *not* + /// including any length prefix bytes. + pub max_script_size: usize, + /// Maximum number of witness stack items. + pub max_witness_items: usize, + /// Maximum number of stack elements during script execution. + pub max_exec_stack_size: usize, + /// Maximum recursive depth allowed in the Miniscript tree. + /// + /// This value is a limitation of the library; it is not related to the maximum depth + /// of Taproot trees (which is enforced elsewhere) or anything else. We hope to relax + /// it in the future once we have eliminated all recursive structures from the library. + /// + /// See for some history of this limit. + pub max_recursive_depth: usize, +} + +impl ValidationParams { + /// A set of parameters representing "anything goes". + pub const MAX: Self = Self { + allow_compressed_keys: true, + allow_duplicate_keys: true, + allow_dup_if: true, + allow_malleability: true, + allow_mixed_time_locks: true, + allow_multi: true, + allow_multi_a: true, + allow_or_i: true, + allow_raw_pkh: true, + allow_sigless_branch: true, + allow_non_b: true, + allow_uncompressed_keys: true, + allow_unsatisfiable: true, + allow_x_only_keys: true, + allow_inconsistent_multipath_keys: true, + max_opcode_count: usize::MAX, + max_script_size: usize::MAX, + max_witness_items: usize::MAX, + max_exec_stack_size: usize::MAX, + max_recursive_depth: 402, // https://github.com/sipa/miniscript/pull/5 + }; + + /// Enforce a basic set of sanity checks. + /// + /// These limitations are those that apply to _all_ contexts. So, for example, `multi_a` + /// is allowed by this constant, and no size/opcode limits are enforced. You likely want + /// to use a context-specific constant such as `crate::Segwitv0::SANE` rather than + /// this constant. + pub const SANE: Self = Self { + allow_compressed_keys: true, + allow_duplicate_keys: false, + allow_dup_if: true, + allow_malleability: false, + allow_mixed_time_locks: false, + allow_multi: true, + allow_multi_a: true, + allow_or_i: true, + allow_raw_pkh: false, + allow_sigless_branch: false, + allow_non_b: false, + allow_uncompressed_keys: true, + // FIXME should make allow_unsatisfiable false, but for compatibility + // with existing code we leave it as true even for "sane" scripts. + allow_unsatisfiable: true, + allow_x_only_keys: true, + allow_inconsistent_multipath_keys: false, + max_opcode_count: usize::MAX, + max_script_size: usize::MAX, + max_witness_items: usize::MAX, + max_exec_stack_size: usize::MAX, + max_recursive_depth: Self::MAX.max_recursive_depth, + }; + + /// Enforce a bare minimum set of sanity checks. + /// + /// This includes any consensus limits that Miniscript understands, and + /// forbids unsatisfiable programs. If you really want to parse anything + /// that can be parsed, use [`Self::MAX`] instead. + pub const CONSENSUS: Self = Self { + allow_compressed_keys: true, + allow_duplicate_keys: true, + allow_dup_if: true, + allow_malleability: true, + allow_mixed_time_locks: true, + allow_multi: true, + allow_multi_a: true, + allow_or_i: true, + allow_raw_pkh: true, + allow_sigless_branch: true, + allow_non_b: false, + allow_uncompressed_keys: true, + // FIXME see above; we should forbid unsatisfiable programs + allow_unsatisfiable: true, + allow_x_only_keys: true, + allow_inconsistent_multipath_keys: true, + max_opcode_count: usize::MAX, + max_script_size: usize::MAX, + max_witness_items: usize::MAX, + max_exec_stack_size: usize::MAX, + max_recursive_depth: Self::MAX.max_recursive_depth, + }; + + /// Whether this set of parameters is equal to another. + /// + /// Defined as an explicit method to get `const`. + pub const fn eq(&self, other: &Self) -> bool { + self.allow_compressed_keys == other.allow_compressed_keys + && self.allow_duplicate_keys == other.allow_duplicate_keys + && self.allow_dup_if == other.allow_dup_if + && self.allow_malleability == other.allow_malleability + && self.allow_mixed_time_locks == other.allow_mixed_time_locks + && self.allow_multi == other.allow_multi + && self.allow_multi_a == other.allow_multi_a + && self.allow_or_i == other.allow_or_i + && self.allow_raw_pkh == other.allow_raw_pkh + && self.allow_sigless_branch == other.allow_sigless_branch + && self.allow_non_b == other.allow_non_b + && self.allow_uncompressed_keys == other.allow_uncompressed_keys + && self.allow_unsatisfiable == other.allow_unsatisfiable + && self.allow_x_only_keys == other.allow_x_only_keys + && self.allow_inconsistent_multipath_keys == other.allow_inconsistent_multipath_keys + && self.max_opcode_count == other.max_opcode_count + && self.max_script_size == other.max_script_size + && self.max_witness_items == other.max_witness_items + && self.max_exec_stack_size == other.max_exec_stack_size + && self.max_recursive_depth == other.max_recursive_depth + } + + /// Whether a script satisfying this set of parameters must also satisfy the + /// other set of parameters. + pub const fn entails(&self, other: &Self) -> bool { self.intersect(other).eq(self) } + + /// Computes the intersection of two sets of validation parameters. + pub const fn intersect(&self, other: &Self) -> Self { + ValidationParams { + allow_compressed_keys: self.allow_compressed_keys && other.allow_compressed_keys, + allow_duplicate_keys: self.allow_duplicate_keys && other.allow_duplicate_keys, + allow_dup_if: self.allow_dup_if && other.allow_dup_if, + allow_malleability: self.allow_malleability && other.allow_malleability, + allow_mixed_time_locks: self.allow_mixed_time_locks && other.allow_mixed_time_locks, + allow_multi: self.allow_multi && other.allow_multi, + allow_multi_a: self.allow_multi_a && other.allow_multi_a, + allow_or_i: self.allow_or_i && other.allow_or_i, + allow_raw_pkh: self.allow_raw_pkh && other.allow_raw_pkh, + allow_sigless_branch: self.allow_sigless_branch && other.allow_sigless_branch, + allow_non_b: self.allow_non_b && other.allow_non_b, + allow_uncompressed_keys: self.allow_uncompressed_keys && other.allow_uncompressed_keys, + allow_unsatisfiable: self.allow_unsatisfiable && other.allow_unsatisfiable, + allow_x_only_keys: self.allow_x_only_keys && other.allow_x_only_keys, + allow_inconsistent_multipath_keys: self.allow_inconsistent_multipath_keys + && other.allow_inconsistent_multipath_keys, + // cannot use cmp::min in const ctx + max_opcode_count: if self.max_opcode_count < other.max_opcode_count { + self.max_opcode_count + } else { + other.max_opcode_count + }, + max_script_size: if self.max_script_size < other.max_script_size { + self.max_script_size + } else { + other.max_script_size + }, + max_witness_items: if self.max_witness_items < other.max_witness_items { + self.max_witness_items + } else { + other.max_witness_items + }, + max_exec_stack_size: if self.max_exec_stack_size < other.max_exec_stack_size { + self.max_exec_stack_size + } else { + other.max_exec_stack_size + }, + max_recursive_depth: if self.max_recursive_depth < other.max_recursive_depth { + self.max_recursive_depth + } else { + other.max_recursive_depth + }, + } + } + + /// Validates a single key against the validation parameters. + pub fn validate_pk(&self, key: &Pk) -> Result<(), KeyError> + where + Pk: crate::MiniscriptKey, + { + if !self.allow_compressed_keys && !key.is_uncompressed() && !key.is_x_only_key() { + return Err(KeyError::IllegalCompressedKey(key.to_string())); + } + if !self.allow_uncompressed_keys && key.is_uncompressed() { + return Err(KeyError::IllegalUncompressedKey(key.to_string())); + } + if !self.allow_x_only_keys && key.is_x_only_key() { + return Err(KeyError::IllegalXOnlyKey(key.to_string())); + } + Ok(()) + } +} + +/// A validation error +#[derive(Clone, PartialEq, Eq, Debug)] +pub enum Error { + /// Script had duplicate public keys. + DuplicateKeys, + /// Script had a `d:` fragment in a context it was not allowed. + IllegalDupIf, + /// Taproot script had a `multi` fragment + IllegalMulti, + /// Non-Taproot script had a `multi_a` fragment + IllegalMultiA, + /// Script had a `or_i` fragment in a context it was not allowed. + IllegalOrI, + /// Script contains an `expr_raw_pkh` fragment. + IllegalRawPkh, + /// Malleable script + Malleable, + /// At least one satisfaction path executes more than the allowable number of + /// non-push opcodes. + #[allow(missing_docs)] + MaxOpCountExceeded { actual: usize, limit: usize }, + /// The script exceeds the maximum allowable script size. + #[allow(missing_docs)] + MaxScriptSizeExceeded { actual: usize, limit: usize }, + /// At least one satisfaction path has more than the allowable number of initial + /// witness stack elements. + #[allow(missing_docs)] + MaxWitnessItemsExceeded { actual: usize, limit: usize }, + /// At least one satisfaction path requires more than the allowable number of elements + /// to be on the stack. + #[allow(missing_docs)] + MaxExecStackSizeExceeded { actual: usize, limit: usize }, + /// A script exceeded the maximum allowable recursive depth for a Miniscript tree. + /// + /// This error triggers as soon as the limit is exceeded, so we do not know the actual + /// depth of the script, only that it is in excess of the limit. + #[allow(missing_docs)] + MaxRecursiveDepthExceeded { limit: usize }, + /// Script had a branch which required a height-based and time-based timelock simultaneously. + MixedTimeLocks, + /// Two multipath keys were present, which had a different number of lengths. + #[allow(missing_docs)] + MultipathKeyLenMismatch { len1: usize, len2: usize }, + /// Top-level script did not have the "B" type, i.e. it is not valid as a standalone script. + NonBase(crate::miniscript::types::Base), + /// Script contains some branch that does not require any signatures. + /// + /// This is both directly dangerous (it likely indicates an "anyone-can-spend" output) + /// and presents a malleability vector, since it allows 3rd parties to modify parts of + /// the transaction such as locktimes or sequence numbers. + SiglessBranch, + /// Illegal key type (e.g. x-only key outside of Taproot). + Key(KeyError), + /// The Miniscript cannot be satisfied. + Unsatisfiable, +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + Error::DuplicateKeys => f.write_str("duplicate public keys"), + Error::IllegalDupIf => f.write_str("legacy script with a `dup_if` fragment"), + Error::IllegalMulti => f.write_str("non-Taproot script with a `multi_a` fragment"), + Error::IllegalMultiA => f.write_str("non-Taproot script with a `multi_a` fragment"), + Error::IllegalOrI => f.write_str("legacy script with a `or_i` fragment"), + Error::IllegalRawPkh => f.write_str("script with a `expr_raw_pkh` fragment"), + Error::Malleable => f.write_str("script has a 3rd-party malleability vector"), + Error::MaxOpCountExceeded { actual, limit } => write!( + f, + "a satisfaction path executes at least {} non-push opcodes (limit: {}).", + actual, limit + ), + Error::MaxScriptSizeExceeded { actual, limit } => write!( + f, + "script has size at least {} (limit: {}).", + actual, limit + ), + Error::MaxWitnessItemsExceeded { actual, limit } => write!( + f, + "a satisfaction path requires at least {} witness items (limit: {}).", + actual, limit + ), + Error::MaxExecStackSizeExceeded { actual, limit } => write!( + f, + "a satisfaction path requires at least {} items on the stack at once (limit: {}).", + actual, limit + ), + Error::MaxRecursiveDepthExceeded { limit } => write!( + f, + "script exceeded the maximum recursive depth limit of {}; if you have a use case for deeper scripts, please file an issue against rust-miniscript", + limit + ), + Error::MixedTimeLocks => f.write_str("mixed a height-based and time-based timelock"), + Error::MultipathKeyLenMismatch { len1, len2 } => write!(f, "found multipath keys with lengths {} and {}; lengths must match", len1, len2), + Error::NonBase(base) => write!(f, "script has type {:?}, which is not allowed for a top-level Miniscript", base), + Error::SiglessBranch => f.write_str("all spending paths must require a signature"), + Error::Key(ref e) => e.fmt(f), + Error::Unsatisfiable => f.write_str("no satisfaction exists for script"), + } + } +} + +#[cfg(feature = "std")] +impl error::Error for Error { + fn cause(&self) -> Option<&dyn error::Error> { + match *self { + Self::Key(ref e) => Some(e), + _ => None, + } + } +} + +impl From for Error { + fn from(i: core::convert::Infallible) -> Self { match i {} } +} + +/// A validation error +#[allow(clippy::enum_variant_names)] // clippy doesn't like the Illegal* prefix +#[derive(Clone, PartialEq, Eq, Debug)] +pub enum KeyError { + /// A compressed key appeared in a context it was not allowed. + IllegalCompressedKey(String), + /// An uncompressed key appeared in a context it was not allowed. + IllegalUncompressedKey(String), + /// An x-only key appeared in a context it was not allowed. + IllegalXOnlyKey(String), +} + +impl fmt::Display for KeyError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + Self::IllegalCompressedKey(ref s) => write!(f, "compressed key `{}` not allowed", s), + Self::IllegalUncompressedKey(ref s) => write!(f, "compressed key `{}` not allowed", s), + Self::IllegalXOnlyKey(ref s) => write!(f, "compressed key `{}` not allowed", s), + } + } +} + +#[cfg(feature = "std")] +impl error::Error for KeyError { + fn cause(&self) -> Option<&dyn error::Error> { None } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn entailment() { + assert!(ValidationParams::SANE.entails(&ValidationParams::CONSENSUS)); + assert!(!ValidationParams::CONSENSUS.entails(&ValidationParams::SANE)); + } +} From 163ba3eb163fb43ed461c131f3bfd039dd1fbd71 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 17 May 2025 18:57:39 +0000 Subject: [PATCH 2/6] miniscript: add CONSENSUS and SANE consts to all contexts 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. --- src/miniscript/context.rs | 78 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index bc2d6bb6f..2aaf154ce 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -15,7 +15,7 @@ use crate::miniscript::limits::{ }; use crate::miniscript::types; use crate::prelude::*; -use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal}; +use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal, ValidationParams}; /// Error for Script Context #[derive(Clone, PartialEq, Eq, Debug)] @@ -176,6 +176,18 @@ where { /// The consensus key associated with the type. Must be a parseable key type Key: ParseableKey; + + /// The validation parameters enforcing consensus limits in this context, and + /// nothing further. + const CONSENSUS: ValidationParams; + + /// Sensible validation parameters for this context. Unless you have a good reason + /// to choose otherwise, these are the validation parameters you want. + /// + /// They are also the validation parameters used throughout this library when no + /// explicit choice of parameters is made. + const SANE: ValidationParams; + /// Depending on ScriptContext, fragments can be malleable. For Example, /// under Legacy context, PkH is malleable because it is possible to /// estimate the cost of satisfaction because of compressed keys @@ -359,6 +371,20 @@ pub enum Legacy {} impl ScriptContext for Legacy { type Key = bitcoin::PublicKey; + + const CONSENSUS: ValidationParams = ValidationParams { + allow_compressed_keys: true, + allow_dup_if: false, + allow_uncompressed_keys: true, + allow_multi_a: false, + allow_or_i: false, + allow_x_only_keys: false, + max_opcode_count: MAX_OPS_PER_SCRIPT, + max_script_size: MAX_SCRIPT_ELEMENT_SIZE, + ..ValidationParams::CONSENSUS + }; + const SANE: ValidationParams = Self::CONSENSUS.intersect(&ValidationParams::SANE); + fn check_terminal_non_malleable( frag: &Terminal, ) -> Result<(), ScriptContextError> { @@ -467,6 +493,22 @@ pub enum Segwitv0 {} impl ScriptContext for Segwitv0 { type Key = bitcoin::PublicKey; + + const CONSENSUS: ValidationParams = ValidationParams { + allow_compressed_keys: true, + allow_uncompressed_keys: false, + allow_multi_a: false, + allow_x_only_keys: false, + max_opcode_count: MAX_OPS_PER_SCRIPT, + max_exec_stack_size: MAX_STACK_SIZE, + ..ValidationParams::CONSENSUS + }; + const SANE: ValidationParams = ValidationParams { + max_script_size: MAX_STANDARD_P2WSH_SCRIPT_SIZE, + max_witness_items: MAX_STANDARD_P2WSH_STACK_ITEMS, + ..Self::CONSENSUS.intersect(&ValidationParams::SANE) + }; + fn check_terminal_non_malleable( _frag: &Terminal, ) -> Result<(), ScriptContextError> { @@ -580,6 +622,22 @@ pub enum Tap {} impl ScriptContext for Tap { type Key = bitcoin::secp256k1::XOnlyPublicKey; + + const CONSENSUS: ValidationParams = ValidationParams { + allow_compressed_keys: false, + allow_uncompressed_keys: false, + allow_multi: false, + allow_x_only_keys: true, + ..ValidationParams::CONSENSUS + }; + const SANE: ValidationParams = ValidationParams { + // Segwit runtime stack item number applies, but no script size limit (though maybe we should + // enforce a 4mb limit?) and no policy limit on number of initial stack items. + // https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#user-content-Resource_limits + max_exec_stack_size: MAX_STACK_SIZE, + ..Self::CONSENSUS.intersect(&ValidationParams::SANE) + }; + fn check_terminal_non_malleable( _frag: &Terminal, ) -> Result<(), ScriptContextError> { @@ -691,6 +749,20 @@ pub enum BareCtx {} impl ScriptContext for BareCtx { type Key = bitcoin::PublicKey; + + const CONSENSUS: ValidationParams = ValidationParams { + allow_compressed_keys: true, + allow_dup_if: false, + allow_uncompressed_keys: true, + allow_multi_a: false, + allow_or_i: false, + allow_x_only_keys: false, + max_opcode_count: MAX_OPS_PER_SCRIPT, + max_script_size: MAX_SCRIPT_SIZE, + ..ValidationParams::CONSENSUS + }; + const SANE: ValidationParams = Self::CONSENSUS.intersect(&ValidationParams::SANE); + fn check_terminal_non_malleable( _frag: &Terminal, ) -> Result<(), ScriptContextError> { @@ -801,6 +873,10 @@ pub enum NoChecks {} impl ScriptContext for NoChecks { // todo: When adding support for interpreter, we need a enum with all supported keys here type Key = bitcoin::PublicKey; + + const CONSENSUS: ValidationParams = ValidationParams::MAX; + const SANE: ValidationParams = ValidationParams::MAX; + fn check_terminal_non_malleable( _frag: &Terminal, ) -> Result<(), ScriptContextError> { From e10b48f96c750043dfa5d87bb57f91abf36bdf45 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 10 May 2025 13:01:14 +0000 Subject: [PATCH 3/6] miniscript: add validate() method to Miniscript type 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.) --- src/interpreter/mod.rs | 2 +- src/miniscript/mod.rs | 174 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 5aed49fb0..90088e0dd 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1568,7 +1568,7 @@ mod tests { } fn x_only_no_checks_ms(ms: &str) -> Miniscript { - let elem: Miniscript = + let elem: Miniscript = Miniscript::from_str_ext(ms, &ExtParams::allow_all()).unwrap(); elem.to_no_checks_ms() } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index cb944b585..4b384ab53 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -39,9 +39,9 @@ use core::cmp; use sync::Arc; +pub use self::context::ScriptContext; use self::lex::{lex, TokenIter}; use crate::expression::{FromTree, TreeIterItem}; -pub use crate::miniscript::context::ScriptContext; use crate::miniscript::decode::Terminal; use crate::{ expression, plan, Error, ForEachKey, FromStrKey, MiniscriptKey, ToPublicKey, Translator, @@ -54,10 +54,13 @@ mod private { use super::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG}; use super::types::{self, ExtData, Type}; + use super::ScriptContext; use crate::iter::TreeLike as _; - pub use crate::miniscript::context::ScriptContext; use crate::prelude::sync::Arc; - use crate::{AbsLockTime, Error, MiniscriptKey, RelLockTime, Terminal, MAX_RECURSION_DEPTH}; + use crate::{ + AbsLockTime, Error, MiniscriptKey, RelLockTime, Terminal, ValidationError, + ValidationParams, MAX_RECURSION_DEPTH, + }; /// The top-level miniscript abstract syntax tree (AST). pub struct Miniscript { @@ -326,6 +329,7 @@ mod private { node: t, phantom: PhantomData, }; + // TODO: This recursion depth is based on segwitv0. // We can relax this in tapscript, but this should be good for almost // all practical cases and we can revisit this if needed. @@ -349,6 +353,170 @@ mod private { ) -> Miniscript { Miniscript { node, ty, ext, phantom: PhantomData } } + + /// Validates whether a given fragment meets the given set of + /// validation parameters. + pub fn validate(&self, params: &ValidationParams) -> Result<(), ValidationError> + where + Pk: MiniscriptKey, + Ctx: ScriptContext, + { + self.validate_non_top_level(params)?; + + // 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() { + return Err(ValidationError::Malleable); + } + + // The B check is inherently top-level. + if !params.allow_non_b && self.ty.corr.base != types::Base::B { + return Err(ValidationError::NonBase(self.ty.corr.base)); + } + + // Sigless branches can be fixed by adding a conjunction with a signature. + if !params.allow_sigless_branch && !self.requires_sig() { + return Err(ValidationError::SiglessBranch); + } + + // Unsatisifiable scripts can be fixed by adding a disjunction with something + // satisfiable. + if !params.allow_unsatisfiable && self.ext.sat_data.is_none() { + return Err(ValidationError::Unsatisfiable); + } + + // All checks passed. + Ok(()) + } + + /// Validates a miniscript, doing only the checks that are applicable to all nodes, + /// not just top-level ones. + /// + /// In particular this excludes the "must be B" and "no sigless branches" checks. + /// To get these, run [`Self::validate`] which also calls through to this method. + pub fn validate_non_top_level( + &self, + params: &ValidationParams, + ) -> Result<(), ValidationError> { + if self.ext.tree_height > params.max_recursive_depth { + return Err(ValidationError::MaxRecursiveDepthExceeded { + limit: params.max_recursive_depth, + }); + } + if !params.allow_duplicate_keys && self.has_repeated_keys() { + return Err(ValidationError::DuplicateKeys); + } + if !params.allow_mixed_time_locks && self.has_mixed_timelocks() { + return Err(ValidationError::MixedTimeLocks); + } + + let mut multipath_len = None; + let mut multipath_check = |pk: &Pk| { + if params.allow_inconsistent_multipath_keys { + return Ok(()); + } + match (multipath_len, pk.num_der_paths()) { + (_, 0) | (_, 1) => {} + (None, n) => multipath_len = Some(n), + (Some(x), y) if x == y => { /* ok */ } + (Some(x), y) => { + return Err(ValidationError::MultipathKeyLenMismatch { len1: x, len2: y }) + } + } + Ok(()) + }; + for ms in self.iter() { + match ms.node { + Terminal::DupIf(..) if !params.allow_dup_if => { + return Err(ValidationError::IllegalDupIf) + } + Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) => { + if !params.allow_multi { + return Err(ValidationError::IllegalMulti); + } + for key in thresh.iter() { + params.validate_pk(key).map_err(ValidationError::Key)?; + multipath_check(key)?; + } + } + Terminal::MultiA(ref thresh) | Terminal::SortedMultiA(ref thresh) => { + if !params.allow_multi_a { + return Err(ValidationError::IllegalMultiA); + } + for key in thresh.iter() { + params.validate_pk(key).map_err(ValidationError::Key)?; + multipath_check(key)?; + } + } + Terminal::OrI(..) if !params.allow_or_i => { + return Err(ValidationError::IllegalOrI) + } + Terminal::RawPkH(..) if !params.allow_raw_pkh => { + return Err(ValidationError::IllegalRawPkh) + } + Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => { + params.validate_pk(pk).map_err(ValidationError::Key)?; + multipath_check(pk)?; + } + _ => {} + } + } + + // FIXME we have to gate this check on params.max_script_size being finite + // because otherwise we'll attempt to call self.script_size(), which calls + // Ctx::pk_len, when validating NoChecks scripts. But NoChecks::pk_len just + // panics because we've forgotten the length of keys once we're in the + // NoChecks context. + // + // This gate will be removed in a later PR which removes Ctx::pk_len and + // replaces it with a less-fragile solution. + if params.max_script_size < usize::MAX && self.script_size() > params.max_script_size { + return Err(ValidationError::MaxScriptSizeExceeded { + actual: self.script_size(), + limit: params.max_script_size, + }); + } + // Satisfiability checks -- if there is no satisfaciton data set then we will + // early-return here, so all other checks should be done before this. + match self.max_satisfaction_witness_elements() { + // No possible satisfactions -- we are doing non-toplevel checks here + // so fail gracefully. + Err(..) => return Ok(()), + Ok(max_n) if max_n > params.max_witness_items => { + return Err(ValidationError::MaxWitnessItemsExceeded { + actual: max_n, + limit: params.max_witness_items, + }) + } + Ok(..) => {} + } + + let sat_op_count = self + .ext + .sat_op_count() + .expect("checked that satisfaction was possible above"); + if sat_op_count > params.max_opcode_count { + return Err(ValidationError::MaxOpCountExceeded { + actual: sat_op_count, + limit: params.max_opcode_count, + }); + } + + let sat_data = self + .ext + .sat_data + .expect("checked that satisfaction was possible above"); + if sat_data.max_witness_stack_count + sat_data.max_exec_stack_count + > params.max_exec_stack_size + { + return Err(ValidationError::MaxExecStackSizeExceeded { + actual: sat_data.max_witness_stack_size + sat_data.max_exec_stack_count, + limit: params.max_exec_stack_size, + }); + } + + Ok(()) + } } } From 93ca9faffbd5be66ec47bd72bb953252f7c4a95b Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 18 May 2025 17:21:18 +0000 Subject: [PATCH 4/6] miniscript: replace decode_ext with decode_with_validation_params This moves us toward dropping ExtData. --- src/miniscript/mod.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 4b384ab53..4e43a55da 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -45,6 +45,7 @@ use crate::expression::{FromTree, TreeIterItem}; use crate::miniscript::decode::Terminal; use crate::{ expression, plan, Error, ForEachKey, FromStrKey, MiniscriptKey, ToPublicKey, Translator, + ValidationParams, }; #[cfg(test)] mod ms_tests; @@ -732,27 +733,24 @@ impl Miniscript { /// embedded in the chain. While it is inadvisable to use insane Miniscripts, /// once it's on the chain you don't have much choice anymore. pub fn decode_consensus(script: &script::Script) -> Result, Error> { - Miniscript::decode_with_ext(script, &ExtParams::allow_all()) + Miniscript::decode_with_validation_params(script, &Ctx::CONSENSUS) } /// Attempt to decode a Miniscript from Script, specifying which validation parameters to apply. - pub fn decode_with_ext( + pub fn decode_with_validation_params( script: &script::Script, - ext: &ExtParams, + params: &ValidationParams, ) -> Result, Error> { let tokens = lex(script)?; let mut iter = TokenIter::new(tokens); let top = decode::decode(&mut iter)?; Ctx::check_global_validity(&top)?; - let type_check = types::Type::type_check(&top.node)?; - if type_check.corr.base != types::Base::B { - return Err(Error::NonTopLevel(format!("{:?}", top))); - }; + types::Type::type_check(&top.node)?; if let Some(leading) = iter.next() { Err(Error::Trailing(leading.to_string())) } else { - top.ext_check(ext)?; + top.validate(params).map_err(Error::Validation)?; Ok(top) } } @@ -790,7 +788,7 @@ impl Miniscript { /// /// ``` pub fn decode(script: &script::Script) -> Result, Error> { - let ms = Self::decode_with_ext(script, &ExtParams::sane())?; + let ms = Self::decode_with_validation_params(script, &Ctx::SANE)?; Ok(ms) } } @@ -1917,9 +1915,10 @@ mod tests { let ms = SegwitMs::from_str_ext(ms_str, &ExtParams::allow_all()).unwrap(); let script = ms.encode(); - // The same test, but parsing from script + // The same test, but parsing from script. Notice that unlike the previous "insane" + // extparams, the Ctx::CONSENSUS constant allows raw pubkeyhashes. SegwitMs::decode(&script).unwrap_err(); - SegwitMs::decode_with_ext(&script, &ExtParams::insane()).unwrap_err(); + SegwitMs::decode_with_validation_params(&script, &Segwitv0::CONSENSUS).unwrap(); SegwitMs::decode_consensus(&script).unwrap(); // Try replacing the raw_pkh with a pkh From cb2fa85fc1bc74a25aaa1c31ce6927c815adf1ed Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 18 May 2025 16:57:05 +0000 Subject: [PATCH 5/6] miniscript: replace from_str_ext with from_str_with_validation_params 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. --- src/descriptor/tr/mod.rs | 6 ++-- src/interpreter/mod.rs | 7 ++--- src/lib.rs | 4 --- src/macros.rs | 2 +- src/miniscript/context.rs | 9 +----- src/miniscript/mod.rs | 63 +++++++++++++++++++-------------------- src/validation.rs | 12 +++++++- 7 files changed, 48 insertions(+), 55 deletions(-) diff --git a/src/descriptor/tr/mod.rs b/src/descriptor/tr/mod.rs index 47a72cfff..7c1ccb5b7 100644 --- a/src/descriptor/tr/mod.rs +++ b/src/descriptor/tr/mod.rs @@ -383,9 +383,9 @@ impl crate::expression::FromTree for Tr { } else { let script = Miniscript::from_tree(node)?; // FIXME hack for https://github.com/rust-bitcoin/rust-miniscript/issues/734 - if script.ty.corr.base != crate::miniscript::types::Base::B { - return Err(Error::NonTopLevel(format!("{:?}", script))); - }; + script + .validate(&Tap::CONSENSUS) + .map_err(Error::Validation)?; tree_builder.push_leaf(script); tap_tree_iter.skip_descendants(); diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 90088e0dd..775141536 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1059,7 +1059,6 @@ mod tests { use super::inner::ToNoChecks; use super::*; - use crate::miniscript::analyzable::ExtParams; #[allow(clippy::type_complexity)] fn setup_keys_sigs( @@ -1562,14 +1561,12 @@ mod tests { // because it does not implement FromStr fn no_checks_ms(ms: &str) -> Miniscript { // Parsing should allow raw hashes in the interpreter - let elem: Miniscript = - Miniscript::from_str_ext(ms, &ExtParams::allow_all()).unwrap(); + let elem: Miniscript = ms.parse().unwrap(); elem.to_no_checks_ms() } fn x_only_no_checks_ms(ms: &str) -> Miniscript { - let elem: Miniscript = - Miniscript::from_str_ext(ms, &ExtParams::allow_all()).unwrap(); + let elem: Miniscript = ms.parse().unwrap(); elem.to_no_checks_ms() } } diff --git a/src/lib.rs b/src/lib.rs index 2591b062b..27c1b8a78 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -443,8 +443,6 @@ pub enum Error { Unexpected(String), /// Encountered a wrapping character that we don't recognize UnknownWrapper(char), - /// Parsed a miniscript and the result was not of type T - NonTopLevel(String), /// Parsed a miniscript but there were more script opcodes after it Trailing(String), /// Could not satisfy a script (fragment) because of a missing signature @@ -515,7 +513,6 @@ impl fmt::Display for Error { Error::UnexpectedStart => f.write_str("unexpected start of script"), Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s), Error::UnknownWrapper(ch) => write!(f, "unknown wrapper «{}:»", ch), - Error::NonTopLevel(ref s) => write!(f, "non-T miniscript: {}", s), Error::Trailing(ref s) => write!(f, "trailing tokens: {}", s), Error::MissingSig(ref pk) => write!(f, "missing signature for key {:?}", pk), Error::CouldNotSatisfy => f.write_str("could not satisfy"), @@ -565,7 +562,6 @@ impl std::error::Error for Error { UnexpectedStart | Unexpected(_) | UnknownWrapper(_) - | NonTopLevel(_) | Trailing(_) | MissingSig(_) | CouldNotSatisfy diff --git a/src/macros.rs b/src/macros.rs index 6e4631f62..f4898b88a 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -8,7 +8,7 @@ /// `ms_str!("c:or_i(pk({}),pk({}))", pk1, pk2)` #[cfg(test)] macro_rules! ms_str { - ($($arg:tt)*) => (Miniscript::from_str_ext(&format!($($arg)*), &$crate::ExtParams::allow_all()).unwrap()) + ($($arg:tt)*) => (Miniscript::from_str_with_validation_params(&format!($($arg)*), &$crate::ValidationParams::MAX).unwrap()) } /// Allows tests to create a concrete policy directly from string as diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index 2aaf154ce..38c330af2 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -13,7 +13,6 @@ use crate::miniscript::limits::{ MAX_OPS_PER_SCRIPT, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE, MAX_SCRIPT_SIZE, MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS, }; -use crate::miniscript::types; use crate::prelude::*; use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal, ValidationParams}; @@ -283,9 +282,6 @@ where /// Check whether the top-level is type B fn top_level_type_check(ms: &Miniscript) -> Result<(), Error> { - if ms.ty.corr.base != types::Base::B { - return Err(Error::NonTopLevel(format!("{:?}", ms))); - } // (Ab)use `for_each_key` to record the number of derivation paths a multipath key has. #[derive(PartialEq)] enum MultipathLenChecker { @@ -941,10 +937,7 @@ impl ScriptContext for NoChecks { Ok(()) } - fn top_level_type_check(ms: &Miniscript) -> Result<(), Error> { - if ms.ty.corr.base != types::Base::B { - return Err(Error::NonTopLevel(format!("{:?}", ms))); - } + fn top_level_type_check(_: &Miniscript) -> Result<(), Error> { Ok(()) } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 4e43a55da..32858b5c8 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -18,7 +18,6 @@ use bitcoin::hashes::hash160; use bitcoin::script; use bitcoin::taproot::{LeafVersion, TapLeafHash}; -use self::analyzable::ExtParams; pub use self::context::{BareCtx, Legacy, Segwitv0, Tap}; use crate::iter::TreeLike; use crate::prelude::*; @@ -1003,33 +1002,26 @@ impl Miniscript { } impl Miniscript { - /// Attempt to parse an insane(scripts don't clear sanity checks) - /// from string into a Miniscript representation. - /// Use this to parse scripts with repeated pubkeys, timelock mixing, malleable - /// scripts without sig or scripts that can exceed resource limits. - /// Some of the analysis guarantees of miniscript are lost when dealing with - /// insane scripts. In general, in a multi-party setting users should only - /// accept sane scripts. - pub fn from_str_insane(s: &str) -> Result, Error> { - Miniscript::from_str_ext(s, &ExtParams::insane()) + /// Attempt to parse a Miniscript, checking only for consensus compatibility, + /// lack of raw pubkeyhashes, and no other checks. + /// + /// It is not recommended to use scripts which require this function in order + /// to parse, especially in a multiparty setting. + pub fn from_str_insane(s: &str) -> Result { + let params = ValidationParams { allow_raw_pkh: false, ..Ctx::CONSENSUS }; + Miniscript::from_str_with_validation_params(s, ¶ms) } - /// Attempt to parse an Miniscripts that don't follow the spec. - /// Use this to parse scripts with repeated pubkeys, timelock mixing, malleable - /// scripts, raw pubkey hashes without sig or scripts that can exceed resource limits. - /// - /// Use [`ExtParams`] builder to specify the types of non-sane rules to allow while parsing. - pub fn from_str_ext(s: &str, ext: &ExtParams) -> Result, Error> { + /// Attempt to parse a Miniscript, specifying which validation parameters to apply. + pub fn from_str_with_validation_params( + s: &str, + params: &ValidationParams, + ) -> Result { // This checks for invalid ASCII chars let top = expression::Tree::from_str(s)?; let ms: Miniscript = expression::FromTree::from_tree(top.root())?; - ms.ext_check(ext)?; - - if ms.ty.corr.base != types::Base::B { - Err(Error::NonTopLevel(format!("{:?}", ms))) - } else { - Ok(ms) - } + ms.validate(params).map_err(Error::Validation)?; + Ok(ms) } } @@ -1247,7 +1239,7 @@ impl str::FromStr for Miniscript { /// See [Miniscript::from_str_insane] to parse scripts from string that /// do not clear the [Miniscript::sanity_check] checks. fn from_str(s: &str) -> Result, Error> { - let ms = Self::from_str_ext(s, &ExtParams::sane())?; + let ms = Self::from_str_with_validation_params(s, &Ctx::SANE)?; Ok(ms) } } @@ -1276,13 +1268,12 @@ mod tests { use bitcoin::taproot::TapLeafHash; use sync::Arc; - use super::{Miniscript, ScriptContext, Segwitv0, Tap}; + use super::*; use crate::miniscript::{types, Terminal}; use crate::policy::Liftable; - use crate::prelude::*; use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator}; use crate::{ - hex_script, BareCtx, Error, ExtParams, Legacy, RelLockTime, Satisfier, ToPublicKey, + hex_script, BareCtx, Error, Legacy, RelLockTime, Satisfier, ToPublicKey, ValidationError, }; type Segwitv0Script = Miniscript; @@ -1912,7 +1903,7 @@ mod tests { // Test that parsing raw hash160 from string does not work without extra features SegwitMs::from_str(ms_str).unwrap_err(); SegwitMs::from_str_insane(ms_str).unwrap_err(); - let ms = SegwitMs::from_str_ext(ms_str, &ExtParams::allow_all()).unwrap(); + let ms = SegwitMs::from_str_with_validation_params(ms_str, &ValidationParams::MAX).unwrap(); let script = ms.encode(); // The same test, but parsing from script. Notice that unlike the previous "insane" @@ -1951,7 +1942,7 @@ mod tests { fn duplicate_keys() { // You cannot parse a Miniscript that has duplicate keys let err = Miniscript::::from_str("and_v(v:pk(A),pk(A))").unwrap_err(); - assert!(matches!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys))); + assert!(matches!(err, Error::Validation(ValidationError::DuplicateKeys))); // ...though you can parse one with from_str_insane let ok_insane = @@ -1974,10 +1965,7 @@ mod tests { "and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", ) .unwrap_err(); - assert!(matches!( - err, - Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination) - )); + assert!(matches!(err, Error::Validation(ValidationError::MixedTimeLocks))); // Though you can in an or() rather than and() let ok_or = Miniscript::::from_str( @@ -2001,6 +1989,15 @@ mod tests { ok_insane.lift().unwrap_err(), Error::LiftError(crate::policy::LiftError::HeightTimelockCombination) )); + // nor can it have sane rules applied to it + assert_eq!( + ok_insane.validate(&ValidationParams::SANE).unwrap_err(), + ValidationError::MixedTimeLocks, + ); + assert_eq!( + ok_insane.validate(&ValidationParams::SANE).unwrap_err(), + ValidationError::MixedTimeLocks, + ); } #[test] diff --git a/src/validation.rs b/src/validation.rs index 177a208a1..e88e1d80e 100644 --- a/src/validation.rs +++ b/src/validation.rs @@ -237,7 +237,17 @@ impl ValidationParams { where Pk: crate::MiniscriptKey, { - if !self.allow_compressed_keys && !key.is_uncompressed() && !key.is_x_only_key() { + // Compressed keys are allowed if -either- compressed keys or x-only keys are allowed: + // for purposes of validating a constructed Miniscript, we treat compressed keys as + // x-only ones, and when encoding them we will just drop their parity. (When decoding + // a Miniscript from Script, we are strict and do not allow compressed keys in place + // of x-only ones. But there we have explicit logic, rather than using this function + // for rejection.) + if !self.allow_compressed_keys + && !self.allow_x_only_keys + && !key.is_uncompressed() + && !key.is_x_only_key() + { return Err(KeyError::IllegalCompressedKey(key.to_string())); } if !self.allow_uncompressed_keys && key.is_uncompressed() { From f7756f5e595b5a65f1459eecea9bd69dc71f030a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 18 May 2025 18:23:55 +0000 Subject: [PATCH 6/6] remove all sanity_check and ext_check methods from the library 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! --- embedded/src/main.rs | 6 - examples/big.rs | 1 - examples/htlc.rs | 4 - examples/parse.rs | 5 - examples/psbt_sign_finalize.rs | 1 - examples/taproot.rs | 3 - fuzz/fuzz_targets/parse_descriptor_priv.rs | 1 - src/descriptor/bare.rs | 6 - src/descriptor/mod.rs | 28 +-- src/descriptor/segwitv0.rs | 15 -- src/descriptor/sh.rs | 10 - src/descriptor/tr/mod.rs | 8 - src/lib.rs | 15 -- src/miniscript/analyzable.rs | 218 --------------------- src/miniscript/mod.rs | 23 +-- src/miniscript/types/correctness.rs | 2 +- src/miniscript/types/extra_props.rs | 4 - src/miniscript/types/mod.rs | 3 +- src/policy/concrete.rs | 6 - 19 files changed, 11 insertions(+), 348 deletions(-) diff --git a/embedded/src/main.rs b/embedded/src/main.rs index df91f4c17..fde0b8e03 100644 --- a/embedded/src/main.rs +++ b/embedded/src/main.rs @@ -44,12 +44,6 @@ fn main() -> ! { hprintln!("p2sh address {}", p2sh_addr).unwrap(); assert_eq!(p2sh_addr, "3CJxbQBfWAe1ZkKiGQNEYrioV73ZwvBWns"); - // Check whether the descriptor is safe - // This checks whether all spend paths are accessible in bitcoin network. - // It maybe possible that some of the spend require more than 100 elements in Wsh scripts - // Or they contain a combination of timelock and heightlock. - assert!(desc.sanity_check().is_ok()); - // Estimate the satisfaction cost assert_eq!(desc.max_weight_to_satisfy().unwrap().to_wu(), 288); // end miniscript test diff --git a/examples/big.rs b/examples/big.rs index a5057937b..f8ea1d991 100644 --- a/examples/big.rs +++ b/examples/big.rs @@ -75,7 +75,6 @@ fn use_descriptor(d: Descriptor) { println!("{}", d); println!("{:?}", d); println!("{:?}", d.desc_type()); - println!("{:?}", d.sanity_check()); } struct StrPkTranslator { diff --git a/examples/htlc.rs b/examples/htlc.rs index 8f864d639..edc662592 100644 --- a/examples/htlc.rs +++ b/examples/htlc.rs @@ -25,10 +25,6 @@ fn main() { ) .expect("Resource limits"); - // Check whether the descriptor is safe. This checks whether all spend paths are accessible in - // the Bitcoin network. It may be possible that some of the spend paths require more than 100 - // elements in Wsh scripts or they contain a combination of timelock and heightlock. - assert!(htlc_descriptor.sanity_check().is_ok()); assert_eq!( format!("{}", htlc_descriptor), "wsh(andor(pk(022222222222222222222222222222222222222222222222222222222222222222),sha256(1111111111111111111111111111111111111111111111111111111111111111),and_v(v:pkh(020202020202020202020202020202020202020202020202020202020202020202),older(4444))))#lfytrjen" diff --git a/examples/parse.rs b/examples/parse.rs index 04c2fd837..80f16c4e5 100644 --- a/examples/parse.rs +++ b/examples/parse.rs @@ -13,11 +13,6 @@ fn main() { ) .unwrap(); - // Check whether the descriptor is safe. This checks whether all spend paths are accessible in - // the Bitcoin network. It may be possible that some of the spend paths require more than 100 - // elements in Wsh scripts or they contain a combination of timelock and heightlock. - assert!(desc.sanity_check().is_ok()); - // Compute the script pubkey. As mentioned in the documentation, script_pubkey only fails // for Tr descriptors that don't have some pre-computed data. assert_eq!( diff --git a/examples/psbt_sign_finalize.rs b/examples/psbt_sign_finalize.rs index 29bf8bc4e..c5c1fe6fb 100644 --- a/examples/psbt_sign_finalize.rs +++ b/examples/psbt_sign_finalize.rs @@ -20,7 +20,6 @@ fn main() { let s = "wsh(t:or_c(pk(027a3565454fe1b749bccaef22aff72843a9c3efefd7b16ac54537a0c23f0ec0de),v:thresh(1,pkh(032d672a1a91cc39d154d366cd231983661b0785c7f27bc338447565844f4a6813),a:pkh(03417129311ed34c242c012cd0a3e0b9bca0065f742d0dfb63c78083ea6a02d4d9),a:pkh(025a687659658baeabdfc415164528065be7bcaade19342241941e556557f01e28))))#7hut9ukn"; let bridge_descriptor = Descriptor::from_str(s).unwrap(); //let bridge_descriptor = Descriptor::::from_str(&s).expect("parse descriptor string"); - assert!(bridge_descriptor.sanity_check().is_ok()); println!("Bridge pubkey script: {}", bridge_descriptor.script_pubkey()); println!("Bridge address: {}", bridge_descriptor.address(Network::Regtest).unwrap()); println!( diff --git a/examples/taproot.rs b/examples/taproot.rs index aea13f6ce..e05f9dfdd 100644 --- a/examples/taproot.rs +++ b/examples/taproot.rs @@ -52,9 +52,6 @@ fn main() { .unwrap(); assert_eq!(desc, expected_desc); - // Check whether the descriptors are safe. - assert!(desc.sanity_check().is_ok()); - // Descriptor type and version should match respectively for taproot let desc_type = desc.desc_type(); assert_eq!(desc_type, DescriptorType::Tr); diff --git a/fuzz/fuzz_targets/parse_descriptor_priv.rs b/fuzz/fuzz_targets/parse_descriptor_priv.rs index 98bfb992c..2de53ff87 100644 --- a/fuzz/fuzz_targets/parse_descriptor_priv.rs +++ b/fuzz/fuzz_targets/parse_descriptor_priv.rs @@ -10,7 +10,6 @@ fn do_test(data: &[u8]) { if let Ok((desc, _)) = Descriptor::parse_descriptor(secp, &data_str) { let _output = desc.to_string(); - let _sanity_check = desc.sanity_check(); } } diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 844516779..14cb4f535 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -47,12 +47,6 @@ impl Bare { /// get the inner pub fn as_inner(&self) -> &Miniscript { &self.ms } - /// Checks whether the descriptor is safe. - pub fn sanity_check(&self) -> Result<(), Error> { - self.ms.sanity_check()?; - Ok(()) - } - /// Computes an upper bound on the difference between a non-satisfied /// `TxIn`'s `segwit_weight` and a satisfied `TxIn`'s `segwit_weight` /// diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 995276c4b..b2a12391e 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -24,7 +24,7 @@ use sync::Arc; use crate::expression::FromTree as _; use crate::miniscript::decode::Terminal; use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG; -use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0}; +use crate::miniscript::{satisfy, Legacy, Miniscript, ScriptContext as _, Segwitv0, Tap}; use crate::plan::{AssetProvider, Plan}; use crate::prelude::*; use crate::{ @@ -305,26 +305,6 @@ impl Descriptor { } } - /// Checks whether the descriptor is safe. - /// - /// Checks whether all the spend paths in the descriptor are possible on the - /// bitcoin network under the current standardness and consensus rules. Also - /// checks whether the descriptor requires signatures on all spend paths and - /// whether the script is malleable. - /// - /// In general, all the guarantees of miniscript hold only for safe scripts. - /// The signer may not be able to find satisfactions even if one exists. - pub fn sanity_check(&self) -> Result<(), Error> { - match *self { - Descriptor::Bare(ref bare) => bare.sanity_check(), - Descriptor::Pkh(_) => Ok(()), - Descriptor::Wpkh(ref wpkh) => wpkh.sanity_check(), - Descriptor::Wsh(ref wsh) => wsh.sanity_check(), - Descriptor::Sh(ref sh) => sh.sanity_check(), - Descriptor::Tr(ref tr) => tr.sanity_check(), - } - } - /// Computes an upper bound on the difference between a non-satisfied /// `TxIn`'s `segwit_weight` and a satisfied `TxIn`'s `segwit_weight` /// @@ -1139,12 +1119,10 @@ impl FromStr for Descriptor { let top = expression::Tree::from_str(s)?; let ret = Self::from_tree(top.root())?; if let Descriptor::Tr(ref inner) = ret { - // FIXME preserve weird/broken behavior from 12.x. - // See https://github.com/rust-bitcoin/rust-miniscript/issues/734 - ret.sanity_check()?; for item in inner.leaves() { item.miniscript() - .ext_check(&crate::miniscript::analyzable::ExtParams::sane())?; + .validate(&Tap::SANE) + .map_err(Error::Validation)?; } } Ok(ret) diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 883ac7b1b..8282f4458 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -53,12 +53,6 @@ impl Wsh { #[deprecated(since = "8.0.0", note = "use format!(\"{:#}\") instead")] pub fn to_string_no_checksum(&self) -> String { format!("{:#}", self) } - /// Checks whether the descriptor is safe. - pub fn sanity_check(&self) -> Result<(), Error> { - self.ms.sanity_check()?; - Ok(()) - } - /// Computes an upper bound on the difference between a non-satisfied /// `TxIn`'s `segwit_weight` and a satisfied `TxIn`'s `segwit_weight` /// @@ -249,15 +243,6 @@ impl Wpkh { #[deprecated(since = "8.0.0", note = "use format!(\"{:#}\") instead")] pub fn to_string_no_checksum(&self) -> String { format!("{:#}", self) } - /// Checks whether the descriptor is safe. - pub fn sanity_check(&self) -> Result<(), Error> { - if self.pk.is_uncompressed() { - Err(Error::ContextError(ScriptContextError::CompressedOnly(self.pk.to_string()))) - } else { - Ok(()) - } - } - /// Computes an upper bound on the difference between a non-satisfied /// `TxIn`'s `segwit_weight` and a satisfied `TxIn`'s `segwit_weight` /// diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index c824c7bda..a9b68d52e 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -132,16 +132,6 @@ impl Sh { /// Create a new p2sh wrapper for the given wsh descriptor pub fn new_with_wsh(wsh: Wsh) -> Self { Self { inner: ShInner::Wsh(wsh) } } - /// Checks whether the descriptor is safe. - pub fn sanity_check(&self) -> Result<(), Error> { - match self.inner { - ShInner::Wsh(ref wsh) => wsh.sanity_check()?, - ShInner::Wpkh(ref wpkh) => wpkh.sanity_check()?, - ShInner::Ms(ref ms) => ms.sanity_check()?, - } - Ok(()) - } - /// Create a new p2sh wrapped wsh sortedmulti descriptor from threshold /// `k` and Vec of `pks` pub fn new_wsh_sortedmulti( diff --git a/src/descriptor/tr/mod.rs b/src/descriptor/tr/mod.rs index 7c1ccb5b7..77a882045 100644 --- a/src/descriptor/tr/mod.rs +++ b/src/descriptor/tr/mod.rs @@ -139,14 +139,6 @@ impl Tr { } } - /// Checks whether the descriptor is safe. - pub fn sanity_check(&self) -> Result<(), Error> { - for leaf in self.leaves() { - leaf.miniscript().sanity_check()?; - } - Ok(()) - } - /// Computes an upper bound on the difference between a non-satisfied /// `TxIn`'s `segwit_weight` and a satisfied `TxIn`'s `segwit_weight` /// diff --git a/src/lib.rs b/src/lib.rs index 27c1b8a78..d783a70d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,11 +56,6 @@ //! "3CJxbQBfWAe1ZkKiGQNEYrioV73ZwvBWns" //! ); //! -//! // Check whether the descriptor is safe. This checks whether all spend paths are accessible in -//! // the Bitcoin network. It may be possible that some of the spend paths require more than 100 -//! // elements in Wsh scripts or they contain a combination of timelock and heightlock. -//! assert!(desc.sanity_check().is_ok()); -//! //! // Estimate the satisfaction cost. //! // scriptSig: OP_PUSH34 > //! // = (1 + 1 + 1 + 32) * 4 = 140 WU @@ -139,7 +134,6 @@ pub use crate::descriptor::{DefiniteDescriptorKey, Descriptor, DescriptorPublicK pub use crate::error::ParseError; pub use crate::expression::{ParseNumError, ParseThresholdError, ParseTreeError}; pub use crate::interpreter::Interpreter; -pub use crate::miniscript::analyzable::{AnalysisError, ExtParams}; pub use crate::miniscript::context::{BareCtx, Legacy, ScriptContext, Segwitv0, SigType, Tap}; pub use crate::miniscript::decode::Terminal; pub use crate::miniscript::satisfy::{Preimage32, Satisfier}; @@ -469,8 +463,6 @@ pub enum Error { /// Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...) /// up to n=3 is invalid by standardness (bare) NonStandardBareScript, - /// Analysis Error - AnalysisError(miniscript::analyzable::AnalysisError), /// Miniscript is equivalent to false. No possible satisfaction ImpossibleSatisfaction, /// Bare descriptors don't have any addresses @@ -535,7 +527,6 @@ impl fmt::Display for Error { up to n=3 is invalid by standardness (bare). " ), - Error::AnalysisError(ref e) => e.fmt(f), Error::ImpossibleSatisfaction => write!(f, "Impossible to satisfy Miniscript"), Error::BareDescriptorAddr => write!(f, "Bare descriptors don't have address"), Error::PubKeyCtxError(ref pk, ref ctx) => { @@ -582,7 +573,6 @@ impl std::error::Error for Error { LiftError(e) => Some(e), ContextError(e) => Some(e), TapTreeDepthError(e) => Some(e), - AnalysisError(e) => Some(e), PubKeyCtxError(e, _) => Some(e), AbsoluteLockTime(e) => Some(e), RelativeLockTime(e) => Some(e), @@ -619,11 +609,6 @@ impl From for Error { fn from(e: miniscript::context::ScriptContextError) -> Error { Error::ContextError(e) } } -#[doc(hidden)] -impl From for Error { - fn from(e: miniscript::analyzable::AnalysisError) -> Error { Error::AnalysisError(e) } -} - #[doc(hidden)] impl From for Error { fn from(e: bitcoin::secp256k1::Error) -> Error { Error::Secp(e) } diff --git a/src/miniscript/analyzable.rs b/src/miniscript/analyzable.rs index 13ccb5e65..b058c84f2 100644 --- a/src/miniscript/analyzable.rs +++ b/src/miniscript/analyzable.rs @@ -5,183 +5,9 @@ //! Tools for determining whether the guarantees offered by the library //! actually hold. -use core::fmt; -#[cfg(feature = "std")] -use std::error; - use crate::prelude::*; use crate::{Miniscript, MiniscriptKey, ScriptContext, Terminal}; -/// Params for parsing miniscripts that either non-sane or non-specified(experimental) in the spec. -/// Used as a parameter [`Miniscript::from_str_ext`] and [`Miniscript::decode_with_ext`]. -/// -/// This allows parsing miniscripts if -/// 1. It is unsafe(does not require a digital signature to spend it) -/// 2. It contains a unspendable path because of either -/// 1. Resource limitations -/// 2. Timelock Mixing -/// 3. The script is malleable and thereby some of satisfaction weight -/// guarantees are not satisfied. -/// 4. It has repeated public keys -/// 5. raw pkh fragments without the pk. This could be obtained when parsing miniscript from script -#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default, Hash)] -pub struct ExtParams { - /// Allow parsing of non-safe miniscripts - pub top_unsafe: bool, - /// Allow parsing of miniscripts with unspendable paths - pub resource_limitations: bool, - /// Allow parsing of miniscripts with timelock mixing - pub timelock_mixing: bool, - /// Allow parsing of malleable miniscripts - pub malleability: bool, - /// Allow parsing of miniscripts with repeated public keys - pub repeated_pk: bool, - /// Allow parsing of miniscripts with raw pkh fragments without the pk. - /// This could be obtained when parsing miniscript from script - pub raw_pkh: bool, -} - -impl ExtParams { - /// Create a new ExtParams that with all the sanity rules - pub fn new() -> ExtParams { - ExtParams { - top_unsafe: false, - resource_limitations: false, - timelock_mixing: false, - malleability: false, - repeated_pk: false, - raw_pkh: false, - } - } - - /// Create a new ExtParams that allows all the sanity rules - pub fn sane() -> ExtParams { ExtParams::new() } - - /// Create a new ExtParams that insanity rules - /// This enables parsing well specified but "insane" miniscripts. - /// Refer to the [`ExtParams`] documentation for more details on "insane" miniscripts. - pub fn insane() -> ExtParams { - ExtParams { - top_unsafe: true, - resource_limitations: true, - timelock_mixing: true, - malleability: true, - repeated_pk: true, - raw_pkh: false, - } - } - - /// Enable all non-sane rules and experimental rules - pub fn allow_all() -> ExtParams { - ExtParams { - top_unsafe: true, - resource_limitations: true, - timelock_mixing: true, - malleability: true, - repeated_pk: true, - raw_pkh: true, - } - } - - /// Builder that allows non-safe miniscripts. - pub fn top_unsafe(mut self) -> ExtParams { - self.top_unsafe = true; - self - } - - /// Builder that allows miniscripts with exceed resource limitations. - pub fn exceed_resource_limitations(mut self) -> ExtParams { - self.resource_limitations = true; - self - } - - /// Builder that allows miniscripts with timelock mixing. - pub fn timelock_mixing(mut self) -> ExtParams { - self.timelock_mixing = true; - self - } - - /// Builder that allows malleable miniscripts. - pub fn malleability(mut self) -> ExtParams { - self.malleability = true; - self - } - - /// Builder that allows miniscripts with repeated public keys. - pub fn repeated_pk(mut self) -> ExtParams { - self.repeated_pk = true; - self - } - - /// Builder that allows miniscripts with raw pkh fragments. - pub fn raw_pkh(mut self) -> ExtParams { - self.raw_pkh = true; - self - } -} - -/// Possible reasons Miniscript guarantees can fail -/// We currently mark Miniscript as Non-Analyzable if -/// 1. It is unsafe(does not require a digital signature to spend it) -/// 2. It contains a unspendable path because of either -/// 1. Resource limitations -/// 2. Timelock Mixing -/// 3. The script is malleable and thereby some of satisfaction weight -/// guarantees are not satisfied. -/// 4. It has repeated publickeys -#[derive(Debug, PartialEq)] -pub enum AnalysisError { - /// Top level is not safe. - SiglessBranch, - /// Repeated Pubkeys - RepeatedPubkeys, - /// Miniscript contains at least one path that exceeds resource limits - BranchExceedResouceLimits, - /// Contains a combination of heightlock and timelock - HeightTimelockCombination, - /// Malleable script - Malleable, - /// Contains partial descriptor raw pkh - ContainsRawPkh, -} - -impl fmt::Display for AnalysisError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - AnalysisError::SiglessBranch => { - f.write_str("All spend paths must require a signature") - } - AnalysisError::RepeatedPubkeys => { - f.write_str("Miniscript contains repeated pubkeys or pubkeyhashes") - } - AnalysisError::BranchExceedResouceLimits => { - f.write_str("At least one spend path exceeds the resource limits(stack depth/satisfaction size..)") - } - AnalysisError::HeightTimelockCombination => { - f.write_str("Contains a combination of heightlock and timelock") - } - AnalysisError::Malleable => f.write_str("Miniscript is malleable"), - AnalysisError::ContainsRawPkh => f.write_str("Miniscript contains raw pkh"), - } - } -} - -#[cfg(feature = "std")] -impl error::Error for AnalysisError { - fn cause(&self) -> Option<&dyn error::Error> { - use self::AnalysisError::*; - - match self { - SiglessBranch - | RepeatedPubkeys - | BranchExceedResouceLimits - | HeightTimelockCombination - | Malleable - | ContainsRawPkh => None, - } - } -} - impl Miniscript { /// Whether all spend paths of miniscript require a signature pub fn requires_sig(&self) -> bool { self.ty.mall.safe } @@ -212,48 +38,4 @@ impl Miniscript { pub fn contains_raw_pkh(&self) -> bool { self.iter().any(|ms| matches!(ms.node, Terminal::RawPkH(_))) } - - /// Check whether the underlying Miniscript is safe under the current context - /// Lifting these polices would create a semantic representation that does - /// not represent the underlying semantics when miniscript is spent. - /// Signing logic may not find satisfaction even if one exists. - /// - /// For most cases, users should be dealing with safe scripts. - /// Use this function to check whether the guarantees of library hold. - /// Most functions of the library like would still - /// work, but results cannot be relied upon - pub fn sanity_check(&self) -> Result<(), AnalysisError> { - if !self.requires_sig() { - Err(AnalysisError::SiglessBranch) - } else if !self.is_non_malleable() { - Err(AnalysisError::Malleable) - } else if !self.within_resource_limits() { - Err(AnalysisError::BranchExceedResouceLimits) - } else if self.has_repeated_keys() { - Err(AnalysisError::RepeatedPubkeys) - } else if self.has_mixed_timelocks() { - Err(AnalysisError::HeightTimelockCombination) - } else { - Ok(()) - } - } - - /// Check whether the miniscript follows the given Extra policy [`ExtParams`] - pub fn ext_check(&self, ext: &ExtParams) -> Result<(), AnalysisError> { - if !ext.top_unsafe && !self.requires_sig() { - Err(AnalysisError::SiglessBranch) - } else if !ext.malleability && !self.is_non_malleable() { - Err(AnalysisError::Malleable) - } else if !ext.resource_limitations && !self.within_resource_limits() { - Err(AnalysisError::BranchExceedResouceLimits) - } else if !ext.repeated_pk && self.has_repeated_keys() { - Err(AnalysisError::RepeatedPubkeys) - } else if !ext.timelock_mixing && self.has_mixed_timelocks() { - Err(AnalysisError::HeightTimelockCombination) - } else if !ext.raw_pkh && self.contains_raw_pkh() { - Err(AnalysisError::ContainsRawPkh) - } else { - Ok(()) - } - } } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 32858b5c8..885b22dc6 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -754,11 +754,11 @@ impl Miniscript { } } - /// Attempt to parse a Script into Miniscript representation. + /// Attempt to decode a Script as a Miniscript. /// - /// This function will fail parsing for scripts that do not clear the - /// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_consensus`] to - /// parse such scripts. + /// This will enforce sanity and standardness rules and fail to decode if they + /// are validated. You may want [`Self::decode_consensus`] if you want to decode + /// anyway. /// /// ## Decode/Parse a miniscript from script hex /// @@ -1235,9 +1235,6 @@ impl FromTree for Miniscript { impl str::FromStr for Miniscript { type Err = Error; - /// Parse a Miniscript from string and perform sanity checks - /// See [Miniscript::from_str_insane] to parse scripts from string that - /// do not clear the [Miniscript::sanity_check] checks. fn from_str(s: &str) -> Result, Error> { let ms = Self::from_str_with_validation_params(s, &Ctx::SANE)?; Ok(ms) @@ -1947,11 +1944,6 @@ mod tests { // ...though you can parse one with from_str_insane let ok_insane = Miniscript::::from_str_insane("and_v(v:pk(A),pk(A))").unwrap(); - // ...but this cannot be sanity checked. - assert!(matches!( - ok_insane.sanity_check().unwrap_err(), - crate::AnalysisError::RepeatedPubkeys - )); // ...it can be lifted, though it's unclear whether this is a deliberate // choice or just an accident. It seems weird given that duplicate public // keys are forbidden in several other places. @@ -1972,7 +1964,6 @@ mod tests { "or_i(and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", ) .unwrap(); - ok_or.sanity_check().unwrap(); ok_or.lift().unwrap(); // ...and you can parse one with from_str_insane @@ -1980,11 +1971,7 @@ mod tests { "and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", ) .unwrap(); - // ...but this cannot be sanity checked or lifted - assert_eq!( - ok_insane.sanity_check().unwrap_err(), - crate::AnalysisError::HeightTimelockCombination - ); + // ...but this cannot lifted assert!(matches!( ok_insane.lift().unwrap_err(), Error::LiftError(crate::policy::LiftError::HeightTimelockCombination) diff --git a/src/miniscript/types/correctness.rs b/src/miniscript/types/correctness.rs index 5db001567..06c149def 100644 --- a/src/miniscript/types/correctness.rs +++ b/src/miniscript/types/correctness.rs @@ -115,7 +115,7 @@ impl Correctness { } /// Confirm invariants of the correctness checker. - pub fn sanity_checks(&self) { + pub(crate) fn sanity_checks(&self) { match self.base { Base::B => {} Base::K => { diff --git a/src/miniscript/types/extra_props.rs b/src/miniscript/types/extra_props.rs index 204b4bddf..b5ea9fbbb 100644 --- a/src/miniscript/types/extra_props.rs +++ b/src/miniscript/types/extra_props.rs @@ -198,9 +198,6 @@ impl ExtData { } impl ExtData { - /// Confirm invariants of the extra property checker. - pub fn sanity_checks(&self) {} - /// Extra properties for the `pk_k` fragment. /// /// The key must be provided to determine its size. @@ -1012,7 +1009,6 @@ impl ExtData { Self::threshold(thresh.k(), thresh.n(), |n| thresh.data()[n].ext) } }; - ret.sanity_checks(); ret } diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 674603edf..472e909c9 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -211,11 +211,12 @@ impl Type { } /// Confirm invariants of the type checker. - pub fn sanity_checks(&self) { + fn sanity_checks(&self) { debug_assert!(!self.corr.dissatisfiable || self.mall.dissat != Dissat::None); debug_assert!(self.mall.dissat == Dissat::None || self.corr.base != Base::V); debug_assert!(self.mall.safe || self.corr.base != Base::K); debug_assert!(self.mall.non_malleable || self.corr.input != Input::Zero); + self.corr.sanity_checks(); } /// Constructor for the type of the `pk_k` fragment. diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 34873e548..5a18ba990 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -251,9 +251,6 @@ impl Policy { continue; } let compilation = compiler::best_compilation::(pol)?; - compilation - .sanity_check() - .expect("compiler produces sane output"); leaf_compilations.push((OrdF64(prob), compilation)); } if !leaf_compilations.is_empty() { @@ -312,9 +309,6 @@ impl Policy { continue; } let compilation = compiler::best_compilation::(pol.as_ref())?; - compilation - .sanity_check() - .expect("compiler produces sane output"); if has_if_fragment(&compilation) { return Err(CompilerError::IfFragmentInNativeLeaf { leaf_index: leaf_idx,