Merge rust-bitcoin/rust-bitcoin#4065: locktimes: Remove `PartialOrd` and `ArbitraryOrd`

4259dab93a Remove rust-ordered dependency (Tobin C. Harding)
d392cdbd7d Remove PartialOrd from locktimes (Tobin C. Harding)

Pull request description:

  These two things are related so we remove them both in the same PR. Please see commit logs for full explanation.

  For more context see discussion below and also:

  - #2500
  - #4002
  - #3881

  Close: #4029

ACKs for top commit:
  Kixunil:
    ACK 4259dab93a
  apoelstra:
    ACK 4259dab93ab52795305db4b889b9151595bcee51; successfully ran local tests

Tree-SHA512: 7526d4faaa9edf8017d2af412c41a33f33d851ad5130c9a745bba86d9c71dc1db7f20d07377aaf3a25fec2c0de79f3ffabc2c538a5a366e415c7a6eaa730153c

Cargo-minimal.lock

@@ -62,7 +62,6 @@ dependencies = [
  "bitcoinconsensus",
  "hex-conservative 0.3.0",
  "hex_lit",
- "ordered",
  "secp256k1",
  "serde",
  "serde_json",
@@ -111,7 +110,6 @@ dependencies = [
  "bitcoin-units",
  "bitcoin_hashes 0.16.0",
  "hex-conservative 0.3.0",
- "ordered",
  "serde",
  "serde_json",
 ]
@@ -254,12 +252,6 @@ dependencies = [
  "libc",
 ]
 
-[[package]]
-name = "ordered"
-version = "0.4.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "79c12388aac4f817eae0359011d67d4072ed84cfc63b0d9a7958ef476fb74bec"
-
 [[package]]
 name = "ppv-lite86"
 version = "0.2.8"

Cargo-recent.lock

@@ -61,7 +61,6 @@ dependencies = [
  "bitcoinconsensus",
  "hex-conservative 0.3.0",
  "hex_lit",
- "ordered",
  "secp256k1",
  "serde",
  "serde_json",
@@ -110,7 +109,6 @@ dependencies = [
  "bitcoin-units",
  "bitcoin_hashes 0.16.0",
  "hex-conservative 0.3.0",
- "ordered",
  "serde",
  "serde_json",
 ]
@@ -262,12 +260,6 @@ dependencies = [
  "libc",
 ]
 
-[[package]]
-name = "ordered"
-version = "0.4.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "79c12388aac4f817eae0359011d67d4072ed84cfc63b0d9a7958ef476fb74bec"
-
 [[package]]
 name = "ppv-lite86"
 version = "0.2.20"

bitcoin/Cargo.toml

@@ -39,7 +39,6 @@ arbitrary = { version = "1.4", optional = true }
 base64 = { version = "0.22.0", optional = true }
 # `bitcoinconsensus` version includes metadata which indicates the version of Core. Use `cargo tree` to see it.
 bitcoinconsensus = { version = "0.106.0", default-features = false, optional = true }
-ordered = { version = "0.4.0", optional = true }
 serde = { version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true }
 
 [dev-dependencies]

bitcoin/contrib/test_vars.sh

@@ -5,10 +5,10 @@
 # shellcheck disable=SC2034
 
 # Test all these features with "std" enabled.
-FEATURES_WITH_STD="rand-std serde secp-recovery bitcoinconsensus base64 ordered arbitrary"
+FEATURES_WITH_STD="rand-std serde secp-recovery bitcoinconsensus base64 arbitrary"
 
 # Test all these features without "std" or "alloc" enabled.
-FEATURES_WITHOUT_STD="rand serde secp-recovery bitcoinconsensus base64 ordered arbitrary"
+FEATURES_WITHOUT_STD="rand serde secp-recovery bitcoinconsensus base64 arbitrary"
 
 # Run these examples.
 EXAMPLES="ecdsa-psbt:std,bitcoinconsensus sign-tx-segwit-v0:rand-std sign-tx-taproot:rand-std taproot-psbt:bitcoinconsensus,rand-std sighash:std"

bitcoin/src/lib.rs

@@ -16,7 +16,6 @@
 //! * `base64` (dependency) - enables encoding of PSBTs and message signatures.
 //! * `bitcoinconsensus` (dependency) - enables validating scripts and transactions.
 //! * `default` - enables `std` and `secp-recovery`.
-//! * `ordered` (dependency) - adds implementations of `ArbitraryOrd` to some structs.
 //! * `rand` (transitive dependency) - makes it more convenient to generate random values.
 //! * `rand-std` - same as `rand` but also enables `std` here and in `secp256k1`.
 //! * `serde` (dependency) - implements `serde`-based serialization and deserialization.
@@ -80,10 +79,6 @@ pub extern crate hex;
 /// Re-export the `bitcoin-io` crate.
 pub extern crate io;
 
-/// Re-export the `ordered` crate.
-#[cfg(feature = "ordered")]
-pub extern crate ordered;
-
 /// Re-export the `rust-secp256k1` crate.
 ///
 /// Rust wrapper library for Pieter Wuille's libsecp256k1. Implements ECDSA and BIP-340 signatures

primitives/Cargo.toml

@@ -28,7 +28,6 @@ internals = { package = "bitcoin-internals", version = "0.4.0" }
 units = { package = "bitcoin-units", version = "0.2.0", default-features = false }
 
 arbitrary = { version = "1.4", optional = true }
-ordered = { version = "0.4.0", optional = true }
 serde = { version = "1.0.103", default-features = false, features = ["derive", "alloc"], optional = true }
 
 [dev-dependencies]

primitives/contrib/test_vars.sh

@@ -5,10 +5,10 @@
 # shellcheck disable=SC2034
 
 # Test these features with "std" enabled.
-FEATURES_WITH_STD="ordered serde arbitrary"
+FEATURES_WITH_STD="serde arbitrary"
 
 # Test these features without "std" enabled.
-FEATURES_WITHOUT_STD="alloc ordered serde arbitrary"
+FEATURES_WITHOUT_STD="alloc serde arbitrary"
 
 # Run these examples.
 EXAMPLES=""

primitives/src/locktime/absolute.rs

@@ -5,7 +5,6 @@
 //! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by
 //! whether `LockTime < LOCKTIME_THRESHOLD`.
 
-use core::cmp::Ordering;
 use core::fmt;
 
 #[cfg(feature = "arbitrary")]
@@ -28,11 +27,12 @@ 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"
+/// consensus encoding to order it.
-/// feature is enabled.
 ///
 /// ### Relevant BIPs
 ///
@@ -319,19 +319,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::*;
@@ -400,20 +387,6 @@ impl<'de> serde::Deserialize<'de> for LockTime {
     }
 }
 
-#[cfg(feature = "ordered")]
-impl ordered::ArbitraryOrd for LockTime {
-    fn arbitrary_cmp(&self, other: &Self) -> Ordering {
-        use LockTime::*;
-
-        match (self, other) {
-            (Blocks(_), Seconds(_)) => Ordering::Less,
-            (Seconds(_), Blocks(_)) => Ordering::Greater,
-            (Blocks(this), Blocks(that)) => this.cmp(that),
-            (Seconds(this), Seconds(that)) => this.cmp(that),
-        }
-    }
-}
-
 #[cfg(feature = "arbitrary")]
 impl<'a> Arbitrary<'a> for LockTime {
     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
@@ -501,9 +474,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 +489,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]

primitives/src/locktime/relative.rs

@@ -5,9 +5,7 @@
 //! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by
 //! whether bit 22 of the `u32` consensus value is set.
 
-#[cfg(feature = "ordered")]
+use core::{convert, fmt};
-use core::cmp::Ordering;
-use core::{cmp, convert, fmt};
 
 use crate::Sequence;
 #[cfg(all(doc, feature = "alloc"))]
@@ -25,8 +23,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
+/// 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
 ///
@@ -352,19 +350,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::*;
@@ -383,20 +368,6 @@ 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> {
@@ -501,9 +472,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]