From eaa48d652ef05949e4a00e0e4008f8d99bd465f9 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Thu, 6 Aug 2020 16:21:47 -0500 Subject: [PATCH] Update lift trait Added height and timelocks types for detecting if there is atleast one spending path that contains a combination of those. Updated the lifting trait so that it can fail --- examples/htlc.rs | 2 +- fuzz/fuzz_targets/compile_descriptor.rs | 2 +- src/miniscript/mod.rs | 2 +- src/policy/compiler.rs | 32 ++++++- src/policy/concrete.rs | 115 +++++++++++++++++++++++- src/policy/mod.rs | 84 ++++++++++++----- 6 files changed, 204 insertions(+), 33 deletions(-) diff --git a/examples/htlc.rs b/examples/htlc.rs index 7b69e2692..422af1c7c 100644 --- a/examples/htlc.rs +++ b/examples/htlc.rs @@ -39,7 +39,7 @@ fn main() { ); assert_eq!( - format!("{}", htlc_descriptor.lift()), + format!("{}", htlc_descriptor.lift().unwrap()), "or(and(pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),and(pkh(4377a5acd66dc5cb67148a24818d1e51fa183bd2),older(4444))),sha256(1111111111111111111111111111111111111111111111111111111111111111))" ); diff --git a/fuzz/fuzz_targets/compile_descriptor.rs b/fuzz/fuzz_targets/compile_descriptor.rs index 8dbef0bc8..94e856526 100644 --- a/fuzz/fuzz_targets/compile_descriptor.rs +++ b/fuzz/fuzz_targets/compile_descriptor.rs @@ -15,7 +15,7 @@ fn do_test(data: &[u8]) { // Compile if let Ok(desc) = pol.compile::() { // 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) { diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index e4a027e20..6a9f007e2 100644 --- a/src/miniscript/mod.rs +++ b/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); diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 2a3320050..8d1a020a2 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -1191,10 +1191,28 @@ mod tests { let policy = DummyPolicy::from_str(s).expect("parse"); let miniscript: Miniscript = 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::().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::().is_ok()); + } #[test] fn compile_basic() { assert!(policy_compile_lift_check("pk()").is_ok()); @@ -1234,7 +1252,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()))" @@ -1243,7 +1264,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] @@ -1313,7 +1337,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); diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs index 476f68036..d45be58a5 100644 --- a/src/policy/concrete.rs +++ b/src/policy/concrete.rs @@ -21,6 +21,7 @@ use std::{error, fmt, str}; use errstr; use expression::{self, FromTree}; +use miniscript::types::extra_props::HEIGHT_TIME_THRESHOLD; #[cfg(feature = "compiler")] use miniscript::ScriptContext; #[cfg(feature = "compiler")] @@ -30,7 +31,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 @@ -72,6 +72,9 @@ pub enum PolicyError { ZeroTime, /// `after` fragment can only have ` n < 2^31` TimeTooFar, + /// lifting error: Cannot lift policies that have + /// a combination of height and timelocks. + HeightTimeLockCombination, } impl error::Error for PolicyError { @@ -97,6 +100,9 @@ impl fmt::Display for PolicyError { f.write_str("Relative/Absolute time must be less than 2^31; n < 2^31") } PolicyError::ZeroTime => f.write_str("Time must be greater than 0; n > 0"), + PolicyError::HeightTimeLockCombination => { + f.write_str("Cannot lift polcies that have a heightlock and timelock combination") + } } } } @@ -150,9 +156,112 @@ impl Policy { } } + /// 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[4] { + 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) -> [bool; 5] { + // timelocks[csv_h, csv_t, cltv_h, cltv_t, combination] + match *self { + Policy::Key(_) + | Policy::Sha256(_) + | Policy::Hash256(_) + | Policy::Ripemd160(_) + | Policy::Hash160(_) => [false; 5], + Policy::After(t) => [ + false, + false, + t < HEIGHT_TIME_THRESHOLD, + t >= HEIGHT_TIME_THRESHOLD, + false, + ], + Policy::Older(t) => [ + t < HEIGHT_TIME_THRESHOLD, + t >= HEIGHT_TIME_THRESHOLD, + false, + false, + false, + ], + Policy::Threshold(k, ref subs) => { + let mut timelocks = [false; 5]; + // k >= 2 implies it is possible to have timelocks and heightlocks + // merged together + // timelocks[0] and timelocks[1] can be directly + // combined together instead of iterating through all the pairs: + // consider the two cases: + // 1) Only one subfragment contains timelock and hieghtlock: In this case, + // timelocks[4] would already by `true`. and other combinations would + // not add any effect. + // 2) hieghtlocks and timelocks are distributed across two branches: + // and we check if any of the existing branches has correponding lock + // that cannot be combined + // If k == 1, then this is no combination possible. + for sub in subs { + let sub_timelocks = sub.check_timelocks_helper(); + if k >= 2 { + timelocks[4] |= (timelocks[0] && sub_timelocks[1]) + || (timelocks[1] && sub_timelocks[0]) + || (timelocks[3] && sub_timelocks[2]) + || (timelocks[2] && sub_timelocks[3]); + } + for i in 0..5 { + timelocks[i] |= sub_timelocks[i]; + } + } + timelocks + } + Policy::And(ref subs) => { + let mut timelocks = [false; 5]; + for sub in subs { + let sub_timelocks = sub.check_timelocks_helper(); + // Check if by adding the new `sub` we create a + // new combination that could be problematic + timelocks[4] |= (timelocks[0] && sub_timelocks[1]) + || (timelocks[1] && sub_timelocks[0]) + || (timelocks[3] && sub_timelocks[2]) + || (timelocks[2] && sub_timelocks[3]); + for i in 0..5 { + timelocks[i] |= sub_timelocks[i]; + } + } + timelocks + } + Policy::Or(ref subs) => { + let mut timelocks = [false; 5]; + for &(ref _prob, ref sub) in subs { + let sub_timelocks = sub.check_timelocks_helper(); + for i in 0..5 { + timelocks[i] |= sub_timelocks[i]; + } + } + timelocks + } + } + } + /// 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 { @@ -344,7 +453,9 @@ where } let tree = expression::Tree::from_str(s)?; - FromTree::from_tree(&tree) + let policy: Policy = FromTree::from_tree(&tree)?; + policy.check_timelocks()?; + Ok(policy) } } diff --git a/src/policy/mod.rs b/src/policy/mod.rs index 68b486e4d..20103f39f 100644 --- a/src/policy/mod.rs +++ b/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; /// Trait describing script representations which can be lifted into @@ -44,18 +45,25 @@ use MiniscriptKey; /// `Lift(Concrete) == Concrete -> Miniscript -> Script -> Miniscript -> Semantic` pub trait Liftable { /// Convert the object into an abstract policy - fn lift(&self) -> Semantic; + fn lift(&self) -> Result, Error>; } impl Liftable for Miniscript { - fn lift(&self) -> Semantic { + fn lift(&self) -> Result, Error> { + // check whether the root miniscript can have a spending path that is + // a combination of heightlock and timelock + if self.ext.contains_timelock[4] { + return Err(Error::PolicyError( + concrete::PolicyError::HeightTimeLockCombination, + )); + } self.as_inner().lift() } } impl Liftable for Terminal { - fn lift(&self) -> Semantic { - match *self { + fn lift(&self) -> Result, 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), @@ -72,22 +80,24 @@ impl Liftable for Terminal { | 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::And(vec![left.node.lift(), right.node.lift()]) + Semantic::And(vec![left.node.lift()?, right.node.lift()?]) } Terminal::AndOr(ref a, ref b, ref c) => Semantic::Or(vec![ - Semantic::And(vec![a.node.lift(), c.node.lift()]), - b.node.lift(), + Semantic::And(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::Or(vec![left.node.lift(), right.node.lift()]) + Semantic::Or(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, @@ -96,32 +106,36 @@ impl Liftable for Terminal { .collect(), ), } - .normalized() + .normalized(); + Ok(ret) } } impl Liftable for Descriptor { - fn lift(&self) -> Semantic { - 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, 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 Liftable for Semantic { - fn lift(&self) -> Semantic { - self.clone() + fn lift(&self) -> Result, Error> { + Ok(self.clone()) } } impl Liftable for Concrete { - fn lift(&self) -> Semantic { - match *self { + fn lift(&self) -> Result, 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), @@ -129,15 +143,22 @@ impl Liftable for Concrete { Concrete::Hash256(h) => Semantic::Hash256(h), Concrete::Ripemd160(h) => Semantic::Ripemd160(h), Concrete::Hash160(h) => Semantic::Hash160(h), - Concrete::And(ref subs) => Semantic::And(subs.iter().map(Liftable::lift).collect()), + Concrete::And(ref subs) => { + let semantic_subs: Result<_, Error> = subs.iter().map(Liftable::lift).collect(); + Semantic::And(semantic_subs?) + } Concrete::Or(ref subs) => { - Semantic::Or(subs.iter().map(|&(_, ref sub)| sub.lift()).collect()) + let semantic_subs: Result<_, Error> = + subs.iter().map(|&(ref _p, ref sub)| sub.lift()).collect(); + Semantic::Or(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) } } @@ -162,6 +183,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()");