From 7c977b5367a6c025aec39d4bb7ab9c1daa9c7802 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Thu, 8 Aug 2024 15:35:37 +0000
Subject: [PATCH] ForEachKey: clean up Concrete impl, remove Semantic impl

The Concrete impl used a weird complier gitch `let pred = |x| pred(x)`
to avoid an infinite recursion bug (which in turn triggered a compiler
bug that they didn't fix even though I filed it a DAY AFTER IT WAS
INTRODUCED ON NIGHTLY and instead they waited 3 months and released it
on stable forcing me to backport a workaround to a gazillion versions of
rust-miniscript).

The bug was https://github.com/rust-lang/rust/issues/110475 BTW, just to
make sure this commit triggers another notification on that issue.

Clippy doesn't like this. Clippy is obviously wrong but rather than
having this stupid fight again just switch to a different idiom.

Meanwhile, the Semantic impl of ForEachKey panics iff any keys are
present, regardless of the closure passed to it. This is funny but
completely useless and we should just remove it.
---
 src/policy/concrete.rs | 13 ++++++++-----
 src/policy/semantic.rs | 22 +---------------------
 2 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs
index 8bf1b859..ab89af78 100644
--- a/src/policy/concrete.rs
+++ b/src/policy/concrete.rs
@@ -662,7 +662,12 @@ impl<Pk: MiniscriptKey> ForEachKey<Pk> for Policy<Pk> {
     where
         Pk: 'a,
     {
-        let mut pred = |x| pred(x);
+        self.for_each_key_internal(&mut pred)
+    }
+}
+
+impl<Pk: MiniscriptKey> Policy<Pk> {
+    fn for_each_key_internal<'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),
@@ -673,14 +678,12 @@ 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.for_each_key_internal(pred))
             }
-            Policy::Or(ref subs) => subs.iter().all(|(_, sub)| sub.for_each_key(&mut pred)),
+            Policy::Or(ref subs) => subs.iter().all(|(_, sub)| sub.for_each_key_internal(pred)),
         }
     }
-}
 
-impl<Pk: MiniscriptKey> Policy<Pk> {
     /// Convert a policy using one kind of public key to another
     /// type of public key
     ///
diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs
index 1dbb30a9..3e7e5325 100644
--- a/src/policy/semantic.rs
+++ b/src/policy/semantic.rs
@@ -10,7 +10,7 @@ use elements::{LockTime, Sequence};
 
 use super::concrete::PolicyError;
 use super::ENTAILMENT_MAX_TERMINALS;
-use crate::{errstr, expression, AbsLockTime, Error, ForEachKey, MiniscriptKey, Translator};
+use crate::{errstr, expression, AbsLockTime, Error, MiniscriptKey, Translator};
 
 /// Abstract policy which corresponds to the semantics of a Miniscript
 /// and which allows complex forms of analysis, e.g. filtering and
@@ -59,26 +59,6 @@ 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,
-    {
-        let mut pred = |x| pred(x);
-        match *self {
-            Policy::Unsatisfiable | Policy::Trivial => true,
-            Policy::Key(ref _pkh) => todo!("Semantic Policy KeyHash must store 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)),
-        }
-    }
-}
-
 impl<Pk: MiniscriptKey> Policy<Pk> {
     /// Convert a policy using one kind of public key to another
     /// type of public key