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.

Author: apoelstra

src/descriptor/tr/mod.rs

@@ -383,9 +383,9 @@ impl<Pk: FromStrKey> crate::expression::FromTree for Tr<Pk> {
             } 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();

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<BitcoinKey, NoChecks> {
         // Parsing should allow raw hashes in the interpreter
-        let elem: Miniscript<bitcoin::PublicKey, NoChecks> =
-            Miniscript::from_str_ext(ms, &ExtParams::allow_all()).unwrap();
+        let elem: Miniscript<bitcoin::PublicKey, NoChecks> = ms.parse().unwrap();
         elem.to_no_checks_ms()
     }
 
     fn x_only_no_checks_ms(ms: &str) -> Miniscript<BitcoinKey, NoChecks> {
-        let elem: Miniscript<bitcoin::XOnlyPublicKey, NoChecks> =
-            Miniscript::from_str_ext(ms, &ExtParams::allow_all()).unwrap();
+        let elem: Miniscript<bitcoin::XOnlyPublicKey, NoChecks> = ms.parse().unwrap();
         elem.to_no_checks_ms()
     }
 }

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

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

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<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> 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 {
@@ -939,10 +935,7 @@ impl ScriptContext for NoChecks {
         Ok(())
     }
 
-    fn top_level_type_check<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Result<(), Error> {
-        if ms.ty.corr.base != types::Base::B {
-            return Err(Error::NonTopLevel(format!("{:?}", ms)));
-        }
+    fn top_level_type_check<Pk: MiniscriptKey>(_: &Miniscript<Pk, Self>) -> Result<(), Error> {
         Ok(())
     }
 

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<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
 }
 
 impl<Pk: FromStrKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
-    /// 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<Miniscript<Pk, Ctx>, 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<Self, Error> {
+        let params = ValidationParams { allow_raw_pkh: false, ..Ctx::CONSENSUS };
+        Miniscript::from_str_with_validation_params(s, &params)
     }
 
-    /// 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<Miniscript<Pk, Ctx>, Error> {
+    /// Attempt to parse a Miniscript, specifying which validation parameters to apply.
+    pub fn from_str_with_validation_params(
+        s: &str,
+        params: &ValidationParams,
+    ) -> Result<Self, Error> {
         // This checks for invalid ASCII chars
         let top = expression::Tree::from_str(s)?;
         let ms: Miniscript<Pk, Ctx> = 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<Pk: FromStrKey, Ctx: ScriptContext> str::FromStr for Miniscript<Pk, Ctx> {
     /// 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<Miniscript<Pk, Ctx>, 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<bitcoin::PublicKey, Segwitv0>;
@@ -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::<String, Segwitv0>::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::<String, Segwitv0>::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]

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() {