From 500b4a5bbdf8389f8ef86783c8816be3d68ab4f8 Mon Sep 17 00:00:00 2001 From: Riccardo Casatta Date: Thu, 29 Feb 2024 09:28:57 +0100 Subject: [PATCH] Remove type_check from Property trait The blanket implementation of `type_check` is only used on `CompilerExtData` while `ExtData` and `Type` override the impl and don't need the `child` parameter. Moreover, the fn isn't called as Property generic. So the blanket implementation of `type_check` is moved as a impl in `CompilerExtData` and the overrides in `ExtData` and `Type` are moved as simple impl on the type, making it possible to remove the unused `child` parameter. --- src/miniscript/astelem.rs | 4 +- src/miniscript/decode.rs | 22 ++-- src/miniscript/mod.rs | 11 +- src/miniscript/types/extra_props.rs | 7 +- src/miniscript/types/mod.rs | 168 +------------------------- src/policy/compiler.rs | 177 +++++++++++++++++++++++++++- 6 files changed, 193 insertions(+), 196 deletions(-) diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs index 22cb2795..13bd9d8a 100644 --- a/src/miniscript/astelem.rs +++ b/src/miniscript/astelem.rs @@ -18,7 +18,7 @@ use elements::{opcodes, script, Sequence}; use super::limits::{MAX_SCRIPT_ELEMENT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEM_SIZE}; use crate::extensions::ParseableExt; use crate::miniscript::context::SigType; -use crate::miniscript::types::{self, Property}; +use crate::miniscript::types; use crate::miniscript::ScriptContext; use crate::util::MsKeyBuilder; use crate::{ @@ -303,7 +303,7 @@ where { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str("[")?; - if let Ok(type_map) = types::Type::type_check(self, |_| None) { + if let Ok(type_map) = types::Type::type_check(self) { f.write_str(match type_map.corr.base { types::Base::B => "B", types::Base::K => "K", diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs index 6f533373..960dde6a 100644 --- a/src/miniscript/decode.rs +++ b/src/miniscript/decode.rs @@ -17,7 +17,7 @@ use crate::extensions::ParseableExt; use crate::miniscript::lex::{Token as Tk, TokenIter}; use crate::miniscript::limits::{MAX_BLOCK_WEIGHT, 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; #[cfg(doc)] use crate::Descriptor; @@ -25,10 +25,6 @@ use crate::{ bitcoin, hash256, AbsLockTime, Error, Extension, Miniscript, MiniscriptKey, NoExt, ToPublicKey, }; -fn return_none(_: usize) -> Option { - None -} - /// Trait for parsing keys from byte slices pub trait ParseableKey: Sized + ToPublicKey + private::Sealed { /// Parse a key from slice @@ -211,8 +207,8 @@ impl TerminalStack) -> Result<(), Error> { - let ty = Type::type_check(&ms, return_none)?; - let ext = ExtData::type_check(&ms, return_none)?; + let ty = Type::type_check(&ms)?; + let ext = ExtData::type_check(&ms)?; let ms = Miniscript { node: ms, ty, @@ -232,8 +228,8 @@ impl TerminalStack TerminalStack( let c = term.pop().unwrap(); let wrapped_ms = Terminal::AndOr(Arc::new(a), Arc::new(c), Arc::new(b)); - let ty = Type::type_check(&wrapped_ms, return_none)?; - let ext = ExtData::type_check(&wrapped_ms, return_none)?; + let ty = Type::type_check(&wrapped_ms)?; + let ext = ExtData::type_check(&wrapped_ms)?; term.0.push(Miniscript { node: wrapped_ms, diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index d220f32d..09fd0d02 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -36,7 +36,6 @@ use std::cmp; use std::sync::Arc; use self::lex::{lex, TokenIter}; -use self::types::Property; use crate::extensions::ParseableExt; pub use crate::miniscript::context::ScriptContext; use crate::miniscript::decode::Terminal; @@ -117,8 +116,8 @@ impl Miniscript) -> Result, Error> { Ok(Miniscript { - ty: Type::type_check(&t, |_| None)?, - ext: ExtData::type_check(&t, |_| None)?, + ty: Type::type_check(&t)?, + ext: ExtData::type_check(&t)?, node: t, phantom: PhantomData, }) @@ -177,7 +176,7 @@ where let top = decode::parse(&mut iter)?; Ctx::check_global_validity(&top)?; - let type_check = types::Type::type_check(&top.node, |_| None)?; + let type_check = types::Type::type_check(&top.node)?; if type_check.corr.base != types::Base::B { return Err(Error::NonTopLevel(format!("{:?}", top))); }; @@ -491,8 +490,8 @@ impl_from_tree!( fn from_tree(top: &expression::Tree<'_>) -> Result, Error> { let inner: Terminal = expression::FromTree::from_tree(top)?; Ok(Miniscript { - ty: Type::type_check(&inner, |_| None)?, - ext: ExtData::type_check(&inner, |_| None)?, + ty: Type::type_check(&inner)?, + ext: ExtData::type_check(&inner)?, node: inner, phantom: PhantomData, }) diff --git a/src/miniscript/types/extra_props.rs b/src/miniscript/types/extra_props.rs index bdbee9b0..bd5f4a69 100644 --- a/src/miniscript/types/extra_props.rs +++ b/src/miniscript/types/extra_props.rs @@ -890,15 +890,14 @@ impl Property for ExtData { fn from_ext(e: &E) -> Self { e.extra_prop() } - +} +impl ExtData { /// Compute the type of a fragment assuming all the children of /// Miniscript have been computed already. - fn type_check( + pub fn type_check( fragment: &Terminal, - _child: C, ) -> Result> where - C: FnMut(usize) -> Option, Ctx: ScriptContext, Pk: MiniscriptKey, Ext: Extension, diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 40135028..2d12bcd7 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -19,12 +19,6 @@ pub use self::malleability::{Dissat, Malleability}; use super::ScriptContext; use crate::{Extension, MiniscriptKey, NoExt, Terminal}; -/// None-returning function to help type inference when we need a -/// closure that simply returns `None` -fn return_none(_: usize) -> Option { - None -} - /// Detailed type of a typechecker error #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] pub enum ErrorKind { @@ -378,161 +372,6 @@ pub trait Property: Sized { /// Extensions are always leaf, they should have a fixed type property and hence /// should not fail fn from_ext(e: &E) -> Self; - - /// 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( - fragment: &Terminal, - mut child: C, - ) -> Result> - where - C: FnMut(usize) -> Option, - Pk: MiniscriptKey, - Ctx: ScriptContext, - Ext: Extension, - { - let mut get_child = |sub, n| { - child(n) - .map(Ok) - .unwrap_or_else(|| Self::type_check(sub, return_none)) - }; - let wrap_err = |result: Result| { - result.map_err(|kind| Error { - fragment: fragment.clone(), - 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::()), - Terminal::PkH(..) | Terminal::RawPkH(..) => Ok(Self::from_pk_h::()), - Terminal::Multi(k, ref pks) | Terminal::MultiA(k, ref pks) => { - if k == 0 { - return Err(Error { - fragment: fragment.clone(), - error: ErrorKind::ZeroThreshold, - }); - } - if k > pks.len() { - return Err(Error { - fragment: fragment.clone(), - 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 == LockTime::ZERO.into() { - return Err(Error { - fragment: fragment.clone(), - 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: fragment.clone(), - 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: fragment.clone(), - error: ErrorKind::ZeroThreshold, - }); - } - if k > subs.len() { - return Err(Error { - fragment: fragment.clone(), - 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); - Err(e.error) - } - }); - - res.map_err(|kind| Error { - fragment: last_err_frag.unwrap_or_else(|| fragment.clone()), - error: kind, - }) - } - Terminal::Ext(ref ext) => Ok(Self::from_ext(ext)), - }; - if let Ok(ref ret) = ret { - ret.sanity_checks() - } - ret - } } impl Property for Type { @@ -783,15 +622,14 @@ impl Property for Type { mall: Property::from_ext(e), } } - +} +impl Type { /// Compute the type of a fragment assuming all the children of /// Miniscript have been computed already. - fn type_check( + pub fn type_check( fragment: &Terminal, - _child: C, ) -> Result> where - C: FnMut(usize) -> Option, Pk: MiniscriptKey, Ctx: ScriptContext, Ext: Extension, diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 5bd37594..7e1e0fc4 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -13,9 +13,11 @@ use std::marker::PhantomData; use std::sync::Arc; use std::{cmp, error, f64, fmt, hash, mem}; +use elements::{LockTime, Sequence}; + use crate::miniscript::context::SigType; use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG; -use crate::miniscript::types::{self, ErrorKind, ExtData, Property, Type}; +use crate::miniscript::types::{self, ErrorKind, ExtData, Property, Type, Error}; use crate::miniscript::ScriptContext; use crate::policy::Concrete; use crate::{policy, Extension, Miniscript, MiniscriptKey, Terminal}; @@ -154,6 +156,169 @@ struct CompilerExtData { dissat_cost: Option, } +/// None-returning function to help type inference when we need a +/// closure that simply returns `None` +fn return_none(_: usize) -> Option { + None +} + +impl CompilerExtData { + /// 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 + pub fn type_check( + fragment: &Terminal, + mut child: C, + ) -> Result> + where + C: FnMut(usize) -> Option, + Pk: MiniscriptKey, + Ctx: ScriptContext, + Ext: Extension, + { + let mut get_child = |sub, n| { + child(n) + .map(Ok) + .unwrap_or_else(|| Self::type_check(sub, return_none)) + }; + let wrap_err = |result: Result| { + result.map_err(|kind| Error { + fragment: fragment.clone(), + 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::()), + Terminal::PkH(..) | Terminal::RawPkH(..) => Ok(Self::from_pk_h::()), + Terminal::Multi(k, ref pks) | Terminal::MultiA(k, ref pks) => { + if k == 0 { + return Err(Error { + fragment: fragment.clone(), + error: ErrorKind::ZeroThreshold, + }); + } + if k > pks.len() { + return Err(Error { + fragment: fragment.clone(), + 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 == LockTime::ZERO.into() { + return Err(Error { + fragment: fragment.clone(), + 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: fragment.clone(), + 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: fragment.clone(), + error: ErrorKind::ZeroThreshold, + }); + } + if k > subs.len() { + return Err(Error { + fragment: fragment.clone(), + 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); + Err(e.error) + } + }); + + res.map_err(|kind| Error { + fragment: last_err_frag.unwrap_or_else(|| fragment.clone()), + error: kind, + }) + } + Terminal::Ext(ref ext) => Ok(Self::from_ext(ext)), + }; + if let Ok(ref ret) = ret { + ret.sanity_checks() + } + ret + } +} + impl Property for CompilerExtData { fn from_true() -> Self { CompilerExtData { @@ -483,7 +648,7 @@ impl AstElemExt { impl AstElemExt { fn terminal(ast: Terminal) -> AstElemExt { AstElemExt { - comp_ext_data: CompilerExtData::type_check(&ast, |_| None).unwrap(), + comp_ext_data: CompilerExtData::type_check(&ast, return_none).unwrap(), ms: Arc::new(Miniscript::from_ast(ast).expect("Terminal creation must always succeed")), } } @@ -500,8 +665,8 @@ impl AstElemExt { }; //Types and ExtData are already cached and stored in children. So, we can //type_check without cache. For Compiler extra data, we supply a cache. - let ty = types::Type::type_check(&ast, |_| None)?; - let ext = types::ExtData::type_check(&ast, |_| None)?; + let ty = types::Type::type_check(&ast)?; + let ext = types::ExtData::type_check(&ast)?; let comp_ext_data = CompilerExtData::type_check(&ast, lookup_ext)?; Ok(AstElemExt { ms: Arc::new(Miniscript { @@ -528,8 +693,8 @@ impl AstElemExt { }; //Types and ExtData are already cached and stored in children. So, we can //type_check without cache. For Compiler extra data, we supply a cache. - let ty = types::Type::type_check(&ast, |_| None)?; - let ext = types::ExtData::type_check(&ast, |_| None)?; + let ty = types::Type::type_check(&ast)?; + let ext = types::ExtData::type_check(&ast)?; let comp_ext_data = CompilerExtData::type_check(&ast, lookup_ext)?; Ok(AstElemExt { ms: Arc::new(Miniscript {