miniscript: eliminate recursion from TranslatePk

apoelstra · apoelstra · commit 8c32a19b4813 · 2023-07-16T13:36:42.000Z
This is again a breaking change because we remove the trait impl from `Terminal`, but again I don't think the trait impl should've existed. This also exposed an "off-label" use of `real_translate_pk` to convert context types as well as pk types, which I apparently explicitly reviewed and ACKed when reviewing rust-bitcoin#426, but which in retrospect looks a little funny. Rename this function to translate_pk_ctx so it's clear that it's doing two jobs.
diff --git a/src/interpreter/inner.rs b/src/interpreter/inner.rs
@@ -380,7 +380,7 @@ impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::PublicKey, Ctx> {
             translate_hash_clone!(bitcoin::PublicKey, BitcoinKey, ());
         }
 
-        self.real_translate_pk(&mut TranslateFullPk)
+        self.translate_pk_ctx(&mut TranslateFullPk)
             .expect("Translation should succeed")
     }
 }
@@ -397,7 +397,7 @@ impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::key::XOnlyPublicKey,
 
             translate_hash_clone!(bitcoin::key::XOnlyPublicKey, BitcoinKey, ());
         }
-        self.real_translate_pk(&mut TranslateXOnlyPk)
+        self.translate_pk_ctx(&mut TranslateXOnlyPk)
             .expect("Translation should succeed")
     }
 }
diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs
@@ -22,7 +22,7 @@ use crate::prelude::*;
 use crate::util::MsKeyBuilder;
 use crate::{
     errstr, expression, script_num_size, AbsLockTime, Error, Miniscript, MiniscriptKey, Terminal,
-    ToPublicKey, TranslateErr, TranslatePk, Translator,
+    ToPublicKey,
 };
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
@@ -46,102 +46,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
     }
 }
 
-impl<Pk, Q, Ctx> TranslatePk<Pk, Q> for Terminal<Pk, Ctx>
-where
-    Pk: MiniscriptKey,
-    Q: MiniscriptKey,
-    Ctx: ScriptContext,
-{
-    type Output = Terminal<Q, Ctx>;
-
-    /// Converts an AST element with one public key type to one of another public key type.
-    fn translate_pk<T, E>(&self, translate: &mut T) -> Result<Self::Output, TranslateErr<E>>
-    where
-        T: Translator<Pk, Q, E>,
-    {
-        self.real_translate_pk(translate)
-    }
-}
-
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
-    pub(super) fn real_translate_pk<Q, CtxQ, T, E>(
-        &self,
-        t: &mut T,
-    ) -> Result<Terminal<Q, CtxQ>, TranslateErr<E>>
-    where
-        Q: MiniscriptKey,
-        CtxQ: ScriptContext,
-        T: Translator<Pk, Q, E>,
-    {
-        let frag: Terminal<Q, CtxQ> = match *self {
-            Terminal::PkK(ref p) => Terminal::PkK(t.pk(p)?),
-            Terminal::PkH(ref p) => Terminal::PkH(t.pk(p)?),
-            Terminal::RawPkH(ref p) => Terminal::RawPkH(*p),
-            Terminal::After(n) => Terminal::After(n),
-            Terminal::Older(n) => Terminal::Older(n),
-            Terminal::Sha256(ref x) => Terminal::Sha256(t.sha256(x)?),
-            Terminal::Hash256(ref x) => Terminal::Hash256(t.hash256(x)?),
-            Terminal::Ripemd160(ref x) => Terminal::Ripemd160(t.ripemd160(x)?),
-            Terminal::Hash160(ref x) => Terminal::Hash160(t.hash160(x)?),
-            Terminal::True => Terminal::True,
-            Terminal::False => Terminal::False,
-            Terminal::Alt(ref sub) => Terminal::Alt(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::Swap(ref sub) => Terminal::Swap(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::Check(ref sub) => Terminal::Check(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::DupIf(ref sub) => Terminal::DupIf(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::Verify(ref sub) => Terminal::Verify(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::NonZero(ref sub) => Terminal::NonZero(Arc::new(sub.real_translate_pk(t)?)),
-            Terminal::ZeroNotEqual(ref sub) => {
-                Terminal::ZeroNotEqual(Arc::new(sub.real_translate_pk(t)?))
-            }
-            Terminal::AndV(ref left, ref right) => Terminal::AndV(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::AndB(ref left, ref right) => Terminal::AndB(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::AndOr(ref a, ref b, ref c) => Terminal::AndOr(
-                Arc::new(a.real_translate_pk(t)?),
-                Arc::new(b.real_translate_pk(t)?),
-                Arc::new(c.real_translate_pk(t)?),
-            ),
-            Terminal::OrB(ref left, ref right) => Terminal::OrB(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::OrD(ref left, ref right) => Terminal::OrD(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::OrC(ref left, ref right) => Terminal::OrC(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::OrI(ref left, ref right) => Terminal::OrI(
-                Arc::new(left.real_translate_pk(t)?),
-                Arc::new(right.real_translate_pk(t)?),
-            ),
-            Terminal::Thresh(k, ref subs) => {
-                let subs: Result<Vec<Arc<Miniscript<Q, _>>>, _> = subs
-                    .iter()
-                    .map(|s| s.real_translate_pk(t).map(Arc::new))
-                    .collect();
-                Terminal::Thresh(k, subs?)
-            }
-            Terminal::Multi(k, ref keys) => {
-                let keys: Result<Vec<Q>, _> = keys.iter().map(|k| t.pk(k)).collect();
-                Terminal::Multi(k, keys?)
-            }
-            Terminal::MultiA(k, ref keys) => {
-                let keys: Result<Vec<Q>, _> = keys.iter().map(|k| t.pk(k)).collect();
-                Terminal::MultiA(k, keys?)
-            }
-        };
-        Ok(frag)
-    }
-
     /// Substitutes raw public keys hashes with the public keys as provided by map.
     pub fn substitute_raw_pkh(&self, pk_map: &BTreeMap<hash160::Hash, Pk>) -> Terminal<Pk, Ctx> {
         match self {
diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs
@@ -331,16 +331,16 @@ where
 
     /// Translates a struct from one generic to another where the translation
     /// for Pk is provided by [`Translator`]
-    fn translate_pk<T, E>(&self, translate: &mut T) -> Result<Self::Output, TranslateErr<E>>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<Pk, Q, E>,
     {
-        self.real_translate_pk(translate)
+        self.translate_pk_ctx(t)
     }
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
-    pub(super) fn real_translate_pk<Q, CtxQ, T, FuncError>(
+    pub(super) fn translate_pk_ctx<Q, CtxQ, T, FuncError>(
         &self,
         t: &mut T,
     ) -> Result<Miniscript<Q, CtxQ>, TranslateErr<FuncError>>
@@ -349,8 +349,54 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
         CtxQ: ScriptContext,
         T: Translator<Pk, Q, FuncError>,
     {
-        let inner = self.node.real_translate_pk(t)?;
-        Miniscript::from_ast(inner).map_err(TranslateErr::OuterError)
+        let mut translated = vec![];
+        for data in Arc::new(self.clone()).post_order_iter() {
+            // convenience method to reduce typing
+            let child_n = |n| Arc::clone(&translated[data.child_indices[n]]);
+
+            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)?),
+                Terminal::RawPkH(ref p) => Terminal::RawPkH(*p),
+                Terminal::After(n) => Terminal::After(n),
+                Terminal::Older(n) => Terminal::Older(n),
+                Terminal::Sha256(ref x) => Terminal::Sha256(t.sha256(x)?),
+                Terminal::Hash256(ref x) => Terminal::Hash256(t.hash256(x)?),
+                Terminal::Ripemd160(ref x) => Terminal::Ripemd160(t.ripemd160(x)?),
+                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(k, ref subs) => {
+                    Terminal::Thresh(k, (0..subs.len()).map(child_n).collect())
+                }
+                Terminal::Multi(k, ref keys) => {
+                    let keys: Result<Vec<Q>, _> = keys.iter().map(|k| t.pk(k)).collect();
+                    Terminal::Multi(k, keys?)
+                }
+                Terminal::MultiA(k, ref keys) => {
+                    let keys: Result<Vec<Q>, _> = keys.iter().map(|k| t.pk(k)).collect();
+                    Terminal::MultiA(k, keys?)
+                }
+            };
+            let new_ms = Miniscript::from_ast(new_term).map_err(TranslateErr::OuterError)?;
+            translated.push(Arc::new(new_ms));
+        }
+
+        Ok(Arc::try_unwrap(translated.pop().unwrap()).unwrap())
     }
 
     /// Substitutes raw public keys hashes with the public keys as provided by map.

Original file line number	Diff line number	Diff line change
`@@ -380,7 +380,7 @@ impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::PublicKey, Ctx> {`
`380`	`380`	`translate_hash_clone!(bitcoin::PublicKey, BitcoinKey, ());`
`381`	`381`	`}`
`382`	`382`
`383`		`- self.real_translate_pk(&mut TranslateFullPk)`
	`383`	`+ self.translate_pk_ctx(&mut TranslateFullPk)`
`384`	`384`	`.expect("Translation should succeed")`
`385`	`385`	`}`
`386`	`386`	`}`
`@@ -397,7 +397,7 @@ impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::key::XOnlyPublicKey,`
`397`	`397`
`398`	`398`	`translate_hash_clone!(bitcoin::key::XOnlyPublicKey, BitcoinKey, ());`
`399`	`399`	`}`
`400`		`- self.real_translate_pk(&mut TranslateXOnlyPk)`
	`400`	`+ self.translate_pk_ctx(&mut TranslateXOnlyPk)`
`401`	`401`	`.expect("Translation should succeed")`
`402`	`402`	`}`
`403`	`403`	`}`