Merge #652: Drop the Property trait entirely

e971e77 types: remove error path from sub-fragment parameter to threshold() (Andrew Poelstra)
d4dc226 types: make a ton of constructors const (Andrew Poelstra)
fdea7f6 types: drop `Property` trait entirely (Andrew Poelstra)
b25023d compiler: remove a bunch of unused error paths (Andrew Poelstra)
eb854aa compiler: stop using Property trait in CompilerExtData (Andrew Poelstra)
602fd29 types: remove all Result returns for ExtData (Andrew Poelstra)
b6c3ca8 types: don't implement Property for ExtData. (Andrew Poelstra)
87294ff types: don't implement Property for Malleability (Andrew Poelstra)
dd0978d types: don't implement Property for Correctness (Andrew Poelstra)
9620380 clippy: fix a couple nits (Andrew Poelstra)
3c1c896 blanket_traits: remove crate:: in a bunch of places (Andrew Poelstra)
9280609 blanket_traits: add some more bounds (Andrew Poelstra)

Pull request description:

  Something of a followup to #649, which specifically removes the `type_check` method from `Property`.

  Basically, this whole trait is confused. Originally it was supposed to express "a recursively defined property of a Miniscript" which could be defined by a couple constructors and then automatically computed for any Miniscript. In fact, the trait required a separate constructor for every single Miniscript fragment so nothing was "automatic" except sometimes reusing code between the different hash fragments and between `older`/`after`.

  Furthermore, every implementor of this trait had slightly different requirements, meaning that there were `unreachable!()`s throughout the code, functions that were never called (e.g. `Type::from_sha256` passing through to `Correctness::from_sha256` and `Malleability::from_sha256` when all three of those types implemented the function by calling `from_hash`, which *also* was defined by `Type::from_hash` calling `Correctness::from_hash` and `Malleability::from_hash`. So much boilerplate) and many instances of methods being implemented but ignoring arguments, or (worse) being generic over `Pk`/`Ctx` and ignoring those types entirely, or returning `Result` but always returing the `Ok` variant.

  So basically, there was no value in being generic over the trait and the trait wasn't even a generalization of any of the types that implemented it.

  **Adding to this** having the trait prevents us from having constfns in our type checking code, which sucks because we have a lot of common fixed fragments. It also prevents us from returning different error types from different parts of the typechecker, which is my specific goal in removing the trait.

  This PR has a lot of commits but most of them are mechanical and/or small.

ACKs for top commit:
  sanket1729:
    ACK e971e77. Looks a lot cleaner. Thanks for these changes. It's nice that we no longer have the artificial error paths because the parent trait had the definition to encompass all possible implementations.

Tree-SHA512: 38c254caf938e5c3f84c1f0007e0bfc93180e678aef3968cb8f16ad1544722a0f7cfda2c014056446f0091b198edb12c91c0424df64f68c31f5d316fac28425c

Author: apoelstra

src/blanket_traits.rs

@@ -12,8 +12,8 @@
 //! automatically implemented if you satisfy all the bounds.
 //!
 
-use core::fmt;
 use core::str::FromStr;
+use core::{fmt, hash};
 
 use crate::MiniscriptKey;
 
@@ -28,19 +28,43 @@ pub trait FromStrKey:
     > + FromStr<Err = Self::_FromStrErr>
 {
     /// Dummy type. Do not use.
-    type _Sha256: FromStr<Err = Self::_Sha256FromStrErr>;
+    type _Sha256: FromStr<Err = Self::_Sha256FromStrErr>
+        + Clone
+        + Eq
+        + Ord
+        + fmt::Display
+        + fmt::Debug
+        + hash::Hash;
     /// Dummy type. Do not use.
     type _Sha256FromStrErr: fmt::Debug + fmt::Display;
     /// Dummy type. Do not use.
-    type _Hash256: FromStr<Err = Self::_Hash256FromStrErr>;
+    type _Hash256: FromStr<Err = Self::_Hash256FromStrErr>
+        + Clone
+        + Eq
+        + Ord
+        + fmt::Display
+        + fmt::Debug
+        + hash::Hash;
     /// Dummy type. Do not use.
     type _Hash256FromStrErr: fmt::Debug + fmt::Display;
     /// Dummy type. Do not use.
-    type _Ripemd160: FromStr<Err = Self::_Ripemd160FromStrErr>;
+    type _Ripemd160: FromStr<Err = Self::_Ripemd160FromStrErr>
+        + Clone
+        + Eq
+        + Ord
+        + fmt::Display
+        + fmt::Debug
+        + hash::Hash;
     /// Dummy type. Do not use.
     type _Ripemd160FromStrErr: fmt::Debug + fmt::Display;
     /// Dummy type. Do not use.
-    type _Hash160: FromStr<Err = Self::_Hash160FromStrErr>;
+    type _Hash160: FromStr<Err = Self::_Hash160FromStrErr>
+        + Clone
+        + Eq
+        + Ord
+        + fmt::Display
+        + fmt::Debug
+        + hash::Hash;
     /// Dummy type. Do not use.
     type _Hash160FromStrErr: fmt::Debug + fmt::Display;
     /// Dummy type. Do not use.

src/descriptor/bare.rs

@@ -22,8 +22,8 @@ use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
-    BareCtx, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, TranslateErr,
-    TranslatePk, Translator,
+    BareCtx, Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey,
+    TranslateErr, TranslatePk, Translator,
 };
 
 /// Create a Bare Descriptor. That is descriptor that is
@@ -166,15 +166,15 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Bare<Pk> {
     fn lift(&self) -> Result<semantic::Policy<Pk>, Error> { self.ms.lift() }
 }
 
-impl<Pk: crate::FromStrKey> FromTree for Bare<Pk> {
+impl<Pk: FromStrKey> FromTree for Bare<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         let sub = Miniscript::<Pk, BareCtx>::from_tree(top)?;
         BareCtx::top_level_checks(&sub)?;
         Bare::new(sub)
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Bare<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Bare<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;
@@ -364,7 +364,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Pkh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> FromTree for Pkh<Pk> {
+impl<Pk: FromStrKey> FromTree for Pkh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         if top.name == "pkh" && top.args.len() == 1 {
             Ok(Pkh::new(expression::terminal(&top.args[0], |pk| Pk::from_str(pk))?)?)
@@ -378,7 +378,7 @@ impl<Pk: crate::FromStrKey> FromTree for Pkh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Pkh<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Pkh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;

src/descriptor/mod.rs

@@ -25,8 +25,8 @@ use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0};
 use crate::plan::{AssetProvider, Plan};
 use crate::prelude::*;
 use crate::{
-    expression, hash256, BareCtx, Error, ForEachKey, MiniscriptKey, Satisfier, ToPublicKey,
-    TranslateErr, TranslatePk, Translator,
+    expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier,
+    ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
 mod bare;
@@ -918,7 +918,7 @@ impl Descriptor<DefiniteDescriptorKey> {
     }
 }
 
-impl<Pk: crate::FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
+impl<Pk: FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
     /// Parse an expression tree into a descriptor.
     fn from_tree(top: &expression::Tree) -> Result<Descriptor<Pk>, Error> {
         Ok(match (top.name, top.args.len() as u32) {
@@ -932,7 +932,7 @@ impl<Pk: crate::FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> FromStr for Descriptor<Pk> {
+impl<Pk: FromStrKey> FromStr for Descriptor<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Descriptor<Pk>, Error> {
         // tr tree parsing has special code

src/descriptor/segwitv0.rs

@@ -20,8 +20,8 @@ use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::varint_len;
 use crate::{
-    Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey, TranslateErr,
-    TranslatePk, Translator,
+    Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey,
+    TranslateErr, TranslatePk, Translator,
 };
 /// A Segwitv0 wsh descriptor
 #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
@@ -231,7 +231,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wsh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> crate::expression::FromTree for Wsh<Pk> {
+impl<Pk: FromStrKey> crate::expression::FromTree for Wsh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         if top.name == "wsh" && top.args.len() == 1 {
             let top = &top.args[0];
@@ -269,7 +269,7 @@ impl<Pk: MiniscriptKey> fmt::Display for Wsh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Wsh<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Wsh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;
@@ -473,7 +473,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wpkh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
+impl<Pk: FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         if top.name == "wpkh" && top.args.len() == 1 {
             Ok(Wpkh::new(expression::terminal(&top.args[0], |pk| Pk::from_str(pk))?)?)
@@ -487,7 +487,7 @@ impl<Pk: crate::FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Wpkh<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Wpkh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;

src/descriptor/sh.rs

@@ -24,8 +24,8 @@ use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
-    push_opcode_size, Error, ForEachKey, Legacy, Miniscript, MiniscriptKey, Satisfier, Segwitv0,
-    ToPublicKey, TranslateErr, TranslatePk, Translator,
+    push_opcode_size, Error, ForEachKey, FromStrKey, Legacy, Miniscript, MiniscriptKey, Satisfier,
+    Segwitv0, ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
 /// A Legacy p2sh Descriptor
@@ -81,7 +81,7 @@ impl<Pk: MiniscriptKey> fmt::Display for Sh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> crate::expression::FromTree for Sh<Pk> {
+impl<Pk: FromStrKey> crate::expression::FromTree for Sh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         if top.name == "sh" && top.args.len() == 1 {
             let top = &top.args[0];
@@ -106,7 +106,7 @@ impl<Pk: crate::FromStrKey> crate::expression::FromTree for Sh<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Sh<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Sh<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;

src/descriptor/tr.rs

@@ -23,8 +23,8 @@ use crate::policy::Liftable;
 use crate::prelude::*;
 use crate::util::{varint_len, witness_size};
 use crate::{
-    errstr, Error, ForEachKey, MiniscriptKey, Satisfier, ScriptContext, Tap, ToPublicKey,
-    TranslateErr, TranslatePk, Translator,
+    errstr, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap,
+    ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
 /// A Taproot Tree representation.
@@ -467,7 +467,7 @@ where
 }
 
 #[rustfmt::skip]
-impl<Pk: crate::FromStrKey> Tr<Pk> {
+impl<Pk: FromStrKey> Tr<Pk> {
     // Helper function to parse taproot script path
     fn parse_tr_script_spend(tree: &expression::Tree,) -> Result<TapTree<Pk>, Error> {
         match tree {
@@ -488,7 +488,7 @@ impl<Pk: crate::FromStrKey> Tr<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> crate::expression::FromTree for Tr<Pk> {
+impl<Pk: FromStrKey> crate::expression::FromTree for Tr<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
         if top.name == "tr" {
             match top.args.len() {
@@ -530,7 +530,7 @@ impl<Pk: crate::FromStrKey> crate::expression::FromTree for Tr<Pk> {
     }
 }
 
-impl<Pk: crate::FromStrKey> core::str::FromStr for Tr<Pk> {
+impl<Pk: FromStrKey> core::str::FromStr for Tr<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let desc_str = verify_checksum(s)?;

src/miniscript/astelem.rs

@@ -19,7 +19,8 @@ use crate::miniscript::{types, ScriptContext};
 use crate::prelude::*;
 use crate::util::MsKeyBuilder;
 use crate::{
-    errstr, expression, AbsLockTime, Error, Miniscript, MiniscriptKey, Terminal, ToPublicKey,
+    errstr, expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, Terminal,
+    ToPublicKey,
 };
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
@@ -240,15 +241,13 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for Terminal<Pk, Ctx> {
     }
 }
 
-impl<Pk: crate::FromStrKey, Ctx: ScriptContext> crate::expression::FromTree
-    for Arc<Terminal<Pk, Ctx>>
-{
+impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Arc<Terminal<Pk, Ctx>> {
     fn from_tree(top: &expression::Tree) -> Result<Arc<Terminal<Pk, Ctx>>, Error> {
         Ok(Arc::new(expression::FromTree::from_tree(top)?))
     }
 }
 
-impl<Pk: crate::FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Terminal<Pk, Ctx> {
+impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Terminal<Pk, Ctx> {
     fn from_tree(top: &expression::Tree) -> Result<Terminal<Pk, Ctx>, Error> {
         let (frag_name, frag_wrap) = super::split_expression_name(top.name)?;
         let unwrapped = match (frag_name, top.args.len()) {

src/miniscript/mod.rs

@@ -45,7 +45,8 @@ use crate::miniscript::decode::Terminal;
 use crate::miniscript::types::extra_props::ExtData;
 use crate::miniscript::types::Type;
 use crate::{
-    expression, plan, Error, ForEachKey, MiniscriptKey, ToPublicKey, TranslatePk, Translator,
+    expression, plan, Error, ForEachKey, FromStrKey, MiniscriptKey, ToPublicKey, TranslatePk,
+    Translator,
 };
 #[cfg(test)]
 mod ms_tests;
@@ -617,7 +618,7 @@ where
     Ok(ms)
 }
 
-impl<Pk: crate::FromStrKey, 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
@@ -648,17 +649,13 @@ impl<Pk: crate::FromStrKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     }
 }
 
-impl<Pk: crate::FromStrKey, Ctx: ScriptContext> crate::expression::FromTree
-    for Arc<Miniscript<Pk, Ctx>>
-{
+impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Arc<Miniscript<Pk, Ctx>> {
     fn from_tree(top: &expression::Tree) -> Result<Arc<Miniscript<Pk, Ctx>>, Error> {
         Ok(Arc::new(expression::FromTree::from_tree(top)?))
     }
 }
 
-impl<Pk: crate::FromStrKey, Ctx: ScriptContext> crate::expression::FromTree
-    for Miniscript<Pk, Ctx>
-{
+impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Miniscript<Pk, Ctx> {
     /// Parse an expression tree into a Miniscript. As a general rule, this
     /// should not be called directly; rather go through the descriptor API.
     fn from_tree(top: &expression::Tree) -> Result<Miniscript<Pk, Ctx>, Error> {
@@ -667,7 +664,7 @@ impl<Pk: crate::FromStrKey, Ctx: ScriptContext> crate::expression::FromTree
     }
 }
 
-impl<Pk: crate::FromStrKey, Ctx: ScriptContext> str::FromStr for Miniscript<Pk, Ctx> {
+impl<Pk: FromStrKey, Ctx: ScriptContext> str::FromStr for Miniscript<Pk, Ctx> {
     type Err = Error;
     /// Parse a Miniscript from string and perform sanity checks
     /// See [Miniscript::from_str_insane] to parse scripts from string that
@@ -705,7 +702,7 @@ mod tests {
     use sync::Arc;
 
     use super::{Miniscript, ScriptContext, Segwitv0, Tap};
-    use crate::miniscript::types::{self, ExtData, Property, Type};
+    use crate::miniscript::types::{self, ExtData, Type};
     use crate::miniscript::Terminal;
     use crate::policy::Liftable;
     use crate::prelude::*;
@@ -893,17 +890,17 @@ mod tests {
 
         let pk_node = Terminal::Check(Arc::new(Miniscript {
             node: Terminal::PkK(String::from("")),
-            ty: Type::from_pk_k::<Segwitv0>(),
-            ext: types::extra_props::ExtData::from_pk_k::<Segwitv0>(),
+            ty: Type::pk_k(),
+            ext: types::extra_props::ExtData::pk_k::<Segwitv0>(),
             phantom: PhantomData,
         }));
         let pkk_ms: Miniscript<String, Segwitv0> = Miniscript::from_ast(pk_node).unwrap();
         dummy_string_rtt(pkk_ms, "[B/onduesm]c:[K/onduesm]pk_k(\"\")", "pk()");
 
         let pkh_node = Terminal::Check(Arc::new(Miniscript {
             node: Terminal::PkH(String::from("")),
-            ty: Type::from_pk_h::<Segwitv0>(),
-            ext: types::extra_props::ExtData::from_pk_h::<Segwitv0>(),
+            ty: Type::pk_h(),
+            ext: types::extra_props::ExtData::pk_h::<Segwitv0>(),
             phantom: PhantomData,
         }));
         let pkh_ms: Miniscript<String, Segwitv0> = Miniscript::from_ast(pkh_node).unwrap();
@@ -923,8 +920,8 @@ mod tests {
 
         let pkk_node = Terminal::Check(Arc::new(Miniscript {
             node: Terminal::PkK(pk),
-            ty: Type::from_pk_k::<Segwitv0>(),
-            ext: types::extra_props::ExtData::from_pk_k::<Segwitv0>(),
+            ty: Type::pk_k(),
+            ext: types::extra_props::ExtData::pk_k::<Segwitv0>(),
             phantom: PhantomData,
         }));
         let pkk_ms: Segwitv0Script = Miniscript::from_ast(pkk_node).unwrap();
@@ -938,12 +935,12 @@ mod tests {
         let pkh_ms: Segwitv0Script = Miniscript {
             node: Terminal::Check(Arc::new(Miniscript {
                 node: Terminal::RawPkH(hash),
-                ty: Type::from_pk_h::<Segwitv0>(),
-                ext: types::extra_props::ExtData::from_pk_h::<Segwitv0>(),
+                ty: Type::pk_h(),
+                ext: types::extra_props::ExtData::pk_h::<Segwitv0>(),
                 phantom: PhantomData,
             })),
-            ty: Type::cast_check(Type::from_pk_h::<Segwitv0>()).unwrap(),
-            ext: ExtData::cast_check(ExtData::from_pk_h::<Segwitv0>()).unwrap(),
+            ty: Type::cast_check(Type::pk_h()).unwrap(),
+            ext: ExtData::cast_check(ExtData::pk_h::<Segwitv0>()),
             phantom: PhantomData,
         };