Merge #957: Introduce ValidationParams structure; use in Miniscript constructors

f7756f5 remove all sanity_check and ext_check methods from the library (Andrew Poelstra)
cb2fa85 miniscript: replace from_str_ext with from_str_with_validation_params (Andrew Poelstra)
93ca9fa miniscript: replace decode_ext with decode_with_validation_params (Andrew Poelstra)
e10b48f miniscript: add validate() method to Miniscript type (Andrew Poelstra)
163ba3e miniscript: add CONSENSUS and SANE consts to all contexts (Andrew Poelstra)
9320f36 miniscript: introduce ValidationParams type (Andrew Poelstra)

Pull request description:

  This PR begins the process of overhauling the validation framework of this library. It introduces a `ValidationParams` struct which is a huge struct containing knobs for every single validation check that this library does. The goal is to centralize all these knobs, document them, apply them consistently, and make it possible for the user to choose any set of them.
  
  Currently validation is spread across a variety of `sanity_check` methods, inconsistently applied in various constructors for `Miniscript` and `Descriptor`, and only configurable in some ad-hoc inconsistent ways via methods like `from_str_insane`. There is no place where these are all listed; the closest thing is the `Ctx` type parameter, which has a bunch of methods like `check_local_policy_validity` which have inscrutable names and are all implemented by calling each other in slightly-different ways.
  
  In short, it's a mess.
  
  As a side-effect of this PR, we introduce some new validation errors which let us eliminate two variants from the top-level `Error` mega-enum. We also delete the `ExtParams` struct, which was similar in spirit to `ValidationParams`, except that it was incomplete and all its names sucked.
  
  This PR also introduces a soft-deprecation of the term "insane", which was used to mean "validate a bunch of rules, but not the ones that we decided are the boundary between a Miniscript that is 'sane' and not". We now use the term "consensus", i.e., "validate the consensus rules but no additional rules". Note that the new notion allows raw pkh fragments while the old one did not. I am open to reverting this change, but my feeling is that nobody will notice because nobody is pushing the boundaries of any of these validation rules. (If they were, they'd be filing a lot more bugs.)


ACKs for top commit:


Tree-SHA512: baced93e3a89aa0e6f3a0e02e4ed13ea5600d55fa345bbc60bf12eadd3aae8fe526115a4758198c6f5f54be5edf35aa8c2172411bba39d1be33ba76ced8fe376

Author: apoelstra

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

examples/big.rs

@@ -75,7 +75,6 @@ fn use_descriptor<K: MiniscriptKey>(d: Descriptor<K>) {
     println!("{}", d);
     println!("{:?}", d);
     println!("{:?}", d.desc_type());
-    println!("{:?}", d.sanity_check());
 }
 
 struct StrPkTranslator {

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"

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!(

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::<bitcoin::PublicKey>::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!(

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);

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();
     }
 }
 

src/descriptor/bare.rs

@@ -47,12 +47,6 @@ impl<Pk: MiniscriptKey> Bare<Pk> {
     /// get the inner
     pub fn as_inner(&self) -> &Miniscript<Pk, BareCtx> { &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`
     ///

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<Pk: MiniscriptKey> Descriptor<Pk> {
         }
     }
 
-    /// 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`
     ///
@@ -1186,12 +1166,10 @@ impl<Pk: FromStrKey> FromStr for Descriptor<Pk> {
         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)

src/descriptor/segwitv0.rs

@@ -53,12 +53,6 @@ impl<Pk: MiniscriptKey> Wsh<Pk> {
     #[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<Pk: MiniscriptKey> Wpkh<Pk> {
     #[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`
     ///