Update lift trait

examples/htlc.rs

@@ -39,8 +39,9 @@ fn main() {
     );
 
     assert_eq!(
-        format!("{}", htlc_descriptor.lift()),
-        "or(and(pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),older(4444)),sha256(1111111111111111111111111111111111111111111111111111111111111111))");
+        format!("{}", htlc_descriptor.lift().unwrap()),
+        "or(and(pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),older(4444)),sha256(1111111111111111111111111111111111111111111111111111111111111111))"
+    );
 
     assert_eq!(
         format!("{:x}", htlc_descriptor.script_pubkey()),

fuzz/fuzz_targets/compile_descriptor.rs

@@ -15,7 +15,7 @@ fn do_test(data: &[u8]) {
         // Compile
         if let Ok(desc) = pol.compile::<Segwitv0>() {
             // Lift
-            assert_eq!(desc.clone().lift(), pol.clone().lift());
+            assert_eq!(desc.clone().lift().unwrap(), pol.clone().lift().unwrap());
             // Try to roundtrip the output of the compiler
             let output = desc.to_string();
             if let Ok(desc) = DummyScript::from_str(&output) {

src/miniscript/mod.rs

@@ -659,7 +659,7 @@ mod tests {
             keys[4].to_string(),
         );
 
-        let mut abs = miniscript.lift();
+        let mut abs = miniscript.lift().unwrap();
         assert_eq!(abs.n_keys(), 5);
         assert_eq!(abs.minimum_n_keys(), 2);
         abs = abs.at_age(10000);

src/policy/compiler.rs

@@ -1205,10 +1205,28 @@ mod tests {
         let policy = DummyPolicy::from_str(s).expect("parse");
         let miniscript: Miniscript<DummyKey, Segwitv0> = policy.compile()?;
 
-        assert_eq!(policy.lift().sorted(), miniscript.lift().sorted());
+        assert_eq!(
+            policy.lift().unwrap().sorted(),
+            miniscript.lift().unwrap().sorted()
+        );
         Ok(())
     }
 
+    #[test]
+    fn compile_timelocks() {
+        // artificially create a policy that is problematic and try to compile
+        let pol: DummyPolicy = Concrete::And(vec![
+            Concrete::Key(DummyKey),
+            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())))")
+                .unwrap();
+        assert!(pol.compile::<Segwitv0>().is_ok());
+    }
     #[test]
     fn compile_basic() {
         assert!(policy_compile_lift_check("pk()").is_ok());
@@ -1248,7 +1266,10 @@ mod tests {
             best_t(&mut HashMap::new(), &policy, 1.0, None).unwrap();
 
         assert_eq!(compilation.cost_1d(1.0, None), 88.0 + 74.109375);
-        assert_eq!(policy.lift().sorted(), compilation.ms.lift().sorted());
+        assert_eq!(
+            policy.lift().unwrap().sorted(),
+            compilation.ms.lift().unwrap().sorted()
+        );
 
         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()))"
@@ -1257,7 +1278,10 @@ mod tests {
             best_t(&mut HashMap::new(), &policy, 1.0, None).unwrap();
 
         assert_eq!(compilation.cost_1d(1.0, None), 437.0 + 299.4003295898438);
-        assert_eq!(policy.lift().sorted(), compilation.ms.lift().sorted());
+        assert_eq!(
+            policy.lift().unwrap().sorted(),
+            compilation.ms.lift().unwrap().sorted()
+        );
     }
 
     #[test]
@@ -1327,7 +1351,7 @@ mod tests {
 
         assert_eq!(ms, ms_comp_res);
 
-        let mut abs = policy.lift();
+        let mut abs = policy.lift().unwrap();
         assert_eq!(abs.n_keys(), 8);
         assert_eq!(abs.minimum_n_keys(), 2);
         abs = abs.at_age(10000);

src/policy/concrete.rs

@@ -22,6 +22,7 @@ use std::{error, fmt, str};
 use super::ENTAILMENT_MAX_TERMINALS;
 use errstr;
 use expression::{self, FromTree};
+use miniscript::types::extra_props::{TimeLockInfo, HEIGHT_TIME_THRESHOLD};
 #[cfg(feature = "compiler")]
 use miniscript::ScriptContext;
 #[cfg(feature = "compiler")]
@@ -31,7 +32,6 @@ use policy::compiler::CompilerError;
 #[cfg(feature = "compiler")]
 use Miniscript;
 use {Error, MiniscriptKey};
-
 /// Concrete policy which corresponds directly to a Miniscript structure,
 /// and whose disjunctions are annotated with satisfaction probabilities
 /// to assist the compiler
@@ -79,6 +79,9 @@ pub enum PolicyError {
     InsufficientArgsforOr,
     /// Entailment max terminals exceeded
     EntailmentMaxTerminals,
+    /// lifting error: Cannot lift policies that have
+    /// a combination of height and timelocks.
+    HeightTimeLockCombination,
 }
 
 impl error::Error for PolicyError {
@@ -115,6 +118,9 @@ impl fmt::Display for PolicyError {
                 "Policy entailment only supports {} terminals",
                 ENTAILMENT_MAX_TERMINALS
             ),
+            PolicyError::HeightTimeLockCombination => {
+                f.write_str("Cannot lift policies that have a heightlock and timelock combination")
+            }
         }
     }
 }
@@ -168,9 +174,72 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
         }
     }
 
+    /// Checks whether the given concrete policy contains a combination of
+    /// timelocks and heightlocks.
+    /// Returns an error if there is atleast one satisfaction that contains
+    /// a combination of hieghtlock and timelock.
+    pub fn check_timelocks(&self) -> Result<(), PolicyError> {
+        let timelocks = self.check_timelocks_helper();
+        if timelocks.contains_combination {
+            Err(PolicyError::HeightTimeLockCombination)
+        } else {
+            Ok(())
+        }
+    }
+
+    // Checks whether the given concrete policy contains a combination of
+    // timelocks and heightlocks
+    // Returns a tuple of five elements:
+    // timelocks[0]: satisfaction contains csv heightlock
+    // timelocks[1]: satisfaction contains csv timelock
+    // timelocks[2]: satisfaction contains cltv heightlock
+    // timelocks[3]: satisfaction contains cltv timelock
+    // timelocks[4]: satisfaction contains an invalid combination
+    fn check_timelocks_helper(&self) -> TimeLockInfo {
+        // timelocks[csv_h, csv_t, cltv_h, cltv_t, combination]
+        match *self {
+            Policy::Key(_)
+            | Policy::Sha256(_)
+            | Policy::Hash256(_)
+            | Policy::Ripemd160(_)
+            | Policy::Hash160(_) => TimeLockInfo::default(),
+            Policy::After(t) => TimeLockInfo {
+                csv_with_height: false,
+                csv_with_time: false,
+                cltv_with_height: t < HEIGHT_TIME_THRESHOLD,
+                cltv_with_time: t >= HEIGHT_TIME_THRESHOLD,
+                contains_combination: false,
+            },
+            Policy::Older(t) => TimeLockInfo {
+                csv_with_height: t < HEIGHT_TIME_THRESHOLD,
+                csv_with_time: t >= HEIGHT_TIME_THRESHOLD,
+                cltv_with_height: false,
+                cltv_with_time: false,
+                contains_combination: false,
+            },
+            Policy::Threshold(k, ref subs) => {
+                let iter = subs.iter().map(|sub| sub.check_timelocks_helper());
+                TimeLockInfo::combine_thresh_timelocks(k, iter)
+            }
+            Policy::And(ref subs) => {
+                let iter = subs.iter().map(|sub| sub.check_timelocks_helper());
+                TimeLockInfo::combine_thresh_timelocks(subs.len(), iter)
+            }
+            Policy::Or(ref subs) => {
+                let iter = subs
+                    .iter()
+                    .map(|&(ref _p, ref sub)| sub.check_timelocks_helper());
+                TimeLockInfo::combine_thresh_timelocks(1, iter)
+            }
+        }
+    }
+
     /// This returns whether the given policy is valid or not. It maybe possible that the policy
     /// contains Non-two argument `and`, `or` or a `0` arg thresh.
+    /// Validity condition also checks whether there is a possible satisfaction
+    /// combination of timelocks and heightlocks
     pub fn is_valid(&self) -> Result<(), PolicyError> {
+        self.check_timelocks()?;
         match *self {
             Policy::And(ref subs) => {
                 if subs.len() != 2 {
@@ -362,7 +431,9 @@ where
         }
 
         let tree = expression::Tree::from_str(s)?;
-        FromTree::from_tree(&tree)
+        let policy: Policy<Pk> = FromTree::from_tree(&tree)?;
+        policy.check_timelocks()?;
+        Ok(policy)
     }
 }
 

src/policy/mod.rs

@@ -35,6 +35,7 @@ pub use self::concrete::Policy as Concrete;
 /// Semantic policies are "abstract" policies elsewhere; but we
 /// avoid this word because it is a reserved keyword in Rust
 pub use self::semantic::Policy as Semantic;
+use Error;
 use MiniscriptKey;
 
 /// Policy entailment algorithm maximum number of terminals allowed
@@ -46,18 +47,25 @@ const ENTAILMENT_MAX_TERMINALS: usize = 20;
 /// `Lift(Concrete) == Concrete -> Miniscript -> Script -> Miniscript -> Semantic`
 pub trait Liftable<Pk: MiniscriptKey> {
     /// Convert the object into an abstract policy
-    fn lift(&self) -> Semantic<Pk>;
+    fn lift(&self) -> Result<Semantic<Pk>, Error>;
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Miniscript<Pk, Ctx> {
-    fn lift(&self) -> Semantic<Pk> {
+    fn lift(&self) -> Result<Semantic<Pk>, Error> {
+        // check whether the root miniscript can have a spending path that is
+        // a combination of heightlock and timelock
+        if self.ext.timelock_info.contains_unspendable_path() {
+            return Err(Error::PolicyError(
+                concrete::PolicyError::HeightTimeLockCombination,
+            ));
+        }
         self.as_inner().lift()
     }
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Terminal<Pk, Ctx> {
-    fn lift(&self) -> Semantic<Pk> {
-        match *self {
+    fn lift(&self) -> Result<Semantic<Pk>, Error> {
+        let ret = match *self {
             Terminal::PkK(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
             Terminal::PkH(ref pkh) => Semantic::KeyHash(pkh.clone()),
             Terminal::After(t) => Semantic::After(t),
@@ -74,25 +82,27 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Terminal<Pk, Ctx> {
             | Terminal::DupIf(ref sub)
             | Terminal::Verify(ref sub)
             | Terminal::NonZero(ref sub)
-            | Terminal::ZeroNotEqual(ref sub) => sub.node.lift(),
+            | Terminal::ZeroNotEqual(ref sub) => sub.node.lift()?,
             Terminal::AndV(ref left, ref right) | Terminal::AndB(ref left, ref right) => {
-                Semantic::Threshold(2, vec![left.node.lift(), right.node.lift()])
+                Semantic::Threshold(2, vec![left.node.lift()?, right.node.lift()?])
             }
             Terminal::AndOr(ref a, ref b, ref c) => Semantic::Threshold(
                 1,
                 vec![
-                    Semantic::Threshold(2, vec![a.node.lift(), c.node.lift()]),
-                    b.node.lift(),
+                    Semantic::Threshold(2, vec![a.node.lift()?, c.node.lift()?]),
+                    b.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::Threshold(1, vec![left.node.lift(), right.node.lift()])
+                Semantic::Threshold(1, vec![left.node.lift()?, right.node.lift()?])
             }
             Terminal::Thresh(k, ref subs) => {
-                Semantic::Threshold(k, subs.into_iter().map(|s| s.node.lift()).collect())
+                let semantic_subs: Result<_, Error> =
+                    subs.into_iter().map(|s| s.node.lift()).collect();
+                Semantic::Threshold(k, semantic_subs?)
             }
             Terminal::Multi(k, ref keys) => Semantic::Threshold(
                 k,
@@ -101,32 +111,36 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Terminal<Pk, Ctx> {
                     .collect(),
             ),
         }
-        .normalized()
+        .normalized();
+        Ok(ret)
     }
 }
 
 impl<Pk: MiniscriptKey> Liftable<Pk> for Descriptor<Pk> {
-    fn lift(&self) -> Semantic<Pk> {
-        match *self {
-            Descriptor::Bare(ref d) | Descriptor::Sh(ref d) => d.node.lift(),
-            Descriptor::Wsh(ref d) | Descriptor::ShWsh(ref d) => d.node.lift(),
+    fn lift(&self) -> Result<Semantic<Pk>, Error> {
+        Ok(match *self {
+            Descriptor::Bare(ref d) | Descriptor::Sh(ref d) => d.node.lift()?,
+            Descriptor::Wsh(ref d) | Descriptor::ShWsh(ref d) => d.node.lift()?,
             Descriptor::Pk(ref p)
             | Descriptor::Pkh(ref p)
             | Descriptor::Wpkh(ref p)
             | Descriptor::ShWpkh(ref p) => Semantic::KeyHash(p.to_pubkeyhash()),
-        }
+        })
     }
 }
 
 impl<Pk: MiniscriptKey> Liftable<Pk> for Semantic<Pk> {
-    fn lift(&self) -> Semantic<Pk> {
-        self.clone()
+    fn lift(&self) -> Result<Semantic<Pk>, Error> {
+        Ok(self.clone())
     }
 }
 
 impl<Pk: MiniscriptKey> Liftable<Pk> for Concrete<Pk> {
-    fn lift(&self) -> Semantic<Pk> {
-        match *self {
+    fn lift(&self) -> Result<Semantic<Pk>, Error> {
+        // do not lift if there is a possible satisfaction
+        // involving combination of timelocks and heightlocks
+        self.check_timelocks()?;
+        let ret = match *self {
             Concrete::Key(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
             Concrete::After(t) => Semantic::After(t),
             Concrete::Older(t) => Semantic::Older(t),
@@ -135,16 +149,21 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Concrete<Pk> {
             Concrete::Ripemd160(h) => Semantic::Ripemd160(h),
             Concrete::Hash160(h) => Semantic::Hash160(h),
             Concrete::And(ref subs) => {
-                Semantic::Threshold(subs.len(), subs.iter().map(Liftable::lift).collect())
+                let semantic_subs: Result<_, Error> = subs.iter().map(Liftable::lift).collect();
+                Semantic::Threshold(2, semantic_subs?)
             }
             Concrete::Or(ref subs) => {
-                Semantic::Threshold(1, subs.iter().map(|&(_, ref sub)| sub.lift()).collect())
+                let semantic_subs: Result<_, Error> =
+                    subs.iter().map(|&(ref _p, ref sub)| sub.lift()).collect();
+                Semantic::Threshold(1, semantic_subs?)
             }
             Concrete::Threshold(k, ref subs) => {
-                Semantic::Threshold(k, subs.iter().map(Liftable::lift).collect())
+                let semantic_subs: Result<_, Error> = subs.iter().map(Liftable::lift).collect();
+                Semantic::Threshold(k, semantic_subs?)
             }
         }
-        .normalized()
+        .normalized();
+        Ok(ret)
     }
 }
 
@@ -169,6 +188,21 @@ mod tests {
         assert_eq!(s.to_lowercase(), output.to_lowercase());
     }
 
+    #[test]
+    fn test_timelock_validity() {
+        // only height
+        assert!(ConcretePol::from_str("after(100)").is_ok());
+        // only time
+        assert!(ConcretePol::from_str("after(1000000000)").is_ok());
+        // disjunction
+        assert!(ConcretePol::from_str("or(after(1000000000),after(100))").is_ok());
+        // conjunction
+        assert!(ConcretePol::from_str("and(after(1000000000),after(100))").is_err());
+        // thresh with k = 1
+        assert!(ConcretePol::from_str("thresh(1,pk(),after(1000000000),after(100))").is_ok());
+        // thresh with k = 2
+        assert!(ConcretePol::from_str("thresh(2,after(1000000000),after(100),pk())").is_err());
+    }
     #[test]
     fn policy_rtt_tests() {
         concrete_policy_rtt("pk()");