From d392cdbd7d1ecc53f4ef26b5557ca3acdffeb1d3 Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Tue, 18 Feb 2025 13:28:11 +1100
Subject: [PATCH] Remove PartialOrd from locktimes

Currently it is possible to write

```rust
if this_locktime < that_locktime {
   do_this();
}
```

with the hope that this code means if a locktime is satisfied by the
value in the other locktime. This is a footgun because locktimes are
incommensurate.

We provide the `is_satisfied_by` API to help users do locktime
comparisons.

Remove the `PartialOrd` implementation from both locktime types.
---
 primitives/src/locktime/absolute.rs | 23 +++--------------------
 primitives/src/locktime/relative.rs | 22 +++-------------------
 2 files changed, 6 insertions(+), 39 deletions(-)

diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs
index da8ef86b7..6d88388db 100644
--- a/primitives/src/locktime/absolute.rs
+++ b/primitives/src/locktime/absolute.rs
@@ -28,7 +28,9 @@ pub use units::locktime::absolute::{ConversionError, Height, ParseHeightError, P
 /// ### Note on ordering
 ///
 /// Locktimes may be height- or time-based, and these metrics are incommensurate; there is no total
-/// ordering on locktimes. We therefore have implemented [`PartialOrd`] but not [`Ord`].
+/// ordering on locktimes. In order to compare locktimes, instead of using `<` or `>` we provide the
+/// [`LockTime::is_satisfied_by`] API.
+///
 /// For [`Transaction`], which has a locktime field, we implement a total ordering to make
 /// it easy to store transactions in sorted data structures, and use the locktime's 32-bit integer
 /// consensus encoding to order it. We also implement [`ordered::ArbitraryOrd`] if the "ordered"
@@ -319,19 +321,6 @@ impl From<Time> for LockTime {
     fn from(t: Time) -> Self { LockTime::Seconds(t) }
 }
 
-impl PartialOrd for LockTime {
-    #[inline]
-    fn partial_cmp(&self, other: &LockTime) -> Option<Ordering> {
-        use LockTime::*;
-
-        match (*self, *other) {
-            (Blocks(ref a), Blocks(ref b)) => a.partial_cmp(b),
-            (Seconds(ref a), Seconds(ref b)) => a.partial_cmp(b),
-            (_, _) => None,
-        }
-    }
-}
-
 impl fmt::Debug for LockTime {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use LockTime::*;
@@ -501,9 +490,6 @@ mod tests {
         assert!(lock_by_height.is_satisfied_by(height_same, time));
         assert!(lock_by_height.is_satisfied_by(height_above, time));
         assert!(!lock_by_height.is_satisfied_by(height_below, time));
-
-        let lock_by_height_above = LockTime::from_consensus(800_000);
-        assert!(lock_by_height < lock_by_height_above)
     }
 
     #[test]
@@ -519,9 +505,6 @@ mod tests {
         assert!(lock_by_time.is_satisfied_by(height, time_same));
         assert!(lock_by_time.is_satisfied_by(height, time_after));
         assert!(!lock_by_time.is_satisfied_by(height, time_before));
-
-        let lock_by_time_after = LockTime::from_consensus(1653282000);
-        assert!(lock_by_time < lock_by_time_after);
     }
 
     #[test]
diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs
index 2cca3889b..2f1bc6072 100644
--- a/primitives/src/locktime/relative.rs
+++ b/primitives/src/locktime/relative.rs
@@ -7,7 +7,7 @@
 
 #[cfg(feature = "ordered")]
 use core::cmp::Ordering;
-use core::{cmp, convert, fmt};
+use core::{convert, fmt};
 
 use crate::Sequence;
 #[cfg(all(doc, feature = "alloc"))]
@@ -25,8 +25,8 @@ pub use units::locktime::relative::{Height, Time, TimeOverflowError};
 /// ### Note on ordering
 ///
 /// Locktimes may be height- or time-based, and these metrics are incommensurate; there is no total
-/// ordering on locktimes. We therefore have implemented [`PartialOrd`] but not [`Ord`]. We also
-/// implement [`ordered::ArbitraryOrd`] if the "ordered" feature is enabled.
+/// ordering on locktimes. In order to compare locktimes, instead of using `<` or `>` we provide the
+/// [`LockTime::is_satisfied_by`] API.
 ///
 /// ### Relevant BIPs
 ///
@@ -352,19 +352,6 @@ impl From<Time> for LockTime {
     fn from(t: Time) -> Self { LockTime::Time(t) }
 }
 
-impl PartialOrd for LockTime {
-    #[inline]
-    fn partial_cmp(&self, other: &LockTime) -> Option<cmp::Ordering> {
-        use LockTime::*;
-
-        match (*self, *other) {
-            (Blocks(ref a), Blocks(ref b)) => a.partial_cmp(b),
-            (Time(ref a), Time(ref b)) => a.partial_cmp(b),
-            (_, _) => None,
-        }
-    }
-}
-
 impl fmt::Display for LockTime {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         use LockTime::*;
@@ -501,9 +488,6 @@ mod tests {
         assert!(!lock_by_height1.is_same_unit(lock_by_time1));
         assert!(lock_by_time1.is_same_unit(lock_by_time2));
         assert!(!lock_by_time1.is_same_unit(lock_by_height1));
-
-        assert!(lock_by_height1 < lock_by_height2);
-        assert!(lock_by_time1 < lock_by_time2);
     }
 
     #[test]