milksad
/
rust-bitcoin-unsafe-fast
14
0
Fork 0
Code Issues Pull Requests Activity

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.
This commit is contained in:
Tobin C. Harding 2025-02-18 13:28:11 +11:00
parent 4712e81b70
commit d392cdbd7d
No known key found for this signature in database
GPG Key ID: 40BF9E4C269D6607
2 changed files with 6 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
primitives/src/locktime/absolute.rs
View File

@ -28,7 +28,9 @@ pub use units::locktime::absolute::{ConversionError, Height, ParseHeightError, P
/// ### Note on ordering /// ### Note on ordering
/// ///
/// Locktimes may be height- or time-based, and these metrics are incommensurate; there is no total /// 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 /// 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 /// 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" /// 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) } 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 { impl fmt::Debug for LockTime {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use LockTime::*; 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_same, time));
assert!(lock_by_height.is_satisfied_by(height_above, time)); assert!(lock_by_height.is_satisfied_by(height_above, time));
assert!(!lock_by_height.is_satisfied_by(height_below, 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] #[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_same));
assert!(lock_by_time.is_satisfied_by(height, time_after)); assert!(lock_by_time.is_satisfied_by(height, time_after));
assert!(!lock_by_time.is_satisfied_by(height, time_before)); 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] #[test]

22
primitives/src/locktime/relative.rs
View File

@ -7,7 +7,7 @@
#[cfg(feature = "ordered")] #[cfg(feature = "ordered")]
use core::cmp::Ordering; use core::cmp::Ordering;
use core::{cmp, convert, fmt}; use core::{convert, fmt};
use crate::Sequence; use crate::Sequence;
#[cfg(all(doc, feature = "alloc"))] #[cfg(all(doc, feature = "alloc"))]
@ -25,8 +25,8 @@ pub use units::locktime::relative::{Height, Time, TimeOverflowError};
/// ### Note on ordering /// ### Note on ordering
/// ///
/// Locktimes may be height- or time-based, and these metrics are incommensurate; there is no total /// 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 /// ordering on locktimes. In order to compare locktimes, instead of using `<` or `>` we provide the
/// implement [`ordered::ArbitraryOrd`] if the "ordered" feature is enabled. /// [`LockTime::is_satisfied_by`] API.
/// ///
/// ### Relevant BIPs /// ### Relevant BIPs
/// ///
@ -352,19 +352,6 @@ impl From<Time> for LockTime {
fn from(t: Time) -> Self { LockTime::Time(t) } 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 { impl fmt::Display for LockTime {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use LockTime::*; use LockTime::*;
@ -501,9 +488,6 @@ mod tests {
assert!(!lock_by_height1.is_same_unit(lock_by_time1)); 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_time2));
assert!(!lock_by_time1.is_same_unit(lock_by_height1)); 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] #[test]