Merge rust-bitcoin/rust-miniscript#546: Fix ForEachKey impls for policies

f2f95fa62777b0bd96071bba5249d62115c73fbb cargo fmt (Andrew Poelstra)
a6340feed7faa3519775f0e79412045662359421 remove a bunch of unnecessary where clauses (Andrew Poelstra)
663bc00d8bfd7a87a0e88ac7d0220674d9272299 concrete: fix infinite recursion in Policy (Andrew Poelstra)
520b9db6e96535bca8e72f4258aca159abaa8fa3 semantic: fix todo and infinite recursion in Policy (Andrew Poelstra)

Pull request description:

  Our `ForEachKey` impls for the two policy types did not work. Both had an infinite type recursion which meant that if anybody had tried to use them they would not have compiled. (Though the fact that this has been happening for at least 2 years with zero bug reports suggests that nobody *has* tried to use them..)

  Recent Rust compiler nightlies have started failing compilation even when the offending impls are *not* used, which means we need to prioritize fixing this. Possibly we should even backport it.

  Fixes #541

ACKs for top commit:
  sanket1729:
    ACK f2f95fa62777b0bd96071bba5249d62115c73fbb

Tree-SHA512: a5d673929cd43187b157cd09941e9f922c605668085870d849fff84a404cb19dbc64694d1c173d4dcb5ff0d85dff450de1c6c6d4ceae038971dafca88ba40447

Author: heap-coder

src/descriptor/bare.rs

@@ -176,10 +176,7 @@ impl_from_str!(
 );
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Bare<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
         self.ms.for_each_key(pred)
     }
 }
@@ -361,10 +358,7 @@ impl_from_str!(
 );
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Pkh<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
         pred(&self.pk)
     }
 }

src/descriptor/mod.rs

@@ -536,10 +536,7 @@ where
 }
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Descriptor<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
         match *self {
             Descriptor::Bare(ref bare) => bare.for_each_key(pred),
             Descriptor::Pkh(ref pkh) => pkh.for_each_key(pred),

src/descriptor/segwitv0.rs

@@ -267,10 +267,7 @@ impl_from_str!(
 );
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Wsh<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
         match self.inner {
             WshInner::SortedMulti(ref smv) => smv.for_each_key(pred),
             WshInner::Ms(ref ms) => ms.for_each_key(pred),
@@ -474,10 +471,7 @@ impl_from_str!(
 );
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Wpkh<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
         pred(&self.pk)
     }
 }

src/descriptor/sh.rs

@@ -420,10 +420,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Sh<Pk> {
 }
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Sh<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
         match self.inner {
             ShInner::Wsh(ref wsh) => wsh.for_each_key(pred),
             ShInner::SortedMulti(ref smv) => smv.for_each_key(pred),

src/descriptor/sortedmulti.rs

@@ -102,10 +102,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> ForEachKey<Pk> for SortedMultiVec<Pk, Ctx> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: F) -> bool {
         self.pks.iter().all(pred)
     }
 }

src/descriptor/tr.rs

@@ -612,10 +612,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Tr<Pk> {
 }
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Tr<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
         let script_keys_res = self
             .iter_scripts()
             .all(|(_d, ms)| ms.for_each_key(&mut pred));

src/miniscript/astelem.rs

@@ -64,10 +64,7 @@ where
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
-    pub(super) fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool
-    where
-        Pk: 'a,
-    {
+    pub(super) fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool {
         match *self {
             Terminal::PkK(ref p) => pred(p),
             Terminal::PkH(ref p) => pred(p),
@@ -182,10 +179,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> ForEachKey<Pk> for Terminal<Pk, Ctx> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
         self.real_for_each_key(&mut pred)
     }
 }

src/miniscript/mod.rs

@@ -273,10 +273,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> ForEachKey<Pk> for Miniscript<Pk, Ctx> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
         self.real_for_each_key(&mut pred)
     }
 }
@@ -300,10 +297,7 @@ where
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
-    fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool {
         self.node.real_for_each_key(pred)
     }
 

src/policy/concrete.rs

@@ -664,10 +664,13 @@ impl<Pk: MiniscriptKey> PolicyArc<Pk> {
 }
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Policy<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
+        self.real_for_each_key(&mut pred)
+    }
+}
+
+impl<Pk: MiniscriptKey> Policy<Pk> {
+    fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool {
         match *self {
             Policy::Unsatisfiable | Policy::Trivial => true,
             Policy::Key(ref pk) => pred(pk),
@@ -678,14 +681,14 @@ impl<Pk: MiniscriptKey> ForEachKey<Pk> for Policy<Pk> {
             | Policy::After(..)
             | Policy::Older(..) => true,
             Policy::Threshold(_, ref subs) | Policy::And(ref subs) => {
-                subs.iter().all(|sub| sub.for_each_key(&mut pred))
+                subs.iter().all(|sub| sub.real_for_each_key(&mut *pred))
             }
-            Policy::Or(ref subs) => subs.iter().all(|(_, sub)| sub.for_each_key(&mut pred)),
+            Policy::Or(ref subs) => subs
+                .iter()
+                .all(|(_, sub)| sub.real_for_each_key(&mut *pred)),
         }
     }
-}
 
-impl<Pk: MiniscriptKey> Policy<Pk> {
     /// Convert a policy using one kind of public key to another
     /// type of public key
     ///
@@ -1291,7 +1294,7 @@ fn generate_combination<Pk: MiniscriptKey>(
 }
 
 #[cfg(all(test, feature = "compiler"))]
-mod tests {
+mod compiler_tests {
     use core::str::FromStr;
 
     use sync::Arc;
@@ -1352,3 +1355,22 @@ mod tests {
         assert_eq!(combinations, expected_comb);
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::str::FromStr;
+
+    use super::*;
+
+    #[test]
+    fn for_each_key() {
+        let liquid_pol = Policy::<String>::from_str(
+            "or(and(older(4096),thresh(2,pk(A),pk(B),pk(C))),thresh(11,pk(F1),pk(F2),pk(F3),pk(F4),pk(F5),pk(F6),pk(F7),pk(F8),pk(F9),pk(F10),pk(F11),pk(F12),pk(F13),pk(F14)))").unwrap();
+        let mut count = 0;
+        assert!(liquid_pol.for_each_key(|_| {
+            count += 1;
+            true
+        }));
+        assert_eq!(count, 17);
+    }
+}

src/policy/semantic.rs

@@ -61,25 +61,28 @@ where
 }
 
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Policy<Pk> {
-    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool
-    where
-        Pk: 'a,
-    {
+    fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool {
+        self.real_for_each_key(&mut pred)
+    }
+}
+
+impl<Pk: MiniscriptKey> Policy<Pk> {
+    fn real_for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, pred: &mut F) -> bool {
         match *self {
             Policy::Unsatisfiable | Policy::Trivial => true,
-            Policy::Key(ref _pkh) => todo!("Semantic Policy KeyHash must store Pk"),
+            Policy::Key(ref pk) => pred(pk),
             Policy::Sha256(..)
             | Policy::Hash256(..)
             | Policy::Ripemd160(..)
             | Policy::Hash160(..)
             | Policy::After(..)
             | Policy::Older(..) => true,
-            Policy::Threshold(_, ref subs) => subs.iter().all(|sub| sub.for_each_key(&mut pred)),
+            Policy::Threshold(_, ref subs) => {
+                subs.iter().all(|sub| sub.real_for_each_key(&mut *pred))
+            }
         }
     }
-}
 
-impl<Pk: MiniscriptKey> Policy<Pk> {
     /// Convert a policy using one kind of public key to another
     /// type of public key
     ///
@@ -970,4 +973,16 @@ mod tests {
         assert!(auth_alice.entails(htlc_pol.clone()).unwrap());
         assert!(htlc_pol.entails(control_alice).unwrap());
     }
+
+    #[test]
+    fn for_each_key() {
+        let liquid_pol = StringPolicy::from_str(
+            "or(and(older(4096),thresh(2,pk(A),pk(B),pk(C))),thresh(11,pk(F1),pk(F2),pk(F3),pk(F4),pk(F5),pk(F6),pk(F7),pk(F8),pk(F9),pk(F10),pk(F11),pk(F12),pk(F13),pk(F14)))").unwrap();
+        let mut count = 0;
+        assert!(liquid_pol.for_each_key(|_| {
+            count += 1;
+            true
+        }));
+        assert_eq!(count, 17);
+    }
 }