Merge rust-bitcoin/rust-miniscript#654: Several locktime improvements

422b83cb198976ee0932686dc7dd4bdf396a9c8b interpreter: use `relative::LockTime` for constraints from Miniscript (Andrew Poelstra)
fc42e48022052935f8d657c7e7755513e8d83223 miniscript: use RelLockTime in Terminal (Andrew Poelstra)
c490f49fcb7eab6794b0c40c83d397f8cd02fe2d policy: use `RelLockTime` in semantic and concrete policies (Andrew Poelstra)
a7a1b9a4876e89cfedc2f03130108b107a3864cd miniscript: use RelLockTime in `Satisfaction` (Andrew Poelstra)
fc6bfa8f57cd050d5ab7c887ca9e15928a7c6416 introduce RelLockTime type (Andrew Poelstra)
637720a8280bfdaeb457cd0f7ec364f945f278b7 plan/satisfy: use relative::LockTime instead of Sequence to indicate relative locktimes (Andrew Poelstra)
bd6ef65f8697501b3798cabe9ab0b6a5aa01b761 interpreter: rename "age" to "sequence" (Andrew Poelstra)
950b79389502b045b603903989b8d58c2382cd74 AbsLockTime: move to module, consolidate error checking (Andrew Poelstra)
60d7c98db16a3c41fb9fabdd53545e40b64826e9 types: drop unused "strongness" errors (Andrew Poelstra)
2f667789a72894ac82a3e5a23d5f1b968230f09d types: replace closure with iterator (Andrew Poelstra)

Pull request description:

  This PR:
  * Changes user-facing "check locktime" functions to use `bitcoin::relative::LockTime` rather than `Sequence` numbers.
  * Introduces a new `RelLockTime` type analogous to `AbsLockTime` which is used to provide a locktime type that implements `Ord`.
  * In both types, validate that the locktimes are legal for use in Miniscript during creation, rather than during typechecking or validity checks.
  * In many places, replace manual code with the new `is_implied_by` method from rust-bitcoin.

  This might be a little hard to review because it messes with locktime code which is so easy to get backward. But it appears that there are pretty extensive unit tests which caught several iterations of this where I did get them backward. Going forward things should be much easier to maintain because we have stronger types and an encapsulated `is_implied_by` method.

  The next PR will do a similar thing with thresholds to validate the `0 < k <= n` invariant when thresholds are created, avoiding the repeated and inconsistent post-hoc error checking logic.

  Fixes #270

ACKs for top commit:
  sanket1729:
    ACK 422b83cb198976ee0932686dc7dd4bdf396a9c8b

Tree-SHA512: 42a749a405c142c7ec468b2fd2e8c3c559c31e6eacf72a351c4dd24887385fa74d8128982a70cef69489c4ef72d6247860e1cd96e52ec83a00bdee0e3359d9b8

Author: heap-coder

src/descriptor/mod.rs

@@ -512,7 +512,7 @@ impl Descriptor<DefiniteDescriptorKey> {
                 descriptor: self,
                 template: stack,
                 absolute_timelock: satisfaction.absolute_timelock.map(Into::into),
-                relative_timelock: satisfaction.relative_timelock,
+                relative_timelock: satisfaction.relative_timelock.map(Into::into),
             })
         } else {
             Err(self)
@@ -540,7 +540,8 @@ impl Descriptor<DefiniteDescriptorKey> {
                 descriptor: self,
                 template: stack,
                 absolute_timelock: satisfaction.absolute_timelock.map(Into::into),
-                relative_timelock: satisfaction.relative_timelock,
+                // unwrap to be removed in a later commit
+                relative_timelock: satisfaction.relative_timelock.map(Into::into),
             })
         } else {
             Err(self)

src/interpreter/error.rs

@@ -9,7 +9,7 @@ use bitcoin::hashes::hash160;
 use bitcoin::hex::DisplayHex;
 #[cfg(not(test))] // https://github.com/rust-lang/rust/issues/121684
 use bitcoin::secp256k1;
-use bitcoin::taproot;
+use bitcoin::{absolute, relative, taproot};
 
 use super::BitcoinKey;
 use crate::prelude::*;
@@ -18,9 +18,9 @@ use crate::prelude::*;
 #[derive(Debug)]
 pub enum Error {
     /// Could not satisfy, absolute locktime not met
-    AbsoluteLocktimeNotMet(u32),
+    AbsoluteLockTimeNotMet(absolute::LockTime),
     /// Could not satisfy, lock time values are different units
-    AbsoluteLocktimeComparisonInvalid(u32, u32),
+    AbsoluteLockTimeComparisonInvalid(absolute::LockTime, absolute::LockTime),
     /// Cannot Infer a taproot descriptor
     /// Key spends cannot infer the internal key of the descriptor
     /// Inferring script spends is possible, but is hidden nodes are currently
@@ -85,7 +85,9 @@ pub enum Error {
     /// Parse Error while parsing a `stack::Element::Push` as a XOnlyPublicKey (32 bytes)
     XOnlyPublicKeyParseError,
     /// Could not satisfy, relative locktime not met
-    RelativeLocktimeNotMet(u32),
+    RelativeLockTimeNotMet(relative::LockTime),
+    /// Could not satisfy, the sequence number on the tx input had the disable flag set.
+    RelativeLockTimeDisabled(relative::LockTime),
     /// Forward-secp related errors
     Secp(secp256k1::Error),
     /// Miniscript requires the entire top level script to be satisfied.
@@ -116,10 +118,10 @@ pub enum Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::AbsoluteLocktimeNotMet(n) => {
+            Error::AbsoluteLockTimeNotMet(n) => {
                 write!(f, "required absolute locktime CLTV of {} blocks, not met", n)
             }
-            Error::AbsoluteLocktimeComparisonInvalid(n, lock_time) => write!(
+            Error::AbsoluteLockTimeComparisonInvalid(n, lock_time) => write!(
                 f,
                 "could not satisfy, lock time values are different units n: {} lock_time: {}",
                 n, lock_time
@@ -159,9 +161,12 @@ impl fmt::Display for Error {
             Error::PkHashVerifyFail(ref hash) => write!(f, "Pubkey Hash check failed {}", hash),
             Error::PubkeyParseError => f.write_str("could not parse pubkey"),
             Error::XOnlyPublicKeyParseError => f.write_str("could not parse x-only pubkey"),
-            Error::RelativeLocktimeNotMet(n) => {
+            Error::RelativeLockTimeNotMet(n) => {
                 write!(f, "required relative locktime CSV of {} blocks, not met", n)
             }
+            Error::RelativeLockTimeDisabled(n) => {
+                write!(f, "required relative locktime CSV of {} blocks, but tx sequence number has disable-flag set", n)
+            }
             Error::ScriptSatisfactionError => f.write_str("Top level script must be satisfied"),
             Error::Secp(ref e) => fmt::Display::fmt(e, f),
             Error::SchnorrSig(ref s) => write!(f, "Schnorr sig error: {}", s),
@@ -188,8 +193,8 @@ impl error::Error for Error {
         use self::Error::*;
 
         match self {
-            AbsoluteLocktimeNotMet(_)
-            | AbsoluteLocktimeComparisonInvalid(_, _)
+            AbsoluteLockTimeNotMet(_)
+            | AbsoluteLockTimeComparisonInvalid(_, _)
             | CannotInferTrDescriptors
             | ControlBlockVerificationError
             | CouldNotEvaluate
@@ -212,7 +217,8 @@ impl error::Error for Error {
             | XOnlyPublicKeyParseError
             | PkEvaluationError(_)
             | PkHashVerifyFail(_)
-            | RelativeLocktimeNotMet(_)
+            | RelativeLockTimeNotMet(_)
+            | RelativeLockTimeDisabled(_)
             | ScriptSatisfactionError
             | TapAnnexUnsupported
             | UncompressedPubkey

src/interpreter/mod.rs

@@ -12,7 +12,7 @@ use core::fmt;
 use core::str::FromStr;
 
 use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
-use bitcoin::{absolute, secp256k1, sighash, taproot, Sequence, TxOut, Witness};
+use bitcoin::{absolute, relative, secp256k1, sighash, taproot, Sequence, TxOut, Witness};
 
 use crate::miniscript::context::{NoChecks, SigType};
 use crate::miniscript::ScriptContext;
@@ -35,7 +35,7 @@ pub struct Interpreter<'txin> {
     /// For non-Taproot spends, the scriptCode; for Taproot script-spends, this
     /// is the leaf script; for key-spends it is `None`.
     script_code: Option<bitcoin::ScriptBuf>,
-    age: Sequence,
+    sequence: Sequence,
     lock_time: absolute::LockTime,
 }
 
@@ -136,11 +136,11 @@ impl<'txin> Interpreter<'txin> {
         spk: &bitcoin::ScriptBuf,
         script_sig: &'txin bitcoin::Script,
         witness: &'txin Witness,
-        age: Sequence,                 // CSV, relative lock time.
+        sequence: Sequence,            // CSV, relative lock time.
         lock_time: absolute::LockTime, // CLTV, absolute lock time.
     ) -> Result<Self, Error> {
         let (inner, stack, script_code) = inner::from_txdata(spk, script_sig, witness)?;
-        Ok(Interpreter { inner, stack, script_code, age, lock_time })
+        Ok(Interpreter { inner, stack, script_code, sequence, lock_time })
     }
 
     /// Same as [`Interpreter::iter`], but allows for a custom verification function.
@@ -165,7 +165,7 @@ impl<'txin> Interpreter<'txin> {
             // Cloning the references to elements of stack should be fine as it allows
             // call interpreter.iter() without mutating interpreter
             stack: self.stack.clone(),
-            age: self.age,
+            sequence: self.sequence,
             lock_time: self.lock_time,
             has_errored: false,
             sig_type: self.sig_type(),
@@ -468,7 +468,7 @@ pub enum SatisfiedConstraint {
     ///Relative Timelock for CSV.
     RelativeTimelock {
         /// The value of RelativeTimelock
-        n: Sequence,
+        n: relative::LockTime,
     },
     ///Absolute Timelock for CLTV.
     AbsoluteTimelock {
@@ -508,7 +508,7 @@ pub struct Iter<'intp, 'txin: 'intp> {
     public_key: Option<&'intp BitcoinKey>,
     state: Vec<NodeEvaluationState<'intp>>,
     stack: Stack<'txin>,
-    age: Sequence,
+    sequence: Sequence,
     lock_time: absolute::LockTime,
     has_errored: bool,
     sig_type: SigType,
@@ -608,7 +608,7 @@ where
                 Terminal::Older(ref n) => {
                     debug_assert_eq!(node_state.n_evaluated, 0);
                     debug_assert_eq!(node_state.n_satisfied, 0);
-                    let res = self.stack.evaluate_older(n, self.age);
+                    let res = self.stack.evaluate_older(&(*n).into(), self.sequence);
                     if res.is_some() {
                         return res;
                     }
@@ -1110,7 +1110,7 @@ mod tests {
                 stack,
                 public_key: None,
                 state: vec![NodeEvaluationState { node: ms, n_evaluated: 0, n_satisfied: 0 }],
-                age: Sequence::from_height(1002),
+                sequence: Sequence::from_height(1002),
                 lock_time: absolute::LockTime::from_height(1002).unwrap(),
                 has_errored: false,
                 sig_type: SigType::Ecdsa,
@@ -1182,7 +1182,9 @@ mod tests {
         let older_satisfied: Result<Vec<SatisfiedConstraint>, Error> = constraints.collect();
         assert_eq!(
             older_satisfied.unwrap(),
-            vec![SatisfiedConstraint::RelativeTimelock { n: Sequence::from_height(1000) }]
+            vec![SatisfiedConstraint::RelativeTimelock {
+                n: crate::RelLockTime::from_height(1000).into()
+            }]
         );
 
         //Check Sha256

src/interpreter/stack.rs

@@ -5,7 +5,7 @@
 
 use bitcoin::blockdata::{opcodes, script};
 use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
-use bitcoin::{absolute, Sequence};
+use bitcoin::{absolute, relative, Sequence};
 
 use super::error::PkEvalErrInner;
 use super::{verify_sersig, BitcoinKey, Error, HashLockType, KeySigPair, SatisfiedConstraint};
@@ -212,19 +212,14 @@ impl<'txin> Stack<'txin> {
         let is_satisfied = match (*n, lock_time) {
             (Blocks(n), Blocks(lock_time)) => n <= lock_time,
             (Seconds(n), Seconds(lock_time)) => n <= lock_time,
-            _ => {
-                return Some(Err(Error::AbsoluteLocktimeComparisonInvalid(
-                    n.to_consensus_u32(),
-                    lock_time.to_consensus_u32(),
-                )))
-            }
+            _ => return Some(Err(Error::AbsoluteLockTimeComparisonInvalid(*n, lock_time))),
         };
 
         if is_satisfied {
             self.push(Element::Satisfied);
             Some(Ok(SatisfiedConstraint::AbsoluteTimelock { n: *n }))
         } else {
-            Some(Err(Error::AbsoluteLocktimeNotMet(n.to_consensus_u32())))
+            Some(Err(Error::AbsoluteLockTimeNotMet(*n)))
         }
     }
 
@@ -236,14 +231,19 @@ impl<'txin> Stack<'txin> {
     /// booleans
     pub(super) fn evaluate_older(
         &mut self,
-        n: &Sequence,
-        age: Sequence,
+        n: &relative::LockTime,
+        sequence: Sequence,
     ) -> Option<Result<SatisfiedConstraint, Error>> {
-        if age >= *n {
-            self.push(Element::Satisfied);
-            Some(Ok(SatisfiedConstraint::RelativeTimelock { n: *n }))
+        if let Some(tx_locktime) = sequence.to_relative_lock_time() {
+            if n.is_implied_by(tx_locktime) {
+                self.push(Element::Satisfied);
+                Some(Ok(SatisfiedConstraint::RelativeTimelock { n: *n }))
+            } else {
+                Some(Err(Error::RelativeLockTimeNotMet(*n)))
+            }
         } else {
-            Some(Err(Error::RelativeLocktimeNotMet(n.to_consensus_u32())))
+            // BIP 112: if the tx locktime has the disable flag set, fail CSV.
+            Some(Err(Error::RelativeLockTimeDisabled(*n)))
         }
     }
 

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,8 @@ 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};
+pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError};
 
 /// Public key trait which can be converted to Hash type
 pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash {
@@ -501,6 +504,10 @@ 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),
+    /// Invalid absolute locktime
+    RelativeLockTime(RelLockTimeError),
 }
 
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
@@ -576,6 +583,8 @@ 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),
+            Error::RelativeLockTime(ref e) => e.fmt(f),
         }
     }
 }
@@ -628,6 +637,8 @@ impl error::Error for Error {
             ContextError(e) => Some(e),
             AnalysisError(e) => Some(e),
             PubKeyCtxError(e, _) => Some(e),
+            AbsoluteLockTime(e) => Some(e),
+            RelativeLockTime(e) => Some(e),
         }
     }
 }
@@ -711,51 +722,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;