Merge pull request #199 from sanket1729/lift_checks

lift failure bug fixes

Author: apoelstra

src/descriptor/mod.rs

@@ -268,15 +268,7 @@ impl fmt::Display for DescriptorKeyParseError {
     }
 }
 
-impl error::Error for DescriptorKeyParseError {
-    fn description(&self) -> &str {
-        ""
-    }
-
-    fn cause(&self) -> Option<&error::Error> {
-        None
-    }
-}
+impl error::Error for DescriptorKeyParseError {}
 
 impl fmt::Display for DescriptorPublicKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

src/interpreter/error.rs

@@ -101,9 +101,6 @@ impl From<::Error> for Error {
 }
 
 impl error::Error for Error {
-    fn description(&self) -> &str {
-        ""
-    }
     fn cause(&self) -> Option<&error::Error> {
         match *self {
             Error::Secp(ref err) => Some(err),

src/lib.rs

@@ -354,15 +354,17 @@ pub enum Error {
     CouldNotSatisfy,
     /// Typechecking failed
     TypeCheck(String),
-    ///General error in creating descriptor
+    /// General error in creating descriptor
     BadDescriptor,
-    ///Forward-secp related errors
+    /// Forward-secp related errors
     Secp(bitcoin::secp256k1::Error),
     #[cfg(feature = "compiler")]
-    ///Compiler related errors
+    /// Compiler related errors
     CompilerError(policy::compiler::CompilerError),
-    ///Errors related to policy
+    /// Errors related to policy
     PolicyError(policy::concrete::PolicyError),
+    /// Errors related to lifting
+    LiftError(policy::LiftError),
     /// Forward script context related errors
     ContextError(miniscript::context::ScriptContextError),
     /// Recursion depth exceeded when parsing policy/miniscript from string
@@ -387,6 +389,13 @@ where
     }
 }
 
+#[doc(hidden)]
+impl From<policy::LiftError> for Error {
+    fn from(e: policy::LiftError) -> Error {
+        Error::LiftError(e)
+    }
+}
+
 #[doc(hidden)]
 impl From<miniscript::context::ScriptContextError> for Error {
     fn from(e: miniscript::context::ScriptContextError) -> Error {
@@ -419,10 +428,6 @@ impl error::Error for Error {
             _ => None,
         }
     }
-
-    fn description(&self) -> &str {
-        ""
-    }
 }
 
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
@@ -472,6 +477,7 @@ impl fmt::Display for Error {
             #[cfg(feature = "compiler")]
             Error::CompilerError(ref e) => fmt::Display::fmt(e, f),
             Error::PolicyError(ref e) => fmt::Display::fmt(e, f),
+            Error::LiftError(ref e) => fmt::Display::fmt(e, f),
             Error::MaxRecursiveDepthExceeded => write!(
                 f,
                 "Recursive depth over {} not permitted",

src/miniscript/analyzable.rs

@@ -65,14 +65,7 @@ impl fmt::Display for AnalysisError {
     }
 }
 
-impl error::Error for AnalysisError {
-    fn description(&self) -> &str {
-        ""
-    }
-    fn cause(&self) -> Option<&error::Error> {
-        None
-    }
-}
+impl error::Error for AnalysisError {}
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     /// Whether all spend paths of miniscript require a signature

src/policy/compiler.rs

@@ -62,15 +62,7 @@ pub enum CompilerError {
     PolicyError(policy::concrete::PolicyError),
 }
 
-impl error::Error for CompilerError {
-    fn cause(&self) -> Option<&error::Error> {
-        None
-    }
-
-    fn description(&self) -> &str {
-        ""
-    }
-}
+impl error::Error for CompilerError {}
 
 impl fmt::Display for CompilerError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

src/policy/concrete.rs

@@ -92,14 +92,7 @@ pub enum PolicyError {
     DuplicatePubKeys,
 }
 
-impl error::Error for PolicyError {
-    fn description(&self) -> &str {
-        ""
-    }
-    fn cause(&self) -> Option<&error::Error> {
-        None
-    }
-}
+impl error::Error for PolicyError {}
 
 impl fmt::Display for PolicyError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

src/policy/mod.rs

@@ -21,6 +21,7 @@
 //! The format represents EC public keys abstractly to allow wallets to replace
 //! these with BIP32 paths, pay-to-contract instructions, etc.
 //!
+use {error, fmt};
 
 #[cfg(feature = "compiler")]
 pub mod compiler;
@@ -43,22 +44,78 @@ const ENTAILMENT_MAX_TERMINALS: usize = 20;
 /// Trait describing script representations which can be lifted into
 /// an abstract policy, by discarding information.
 /// After Lifting all policies are converted into `KeyHash(Pk::HasH)` to
-/// maintain the following invariant:
+/// maintain the following invariant(modulo resource limits):
 /// `Lift(Concrete) == Concrete -> Miniscript -> Script -> Miniscript -> Semantic`
+/// Lifting from [miniscript.Miniscript], [descriptor.Descriptor] can fail
+/// if the miniscript contains a timelock combination or if it contains a
+/// branch that exceeds resource limits.
+/// Lifting from Concrete policies can fail if it contains a timelock
+/// combination. It is possible that concrete policy has some branches that
+/// exceed resource limits for any compilation, but cannot detect such
+/// policies while lifting. Note that our compiler would not succeed for any
+/// such policies.
 pub trait Liftable<Pk: MiniscriptKey> {
     /// Convert the object into an abstract policy
     fn lift(&self) -> Result<Semantic<Pk>, Error>;
 }
 
+/// Detailed Error type for Policies
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum LiftError {
+    /// Cannot lift policies that have
+    /// a combination of height and timelocks.
+    HeightTimeLockCombination,
+    /// Duplicate Public Keys
+    BranchExceedResourceLimits,
+}
+
+impl error::Error for LiftError {
+    fn description(&self) -> &str {
+        ""
+    }
+    fn cause(&self) -> Option<&error::Error> {
+        None
+    }
+}
+
+impl fmt::Display for LiftError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            LiftError::HeightTimeLockCombination => {
+                f.write_str("Cannot lift policies that have a heightlock and timelock combination")
+            }
+            LiftError::BranchExceedResourceLimits => f.write_str(
+                "Cannot lift policies containing one branch that exceeds resource limits",
+            ),
+        }
+    }
+}
+
+impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
+    /// Lifting corresponds conversion of miniscript into Policy
+    /// [policy.semantic.Policy] for human readable or machine analysis.
+    /// However, naively lifting miniscripts can result in incorrect
+    /// interpretations that don't correspond underlying semantics when
+    /// we try to spend them on bitcoin network.
+    /// This can occur if the miniscript contains a
+    /// 1. Timelock combination
+    /// 2. Contains a spend that exceeds resource limits
+    pub fn lift_check(&self) -> Result<(), LiftError> {
+        if !self.within_resource_limits() {
+            Err(LiftError::BranchExceedResourceLimits)
+        } else if self.has_mixed_timelocks() {
+            Err(LiftError::HeightTimeLockCombination)
+        } else {
+            Ok(())
+        }
+    }
+}
+
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Miniscript<Pk, Ctx> {
     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.lift_check()?;
         self.as_inner().lift()
     }
 }

src/psbt/mod.rs

@@ -183,15 +183,7 @@ impl From<bitcoin::util::key::Error> for InputError {
     }
 }
 
-impl error::Error for Error {
-    fn cause(&self) -> Option<&error::Error> {
-        None
-    }
-
-    fn description(&self) -> &str {
-        ""
-    }
-}
+impl error::Error for Error {}
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {