AbsLockTime: move to module, consolidate error checking

We have the type `AbsLockTime` whose purpose is to provide an ordering
for locktimes. This type is converted from strings and script values,
but we don't check validity during construction. Instead we have
multiple places where validity is manually checked. Fix this.

Note that this is an API-breaking change for people who are manually
constructing policies and Miniscripts since it removes a few ways to
unconditionally convert u32s into locktimes.

Author: apoelstra

src/lib.rs

@@ -87,6 +87,7 @@
 #![deny(missing_docs)]
 // Clippy lints that we have disabled
 #![allow(clippy::iter_kv_map)] // https://github.com/rust-lang/rust-clippy/issues/11752
+#![allow(clippy::manual_range_contains)] // I hate this lint -asp
 
 #[cfg(target_pointer_width = "16")]
 compile_error!(
@@ -125,19 +126,19 @@ pub mod iter;
 pub mod miniscript;
 pub mod plan;
 pub mod policy;
+mod primitives;
 pub mod psbt;
 
 #[cfg(test)]
 mod test_utils;
 mod util;
 
-use core::{cmp, fmt, hash, str};
+use core::{fmt, hash, str};
 #[cfg(feature = "std")]
 use std::error;
 
 use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
 use bitcoin::hex::DisplayHex;
-use bitcoin::locktime::absolute;
 use bitcoin::{script, Opcode};
 
 pub use crate::blanket_traits::FromStrKey;
@@ -149,6 +150,7 @@ pub use crate::miniscript::decode::Terminal;
 pub use crate::miniscript::satisfy::{Preimage32, Satisfier};
 pub use crate::miniscript::{hash256, Miniscript};
 use crate::prelude::*;
+pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError};
 
 /// Public key trait which can be converted to Hash type
 pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash {
@@ -501,6 +503,8 @@ pub enum Error {
     /// At least two BIP389 key expressions in the descriptor contain tuples of
     /// derivation indexes of different lengths.
     MultipathDescLenMismatch,
+    /// Invalid absolute locktime
+    AbsoluteLockTime(AbsLockTimeError),
 }
 
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
@@ -576,6 +580,7 @@ impl fmt::Display for Error {
             Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"),
             Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"),
             Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"),
+            Error::AbsoluteLockTime(ref e) => e.fmt(f),
         }
     }
 }
@@ -628,6 +633,7 @@ impl error::Error for Error {
             ContextError(e) => Some(e),
             AnalysisError(e) => Some(e),
             PubKeyCtxError(e, _) => Some(e),
+            AbsoluteLockTime(e) => Some(e),
         }
     }
 }
@@ -711,51 +717,6 @@ fn hex_script(s: &str) -> bitcoin::ScriptBuf {
     bitcoin::ScriptBuf::from(v)
 }
 
-/// An absolute locktime that implements `Ord`.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-pub struct AbsLockTime(absolute::LockTime);
-
-impl AbsLockTime {
-    /// Constructs an `AbsLockTime` from an nLockTime value or the argument to OP_CHEKCLOCKTIMEVERIFY.
-    pub fn from_consensus(n: u32) -> Self { Self(absolute::LockTime::from_consensus(n)) }
-
-    /// Returns the inner `u32` value. This is the value used when creating this `LockTime`
-    /// i.e., `n OP_CHECKLOCKTIMEVERIFY` or nLockTime.
-    ///
-    /// This calls through to `absolute::LockTime::to_consensus_u32()` and the same usage warnings
-    /// apply.
-    pub fn to_consensus_u32(self) -> u32 { self.0.to_consensus_u32() }
-
-    /// Returns the inner `u32` value.
-    ///
-    /// Equivalent to `AbsLockTime::to_consensus_u32()`.
-    pub fn to_u32(self) -> u32 { self.to_consensus_u32() }
-}
-
-impl From<absolute::LockTime> for AbsLockTime {
-    fn from(lock_time: absolute::LockTime) -> Self { Self(lock_time) }
-}
-
-impl From<AbsLockTime> for absolute::LockTime {
-    fn from(lock_time: AbsLockTime) -> absolute::LockTime { lock_time.0 }
-}
-
-impl cmp::PartialOrd for AbsLockTime {
-    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { Some(self.cmp(other)) }
-}
-
-impl cmp::Ord for AbsLockTime {
-    fn cmp(&self, other: &Self) -> cmp::Ordering {
-        let this = self.0.to_consensus_u32();
-        let that = other.0.to_consensus_u32();
-        this.cmp(&that)
-    }
-}
-
-impl fmt::Display for AbsLockTime {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
-}
-
 #[cfg(test)]
 mod tests {
     use core::str::FromStr;

src/miniscript/astelem.rs

@@ -256,9 +256,9 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
                 expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkH))
             }
             ("after", 1) => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x).map(|x| {
-                    Terminal::After(AbsLockTime::from(absolute::LockTime::from_consensus(x)))
-                })
+                expression::parse_num(x)
+                    .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
+                    .map(Terminal::After)
             }),
             ("older", 1) => expression::terminal(&top.args[0], |x| {
                 expression::parse_num(x).map(|x| Terminal::Older(Sequence::from_consensus(x)))

src/miniscript/decode.rs

@@ -359,7 +359,7 @@ pub fn parse<Ctx: ScriptContext>(
                     Tk::CheckSequenceVerify, Tk::Num(n)
                         => term.reduce0(Terminal::Older(Sequence::from_consensus(n)))?,
                     Tk::CheckLockTimeVerify, Tk::Num(n)
-                        => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n)))?,
+                        => term.reduce0(Terminal::After(AbsLockTime::from_consensus(n).map_err(Error::AbsoluteLockTime)?))?,
                     // hashlocks
                     Tk::Equal => match_token!(
                         tokens,

src/miniscript/mod.rs

@@ -1370,7 +1370,7 @@ mod tests {
             (format!("t:or_c(pk({}),v:pkh({}))", key_present, key_missing), None, None),
             (
                 format!("thresh(2,pk({}),s:pk({}),snl:after(1))", key_present, key_missing),
-                Some(AbsLockTime::from_consensus(1)),
+                Some(AbsLockTime::from_consensus(1).unwrap()),
                 None,
             ),
             (
@@ -1388,7 +1388,7 @@ mod tests {
                     "thresh(3,pk({}),s:pk({}),snl:older(10),snl:after(11))",
                     key_present, key_missing
                 ),
-                Some(AbsLockTime::from_consensus(11)),
+                Some(AbsLockTime::from_consensus(11).unwrap()),
                 Some(bitcoin::Sequence(10)),
             ),
             (

src/miniscript/types/extra_props.rs

@@ -6,12 +6,12 @@
 use core::cmp;
 use core::iter::once;
 
-use bitcoin::{absolute, Sequence};
+use bitcoin::Sequence;
 
 use super::{Error, ErrorKind, ScriptContext};
 use crate::miniscript::context::SigType;
 use crate::prelude::*;
-use crate::{script_num_size, MiniscriptKey, Terminal};
+use crate::{script_num_size, AbsLockTime, MiniscriptKey, Terminal};
 
 /// Timelock information for satisfaction of a fragment.
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Default, Hash)]
@@ -343,7 +343,7 @@ impl ExtData {
     }
 
     /// Extra properties for the `after` fragment.
-    pub fn after(t: absolute::LockTime) -> Self {
+    pub fn after(t: AbsLockTime) -> Self {
         ExtData {
             pk_cost: script_num_size(t.to_consensus_u32() as usize) + 1,
             has_free_verify: false,
@@ -923,18 +923,7 @@ impl ExtData {
                     _ => unreachable!(),
                 }
             }
-            Terminal::After(t) => {
-                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
-                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
-                // only consumes 4 bytes from the stack.
-                if t == absolute::LockTime::ZERO.into() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Self::after(t.into())
-            }
+            Terminal::After(t) => Self::after(t),
             Terminal::Older(t) => {
                 if t == Sequence::ZERO || !t.is_relative_lock_time() {
                     return Err(Error {

src/miniscript/types/mod.rs

@@ -14,7 +14,7 @@ use core::fmt;
 #[cfg(feature = "std")]
 use std::error;
 
-use bitcoin::{absolute, Sequence};
+use bitcoin::Sequence;
 
 pub use self::correctness::{Base, Correctness, Input};
 pub use self::extra_props::ExtData;
@@ -478,18 +478,7 @@ impl Type {
                     _ => unreachable!(),
                 }
             }
-            Terminal::After(t) => {
-                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
-                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
-                // only consumes 4 bytes from the stack.
-                if t == absolute::LockTime::ZERO.into() {
-                    return Err(Error {
-                        fragment_string: fragment.to_string(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::time())
-            }
+            Terminal::After(_) => Ok(Self::time()),
             Terminal::Older(t) => {
                 if t == Sequence::ZERO || !t.is_relative_lock_time() {
                     return Err(Error {

src/policy/compiler.rs

@@ -9,7 +9,7 @@ use core::{cmp, f64, fmt, hash, mem};
 #[cfg(feature = "std")]
 use std::error;
 
-use bitcoin::{absolute, Sequence};
+use bitcoin::Sequence;
 use sync::Arc;
 
 use crate::miniscript::context::SigType;
@@ -446,18 +446,7 @@ impl CompilerExtData {
                     _ => unreachable!(),
                 }
             }
-            Terminal::After(t) => {
-                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
-                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
-                // only consumes 4 bytes from the stack.
-                if t == absolute::LockTime::ZERO.into() {
-                    return Err(types::Error {
-                        fragment_string: fragment.to_string(),
-                        error: types::ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::time())
-            }
+            Terminal::After(_) => Ok(Self::time()),
             Terminal::Older(t) => {
                 if t == Sequence::ZERO || !t.is_relative_lock_time() {
                     return Err(types::Error {
@@ -1265,7 +1254,7 @@ mod tests {
     use super::*;
     use crate::miniscript::{Legacy, Segwitv0, Tap};
     use crate::policy::Liftable;
-    use crate::{script_num_size, ToPublicKey};
+    use crate::{script_num_size, AbsLockTime, ToPublicKey};
 
     type SPolicy = Concrete<String>;
     type BPolicy = Concrete<bitcoin::PublicKey>;
@@ -1311,8 +1300,8 @@ mod tests {
         let pol: SPolicy = Concrete::And(vec![
             Arc::new(Concrete::Key("A".to_string())),
             Arc::new(Concrete::And(vec![
-                Arc::new(Concrete::after(9)),
-                Arc::new(Concrete::after(1000_000_000)),
+                Arc::new(Concrete::After(AbsLockTime::from_consensus(9).unwrap())),
+                Arc::new(Concrete::After(AbsLockTime::from_consensus(1_000_000_000).unwrap())),
             ])),
         ]);
         assert!(pol.compile::<Segwitv0>().is_err());

src/policy/concrete.rs

@@ -74,12 +74,6 @@ impl<Pk> Policy<Pk>
 where
     Pk: MiniscriptKey,
 {
-    /// Construct a `Policy::After` from `n`. Helper function equivalent to
-    /// `Policy::After(absolute::LockTime::from_consensus(n))`.
-    pub fn after(n: u32) -> Policy<Pk> {
-        Policy::After(AbsLockTime::from(absolute::LockTime::from_consensus(n)))
-    }
-
     /// Construct a `Policy::Older` from `n`. Helper function equivalent to
     /// `Policy::Older(Sequence::from_consensus(n))`.
     pub fn older(n: u32) -> Policy<Pk> { Policy::Older(Sequence::from_consensus(n)) }
@@ -756,13 +750,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
 
         for policy in self.pre_order_iter() {
             match *policy {
-                After(n) => {
-                    if n == absolute::LockTime::ZERO.into() {
-                        return Err(PolicyError::ZeroTime);
-                    } else if n.to_u32() > 2u32.pow(31) {
-                        return Err(PolicyError::TimeTooFar);
-                    }
-                }
+                After(_) => {}
                 Older(n) => {
                     if n == Sequence::ZERO {
                         return Err(PolicyError::ZeroTime);
@@ -978,15 +966,11 @@ impl<Pk: FromStrKey> Policy<Pk> {
             ("UNSATISFIABLE", 0) => Ok(Policy::Unsatisfiable),
             ("TRIVIAL", 0) => Ok(Policy::Trivial),
             ("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)),
-            ("after", 1) => {
-                let num = expression::terminal(&top.args[0], expression::parse_num)?;
-                if num > 2u32.pow(31) {
-                    return Err(Error::PolicyError(PolicyError::TimeTooFar));
-                } else if num == 0 {
-                    return Err(Error::PolicyError(PolicyError::ZeroTime));
-                }
-                Ok(Policy::after(num))
-            }
+            ("after", 1) => expression::terminal(&top.args[0], |x| {
+                expression::parse_num(x)
+                    .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
+                    .map(Policy::After)
+            }),
             ("older", 1) => {
                 let num = expression::terminal(&top.args[0], expression::parse_num)?;
                 if num > 2u32.pow(31) {

src/policy/mod.rs

@@ -323,11 +323,13 @@ mod tests {
             ConcretePol::from_str("or(pk())").unwrap_err().to_string(),
             "Or policy fragment must take 2 arguments"
         );
+        // this weird "unexpected" wrapping of the error will go away in a later PR
+        // which rewrites the expression parser
         assert_eq!(
             ConcretePol::from_str("thresh(3,after(0),pk(),pk())")
                 .unwrap_err()
                 .to_string(),
-            "Time must be greater than 0; n > 0"
+            "unexpected «absolute locktimes in Miniscript have a minimum value of 1»",
         );
 
         assert_eq!(