From 02c1cd62918eac214019696c7a32e830e41d5154 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Thu, 15 Dec 2022 14:39:41 +0000 Subject: [PATCH] add some documentation clarifying the locktime ordering shenanigans in #1330 --- bitcoin/CHANGELOG.md | 9 +++++++++ bitcoin/src/blockdata/locktime/absolute.rs | 8 ++++++++ bitcoin/src/blockdata/transaction.rs | 13 +++++++++++++ 3 files changed, 30 insertions(+) diff --git a/bitcoin/CHANGELOG.md b/bitcoin/CHANGELOG.md index 64859f02..099646b7 100644 --- a/bitcoin/CHANGELOG.md +++ b/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 diff --git a/bitcoin/src/blockdata/locktime/absolute.rs b/bitcoin/src/blockdata/locktime/absolute.rs index 81cc8501..34c255f3 100644 --- a/bitcoin/src/blockdata/locktime/absolute.rs +++ b/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) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 2a4b374b..a30a841b 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -580,6 +580,19 @@ impl EncodeSigningDataResult { /// /// 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"))]