Merge #649: Refactor out type_check

5f41cb6 change FnMut -> Fn (Riccardo Casatta)
cdcd53e Refactor out type_check (Riccardo Casatta)

Pull request description:

  This apply the same logic of ElementsProject/elements-miniscript#74
  removing a couple of unreachable, simplify things and very likely reduce binary bloat.

  ~~However, in doing so, I don't think this MR is equivalent to master for the logic used for `get_child`.~~

  ~~I suspect current master is broken and this fix it (or viceversa?), but I have to suspend this work for now.~~

ACKs for top commit:
  apoelstra:
    ACK 5f41cb6 this is great, thanks!

Tree-SHA512: 4b904fde55ba75895d377babde5e7a3a215cdf3b0ccff0c90572bb286de9d8a80234fc6a4921ad83bcb8c947affa1e3c23b2135534cb4363f38f1bab89368ba4

Author: apoelstra

src/miniscript/astelem.rs

@@ -15,8 +15,7 @@ use bitcoin::{absolute, opcodes, script, Sequence};
 use sync::Arc;
 
 use crate::miniscript::context::SigType;
-use crate::miniscript::types::{self, Property};
-use crate::miniscript::ScriptContext;
+use crate::miniscript::{types, ScriptContext};
 use crate::prelude::*;
 use crate::util::MsKeyBuilder;
 use crate::{

src/miniscript/decode.rs

@@ -17,7 +17,7 @@ use sync::Arc;
 use crate::miniscript::lex::{Token as Tk, TokenIter};
 use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use crate::miniscript::types::extra_props::ExtData;
-use crate::miniscript::types::{Property, Type};
+use crate::miniscript::types::Type;
 use crate::miniscript::ScriptContext;
 use crate::prelude::*;
 #[cfg(doc)]

src/miniscript/mod.rs

@@ -40,7 +40,6 @@ use core::cmp;
 use sync::Arc;
 
 use self::lex::{lex, TokenIter};
-use self::types::Property;
 pub use crate::miniscript::context::ScriptContext;
 use crate::miniscript::decode::Terminal;
 use crate::miniscript::types::extra_props::ExtData;

src/miniscript/types/extra_props.rs

@@ -856,22 +856,12 @@ impl Property for ExtData {
             exec_stack_elem_count_dissat,
         })
     }
+}
 
-    fn type_check_with_child<Pk, Ctx, C>(
-        _fragment: &Terminal<Pk, Ctx>,
-        mut _child: C,
-    ) -> Result<Self, Error>
-    where
-        C: FnMut(usize) -> Self,
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-    {
-        unreachable!()
-    }
-
+impl ExtData {
     /// Compute the type of a fragment assuming all the children of
     /// Miniscript have been computed already.
-    fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, Error>
+    pub fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, Error>
     where
         Ctx: ScriptContext,
         Pk: MiniscriptKey,

src/miniscript/types/mod.rs

@@ -346,175 +346,6 @@ pub trait Property: Sized {
     fn threshold<S>(k: usize, n: usize, sub_ck: S) -> Result<Self, ErrorKind>
     where
         S: FnMut(usize) -> Result<Self, ErrorKind>;
-
-    /// Compute the type of a fragment, given a function to look up
-    /// the types of its children, if available and relevant for the
-    /// given fragment
-    fn type_check_common<'a, Pk, Ctx, C>(
-        fragment: &'a Terminal<Pk, Ctx>,
-        mut get_child: C,
-    ) -> Result<Self, Error>
-    where
-        C: FnMut(&'a Terminal<Pk, Ctx>, usize) -> Result<Self, Error>,
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-    {
-        let wrap_err = |result: Result<Self, ErrorKind>| {
-            result.map_err(|kind| Error { fragment_string: fragment.to_string(), error: kind })
-        };
-
-        let ret = match *fragment {
-            Terminal::True => Ok(Self::from_true()),
-            Terminal::False => Ok(Self::from_false()),
-            Terminal::PkK(..) => Ok(Self::from_pk_k::<Ctx>()),
-            Terminal::PkH(..) | Terminal::RawPkH(..) => Ok(Self::from_pk_h::<Ctx>()),
-            Terminal::Multi(k, ref pks) | Terminal::MultiA(k, ref pks) => {
-                if k == 0 {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::ZeroThreshold,
-                    });
-                }
-                if k > pks.len() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::OverThreshold(k, pks.len()),
-                    });
-                }
-                match *fragment {
-                    Terminal::Multi(..) => Ok(Self::from_multi(k, pks.len())),
-                    Terminal::MultiA(..) => Ok(Self::from_multi_a(k, pks.len())),
-                    _ => unreachable!(),
-                }
-            }
-            Terminal::After(t) => {
-                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
-                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
-                // only consumes 4 bytes from the stack.
-                if t == absolute::LockTime::ZERO.into() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::from_after(t.into()))
-            }
-            Terminal::Older(t) => {
-                if t == Sequence::ZERO || !t.is_relative_lock_time() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::from_older(t))
-            }
-            Terminal::Sha256(..) => Ok(Self::from_sha256()),
-            Terminal::Hash256(..) => Ok(Self::from_hash256()),
-            Terminal::Ripemd160(..) => Ok(Self::from_ripemd160()),
-            Terminal::Hash160(..) => Ok(Self::from_hash160()),
-            Terminal::Alt(ref sub) => wrap_err(Self::cast_alt(get_child(&sub.node, 0)?)),
-            Terminal::Swap(ref sub) => wrap_err(Self::cast_swap(get_child(&sub.node, 0)?)),
-            Terminal::Check(ref sub) => wrap_err(Self::cast_check(get_child(&sub.node, 0)?)),
-            Terminal::DupIf(ref sub) => wrap_err(Self::cast_dupif(get_child(&sub.node, 0)?)),
-            Terminal::Verify(ref sub) => wrap_err(Self::cast_verify(get_child(&sub.node, 0)?)),
-            Terminal::NonZero(ref sub) => wrap_err(Self::cast_nonzero(get_child(&sub.node, 0)?)),
-            Terminal::ZeroNotEqual(ref sub) => {
-                wrap_err(Self::cast_zeronotequal(get_child(&sub.node, 0)?))
-            }
-            Terminal::AndB(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::and_b(ltype, rtype))
-            }
-            Terminal::AndV(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::and_v(ltype, rtype))
-            }
-            Terminal::OrB(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_b(ltype, rtype))
-            }
-            Terminal::OrD(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_d(ltype, rtype))
-            }
-            Terminal::OrC(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_c(ltype, rtype))
-            }
-            Terminal::OrI(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_i(ltype, rtype))
-            }
-            Terminal::AndOr(ref a, ref b, ref c) => {
-                let atype = get_child(&a.node, 0)?;
-                let btype = get_child(&b.node, 1)?;
-                let ctype = get_child(&c.node, 2)?;
-                wrap_err(Self::and_or(atype, btype, ctype))
-            }
-            Terminal::Thresh(k, ref subs) => {
-                if k == 0 {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::ZeroThreshold,
-                    });
-                }
-                if k > subs.len() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::OverThreshold(k, subs.len()),
-                    });
-                }
-
-                let mut last_err_frag = None;
-                let res = Self::threshold(k, subs.len(), |n| match get_child(&subs[n].node, n) {
-                    Ok(x) => Ok(x),
-                    Err(e) => {
-                        last_err_frag = Some(e.fragment_string);
-                        Err(e.error)
-                    }
-                });
-
-                res.map_err(|kind| Error {
-                    fragment_string: last_err_frag.unwrap_or_else(|| fragment.to_string()),
-                    error: kind,
-                })
-            }
-        };
-        if let Ok(ref ret) = ret {
-            ret.sanity_checks()
-        }
-        ret
-    }
-
-    /// Compute the type of a fragment, given a function to look up
-    /// the types of its children.
-    fn type_check_with_child<Pk, Ctx, C>(
-        fragment: &Terminal<Pk, Ctx>,
-        mut child: C,
-    ) -> Result<Self, Error>
-    where
-        C: FnMut(usize) -> Self,
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-    {
-        let get_child = |_sub, n| Ok(child(n));
-        Self::type_check_common(fragment, get_child)
-    }
-
-    /// Compute the type of a fragment.
-    fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, Error>
-    where
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-    {
-        Self::type_check_common(fragment, |sub, _n| Self::type_check(sub))
-    }
 }
 
 impl Property for Type {
@@ -695,22 +526,12 @@ impl Property for Type {
             mall: Property::threshold(k, n, |n| Ok(sub_ck(n)?.mall))?,
         })
     }
+}
 
-    fn type_check_with_child<Pk, Ctx, C>(
-        _fragment: &Terminal<Pk, Ctx>,
-        mut _child: C,
-    ) -> Result<Self, Error>
-    where
-        C: FnMut(usize) -> Self,
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-    {
-        unreachable!()
-    }
-
+impl Type {
     /// Compute the type of a fragment assuming all the children of
     /// Miniscript have been computed already.
-    fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, Error>
+    pub fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, Error>
     where
         Pk: MiniscriptKey,
         Ctx: ScriptContext,

src/policy/compiler.rs

@@ -9,6 +9,7 @@ use core::{cmp, f64, fmt, hash, mem};
 #[cfg(feature = "std")]
 use std::error;
 
+use bitcoin::{absolute, Sequence};
 use sync::Arc;
 
 use crate::miniscript::context::SigType;
@@ -413,6 +414,179 @@ impl Property for CompilerExtData {
     }
 }
 
+impl CompilerExtData {
+    /// Compute the type of a fragment, given a function to look up
+    /// the types of its children.
+    fn type_check_with_child<Pk, Ctx, C>(
+        fragment: &Terminal<Pk, Ctx>,
+        child: C,
+    ) -> Result<Self, types::Error>
+    where
+        C: Fn(usize) -> Self,
+        Pk: MiniscriptKey,
+        Ctx: ScriptContext,
+    {
+        let get_child = |_sub, n| Ok(child(n));
+        Self::type_check_common(fragment, get_child)
+    }
+
+    /// Compute the type of a fragment.
+    fn type_check<Pk, Ctx>(fragment: &Terminal<Pk, Ctx>) -> Result<Self, types::Error>
+    where
+        Pk: MiniscriptKey,
+        Ctx: ScriptContext,
+    {
+        let check_child = |sub, _n| Self::type_check(sub);
+        Self::type_check_common(fragment, check_child)
+    }
+
+    /// Compute the type of a fragment, given a function to look up
+    /// the types of its children, if available and relevant for the
+    /// given fragment
+    fn type_check_common<'a, Pk, Ctx, C>(
+        fragment: &'a Terminal<Pk, Ctx>,
+        get_child: C,
+    ) -> Result<Self, types::Error>
+    where
+        C: Fn(&'a Terminal<Pk, Ctx>, usize) -> Result<Self, types::Error>,
+        Pk: MiniscriptKey,
+        Ctx: ScriptContext,
+    {
+        let wrap_err = |result: Result<Self, ErrorKind>| {
+            result
+                .map_err(|kind| types::Error { fragment_string: fragment.to_string(), error: kind })
+        };
+
+        let ret = match *fragment {
+            Terminal::True => Ok(Self::from_true()),
+            Terminal::False => Ok(Self::from_false()),
+            Terminal::PkK(..) => Ok(Self::from_pk_k::<Ctx>()),
+            Terminal::PkH(..) | Terminal::RawPkH(..) => Ok(Self::from_pk_h::<Ctx>()),
+            Terminal::Multi(k, ref pks) | Terminal::MultiA(k, ref pks) => {
+                if k == 0 {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::ZeroThreshold,
+                    });
+                }
+                if k > pks.len() {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::OverThreshold(k, pks.len()),
+                    });
+                }
+                match *fragment {
+                    Terminal::Multi(..) => Ok(Self::from_multi(k, pks.len())),
+                    Terminal::MultiA(..) => Ok(Self::from_multi_a(k, pks.len())),
+                    _ => unreachable!(),
+                }
+            }
+            Terminal::After(t) => {
+                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
+                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
+                // only consumes 4 bytes from the stack.
+                if t == absolute::LockTime::ZERO.into() {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::InvalidTime,
+                    });
+                }
+                Ok(Self::from_after(t.into()))
+            }
+            Terminal::Older(t) => {
+                if t == Sequence::ZERO || !t.is_relative_lock_time() {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::InvalidTime,
+                    });
+                }
+                Ok(Self::from_older(t))
+            }
+            Terminal::Sha256(..) => Ok(Self::from_sha256()),
+            Terminal::Hash256(..) => Ok(Self::from_hash256()),
+            Terminal::Ripemd160(..) => Ok(Self::from_ripemd160()),
+            Terminal::Hash160(..) => Ok(Self::from_hash160()),
+            Terminal::Alt(ref sub) => wrap_err(Self::cast_alt(get_child(&sub.node, 0)?)),
+            Terminal::Swap(ref sub) => wrap_err(Self::cast_swap(get_child(&sub.node, 0)?)),
+            Terminal::Check(ref sub) => wrap_err(Self::cast_check(get_child(&sub.node, 0)?)),
+            Terminal::DupIf(ref sub) => wrap_err(Self::cast_dupif(get_child(&sub.node, 0)?)),
+            Terminal::Verify(ref sub) => wrap_err(Self::cast_verify(get_child(&sub.node, 0)?)),
+            Terminal::NonZero(ref sub) => wrap_err(Self::cast_nonzero(get_child(&sub.node, 0)?)),
+            Terminal::ZeroNotEqual(ref sub) => {
+                wrap_err(Self::cast_zeronotequal(get_child(&sub.node, 0)?))
+            }
+            Terminal::AndB(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::and_b(ltype, rtype))
+            }
+            Terminal::AndV(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::and_v(ltype, rtype))
+            }
+            Terminal::OrB(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::or_b(ltype, rtype))
+            }
+            Terminal::OrD(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::or_d(ltype, rtype))
+            }
+            Terminal::OrC(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::or_c(ltype, rtype))
+            }
+            Terminal::OrI(ref l, ref r) => {
+                let ltype = get_child(&l.node, 0)?;
+                let rtype = get_child(&r.node, 1)?;
+                wrap_err(Self::or_i(ltype, rtype))
+            }
+            Terminal::AndOr(ref a, ref b, ref c) => {
+                let atype = get_child(&a.node, 0)?;
+                let btype = get_child(&b.node, 1)?;
+                let ctype = get_child(&c.node, 2)?;
+                wrap_err(Self::and_or(atype, btype, ctype))
+            }
+            Terminal::Thresh(k, ref subs) => {
+                if k == 0 {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::ZeroThreshold,
+                    });
+                }
+                if k > subs.len() {
+                    return Err(types::Error {
+                        fragment_string: fragment.to_string(),
+                        error: types::ErrorKind::OverThreshold(k, subs.len()),
+                    });
+                }
+
+                let mut last_err_frag = None;
+                let res = Self::threshold(k, subs.len(), |n| match get_child(&subs[n].node, n) {
+                    Ok(x) => Ok(x),
+                    Err(e) => {
+                        last_err_frag = Some(e.fragment_string);
+                        Err(e.error)
+                    }
+                });
+
+                res.map_err(|kind| types::Error {
+                    fragment_string: last_err_frag.unwrap_or_else(|| fragment.to_string()),
+                    error: kind,
+                })
+            }
+        };
+        if let Ok(ref ret) = ret {
+            ret.sanity_checks()
+        }
+        ret
+    }
+}
+
 /// Miniscript AST fragment with additional data needed by the compiler
 #[derive(Clone, Debug)]
 struct AstElemExt<Pk: MiniscriptKey, Ctx: ScriptContext> {
@@ -1151,7 +1325,7 @@ mod tests {
     use core::str::FromStr;
 
     use bitcoin::blockdata::{opcodes, script};
-    use bitcoin::{hashes, Sequence};
+    use bitcoin::hashes;
 
     use super::*;
     use crate::miniscript::{Legacy, Segwitv0, Tap};