Merge rust-bitcoin/rust-miniscript#652: Drop the Property trait entirely

e971e77d64527d8f70bad690500ea12a51cfdf08 types: remove error path from sub-fragment parameter to threshold() (Andrew Poelstra)
d4dc2267e40f2a2253fa0bf4ef2b67b223533d3a types: make a ton of constructors const (Andrew Poelstra)
fdea7f642e77be14cbaaf8c275f4e8bca316460f types: drop `Property` trait entirely (Andrew Poelstra)
b25023db3a54b4034339bdbea315263ba867326d compiler: remove a bunch of unused error paths (Andrew Poelstra)
eb854aacc5eeab55477b8fa9e7f659ea176c6a81 compiler: stop using Property trait in CompilerExtData (Andrew Poelstra)
602fd29ba4d7e701a97888a0f41e7fffe7e54d01 types: remove all Result returns for ExtData (Andrew Poelstra)
b6c3ca859b56a9cbe0ccdee1d42d5cb2bbdd3b92 types: don't implement Property for ExtData. (Andrew Poelstra)
87294ff7d350c4e9d64956a4d85b93b78d4e2ebb types: don't implement Property for Malleability (Andrew Poelstra)
dd0978d547e45e0c61702c8c91de97df6f79494a types: don't implement Property for Correctness (Andrew Poelstra)
962038034c607c3690acc450f5dcdea39bcca94e clippy: fix a couple nits (Andrew Poelstra)
3c1c896ace7134015ede5ab8ba580c38e6dc14f6 blanket_traits: remove crate:: in a bunch of places (Andrew Poelstra)
9280609aa3500f43798f34b4f3995f335e67c21e 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 e971e77d64527d8f70bad690500ea12a51cfdf08. 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: heap-coder

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,
         };