Only compile polcies containing non repeated keys

Some of the compiler guanrantees are lost if we
allow repeated keys. The compiler should error on
these keys

Author: sanket1729

src/policy/compiler.rs

@@ -1177,11 +1177,9 @@ mod tests {
     use policy::Liftable;
     use script_num_size;
     use BitcoinSig;
-    use DummyKey;
     use NullCtx;
 
     type SPolicy = Concrete<String>;
-    type DummyPolicy = Concrete<DummyKey>;
     type BPolicy = Concrete<bitcoin::PublicKey>;
     type DummySegwitAstElemExt = policy::compiler::AstElemExt<String, Segwitv0>;
     type SegwitMiniScript = Miniscript<bitcoin::PublicKey, Segwitv0>;
@@ -1212,8 +1210,8 @@ mod tests {
     }
 
     fn policy_compile_lift_check(s: &str) -> Result<(), CompilerError> {
-        let policy = DummyPolicy::from_str(s).expect("parse");
-        let miniscript: Miniscript<DummyKey, Segwitv0> = policy.compile()?;
+        let policy = SPolicy::from_str(s).expect("parse");
+        let miniscript: Miniscript<String, Segwitv0> = policy.compile()?;
 
         assert_eq!(
             policy.lift().unwrap().sorted(),
@@ -1225,21 +1223,21 @@ mod tests {
     #[test]
     fn compile_timelocks() {
         // artificially create a policy that is problematic and try to compile
-        let pol: DummyPolicy = Concrete::And(vec![
-            Concrete::Key(DummyKey),
+        let pol: SPolicy = Concrete::And(vec![
+            Concrete::Key("A".to_string()),
             Concrete::And(vec![Concrete::After(9), Concrete::After(1000_000_000)]),
         ]);
         assert!(pol.compile::<Segwitv0>().is_err());
 
         // This should compile
-        let pol: DummyPolicy =
-            DummyPolicy::from_str("and(pk(),or(and(after(9),pk()),and(after(1000000000),pk())))")
+        let pol: SPolicy =
+            SPolicy::from_str("and(pk(A),or(and(after(9),pk(B)),and(after(1000000000),pk(C))))")
                 .unwrap();
         assert!(pol.compile::<Segwitv0>().is_ok());
     }
     #[test]
     fn compile_basic() {
-        assert!(policy_compile_lift_check("pk()").is_ok());
+        assert!(policy_compile_lift_check("pk(A)").is_ok());
         assert_eq!(
             policy_compile_lift_check("after(9)"),
             Err(CompilerError::TopLevelNonSafe)
@@ -1254,24 +1252,24 @@ mod tests {
             ),
             Err(CompilerError::TopLevelNonSafe)
         );
-        assert!(policy_compile_lift_check("and(pk(),pk())").is_ok());
-        assert!(policy_compile_lift_check("or(pk(),pk())").is_ok());
-        assert!(policy_compile_lift_check("thresh(2,pk(),pk(),pk())").is_ok());
+        assert!(policy_compile_lift_check("and(pk(A),pk(B))").is_ok());
+        assert!(policy_compile_lift_check("or(pk(A),pk(B))").is_ok());
+        assert!(policy_compile_lift_check("thresh(2,pk(A),pk(B),pk(C))").is_ok());
 
         assert_eq!(
-            policy_compile_lift_check("thresh(2,after(9),after(9),pk())"),
+            policy_compile_lift_check("thresh(2,after(9),after(9),pk(A))"),
             Err(CompilerError::TopLevelNonSafe)
         );
 
         assert_eq!(
-            policy_compile_lift_check("and(pk(),or(after(9),after(9)))"),
+            policy_compile_lift_check("and(pk(A),or(after(9),after(9)))"),
             Err(CompilerError::ImpossibleNonMalleableCompilation)
         );
     }
 
     #[test]
     fn compile_q() {
-        let policy = SPolicy::from_str("or(1@and(pk(),pk()),127@pk())").expect("parsing");
+        let policy = SPolicy::from_str("or(1@and(pk(A),pk(B)),127@pk(C))").expect("parsing");
         let compilation: DummySegwitAstElemExt =
             best_t(&mut BTreeMap::new(), &policy, 1.0, None).unwrap();
 
@@ -1282,7 +1280,7 @@ mod tests {
         );
 
         let policy = SPolicy::from_str(
-                "and(and(and(or(127@thresh(2,pk(),pk(),thresh(2,or(127@pk(),1@pk()),after(100),or(and(pk(),after(200)),and(pk(),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925))),pk())),1@pk()),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925)),or(127@pk(),1@after(300))),or(127@after(400),pk()))"
+                "and(and(and(or(127@thresh(2,pk(A),pk(B),thresh(2,or(127@pk(A),1@pk(B)),after(100),or(and(pk(C),after(200)),and(pk(D),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925))),pk(E))),1@pk(F)),sha256(66687aadf862bd776c8fc18b8e9f8e20089714856ee233b3902a591d0d5f2925)),or(127@pk(G),1@after(300))),or(127@after(400),pk(H)))"
             ).expect("parsing");
         let compilation: DummySegwitAstElemExt =
             best_t(&mut BTreeMap::new(), &policy, 1.0, None).unwrap();
@@ -1488,42 +1486,41 @@ mod benches {
 
     use super::{CompilerError, Concrete};
     use miniscript::Segwitv0;
-    use DummyKey;
     use Miniscript;
-    type DummySegwitMiniscriptRes = Result<Miniscript<DummyKey, Segwitv0>, CompilerError>;
+    type SegwitMsRes = Result<Miniscript<String, Segwitv0>, CompilerError>;
     #[bench]
     pub fn compile_basic(bh: &mut Bencher) {
         let h = (0..64).map(|_| "a").collect::<String>();
-        let pol = Concrete::<DummyKey>::from_str(&format!(
-            "and(thresh(2,and(sha256({}),or(sha256({}),pk())),pk(),pk(),pk(),sha256({})),pk())",
+        let pol = Concrete::<String>::from_str(&format!(
+            "and(thresh(2,and(sha256({}),or(sha256({}),pk(A))),pk(B),pk(C),pk(D),sha256({})),pk(E))",
             h, h, h
         ))
         .expect("parsing");
         bh.iter(|| {
-            let pt: DummySegwitMiniscriptRes = pol.compile();
+            let pt: SegwitMsRes = pol.compile();
             black_box(pt).unwrap();
         });
     }
 
     #[bench]
     pub fn compile_large(bh: &mut Bencher) {
         let h = (0..64).map(|_| "a").collect::<String>();
-        let pol = Concrete::<DummyKey>::from_str(
-            &format!("or(pk(),thresh(9,sha256({}),pk(),pk(),and(or(pk(),pk()),pk()),after(100),pk(),pk(),pk(),pk(),and(pk(),pk())))", h)
+        let pol = Concrete::<String>::from_str(
+            &format!("or(pk(L),thresh(9,sha256({}),pk(A),pk(B),and(or(pk(C),pk(D)),pk(E)),after(100),pk(F),pk(G),pk(H),pk(I),and(pk(J),pk(K))))", h)
         ).expect("parsing");
         bh.iter(|| {
-            let pt: DummySegwitMiniscriptRes = pol.compile();
+            let pt: SegwitMsRes = pol.compile();
             black_box(pt).unwrap();
         });
     }
 
     #[bench]
     pub fn compile_xlarge(bh: &mut Bencher) {
-        let pol = Concrete::<DummyKey>::from_str(
-            "or(pk(),thresh(4,pk(),older(100),pk(),and(after(100),or(pk(),or(pk(),and(pk(),thresh(2,pk(),or(pk(),and(thresh(5,pk(),or(pk(),pk()),pk(),pk(),pk(),pk(),pk(),pk(),pk(),pk(),pk()),pk())),pk(),or(and(pk(),pk()),pk()),after(100)))))),pk()))"
+        let pol = Concrete::<String>::from_str(
+            "or(pk(A),thresh(4,pk(B),older(100),pk(C),and(after(100),or(pk(D),or(pk(E),and(pk(F),thresh(2,pk(G),or(pk(H),and(thresh(5,pk(I),or(pk(J),pk(K)),pk(L),pk(M),pk(N),pk(O),pk(P),pk(Q),pk(R),pk(S),pk(T)),pk(U))),pk(V),or(and(pk(W),pk(X)),pk(Y)),after(100)))))),pk(Z)))"
         ).expect("parsing");
         bh.iter(|| {
-            let pt: DummySegwitMiniscriptRes = pol.compile();
+            let pt: SegwitMsRes = pol.compile();
             black_box(pt).unwrap();
         });
     }

src/policy/concrete.rs

@@ -17,6 +17,7 @@
 
 use bitcoin::hashes::hex::FromHex;
 use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d};
+use std::collections::HashSet;
 use std::{error, fmt, str};
 
 use super::ENTAILMENT_MAX_TERMINALS;
@@ -88,6 +89,8 @@ pub enum PolicyError {
     /// lifting error: Cannot lift policies that have
     /// a combination of height and timelocks.
     HeightTimeLockCombination,
+    /// Duplicate Public Keys
+    DuplicatePubKeys,
 }
 
 impl error::Error for PolicyError {
@@ -127,6 +130,7 @@ impl fmt::Display for PolicyError {
             PolicyError::HeightTimeLockCombination => {
                 f.write_str("Cannot lift policies that have a heightlock and timelock combination")
             }
+            PolicyError::DuplicatePubKeys => f.write_str("Policy contains duplicate keys"),
         }
     }
 }
@@ -182,6 +186,43 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
         }
     }
 
+    /// Get all keys in the policy
+    pub fn keys(&self) -> Vec<&Pk> {
+        match *self {
+            Policy::Key(ref pk) => vec![pk],
+            Policy::Threshold(_k, ref subs) => subs
+                .iter()
+                .map(|sub| sub.keys())
+                .flatten()
+                .collect::<Vec<_>>(),
+            Policy::And(ref subs) => subs
+                .iter()
+                .map(|sub| sub.keys())
+                .flatten()
+                .collect::<Vec<_>>(),
+            Policy::Or(ref subs) => subs
+                .iter()
+                .map(|(ref _k, ref sub)| sub.keys())
+                .flatten()
+                .collect::<Vec<_>>(),
+            // map all hashes and time
+            _ => vec![],
+        }
+    }
+
+    /// Check whether the policy contains duplicate public keys
+    pub fn check_duplicate_keys(&self) -> Result<(), PolicyError> {
+        let pks = self.keys();
+        let pks_len = pks.len();
+        let unique_pks_len = pks.into_iter().collect::<HashSet<_>>().len();
+
+        if pks_len > unique_pks_len {
+            Err(PolicyError::DuplicatePubKeys)
+        } else {
+            Ok(())
+        }
+    }
+
     /// Checks whether the given concrete policy contains a combination of
     /// timelocks and heightlocks.
     /// Returns an error if there is atleast one satisfaction that contains
@@ -244,6 +285,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
     /// combination of timelocks and heightlocks
     pub fn is_valid(&self) -> Result<(), PolicyError> {
         self.check_timelocks()?;
+        self.check_duplicate_keys()?;
         match *self {
             Policy::And(ref subs) => {
                 if subs.len() != 2 {