Merge rust-bitcoin/rust-bitcoin#1475: add some documentation clarifying the locktime ordering shenanigans in #1330

02c1cd6291 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra)

Pull request description:

  Updates the CHANGELOG and also the doccomment on `Transaction`.

ACKs for top commit:
  tcharding:
    ACK 02c1cd6291
  Kixunil:
    ACK 02c1cd6291
  sanket1729:
    ACK 02c1cd6291

Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd

bitcoin/CHANGELOG.md

@@ -1,3 +1,12 @@
+# 0.30 - 2023-XX-XX "The First Crate-Smashing Release"
+
+* Drop `PackedLockTime`, [replace with richer `LockTime` everywhere](https://github.com/rust-bitcoin/rust-bitcoin/pull/1330)
+  Be aware that `LockTime` does not have an `Ord` implementation, so users who need a
+  total ordering on locktimes will be forced to wrap this type. In `Transaction`, which
+  contains a `LockTime` but is `Ord`, we have manually sorted the locktimes based on
+  their consensus encoding. This ordering is somewhat arbitrary -- there is no total
+  ordering on locktimes since they may be measured in either blocks or seconds.
+
 # 0.29 - 2022-07-20 "Edition 2018 Release"
 
 As promised, this is our quick release to bring on edition 2018 by increasing our MSRV to Rust

bitcoin/src/blockdata/locktime/absolute.rs

@@ -43,6 +43,14 @@ pub const LOCK_TIME_THRESHOLD: u32 = 500_000_000;
 /// Used for transaction lock time (`nLockTime` in Bitcoin Core and [`crate::Transaction::lock_time`]
 /// in this library) and also for the argument to opcode 'OP_CHECKLOCKTIMEVERIFY`.
 ///
+/// ### 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`].
+/// 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.
+///
 /// ### Relevant BIPs
 ///
 /// * [BIP-65 OP_CHECKLOCKTIMEVERIFY](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki)

bitcoin/src/blockdata/transaction.rs

@@ -627,6 +627,19 @@ impl<E> EncodeSigningDataResult<E> {
 ///
 /// We therefore deviate from the spec by always using the Segwit witness encoding
 /// for 0-input transactions, which results in unambiguously parseable transactions.
+///
+/// ### A note on ordering
+///
+/// This type implements `Ord`, even though it contains a locktime, which is not
+/// itself `Ord`. This was done to simplify applications that may need to hold
+/// transactions inside a sorted container. We have ordered the locktimes based
+/// on their representation as a `u32`, which is not a semantically meaningful
+/// order, and therefore the ordering on `Transaction` itself is not semantically
+/// meaningful either.
+///
+/// The ordering is, however, consistent with the ordering present in this library
+/// before this change, so users should not notice any breakage (here) when
+/// transitioning from 0.29 to 0.30.
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]