From 1b49a3940b329e5e43d49fb534dc6393afad26f3 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 19:11:30 +0000 Subject: [PATCH 1/7] types: clean up some error messages There were accidental newlines in some error messages. --- src/miniscript/types/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs index 174a5e5dd..ca3c4e72a 100644 --- a/src/miniscript/types/mod.rs +++ b/src/miniscript/types/mod.rs @@ -87,19 +87,19 @@ impl fmt::Display for Error { ), ErrorKind::SwapNonOne => write!( f, - "fragment «{}» attempts to use `SWAP` to prefix something + "fragment «{}» attempts to use `SWAP` to prefix something \ which does not take exactly one input", self.fragment_string, ), ErrorKind::NonZeroZero => write!( f, - "fragment «{}» attempts to use use the `j:` wrapper around a + "fragment «{}» attempts to use use the `j:` wrapper around a \ fragment which might be satisfied by an input of size zero", self.fragment_string, ), ErrorKind::LeftNotUnit => write!( f, - "fragment «{}» requires its left child be a unit (outputs + "fragment «{}» requires its left child be a unit (outputs \ exactly 1 given a satisfying input)", self.fragment_string, ), From ff0509fcfa2d58007bd0691b791a760baeea10aa Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 22:12:54 +0000 Subject: [PATCH 2/7] policy: stop using duplicate keys in unit tests We will want to start rejecting duplicate keys in policy. Currently they are rejected in the compiler and when parsing (sane) Miniscripts and when calling `is_valid` on concrete policies, but NOT when lifting insane Miniscripts or when parsing concrete or semantic policies. Meanwhile, mixing timelocks is checked in all the above places EXCEPT when parsing concrete or semantic policies. And of course, neither is checked when directly constructing Miniscripts or policies. It's all very inconsistent. My eventual goal is to use the same set of "sane" checks everywhere. To do this, I will need to embed a set of checks into Miniscript objects, and then later do the same with Policy objects (which will need to stop being "bare recursive structs" and start having more structure, same as Miniscript). --- src/policy/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 5c2f83448..145c3fe61 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -278,13 +278,13 @@ mod tests { #[test] fn policy_rtt_tests() { concrete_policy_rtt("pk()"); - concrete_policy_rtt("or(1@pk(),1@pk())"); - concrete_policy_rtt("or(99@pk(),1@pk())"); - concrete_policy_rtt("and(pk(),or(99@pk(),1@older(12960)))"); + concrete_policy_rtt("or(1@pk(X),1@pk(Y))"); + concrete_policy_rtt("or(99@pk(X),1@pk(Y))"); + concrete_policy_rtt("and(pk(X),or(99@pk(Y),1@older(12960)))"); semantic_policy_rtt("pk()"); - semantic_policy_rtt("or(pk(),pk())"); - semantic_policy_rtt("and(pk(),pk())"); + semantic_policy_rtt("or(pk(X),pk(Y))"); + semantic_policy_rtt("and(pk(X),pk(Y))"); //fuzzer crashes assert!(ConcretePol::from_str("thresh()").is_err()); From 7bb9b04c41b929a85adad4e59216f7ec1ef5577d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 19:15:50 +0000 Subject: [PATCH 3/7] miniscript: add unit test for timelock mixing --- src/miniscript/mod.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 0d3f78c48..c43d9ea93 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1408,6 +1408,39 @@ mod tests { ms.translate_pk(&mut t).unwrap_err(); } + #[test] + fn mixed_timelocks() { + // You cannot parse a Miniscript that mixes timelocks. + let err = Miniscript::::from_str( + "and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", + ) + .unwrap_err(); + assert_eq!(err, Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination)); + + // Though you can in an or() rather than and() + let ok_or = Miniscript::::from_str( + "or_i(and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", + ) + .unwrap(); + ok_or.sanity_check().unwrap(); + ok_or.lift().unwrap(); + + // ...and you can parse one with from_str_insane + let ok_insane = Miniscript::::from_str_insane( + "and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))", + ) + .unwrap(); + // ...but this cannot be sanity checked or lifted + assert_eq!( + ok_insane.sanity_check().unwrap_err(), + crate::AnalysisError::HeightTimelockCombination + ); + assert_eq!( + ok_insane.lift().unwrap_err(), + Error::LiftError(crate::policy::LiftError::HeightTimelockCombination) + ); + } + #[test] fn template_timelocks() { use crate::{AbsLockTime, RelLockTime}; From 5da250a708c22310eccfab84863ad72732286357 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 20:33:12 +0000 Subject: [PATCH 4/7] miniscript: add unit test for repeated pubkeys --- src/miniscript/mod.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index c43d9ea93..b02feb952 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1408,6 +1408,23 @@ mod tests { ms.translate_pk(&mut t).unwrap_err(); } + #[test] + fn duplicate_keys() { + // You cannot parse a Miniscript that has duplicate keys + let err = Miniscript::::from_str("and_v(v:pk(A),pk(A))").unwrap_err(); + assert_eq!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys)); + + // ...though you can parse one with from_str_insane + let ok_insane = + Miniscript::::from_str_insane("and_v(v:pk(A),pk(A))").unwrap(); + // ...but this cannot be sanity checked. + assert_eq!(ok_insane.sanity_check().unwrap_err(), crate::AnalysisError::RepeatedPubkeys); + // ...it can be lifted, though it's unclear whether this is a deliberate + // choice or just an accident. It seems weird given that duplicate public + // keys are forbidden in several other places. + ok_insane.lift().unwrap(); + } + #[test] fn mixed_timelocks() { // You cannot parse a Miniscript that mixes timelocks. From e31d52b2fbe9e1b3826a67f6785afebe851d2265 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 23:34:11 +0000 Subject: [PATCH 5/7] iter: introduce right-to-left post-order iterator, use throughout codebase We have several algorithms throughout the codebase which "translate" a recursive object by running a post-order iterator over it, building a modified copy node-by-node. We frequently do this by iterating over an Arc structure, pushing each created node onto a stack, and then using the `child_indices` member of the `PostOrderIterItem` struct to index into the stack. We copy elements out of the stack using Arc::clone and then push a new element. The result is that for an object with N nodes, we construct a stack with N elements, call Arc::clone N-1 times, and often we need to bend over backward to turn &self into an Arc before starting. There is a much more efficient way: with a post-order iterator, each node appears directly after its children. So we can just pop children off of the stack, construct the new node, and push that onto the stack. As long as we always pop all of the children, our stack will never grow beyond the depth of the object in question, and we can avoid some Arc::clones. Using a right-to-left iterator means that we can call .pop() in the "natural" way rather than having to muck about reordering the children. This commit converts every single use of post_order_iter in the library to use this new algorithm. In the case of Miniscript::substitute_raw_pkh, the old algorithm was actually completely wrong. The next commit adds a unit test. --- src/iter/tree.rs | 55 +++++++++++++++ src/miniscript/mod.rs | 156 ++++++++++++++++++++++++++++------------- src/policy/concrete.rs | 65 +++++++---------- src/policy/semantic.rs | 35 ++++----- 4 files changed, 200 insertions(+), 111 deletions(-) diff --git a/src/iter/tree.rs b/src/iter/tree.rs index 53208894d..5084d1f8d 100644 --- a/src/iter/tree.rs +++ b/src/iter/tree.rs @@ -104,6 +104,14 @@ pub trait TreeLike: Clone + Sized { fn post_order_iter(self) -> PostOrderIter { PostOrderIter { index: 0, stack: vec![IterStackItem::unprocessed(self, None)] } } + + /// Obtains an iterator of all the nodes rooted at the DAG, in right-to-left post order. + /// + /// This ordering is useful for "translation" algorithms which iterate over a + /// structure, pushing translated nodes and popping children. + fn rtl_post_order_iter(self) -> RtlPostOrderIter { + RtlPostOrderIter { inner: Rtl(self).post_order_iter() } + } } /// Element stored internally on the stack of a [`PostOrderIter`]. @@ -202,6 +210,53 @@ impl Iterator for PostOrderIter { } } +/// Adaptor structure to allow iterating in right-to-left order. +#[derive(Clone, Debug)] +struct Rtl(pub T); + +impl TreeLike for Rtl { + type NaryChildren = T::NaryChildren; + + fn nary_len(tc: &Self::NaryChildren) -> usize { T::nary_len(tc) } + fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { + let rtl_idx = T::nary_len(&tc) - idx - 1; + Rtl(T::nary_index(tc, rtl_idx)) + } + + fn as_node(&self) -> Tree { + match self.0.as_node() { + Tree::Nullary => Tree::Nullary, + Tree::Unary(a) => Tree::Unary(Rtl(a)), + Tree::Binary(a, b) => Tree::Binary(Rtl(b), Rtl(a)), + Tree::Ternary(a, b, c) => Tree::Ternary(Rtl(c), Rtl(b), Rtl(a)), + Tree::Nary(data) => Tree::Nary(data), + } + } +} + +/// Iterates over a DAG in _right-to-left post order_. +/// +/// That means nodes are yielded in the order (right child, left child, parent). +#[derive(Clone, Debug)] +pub struct RtlPostOrderIter { + inner: PostOrderIter>, +} + +impl Iterator for RtlPostOrderIter { + type Item = PostOrderIterItem; + + fn next(&mut self) -> Option { + self.inner.next().map(|mut item| { + item.child_indices.reverse(); + PostOrderIterItem { + child_indices: item.child_indices, + index: item.index, + node: item.node.0, + } + }) + } +} + /// Iterates over a [`TreeLike`] in _pre order_. /// /// Unlike the post-order iterator, this one does not keep track of indices diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index b02feb952..7cb9d61e0 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -86,9 +86,7 @@ mod private { /// and they can call `Miniscript::clone`. fn clone(&self) -> Self { let mut stack = vec![]; - for item in self.post_order_iter() { - let child_n = |n| Arc::clone(&stack[item.child_indices[n]]); - + for item in self.rtl_post_order_iter() { let new_term = match item.node.node { Terminal::PkK(ref p) => Terminal::PkK(p.clone()), Terminal::PkH(ref p) => Terminal::PkH(p.clone()), @@ -101,23 +99,31 @@ mod private { Terminal::Hash160(ref x) => Terminal::Hash160(x.clone()), Terminal::True => Terminal::True, Terminal::False => Terminal::False, - Terminal::Alt(..) => Terminal::Alt(child_n(0)), - Terminal::Swap(..) => Terminal::Swap(child_n(0)), - Terminal::Check(..) => Terminal::Check(child_n(0)), - Terminal::DupIf(..) => Terminal::DupIf(child_n(0)), - Terminal::Verify(..) => Terminal::Verify(child_n(0)), - Terminal::NonZero(..) => Terminal::NonZero(child_n(0)), - Terminal::ZeroNotEqual(..) => Terminal::ZeroNotEqual(child_n(0)), - Terminal::AndV(..) => Terminal::AndV(child_n(0), child_n(1)), - Terminal::AndB(..) => Terminal::AndB(child_n(0), child_n(1)), - Terminal::AndOr(..) => Terminal::AndOr(child_n(0), child_n(1), child_n(2)), - Terminal::OrB(..) => Terminal::OrB(child_n(0), child_n(1)), - Terminal::OrD(..) => Terminal::OrD(child_n(0), child_n(1)), - Terminal::OrC(..) => Terminal::OrC(child_n(0), child_n(1)), - Terminal::OrI(..) => Terminal::OrI(child_n(0), child_n(1)), - Terminal::Thresh(ref thresh) => Terminal::Thresh( - thresh.map_from_post_order_iter(&item.child_indices, &stack), + Terminal::Alt(..) => Terminal::Alt(stack.pop().unwrap()), + Terminal::Swap(..) => Terminal::Swap(stack.pop().unwrap()), + Terminal::Check(..) => Terminal::Check(stack.pop().unwrap()), + Terminal::DupIf(..) => Terminal::DupIf(stack.pop().unwrap()), + Terminal::Verify(..) => Terminal::Verify(stack.pop().unwrap()), + Terminal::NonZero(..) => Terminal::NonZero(stack.pop().unwrap()), + Terminal::ZeroNotEqual(..) => Terminal::ZeroNotEqual(stack.pop().unwrap()), + Terminal::AndV(..) => { + Terminal::AndV(stack.pop().unwrap(), stack.pop().unwrap()) + } + Terminal::AndB(..) => { + Terminal::AndB(stack.pop().unwrap(), stack.pop().unwrap()) + } + Terminal::AndOr(..) => Terminal::AndOr( + stack.pop().unwrap(), + stack.pop().unwrap(), + stack.pop().unwrap(), ), + Terminal::OrB(..) => Terminal::OrB(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrD(..) => Terminal::OrD(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrC(..) => Terminal::OrC(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrI(..) => Terminal::OrI(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::Thresh(ref thresh) => { + Terminal::Thresh(thresh.map_ref(|_| stack.pop().unwrap())) + } Terminal::Multi(ref thresh) => Terminal::Multi(thresh.clone()), Terminal::MultiA(ref thresh) => Terminal::MultiA(thresh.clone()), }; @@ -130,6 +136,7 @@ mod private { })); } + assert_eq!(stack.len(), 1); Arc::try_unwrap(stack.pop().unwrap()).unwrap() } } @@ -536,9 +543,7 @@ impl Miniscript { T: Translator, { let mut translated = vec![]; - for data in Arc::new(self.clone()).post_order_iter() { - let child_n = |n| Arc::clone(&translated[data.child_indices[n]]); - + for data in self.rtl_post_order_iter() { let new_term = match data.node.node { Terminal::PkK(ref p) => Terminal::PkK(t.pk(p)?), Terminal::PkH(ref p) => Terminal::PkH(t.pk(p)?), @@ -551,23 +556,39 @@ impl Miniscript { Terminal::Hash160(ref x) => Terminal::Hash160(t.hash160(x)?), Terminal::True => Terminal::True, Terminal::False => Terminal::False, - Terminal::Alt(..) => Terminal::Alt(child_n(0)), - Terminal::Swap(..) => Terminal::Swap(child_n(0)), - Terminal::Check(..) => Terminal::Check(child_n(0)), - Terminal::DupIf(..) => Terminal::DupIf(child_n(0)), - Terminal::Verify(..) => Terminal::Verify(child_n(0)), - Terminal::NonZero(..) => Terminal::NonZero(child_n(0)), - Terminal::ZeroNotEqual(..) => Terminal::ZeroNotEqual(child_n(0)), - Terminal::AndV(..) => Terminal::AndV(child_n(0), child_n(1)), - Terminal::AndB(..) => Terminal::AndB(child_n(0), child_n(1)), - Terminal::AndOr(..) => Terminal::AndOr(child_n(0), child_n(1), child_n(2)), - Terminal::OrB(..) => Terminal::OrB(child_n(0), child_n(1)), - Terminal::OrD(..) => Terminal::OrD(child_n(0), child_n(1)), - Terminal::OrC(..) => Terminal::OrC(child_n(0), child_n(1)), - Terminal::OrI(..) => Terminal::OrI(child_n(0), child_n(1)), - Terminal::Thresh(ref thresh) => Terminal::Thresh( - thresh.map_from_post_order_iter(&data.child_indices, &translated), + Terminal::Alt(..) => Terminal::Alt(translated.pop().unwrap()), + Terminal::Swap(..) => Terminal::Swap(translated.pop().unwrap()), + Terminal::Check(..) => Terminal::Check(translated.pop().unwrap()), + Terminal::DupIf(..) => Terminal::DupIf(translated.pop().unwrap()), + Terminal::Verify(..) => Terminal::Verify(translated.pop().unwrap()), + Terminal::NonZero(..) => Terminal::NonZero(translated.pop().unwrap()), + Terminal::ZeroNotEqual(..) => Terminal::ZeroNotEqual(translated.pop().unwrap()), + Terminal::AndV(..) => { + Terminal::AndV(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::AndB(..) => { + Terminal::AndB(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::AndOr(..) => Terminal::AndOr( + translated.pop().unwrap(), + translated.pop().unwrap(), + translated.pop().unwrap(), ), + Terminal::OrB(..) => { + Terminal::OrB(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::OrD(..) => { + Terminal::OrD(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::OrC(..) => { + Terminal::OrC(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::OrI(..) => { + Terminal::OrI(translated.pop().unwrap(), translated.pop().unwrap()) + } + Terminal::Thresh(ref thresh) => { + Terminal::Thresh(thresh.map_ref(|_| translated.pop().unwrap())) + } Terminal::Multi(ref thresh) => Terminal::Multi(thresh.translate_ref(|k| t.pk(k))?), Terminal::MultiA(ref thresh) => { Terminal::MultiA(thresh.translate_ref(|k| t.pk(k))?) @@ -582,22 +603,58 @@ impl Miniscript { /// Substitutes raw public keys hashes with the public keys as provided by map. pub fn substitute_raw_pkh(&self, pk_map: &BTreeMap) -> Miniscript { - let mut translated = vec![]; - for data in Arc::new(self.clone()).post_order_iter() { - let new_term = if let Terminal::RawPkH(ref p) = data.node.node { - match pk_map.get(p) { - Some(pk) => Terminal::PkH(pk.clone()), - None => Terminal::RawPkH(*p), + let mut stack = vec![]; + for item in self.rtl_post_order_iter() { + let new_term = match item.node.node { + Terminal::PkK(ref p) => Terminal::PkK(p.clone()), + Terminal::PkH(ref p) => Terminal::PkH(p.clone()), + // This algorithm is identical to Clone::clone except for this line. + Terminal::RawPkH(ref hash) => match pk_map.get(hash) { + Some(p) => Terminal::PkH(p.clone()), + None => Terminal::RawPkH(*hash), + }, + Terminal::After(ref n) => Terminal::After(*n), + Terminal::Older(ref n) => Terminal::Older(*n), + Terminal::Sha256(ref x) => Terminal::Sha256(x.clone()), + Terminal::Hash256(ref x) => Terminal::Hash256(x.clone()), + Terminal::Ripemd160(ref x) => Terminal::Ripemd160(x.clone()), + Terminal::Hash160(ref x) => Terminal::Hash160(x.clone()), + Terminal::True => Terminal::True, + Terminal::False => Terminal::False, + Terminal::Alt(..) => Terminal::Alt(stack.pop().unwrap()), + Terminal::Swap(..) => Terminal::Swap(stack.pop().unwrap()), + Terminal::Check(..) => Terminal::Check(stack.pop().unwrap()), + Terminal::DupIf(..) => Terminal::DupIf(stack.pop().unwrap()), + Terminal::Verify(..) => Terminal::Verify(stack.pop().unwrap()), + Terminal::NonZero(..) => Terminal::NonZero(stack.pop().unwrap()), + Terminal::ZeroNotEqual(..) => Terminal::ZeroNotEqual(stack.pop().unwrap()), + Terminal::AndV(..) => Terminal::AndV(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::AndB(..) => Terminal::AndB(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::AndOr(..) => Terminal::AndOr( + stack.pop().unwrap(), + stack.pop().unwrap(), + stack.pop().unwrap(), + ), + Terminal::OrB(..) => Terminal::OrB(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrD(..) => Terminal::OrD(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrC(..) => Terminal::OrC(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::OrI(..) => Terminal::OrI(stack.pop().unwrap(), stack.pop().unwrap()), + Terminal::Thresh(ref thresh) => { + Terminal::Thresh(thresh.map_ref(|_| stack.pop().unwrap())) } - } else { - data.node.node.clone() + Terminal::Multi(ref thresh) => Terminal::Multi(thresh.clone()), + Terminal::MultiA(ref thresh) => Terminal::MultiA(thresh.clone()), }; - let new_ms = Miniscript::from_ast(new_term).expect("typeck"); - translated.push(Arc::new(new_ms)); + stack.push(Arc::new(Miniscript::from_components_unchecked( + new_term, + item.node.ty, + item.node.ext, + ))); } - Arc::try_unwrap(translated.pop().unwrap()).unwrap() + assert_eq!(stack.len(), 1); + Arc::try_unwrap(stack.pop().unwrap()).unwrap() } } @@ -822,6 +879,7 @@ mod tests { } let roundtrip = Miniscript::from_str(&display).expect("parse string serialization"); assert_eq!(roundtrip, script); + assert_eq!(roundtrip.clone(), script); } fn string_display_debug_test( diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index d1b3b6353..fbefa510f 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -540,9 +540,7 @@ impl Policy { use Policy::*; let mut translated = vec![]; - for data in self.post_order_iter() { - let child_n = |n| Arc::clone(&translated[data.child_indices[n]]); - + for data in self.rtl_post_order_iter() { let new_policy = match data.node { Unsatisfiable => Unsatisfiable, Trivial => Trivial, @@ -553,15 +551,12 @@ impl Policy { Hash160(ref h) => t.hash160(h).map(Hash160)?, Older(ref n) => Older(*n), After(ref n) => After(*n), - And(ref subs) => And((0..subs.len()).map(child_n).collect()), + And(ref subs) => And((0..subs.len()).map(|_| translated.pop().unwrap()).collect()), Or(ref subs) => Or(subs .iter() - .enumerate() - .map(|(i, (prob, _))| (*prob, child_n(i))) + .map(|(prob, _)| (*prob, translated.pop().unwrap())) .collect()), - Thresh(ref thresh) => { - Thresh(thresh.map_from_post_order_iter(&data.child_indices, &translated)) - } + Thresh(ref thresh) => Thresh(thresh.map_ref(|_| translated.pop().unwrap())), }; translated.push(Arc::new(new_policy)); } @@ -576,20 +571,17 @@ impl Policy { use Policy::*; let mut translated = vec![]; - for data in Arc::new(self).post_order_iter() { - let child_n = |n| Arc::clone(&translated[data.child_indices[n]]); - + for data in Arc::new(self).rtl_post_order_iter() { let new_policy = match data.node.as_ref() { Policy::Key(ref k) if k.clone() == *key => Some(Policy::Unsatisfiable), - And(ref subs) => Some(And((0..subs.len()).map(child_n).collect())), + And(ref subs) => { + Some(And((0..subs.len()).map(|_| translated.pop().unwrap()).collect())) + } Or(ref subs) => Some(Or(subs .iter() - .enumerate() - .map(|(i, (prob, _))| (*prob, child_n(i))) + .map(|(prob, _)| (*prob, translated.pop().unwrap())) .collect())), - Thresh(ref thresh) => { - Some(Thresh(thresh.map_from_post_order_iter(&data.child_indices, &translated))) - } + Thresh(ref thresh) => Some(Thresh(thresh.map_ref(|_| translated.pop().unwrap()))), _ => None, }; match new_policy { @@ -620,12 +612,12 @@ impl Policy { use Policy::*; let mut nums = vec![]; - for data in Arc::new(self).post_order_iter() { - let num_for_child_n = |n| nums[data.child_indices[n]]; - + for data in self.rtl_post_order_iter() { let num = match data.node { - Or(subs) => (0..subs.len()).map(num_for_child_n).sum(), - Thresh(thresh) if thresh.is_or() => (0..thresh.n()).map(num_for_child_n).sum(), + Or(subs) => (0..subs.len()).map(|_| nums.pop().unwrap()).sum(), + Thresh(thresh) if thresh.is_or() => { + (0..thresh.n()).map(|_| nums.pop().unwrap()).sum() + } _ => 1, }; nums.push(num); @@ -682,9 +674,7 @@ impl Policy { use Policy::*; let mut infos = vec![]; - for data in Arc::new(self).post_order_iter() { - let info_for_child_n = |n| infos[data.child_indices[n]]; - + for data in self.rtl_post_order_iter() { let info = match data.node { Policy::After(ref t) => TimelockInfo { csv_with_height: false, @@ -701,15 +691,15 @@ impl Policy { contains_combination: false, }, And(ref subs) => { - let iter = (0..subs.len()).map(info_for_child_n); + let iter = (0..subs.len()).map(|_| infos.pop().unwrap()); TimelockInfo::combine_threshold(subs.len(), iter) } Or(ref subs) => { - let iter = (0..subs.len()).map(info_for_child_n); + let iter = (0..subs.len()).map(|_| infos.pop().unwrap()); TimelockInfo::combine_threshold(1, iter) } Thresh(ref thresh) => { - let iter = (0..thresh.n()).map(info_for_child_n); + let iter = (0..thresh.n()).map(|_| infos.pop().unwrap()); TimelockInfo::combine_threshold(thresh.k(), iter) } _ => TimelockInfo::default(), @@ -759,9 +749,7 @@ impl Policy { use Policy::*; let mut acc = vec![]; - for data in Arc::new(self).post_order_iter() { - let acc_for_child_n = |n| acc[data.child_indices[n]]; - + for data in self.rtl_post_order_iter() { let new = match data.node { Unsatisfiable | Trivial | Key(_) => (true, true), Sha256(_) | Hash256(_) | Ripemd160(_) | Hash160(_) | After(_) | Older(_) => { @@ -769,25 +757,24 @@ impl Policy { } And(ref subs) => { let (atleast_one_safe, all_non_mall) = (0..subs.len()) - .map(acc_for_child_n) + .map(|_| acc.pop().unwrap()) .fold((false, true), |acc, x: (bool, bool)| (acc.0 || x.0, acc.1 && x.1)); (atleast_one_safe, all_non_mall) } Or(ref subs) => { let (all_safe, atleast_one_safe, all_non_mall) = (0..subs.len()) - .map(acc_for_child_n) + .map(|_| acc.pop().unwrap()) .fold((true, false, true), |acc, x| { (acc.0 && x.0, acc.1 || x.0, acc.2 && x.1) }); (all_safe, atleast_one_safe && all_non_mall) } Thresh(ref thresh) => { - let (safe_count, non_mall_count) = (0..thresh.n()).map(acc_for_child_n).fold( - (0, 0), - |(safe_count, non_mall_count), (safe, non_mall)| { + let (safe_count, non_mall_count) = (0..thresh.n()) + .map(|_| acc.pop().unwrap()) + .fold((0, 0), |(safe_count, non_mall_count), (safe, non_mall)| { (safe_count + safe as usize, non_mall_count + non_mall as usize) - }, - ); + }); ( safe_count >= (thresh.n() - thresh.k() + 1), non_mall_count == thresh.n() && safe_count >= (thresh.n() - thresh.k()), diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs index d7258e6d1..794ab286b 100644 --- a/src/policy/semantic.rs +++ b/src/policy/semantic.rs @@ -109,7 +109,7 @@ impl Policy { use Policy::*; let mut translated = vec![]; - for data in self.post_order_iter() { + for data in self.rtl_post_order_iter() { let new_policy = match data.node { Unsatisfiable => Unsatisfiable, Trivial => Trivial, @@ -120,9 +120,7 @@ impl Policy { Hash160(ref h) => t.hash160(h).map(Hash160)?, Older(ref n) => Older(*n), After(ref n) => After(*n), - Thresh(ref thresh) => { - Thresh(thresh.map_from_post_order_iter(&data.child_indices, &translated)) - } + Thresh(ref thresh) => Thresh(thresh.map_ref(|_| translated.pop().unwrap())), }; translated.push(Arc::new(new_policy)); } @@ -170,11 +168,9 @@ impl Policy { use Policy::*; let mut n_terminals = vec![]; - for data in self.post_order_iter() { - let n_terminals_for_child_n = |n| n_terminals[data.child_indices[n]]; - + for data in self.rtl_post_order_iter() { let num = match data.node { - Thresh(thresh) => (0..thresh.n()).map(n_terminals_for_child_n).sum(), + Thresh(thresh) => (0..thresh.n()).map(|_| n_terminals.pop().unwrap()).sum(), Trivial | Unsatisfiable => 0, _leaf => 1, }; @@ -483,7 +479,7 @@ impl Policy { use Policy::*; let mut at_age = vec![]; - for data in Arc::new(self).post_order_iter() { + for data in Arc::new(self).rtl_post_order_iter() { let new_policy = match data.node.as_ref() { Older(ref t) => { if relative::LockTime::from(*t).is_implied_by(age) { @@ -492,9 +488,7 @@ impl Policy { Some(Unsatisfiable) } } - Thresh(ref thresh) => { - Some(Thresh(thresh.map_from_post_order_iter(&data.child_indices, &at_age))) - } + Thresh(ref thresh) => Some(Thresh(thresh.map_ref(|_| at_age.pop().unwrap()))), _ => None, }; match new_policy { @@ -515,7 +509,7 @@ impl Policy { use Policy::*; let mut at_age = vec![]; - for data in Arc::new(self).post_order_iter() { + for data in Arc::new(self).rtl_post_order_iter() { let new_policy = match data.node.as_ref() { After(t) => { if absolute::LockTime::from(*t).is_implied_by(n) { @@ -524,9 +518,7 @@ impl Policy { Some(Unsatisfiable) } } - Thresh(ref thresh) => { - Some(Thresh(thresh.map_from_post_order_iter(&data.child_indices, &at_age))) - } + Thresh(ref thresh) => Some(Thresh(thresh.map_ref(|_| at_age.pop().unwrap()))), _ => None, }; match new_policy { @@ -559,9 +551,7 @@ impl Policy { use Policy::*; let mut minimum_n_keys = vec![]; - for data in Arc::new(self).post_order_iter() { - let minimum_n_keys_for_child_n = |n| minimum_n_keys[data.child_indices[n]]; - + for data in self.rtl_post_order_iter() { let minimum_n_key = match data.node { Unsatisfiable => None, Trivial | After(..) | Older(..) | Sha256(..) | Hash256(..) | Ripemd160(..) @@ -569,7 +559,7 @@ impl Policy { Key(..) => Some(1), Thresh(ref thresh) => { let mut sublens = (0..thresh.n()) - .filter_map(minimum_n_keys_for_child_n) + .filter_map(|_| minimum_n_keys.pop().unwrap()) .collect::>(); if sublens.len() < thresh.k() { // Not enough branches are satisfiable @@ -597,11 +587,10 @@ impl Policy { use Policy::*; let mut sorted = vec![]; - for data in Arc::new(self).post_order_iter() { + for data in Arc::new(self).rtl_post_order_iter() { let new_policy = match data.node.as_ref() { Thresh(ref thresh) => { - let mut new_thresh = - thresh.map_from_post_order_iter(&data.child_indices, &sorted); + let mut new_thresh = thresh.map_ref(|_| sorted.pop().unwrap()); new_thresh.data_mut().sort(); Some(Thresh(new_thresh)) } From 44e05502cd23955c537e0f0e0cad9d28e0e24086 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 20 Aug 2024 16:55:02 +0000 Subject: [PATCH 6/7] miniscript: add unit test for substitute_raw_pkh --- src/miniscript/mod.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 7cb9d61e0..5a48d74a2 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -1431,8 +1431,12 @@ mod tests { #[test] fn expr_features() { // test that parsing raw hash160 does not work with - let hash160_str = "e9f171df53e04b270fa6271b42f66b0f4a99c5a2"; - let ms_str = &format!("c:expr_raw_pkh({})", hash160_str); + let pk = bitcoin::PublicKey::from_str( + "02c2fd50ceae468857bb7eb32ae9cd4083e6c7e42fbbec179d81134b3e3830586c", + ) + .unwrap(); + let hash160 = pk.pubkey_hash().to_raw_hash(); + let ms_str = &format!("c:expr_raw_pkh({})", hash160); type SegwitMs = Miniscript; // Test that parsing raw hash160 from string does not work without extra features @@ -1445,6 +1449,12 @@ mod tests { SegwitMs::parse(&script).unwrap_err(); SegwitMs::parse_insane(&script).unwrap_err(); SegwitMs::parse_with_ext(&script, &ExtParams::allow_all()).unwrap(); + + // Try replacing the raw_pkh with a pkh + let mut map = BTreeMap::new(); + map.insert(hash160, pk); + let ms_no_raw = ms.substitute_raw_pkh(&map); + assert_eq!(ms_no_raw.to_string(), format!("pkh({})", pk),); } #[test] From 8e2d90c50105d03fdc3e52c18d99c488886aa410 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 19 Aug 2024 20:59:12 +0000 Subject: [PATCH 7/7] policy: make lifting algorithm non-recursive, remove Liftable for Terminal It doesn't really make conceptual sense that Terminal would be Liftable. Terminal itself has no meaning; it isn't even typechecked. The recursive algorithm also repeats a lot of checks unnecessarily. Better to just call lift_check() once at the start of a Miniscript lift. --- src/policy/mod.rs | 115 ++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 59 deletions(-) diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 145c3fe61..8f7c1cc30 100644 --- a/src/policy/mod.rs +++ b/src/policy/mod.rs @@ -21,6 +21,7 @@ pub mod semantic; pub use self::concrete::Policy as Concrete; pub use self::semantic::Policy as Semantic; use crate::descriptor::Descriptor; +use crate::iter::TreeLike as _; use crate::miniscript::{Miniscript, ScriptContext}; use crate::sync::Arc; #[cfg(all(not(feature = "std"), not(test)))] @@ -111,68 +112,64 @@ impl Liftable for Miniscript // check whether the root miniscript can have a spending path that is // a combination of heightlock and timelock self.lift_check()?; - self.as_inner().lift() - } -} -impl Liftable for Terminal { - fn lift(&self) -> Result, Error> { - let ret = match *self { - Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => Semantic::Key(pk.clone()), - Terminal::RawPkH(ref _pkh) => { - return Err(Error::LiftError(LiftError::RawDescriptorLift)) - } - Terminal::After(t) => Semantic::After(t), - Terminal::Older(t) => Semantic::Older(t), - Terminal::Sha256(ref h) => Semantic::Sha256(h.clone()), - Terminal::Hash256(ref h) => Semantic::Hash256(h.clone()), - Terminal::Ripemd160(ref h) => Semantic::Ripemd160(h.clone()), - Terminal::Hash160(ref h) => Semantic::Hash160(h.clone()), - Terminal::False => Semantic::Unsatisfiable, - Terminal::True => Semantic::Trivial, - Terminal::Alt(ref sub) - | Terminal::Swap(ref sub) - | Terminal::Check(ref sub) - | Terminal::DupIf(ref sub) - | Terminal::Verify(ref sub) - | Terminal::NonZero(ref sub) - | Terminal::ZeroNotEqual(ref sub) => sub.node.lift()?, - Terminal::AndV(ref left, ref right) | Terminal::AndB(ref left, ref right) => { - Semantic::Thresh(Threshold::and( - Arc::new(left.node.lift()?), - Arc::new(right.node.lift()?), - )) - } - Terminal::AndOr(ref a, ref b, ref c) => Semantic::Thresh(Threshold::or( - Arc::new(Semantic::Thresh(Threshold::and( - Arc::new(a.node.lift()?), - Arc::new(b.node.lift()?), + let mut stack = vec![]; + for item in self.rtl_post_order_iter() { + let new_term = match item.node.node { + Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => { + Arc::new(Semantic::Key(pk.clone())) + } + Terminal::RawPkH(ref _pkh) => { + return Err(Error::LiftError(LiftError::RawDescriptorLift)) + } + Terminal::After(t) => Arc::new(Semantic::After(t)), + Terminal::Older(t) => Arc::new(Semantic::Older(t)), + Terminal::Sha256(ref h) => Arc::new(Semantic::Sha256(h.clone())), + Terminal::Hash256(ref h) => Arc::new(Semantic::Hash256(h.clone())), + Terminal::Ripemd160(ref h) => Arc::new(Semantic::Ripemd160(h.clone())), + Terminal::Hash160(ref h) => Arc::new(Semantic::Hash160(h.clone())), + Terminal::False => Arc::new(Semantic::Unsatisfiable), + Terminal::True => Arc::new(Semantic::Trivial), + Terminal::Alt(..) + | Terminal::Swap(..) + | Terminal::Check(..) + | Terminal::DupIf(..) + | Terminal::Verify(..) + | Terminal::NonZero(..) + | Terminal::ZeroNotEqual(..) => stack.pop().unwrap(), + Terminal::AndV(..) | Terminal::AndB(..) => Arc::new(Semantic::Thresh( + Threshold::and(stack.pop().unwrap(), stack.pop().unwrap()), + )), + Terminal::AndOr(..) => Arc::new(Semantic::Thresh(Threshold::or( + Arc::new(Semantic::Thresh(Threshold::and( + stack.pop().unwrap(), + stack.pop().unwrap(), + ))), + stack.pop().unwrap(), ))), - Arc::new(c.node.lift()?), - )), - Terminal::OrB(ref left, ref right) - | Terminal::OrD(ref left, ref right) - | Terminal::OrC(ref left, ref right) - | Terminal::OrI(ref left, ref right) => Semantic::Thresh(Threshold::or( - Arc::new(left.node.lift()?), - Arc::new(right.node.lift()?), - )), - Terminal::Thresh(ref thresh) => thresh - .translate_ref(|sub| sub.lift().map(Arc::new)) - .map(Semantic::Thresh)?, - Terminal::Multi(ref thresh) => Semantic::Thresh( - thresh - .map_ref(|key| Arc::new(Semantic::Key(key.clone()))) - .forget_maximum(), - ), - Terminal::MultiA(ref thresh) => Semantic::Thresh( - thresh - .map_ref(|key| Arc::new(Semantic::Key(key.clone()))) - .forget_maximum(), - ), + Terminal::OrB(..) | Terminal::OrD(..) | Terminal::OrC(..) | Terminal::OrI(..) => { + Arc::new(Semantic::Thresh(Threshold::or( + stack.pop().unwrap(), + stack.pop().unwrap(), + ))) + } + Terminal::Thresh(ref thresh) => { + Arc::new(Semantic::Thresh(thresh.map_ref(|_| stack.pop().unwrap()))) + } + Terminal::Multi(ref thresh) => Arc::new(Semantic::Thresh( + thresh + .map_ref(|key| Arc::new(Semantic::Key(key.clone()))) + .forget_maximum(), + )), + Terminal::MultiA(ref thresh) => Arc::new(Semantic::Thresh( + thresh + .map_ref(|key| Arc::new(Semantic::Key(key.clone()))) + .forget_maximum(), + )), + }; + stack.push(new_term) } - .normalized(); - Ok(ret) + Ok(Arc::try_unwrap(stack.pop().unwrap()).unwrap().normalized()) } }