Skip to content

Commit

Permalink
Implement ArbitraryOrd for relative::LockTime
Browse files Browse the repository at this point in the history
TL;DR As we do for `absolute::LockTime` and for the same reasons;
implement `ArbitraryOrd` for `relative::LockTime`.

locktimes do not have a semantic ordering if they differ (blocks, time)
so we do not derive `Ord` however it is useful for downstream to be able
to order structs that contain lock times. This is exactly what the
`ArbitraryOrd` trait is for.

Update the rustdocs in `relative` and mirror the docs changes in
`absolute`.

Fix: #2566
  • Loading branch information
tcharding committed Mar 25, 2024
1 parent c211e7b commit 3520f55
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 5 deletions.
7 changes: 4 additions & 3 deletions bitcoin/src/blockdata/locktime/absolute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ pub use units::locktime::absolute::{
///
/// ### Note on ordering
///
/// Because 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`].
/// 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`].
/// For [`crate::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.
/// consensus encoding to order it. We also implement [`ordered::ArbitraryOrd`] if the "ordered"
/// feature is enabled.
///
/// ### Relevant BIPs
///
Expand Down
21 changes: 19 additions & 2 deletions bitcoin/src/blockdata/locktime/relative.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
//! whether bit 22 of the `u32` consensus value is set.
//!

#[cfg(feature = "ordered")]
use core::cmp::Ordering;
use core::{cmp, convert, fmt};

#[cfg(all(test, mutate))]
Expand All @@ -26,8 +28,9 @@ pub use units::locktime::relative::{Height, Time, TimeOverflowError};
///
/// ### Note on ordering
///
/// Because 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`].
/// 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.
///
/// ### Relevant BIPs
///
Expand Down Expand Up @@ -338,6 +341,20 @@ impl fmt::Display for LockTime {
}
}

#[cfg(feature = "ordered")]
impl ordered::ArbitraryOrd for LockTime {
fn arbitrary_cmp(&self, other: &Self) -> Ordering {
use LockTime::*;

match (self, other) {
(Blocks(_), Time(_)) => Ordering::Less,
(Time(_), Blocks(_)) => Ordering::Greater,
(Blocks(this), Blocks(that)) => this.cmp(that),
(Time(this), Time(that)) => this.cmp(that),
}
}
}

impl convert::TryFrom<Sequence> for LockTime {
type Error = DisabledLockTimeError;
fn try_from(seq: Sequence) -> Result<LockTime, DisabledLockTimeError> {
Expand Down

0 comments on commit 3520f55

Please sign in to comment.