Fix off-by-one bug in relative locktime

Define 'is satisfied by' - this is a classic off-by-one problem, if a
relative lock is satisfied does that mean it can go in this block or the
next? Its most useful if it means 'it can go in the next' and this is
how relative height and MTP are used in Core.

Ramifications:

- When checking a time based lock we check against the chain tip MTP,
then when Core verifies a block with the output in it it uses the
previous block (and this is still the chain tip).
- When checking a height base lock we check against chain tip height + 1
because Core checks against height of the block being verified.

Additionally we currently have a false negative in the satisfaction
functions when the `crate` type (height or MTP) is to big to fit in a
u16 - in this case we should return true not false because a value too
big definitely is > the lock value.

One final API paper cut - currently if the caller puts the args in the
wrong order they get a false negative instead of an error.

Fix all this by making the satisfaction functions return errors, update
the docs to explicitly define 'satisfaction'.

For now remove the examples in rustdocs, we can circle back to these
once the dust settles.

API test of Errors:

Some of the errors are being 'API tested' tested in `primitives` but
they should be being done in `units/tests/api.rs` - put all the new
errors in the correct places.

bitcoin/src/blockdata/mod.rs

@@ -71,7 +71,7 @@ pub mod locktime {
 
         /// Re-export everything from the `primitives::locktime::relative` module.
         pub use primitives::locktime::relative::{
-            DisabledLockTimeError, IncompatibleHeightError, IncompatibleTimeError, LockTime,
+            DisabledLockTimeError, InvalidHeightError, InvalidTimeError, LockTime,
             NumberOf512Seconds, NumberOfBlocks, TimeOverflowError,
         };
 

primitives/src/locktime/relative.rs

@@ -7,13 +7,15 @@
 
 use core::{convert, fmt};
 
+use internals::write_err;
+
 use crate::Sequence;
 #[cfg(all(doc, feature = "alloc"))]
 use crate::{relative, TxIn};
 
 #[rustfmt::skip]                // Keep public re-exports separate.
 #[doc(inline)]
-pub use units::locktime::relative::{NumberOfBlocks, NumberOf512Seconds, TimeOverflowError};
+pub use units::locktime::relative::{NumberOfBlocks, NumberOf512Seconds, TimeOverflowError, InvalidHeightError, InvalidTimeError};
 use units::{BlockHeight, BlockMtp};
 
 #[deprecated(since = "TBD", note = "use `NumberOfBlocks` instead")]
@@ -39,40 +41,6 @@ pub type Time = NumberOf512Seconds;
 ///
 /// * [BIP 68 Relative lock-time using consensus-enforced sequence numbers](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki)
 /// * [BIP 112 CHECKSEQUENCEVERIFY](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki)
-///
-/// # Examples
-///
-/// ```
-/// use bitcoin_primitives::relative;
-/// use bitcoin_primitives::{BlockHeight, BlockMtp, BlockTime};
-/// let lock_by_height = relative::LockTime::from_height(144); // 144 blocks, approx 24h.
-/// assert!(lock_by_height.is_block_height());
-///
-/// let lock_by_time = relative::LockTime::from_512_second_intervals(168); // 168 time intervals, approx 24h.
-/// assert!(lock_by_time.is_block_time());
-///
-/// fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] {
-///     let mut timestamps = [BlockTime::from_u32(0); 11];
-///     for (i, ts) in timestamps.iter_mut().enumerate() {
-///         *ts = BlockTime::from_u32(start.saturating_sub((step * i as u16).into()));
-///     }
-///     timestamps
-/// }
-/// // time extracted from BlockHeader
-/// let timestamps: [BlockTime; 11] = generate_timestamps(1_600_000_000, 200);
-/// let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200);
-///
-/// let current_height = BlockHeight::from(100);
-/// let current_mtp = BlockMtp::new(timestamps);
-///
-/// let utxo_height = BlockHeight::from(80);
-/// let utxo_mtp = BlockMtp::new(utxo_timestamps);
-///
-/// let locktime = relative::LockTime::Time(relative::NumberOf512Seconds::from_512_second_intervals(10));
-///
-/// // Check if locktime is satisfied
-/// assert!(locktime.is_satisfied_by(current_height, current_mtp, utxo_height, utxo_mtp));
-/// ```
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub enum LockTime {
@@ -221,45 +189,27 @@ impl LockTime {
     pub const fn is_block_time(self) -> bool { !self.is_block_height() }
 
     /// Returns true if this [`relative::LockTime`] is satisfied by the given chain state.
-    /// # Examples
     ///
-    /// ```rust
+    /// If this function returns true then an output with this locktime can be spent in the next
-    /// # use bitcoin_primitives::relative::Time;
+    /// block.
-    /// # use bitcoin_primitives::{BlockHeight, BlockMtp, BlockTime};
-    /// # use bitcoin_primitives::relative::LockTime;
     ///
-    /// fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] {
+    /// # Errors
-    ///     let mut timestamps = [BlockTime::from_u32(0); 11];
-    ///     for (i, ts) in timestamps.iter_mut().enumerate() {
-    ///         *ts = BlockTime::from_u32(start.saturating_sub((step * i as u16).into()));
-    ///     }
-    ///     timestamps
-    /// }
-    /// // time extracted from BlockHeader
-    /// let timestamps: [BlockTime; 11] = generate_timestamps(1_600_000_000, 200);
-    /// let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200);
     ///
-    /// let current_height = BlockHeight::from_u32(100);
+    /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up.
-    /// let current_mtp = BlockMtp::new(timestamps);
-    /// let utxo_height = BlockHeight::from_u32(80);
-    /// let utxo_mtp = BlockMtp::new(utxo_timestamps);
-    ///
-    /// let locktime = LockTime::Time(Time::from_512_second_intervals(10));
-    ///
-    /// // Check if locktime is satisfied
-    /// assert!(locktime.is_satisfied_by(current_height, current_mtp, utxo_height, utxo_mtp));
-    /// ```
     pub fn is_satisfied_by(
         self,
         chain_tip_height: BlockHeight,
         chain_tip_mtp: BlockMtp,
         utxo_mined_at_height: BlockHeight,
         utxo_mined_at_mtp: BlockMtp,
-    ) -> bool {
+    ) -> Result<bool, IsSatisfiedByError> {
         match self {
-            LockTime::Blocks(blocks) =>
+            LockTime::Blocks(blocks) => blocks
-                blocks.is_satisfied_by(chain_tip_height, utxo_mined_at_height),
+                .is_satisfied_by(chain_tip_height, utxo_mined_at_height)
-            LockTime::Time(time) => time.is_satisfied_by(chain_tip_mtp, utxo_mined_at_mtp),
+                .map_err(IsSatisfiedByError::Blocks),
+            LockTime::Time(time) => time
+                .is_satisfied_by(chain_tip_mtp, utxo_mined_at_mtp)
+                .map_err(IsSatisfiedByError::Time),
         }
     }
 
@@ -334,6 +284,9 @@ impl LockTime {
 
     /// Returns true if an output with this locktime can be spent in the next block.
     ///
+    /// If this function returns true then an output with this locktime can be spent in the next
+    /// block.
+    ///
     /// # Errors
     ///
     /// Returns an error if this lock is not lock-by-height.
@@ -342,17 +295,22 @@ impl LockTime {
         self,
         chain_tip: BlockHeight,
         utxo_mined_at: BlockHeight,
-    ) -> Result<bool, IncompatibleHeightError> {
+    ) -> Result<bool, IsSatisfiedByHeightError> {
         use LockTime as L;
 
         match self {
-            L::Blocks(blocks) => Ok(blocks.is_satisfied_by(chain_tip, utxo_mined_at)),
+            L::Blocks(blocks) => blocks
-            L::Time(time) => Err(IncompatibleHeightError { time }),
+                .is_satisfied_by(chain_tip, utxo_mined_at)
+                .map_err(IsSatisfiedByHeightError::Satisfaction),
+            L::Time(time) => Err(IsSatisfiedByHeightError::Incompatible(time)),
         }
     }
 
     /// Returns true if an output with this locktime can be spent in the next block.
     ///
+    /// If this function returns true then an output with this locktime can be spent in the next
+    /// block.
+    ///
     /// # Errors
     ///
     /// Returns an error if this lock is not lock-by-time.
@@ -361,12 +319,14 @@ impl LockTime {
         self,
         chain_tip: BlockMtp,
         utxo_mined_at: BlockMtp,
-    ) -> Result<bool, IncompatibleTimeError> {
+    ) -> Result<bool, IsSatisfiedByTimeError> {
         use LockTime as L;
 
         match self {
-            L::Time(time) => Ok(time.is_satisfied_by(chain_tip, utxo_mined_at)),
+            L::Time(time) => time
-            L::Blocks(blocks) => Err(IncompatibleTimeError { blocks }),
+                .is_satisfied_by(chain_tip, utxo_mined_at)
+                .map_err(IsSatisfiedByTimeError::Satisfaction),
+            L::Blocks(blocks) => Err(IsSatisfiedByTimeError::Incompatible(blocks)),
         }
     }
 }
@@ -434,49 +394,108 @@ impl fmt::Display for DisabledLockTimeError {
 #[cfg(feature = "std")]
 impl std::error::Error for DisabledLockTimeError {}
 
-/// Tried to satisfy a lock-by-blocktime lock using a height value.
+/// Error returned when attempting to satisfy lock fails.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, Eq, PartialEq)]
-pub struct IncompatibleHeightError {
+pub enum IsSatisfiedByError {
-    /// The inner time value of the lock-by-blocktime lock.
+    /// Error when attempting to satisfy lock by height.
-    time: NumberOf512Seconds,
+    Blocks(InvalidHeightError),
+    /// Error when attempting to satisfy lock by time.
+    Time(InvalidTimeError),
 }
 
-impl IncompatibleHeightError {
+impl fmt::Display for IsSatisfiedByError {
-    /// Returns the time value of the lock-by-blocktime lock.
-    pub fn expected(&self) -> NumberOf512Seconds { self.time }
-}
-
-impl fmt::Display for IncompatibleHeightError {
     #[inline]
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "tried to satisfy a lock-by-blocktime lock {} by height", self.time,)
+        use IsSatisfiedByError as E;
+
+        match *self {
+            E::Blocks(ref e) => write_err!(f, "blocks"; e),
+            E::Time(ref e) => write_err!(f, "time"; e),
+        }
     }
 }
 
 #[cfg(feature = "std")]
-impl std::error::Error for IncompatibleHeightError {}
+impl std::error::Error for IsSatisfiedByError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use IsSatisfiedByError as E;
 
-/// Tried to satisfy a lock-by-blockheight lock using a time value.
+        match *self {
+            E::Blocks(ref e) => Some(e),
+            E::Time(ref e) => Some(e),
+        }
+    }
+}
+
+/// Error returned when `is_satisfied_by_height` fails.
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct IncompatibleTimeError {
+pub enum IsSatisfiedByHeightError {
-    /// The inner value of the lock-by-blockheight lock.
+    /// Satisfaction of the lock height value failed.
-    blocks: NumberOfBlocks,
+    Satisfaction(InvalidHeightError),
+    /// Tried to satisfy a lock-by-height locktime using seconds.
+    // TODO: Hide inner value in a new struct error type.
+    Incompatible(NumberOf512Seconds),
 }
 
-impl IncompatibleTimeError {
+impl fmt::Display for IsSatisfiedByHeightError {
-    /// Returns the height value of the lock-by-blockheight lock.
-    pub fn expected(&self) -> NumberOfBlocks { self.blocks }
-}
-
-impl fmt::Display for IncompatibleTimeError {
     #[inline]
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "tried to satisfy a lock-by-blockheight lock {} by time", self.blocks,)
+        use IsSatisfiedByHeightError as E;
+
+        match *self {
+            E::Satisfaction(ref e) => write_err!(f, "satisfaction"; e),
+            E::Incompatible(time) =>
+                write!(f, "tried to satisfy a lock-by-height locktime using seconds {}", time),
+        }
     }
 }
 
 #[cfg(feature = "std")]
-impl std::error::Error for IncompatibleTimeError {}
+impl std::error::Error for IsSatisfiedByHeightError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use IsSatisfiedByHeightError as E;
+
+        match *self {
+            E::Satisfaction(ref e) => Some(e),
+            E::Incompatible(_) => None,
+        }
+    }
+}
+
+/// Error returned when `is_satisfied_by_time` fails.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum IsSatisfiedByTimeError {
+    /// Satisfaction of the lock time value failed.
+    Satisfaction(InvalidTimeError),
+    /// Tried to satisfy a lock-by-time locktime using number of blocks.
+    // TODO: Hide inner value in a new struct error type.
+    Incompatible(NumberOfBlocks),
+}
+
+impl fmt::Display for IsSatisfiedByTimeError {
+    #[inline]
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use IsSatisfiedByTimeError as E;
+
+        match *self {
+            E::Satisfaction(ref e) => write_err!(f, "satisfaction"; e),
+            E::Incompatible(blocks) =>
+                write!(f, "tried to satisfy a lock-by-height locktime using blocks {}", blocks),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for IsSatisfiedByTimeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use IsSatisfiedByTimeError as E;
+
+        match *self {
+            E::Satisfaction(ref e) => Some(e),
+            E::Incompatible(_) => None,
+        }
+    }
+}
 
 #[cfg(test)]
 mod tests {
@@ -634,7 +653,7 @@ mod tests {
         let err = lock_by_time.is_satisfied_by_height(chain_tip, mined_at).unwrap_err();
 
         let expected_time = NumberOf512Seconds::from_512_second_intervals(70);
-        assert_eq!(err.expected(), expected_time);
+        assert_eq!(err, IsSatisfiedByHeightError::Incompatible(expected_time));
         assert!(!format!("{}", err).is_empty());
     }
 
@@ -648,7 +667,7 @@ mod tests {
         let err = lock_by_height.is_satisfied_by_time(chain_tip, mined_at).unwrap_err();
 
         let expected_height = NumberOfBlocks::from(10);
-        assert_eq!(err.expected(), expected_height);
+        assert_eq!(err, IsSatisfiedByTimeError::Incompatible(expected_height));
         assert!(!format!("{}", err).is_empty());
     }
 
@@ -671,49 +690,46 @@ mod tests {
         let utxo_mtp = BlockMtp::new(utxo_timestamps);
 
         let lock1 = LockTime::Blocks(NumberOfBlocks::from(10));
-        assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
 
         let lock2 = LockTime::Blocks(NumberOfBlocks::from(21));
-        assert!(!lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
 
         let lock3 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(10));
-        assert!(lock3.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(lock3.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
 
         let lock4 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(20000));
-        assert!(!lock4.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(!lock4.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
 
-        assert!(LockTime::ZERO.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(LockTime::ZERO
-        assert!(LockTime::from_512_second_intervals(0).is_satisfied_by(
+            .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)
-            chain_height,
+            .unwrap());
-            chain_mtp,
+        assert!(LockTime::from_512_second_intervals(0)
-            utxo_height,
+            .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)
-            utxo_mtp
+            .unwrap());
-        ));
 
         let lock6 = LockTime::from_seconds_floor(5000).unwrap();
-        assert!(lock6.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(lock6.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
 
         let max_height_lock = LockTime::Blocks(NumberOfBlocks::MAX);
-        assert!(!max_height_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(!max_height_lock
+            .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)
+            .unwrap());
 
         let max_time_lock = LockTime::Time(NumberOf512Seconds::MAX);
-        assert!(!max_time_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+        assert!(!max_time_lock
+            .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)
+            .unwrap());
 
         let max_chain_height = BlockHeight::from_u32(u32::MAX);
         let max_chain_mtp = BlockMtp::new(generate_timestamps(u32::MAX, 100));
         let max_utxo_height = BlockHeight::MAX;
         let max_utxo_mtp = max_chain_mtp;
-        assert!(!max_height_lock.is_satisfied_by(
+        assert!(!max_height_lock
-            max_chain_height,
+            .is_satisfied_by(max_chain_height, max_chain_mtp, max_utxo_height, max_utxo_mtp)
-            max_chain_mtp,
+            .unwrap());
-            max_utxo_height,
+        assert!(!max_time_lock
-            max_utxo_mtp
+            .is_satisfied_by(max_chain_height, max_chain_mtp, max_utxo_height, max_utxo_mtp)
-        ));
+            .unwrap());
-        assert!(!max_time_lock.is_satisfied_by(
-            max_chain_height,
-            max_chain_mtp,
-            max_utxo_height,
-            max_utxo_mtp
-        ));
     }
 }

primitives/tests/api.rs

@@ -163,14 +163,12 @@ struct Default {
 #[derive(Debug, Clone, PartialEq, Eq)] // All public types implement Debug (C-DEBUG).
 struct Errors {
     a: transaction::ParseOutPointError,
-    b: relative::IncompatibleHeightError,
+    b: relative::DisabledLockTimeError,
-    c: relative::IncompatibleTimeError,
+    c: relative::IsSatisfiedByError,
-    d: relative::IncompatibleHeightError,
+    d: relative::IsSatisfiedByHeightError,
-    e: relative::IncompatibleTimeError,
+    e: relative::IsSatisfiedByTimeError,
-    f: relative::DisabledLockTimeError,
+    f: script::RedeemScriptSizeError,
-    g: relative::DisabledLockTimeError,
+    g: script::WitnessScriptSizeError,
-    h: script::RedeemScriptSizeError,
-    i: script::WitnessScriptSizeError,
 }
 
 #[test]
@@ -212,7 +210,7 @@ fn api_can_use_types_from_crate_root() {
 #[test]
 fn api_can_use_all_types_from_module_locktime() {
     use bitcoin_primitives::locktime::relative::{
-        DisabledLockTimeError, IncompatibleHeightError, IncompatibleTimeError, LockTime,
+        DisabledLockTimeError, InvalidHeightError, InvalidTimeError, LockTime,
     };
     use bitcoin_primitives::locktime::{absolute, relative};
 }

units/src/locktime/relative.rs

@@ -55,29 +55,27 @@ impl NumberOfBlocks {
         self.0 as u32 // cast safety: u32 is wider than u16 on all architectures
     }
 
-    /// Determines whether a relative‐height locktime has matured, taking into account
+    /// Returns true if an output locked by height can be spent in the next block.
-    /// both the chain tip and the height at which the UTXO was confirmed.
     ///
-    /// If you have two height intervals `x` and `y`, and want to know whether `x`
+    /// # Errors
-    /// is satisfied by `y`, use `x >= y`.
     ///
-    /// # Parameters
+    /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up.
-    /// - `self` – the relative block‐height delay (`h`) required after confirmation.
-    /// - `chain_tip` – the height of the current chain tip
-    /// - `utxo_mined_at` – the height of the UTXO’s confirmation block
-    ///
-    /// # Returns
-    /// - `true` if a UTXO locked by `self` can be spent in a block after `chain_tip`.
-    /// - `false` if the UTXO is still locked at `chain_tip`.
     pub fn is_satisfied_by(
         self,
         chain_tip: crate::BlockHeight,
         utxo_mined_at: crate::BlockHeight,
-    ) -> bool {
+    ) -> Result<bool, InvalidHeightError> {
-        chain_tip
+        match chain_tip.checked_sub(utxo_mined_at) {
-            .checked_sub(utxo_mined_at)
+            Some(diff) => {
-            .and_then(|diff: crate::BlockHeightInterval| diff.try_into().ok())
+                if diff.to_u32() == u32::MAX {
-            .map_or(false, |diff: Self| diff >= self)
+                    // Weird but ok none the less - protects against overflow below.
+                    return Ok(true);
+                }
+                // +1 because the next block will have height 1 higher than `chain_tip`.
+                Ok(u32::from(self.to_height()) <= diff.to_u32() + 1)
+            }
+            None => Err(InvalidHeightError { chain_tip, utxo_mined_at }),
+        }
     }
 }
 
@@ -181,29 +179,24 @@ impl NumberOf512Seconds {
         (1u32 << 22) | self.0 as u32 // cast safety: u32 is wider than u16 on all architectures
     }
 
-    /// Determines whether a relative‑time lock has matured, taking into account both
+    /// Returns true if an output locked by time can be spent in the next block.
-    /// the UTXO’s Median Time Past at confirmation and the required delay.
     ///
-    /// If you have two MTP intervals `x` and `y`, and want to know whether `x`
+    /// # Errors
-    /// is satisfied by `y`, use `x >= y`.
     ///
-    /// # Parameters
+    /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up.
-    /// - `self` – the relative time delay (`t`) in 512‑second intervals.
-    /// - `chain_tip` – the MTP of the current chain tip
-    /// - `utxo_mined_at` – the MTP of the UTXO’s confirmation block
-    ///
-    /// # Returns
-    /// - `true` if the relative‐time lock has expired by the tip’s MTP
-    /// - `false` if the lock has not yet expired by the tip’s MTP
     pub fn is_satisfied_by(
         self,
         chain_tip: crate::BlockMtp,
         utxo_mined_at: crate::BlockMtp,
-    ) -> bool {
+    ) -> Result<bool, InvalidTimeError> {
-        chain_tip
+        match chain_tip.checked_sub(utxo_mined_at) {
-            .checked_sub(utxo_mined_at)
+            Some(diff) => {
-            .and_then(|diff: crate::BlockMtpInterval| diff.to_relative_mtp_interval_floor().ok())
+                // The locktime check in Core during block validation uses the MTP of the previous
-            .map_or(false, |diff: Self| diff >= self)
+                // block - which is `chain_tip` here.
+                Ok(self.to_seconds() <= diff.to_u32())
+            }
+            None => Err(InvalidTimeError { chain_tip, utxo_mined_at }),
+        }
     }
 }
 
@@ -246,6 +239,44 @@ impl fmt::Display for TimeOverflowError {
 #[cfg(feature = "std")]
 impl std::error::Error for TimeOverflowError {}
 
+/// Error returned when `NumberOfBlocks::is_satisfied_by` is incorrectly called.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct InvalidHeightError {
+    /// The `chain_tip` argument.
+    pub(crate) chain_tip: crate::BlockHeight,
+    /// The `utxo_mined_at` argument.
+    pub(crate) utxo_mined_at: crate::BlockHeight,
+}
+
+impl fmt::Display for InvalidHeightError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "is_satisfied_by arguments invalid (probably the wrong way around) chain_tip: {} utxo_mined_at: {}", self.chain_tip, self.utxo_mined_at
+        )
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidHeightError {}
+
+/// Error returned when `NumberOf512Seconds::is_satisfied_by` is incorrectly called.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct InvalidTimeError {
+    /// The `chain_tip` argument.
+    pub(crate) chain_tip: crate::BlockMtp,
+    /// The `utxo_mined_at` argument.
+    pub(crate) utxo_mined_at: crate::BlockMtp,
+}
+
+impl fmt::Display for InvalidTimeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "is_satisfied_by arguments invalid (probably the wrong way around) chain_tip: {} utxo_mined_at: {}", self.chain_tip, self.utxo_mined_at
+        )
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidTimeError {}
+
 #[cfg(feature = "arbitrary")]
 impl<'a> Arbitrary<'a> for NumberOfBlocks {
     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
@@ -383,24 +414,24 @@ mod tests {
         let time_lock = NumberOf512Seconds::from_512_second_intervals(10);
         let chain_state1 = BlockMtp::new(timestamps);
         let utxo_state1 = BlockMtp::new(utxo_timestamps);
-        assert!(time_lock.is_satisfied_by(chain_state1, utxo_state1));
+        assert!(time_lock.is_satisfied_by(chain_state1, utxo_state1).unwrap());
 
         // Test case 2: Not satisfied (current_mtp < utxo_mtp + required_seconds)
         let chain_state2 = BlockMtp::new(timestamps2);
         let utxo_state2 = BlockMtp::new(utxo_timestamps2);
-        assert!(!time_lock.is_satisfied_by(chain_state2, utxo_state2));
+        assert!(!time_lock.is_satisfied_by(chain_state2, utxo_state2).unwrap());
 
         // Test case 3: Test with a larger value (100 intervals = 51200 seconds)
         let larger_lock = NumberOf512Seconds::from_512_second_intervals(100);
         let chain_state3 = BlockMtp::new(timestamps3);
         let utxo_state3 = BlockMtp::new(utxo_timestamps3);
-        assert!(larger_lock.is_satisfied_by(chain_state3, utxo_state3));
+        assert!(larger_lock.is_satisfied_by(chain_state3, utxo_state3).unwrap());
 
         // Test case 4: Overflow handling - tests that is_satisfied_by handles overflow gracefully
         let max_time_lock = NumberOf512Seconds::MAX;
         let chain_state4 = BlockMtp::new(timestamps);
         let utxo_state4 = BlockMtp::new(utxo_timestamps);
-        assert!(!max_time_lock.is_satisfied_by(chain_state4, utxo_state4));
+        assert!(!max_time_lock.is_satisfied_by(chain_state4, utxo_state4).unwrap());
     }
 
     #[test]
@@ -410,19 +441,19 @@ mod tests {
         let height_lock = NumberOfBlocks(10);
 
         // Test case 1: Satisfaction (current_height >= utxo_height + required)
-        let chain_state1 = BlockHeight::from_u32(100);
+        let chain_state1 = BlockHeight::from_u32(89);
         let utxo_state1 = BlockHeight::from_u32(80);
-        assert!(height_lock.is_satisfied_by(chain_state1, utxo_state1));
+        assert!(height_lock.is_satisfied_by(chain_state1, utxo_state1).unwrap());
 
         // Test case 2: Not satisfied (current_height < utxo_height + required)
-        let chain_state2 = BlockHeight::from_u32(89);
+        let chain_state2 = BlockHeight::from_u32(88);
         let utxo_state2 = BlockHeight::from_u32(80);
-        assert!(!height_lock.is_satisfied_by(chain_state2, utxo_state2));
+        assert!(!height_lock.is_satisfied_by(chain_state2, utxo_state2).unwrap());
 
         // Test case 3: Overflow handling - tests that is_satisfied_by handles overflow gracefully
         let max_height_lock = NumberOfBlocks::MAX;
         let chain_state3 = BlockHeight::from_u32(1000);
         let utxo_state3 = BlockHeight::from_u32(80);
-        assert!(!max_height_lock.is_satisfied_by(chain_state3, utxo_state3));
+        assert!(!max_height_lock.is_satisfied_by(chain_state3, utxo_state3).unwrap());
     }
 }

units/tests/api.rs

@@ -139,11 +139,13 @@ struct Errors {
     x: locktime::absolute::ConversionError,
     y: locktime::absolute::Height,
     z: locktime::absolute::ParseHeightError,
-    _a: locktime::absolute::ParseTimeError,
+    aa: locktime::absolute::ParseTimeError,
-    _b: locktime::relative::TimeOverflowError,
+    ab: locktime::relative::TimeOverflowError,
-    _e: parse::ParseIntError,
+    ac: locktime::relative::InvalidHeightError,
-    _f: parse::PrefixedHexError,
+    ad: locktime::relative::InvalidTimeError,
-    _g: parse::UnprefixedHexError,
+    ae: parse::ParseIntError,
+    af: parse::PrefixedHexError,
+    ag: parse::UnprefixedHexError,
 }
 
 #[test]