policy: use RelLockTime in semantic and concrete policies

apoelstra · apoelstra · commit c490f49fcb7e · 2024-03-07T21:58:35.000Z
This changes the signature of `at_age` to take a `relative::LockTime` to be symmetric with `at_lock_time`. But because `relative::LockTime` has no constructors this is kinda annoying to use. See rust-bitcoin/rust-bitcoin#2547 Anyway for now you can just construct a RelLockTime and tack .into() onto it, which is fine as a workaround. We may want to update docs for it.
diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs
@@ -698,7 +698,6 @@ mod tests {
     use bitcoin::hashes::{hash160, sha256, Hash};
     use bitcoin::secp256k1::XOnlyPublicKey;
     use bitcoin::taproot::TapLeafHash;
-    use bitcoin::Sequence;
     use sync::Arc;
 
     use super::{Miniscript, ScriptContext, Segwitv0, Tap};
@@ -707,7 +706,7 @@ mod tests {
     use crate::policy::Liftable;
     use crate::prelude::*;
     use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator};
-    use crate::{hex_script, ExtParams, Satisfier, ToPublicKey, TranslatePk};
+    use crate::{hex_script, ExtParams, RelLockTime, Satisfier, ToPublicKey, TranslatePk};
 
     type Segwitv0Script = Miniscript<bitcoin::PublicKey, Segwitv0>;
     type Tapscript = Miniscript<bitcoin::secp256k1::XOnlyPublicKey, Tap>;
@@ -1084,13 +1083,13 @@ mod tests {
         let mut abs = miniscript.lift().unwrap();
         assert_eq!(abs.n_keys(), 5);
         assert_eq!(abs.minimum_n_keys(), Some(2));
-        abs = abs.at_age(Sequence::from_height(10000));
+        abs = abs.at_age(RelLockTime::from_height(10000).into());
         assert_eq!(abs.n_keys(), 5);
         assert_eq!(abs.minimum_n_keys(), Some(2));
-        abs = abs.at_age(Sequence::from_height(9999));
+        abs = abs.at_age(RelLockTime::from_height(9999).into());
         assert_eq!(abs.n_keys(), 3);
         assert_eq!(abs.minimum_n_keys(), Some(3));
-        abs = abs.at_age(Sequence::ZERO);
+        abs = abs.at_age(RelLockTime::ZERO.into());
         assert_eq!(abs.n_keys(), 3);
         assert_eq!(abs.minimum_n_keys(), Some(3));
 
diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs
@@ -871,7 +871,7 @@ where
             insert_wrap!(AstElemExt::terminal(Terminal::PkK(pk.clone())));
         }
         Concrete::After(n) => insert_wrap!(AstElemExt::terminal(Terminal::After(n))),
-        Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n))),
+        Concrete::Older(n) => insert_wrap!(AstElemExt::terminal(Terminal::Older(n.into()))),
         Concrete::Sha256(ref hash) => {
             insert_wrap!(AstElemExt::terminal(Terminal::Sha256(hash.clone())))
         }
@@ -1254,7 +1254,7 @@ mod tests {
     use super::*;
     use crate::miniscript::{Legacy, Segwitv0, Tap};
     use crate::policy::Liftable;
-    use crate::{script_num_size, AbsLockTime, ToPublicKey};
+    use crate::{script_num_size, AbsLockTime, RelLockTime, ToPublicKey};
 
     type SPolicy = Concrete<String>;
     type BPolicy = Concrete<bitcoin::PublicKey>;
@@ -1406,7 +1406,7 @@ mod tests {
             (
                 1,
                 Arc::new(Concrete::And(vec![
-                    Arc::new(Concrete::Older(Sequence::from_height(10000))),
+                    Arc::new(Concrete::Older(RelLockTime::from_height(10000))),
                     Arc::new(Concrete::Threshold(
                         2,
                         key_pol[5..8].iter().map(|p| (p.clone()).into()).collect(),
@@ -1436,13 +1436,13 @@ mod tests {
         let mut abs = policy.lift().unwrap();
         assert_eq!(abs.n_keys(), 8);
         assert_eq!(abs.minimum_n_keys(), Some(2));
-        abs = abs.at_age(Sequence::from_height(10000));
+        abs = abs.at_age(RelLockTime::from_height(10000).into());
         assert_eq!(abs.n_keys(), 8);
         assert_eq!(abs.minimum_n_keys(), Some(2));
-        abs = abs.at_age(Sequence::from_height(9999));
+        abs = abs.at_age(RelLockTime::from_height(9999).into());
         assert_eq!(abs.n_keys(), 5);
         assert_eq!(abs.minimum_n_keys(), Some(3));
-        abs = abs.at_age(Sequence::ZERO);
+        abs = abs.at_age(RelLockTime::ZERO.into());
         assert_eq!(abs.n_keys(), 5);
         assert_eq!(abs.minimum_n_keys(), Some(3));
 
@@ -1467,15 +1467,16 @@ mod tests {
         assert!(ms.satisfy(no_sat).is_err());
         assert!(ms.satisfy(&left_sat).is_ok());
         assert!(ms
-            .satisfy((&right_sat, Sequence::from_height(10001)))
+            .satisfy((&right_sat, RelLockTime::from_height(10001)))
             .is_ok());
         //timelock not met
         assert!(ms
-            .satisfy((&right_sat, Sequence::from_height(9999)))
+            .satisfy((&right_sat, RelLockTime::from_height(9999)))
             .is_err());
 
         assert_eq!(
-            ms.satisfy((left_sat, Sequence::from_height(9999))).unwrap(),
+            ms.satisfy((left_sat, RelLockTime::from_height(9999)))
+                .unwrap(),
             vec![
                 // sat for left branch
                 vec![],
@@ -1486,7 +1487,7 @@ mod tests {
         );
 
         assert_eq!(
-            ms.satisfy((right_sat, Sequence::from_height(10000)))
+            ms.satisfy((right_sat, RelLockTime::from_height(10000)))
                 .unwrap(),
             vec![
                 // sat for right branch
diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs
@@ -7,7 +7,7 @@ use core::{fmt, str};
 #[cfg(feature = "std")]
 use std::error;
 
-use bitcoin::{absolute, Sequence};
+use bitcoin::absolute;
 #[cfg(feature = "compiler")]
 use {
     crate::descriptor::TapTree,
@@ -29,7 +29,9 @@ use crate::prelude::*;
 use crate::sync::Arc;
 #[cfg(all(doc, not(feature = "compiler")))]
 use crate::Descriptor;
-use crate::{errstr, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, Translator};
+use crate::{
+    errstr, AbsLockTime, Error, ForEachKey, FromStrKey, MiniscriptKey, RelLockTime, Translator,
+};
 
 /// Maximum TapLeafs allowed in a compiled TapTree
 #[cfg(feature = "compiler")]
@@ -52,7 +54,7 @@ pub enum Policy<Pk: MiniscriptKey> {
     /// An absolute locktime restriction.
     After(AbsLockTime),
     /// A relative locktime restriction.
-    Older(Sequence),
+    Older(RelLockTime),
     /// A SHA256 whose preimage must be provided to satisfy the descriptor.
     Sha256(Pk::Sha256),
     /// A SHA256d whose preimage must be provided to satisfy the descriptor.
@@ -70,15 +72,6 @@ pub enum Policy<Pk: MiniscriptKey> {
     Threshold(usize, Vec<Arc<Policy<Pk>>>),
 }
 
-impl<Pk> Policy<Pk>
-where
-    Pk: MiniscriptKey,
-{
-    /// 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)) }
-}
-
 /// Detailed error type for concrete policies.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum PolicyError {
@@ -88,10 +81,6 @@ pub enum PolicyError {
     NonBinaryArgOr,
     /// `Thresh` fragment can only have `1<=k<=n`.
     IncorrectThresh,
-    /// `older` or `after` fragment can only have `n = 0`.
-    ZeroTime,
-    /// `after` fragment can only have `n < 2^31`.
-    TimeTooFar,
     /// Semantic Policy Error: `And` `Or` fragments must take args: `k > 1`.
     InsufficientArgsforAnd,
     /// Semantic policy error: `And` `Or` fragments must take args: `k > 1`.
@@ -129,10 +118,6 @@ impl fmt::Display for PolicyError {
             PolicyError::IncorrectThresh => {
                 f.write_str("Threshold k must be greater than 0 and less than or equal to n 0<k<=n")
             }
-            PolicyError::TimeTooFar => {
-                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::InsufficientArgsforAnd => {
                 f.write_str("Semantic Policy 'And' fragment must have at least 2 args ")
             }
@@ -159,8 +144,6 @@ impl error::Error for PolicyError {
             NonBinaryArgAnd
             | NonBinaryArgOr
             | IncorrectThresh
-            | ZeroTime
-            | TimeTooFar
             | InsufficientArgsforAnd
             | InsufficientArgsforOr
             | EntailmentMaxTerminals
@@ -751,13 +734,7 @@ impl<Pk: MiniscriptKey> Policy<Pk> {
         for policy in self.pre_order_iter() {
             match *policy {
                 After(_) => {}
-                Older(n) => {
-                    if n == Sequence::ZERO {
-                        return Err(PolicyError::ZeroTime);
-                    } else if n.to_consensus_u32() > 2u32.pow(31) {
-                        return Err(PolicyError::TimeTooFar);
-                    }
-                }
+                Older(_) => {}
                 And(ref subs) => {
                     if subs.len() != 2 {
                         return Err(PolicyError::NonBinaryArgAnd);
@@ -971,15 +948,11 @@ impl<Pk: FromStrKey> Policy<Pk> {
                     .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) {
-                    return Err(Error::PolicyError(PolicyError::TimeTooFar));
-                } else if num == 0 {
-                    return Err(Error::PolicyError(PolicyError::ZeroTime));
-                }
-                Ok(Policy::older(num))
-            }
+            ("older", 1) => expression::terminal(&top.args[0], |x| {
+                expression::parse_num(x)
+                    .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime))
+                    .map(Policy::Older)
+            }),
             ("sha256", 1) => expression::terminal(&top.args[0], |x| {
                 <Pk::Sha256 as core::str::FromStr>::from_str(x).map(Policy::Sha256)
             }),
diff --git a/src/policy/mod.rs b/src/policy/mod.rs
@@ -123,7 +123,9 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Liftable<Pk> for Terminal<Pk, Ctx> {
                 return Err(Error::LiftError(LiftError::RawDescriptorLift))
             }
             Terminal::After(t) => Semantic::After(t),
-            Terminal::Older(t) => Semantic::Older(t),
+            Terminal::Older(t) => Semantic::Older(
+                <crate::RelLockTime as core::convert::TryFrom<_>>::try_from(t).unwrap(),
+            ), // unwrap to be removed in future commit
             Terminal::Sha256(ref h) => Semantic::Sha256(h.clone()),
             Terminal::Hash256(ref h) => Semantic::Hash256(h.clone()),
             Terminal::Ripemd160(ref h) => Semantic::Ripemd160(h.clone()),
@@ -241,13 +243,12 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Arc<Concrete<Pk>> {
 mod tests {
     use core::str::FromStr;
 
-    use bitcoin::Sequence;
-
     use super::*;
     #[cfg(feature = "compiler")]
     use crate::descriptor::Tr;
     use crate::miniscript::context::Segwitv0;
     use crate::prelude::*;
+    use crate::RelLockTime;
     #[cfg(feature = "compiler")]
     use crate::{descriptor::TapTree, Tap};
 
@@ -323,7 +324,7 @@ 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
+        // these weird "unexpected" wrapping of errors will go away in a later PR
         // which rewrites the expression parser
         assert_eq!(
             ConcretePol::from_str("thresh(3,after(0),pk(),pk())")
@@ -336,7 +337,7 @@ mod tests {
             ConcretePol::from_str("thresh(2,older(2147483650),pk(),pk())")
                 .unwrap_err()
                 .to_string(),
-            "Relative/Absolute time must be less than 2^31; n < 2^31"
+            "unexpected «locktime value 2147483650 is not a valid BIP68 relative locktime»"
         );
     }
 
@@ -370,7 +371,7 @@ mod tests {
                         2,
                         vec![
                             Arc::new(Semantic::Key(key_a)),
-                            Arc::new(Semantic::Older(Sequence::from_height(42)))
+                            Arc::new(Semantic::Older(RelLockTime::from_height(42)))
                         ]
                     )),
                     Arc::new(Semantic::Key(key_b))
diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs
