From a2ff8ddbbbb45492c967c27c89f89205e77cdf0b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 8 May 2025 11:10:57 +1000 Subject: [PATCH 1/8] Improve relative::LockTime is_satisfied_by_{height, time} We recently improved the relative locktime function `is_satisfied_by` by adding mined at and chain tip. We can now do the same for the height/time satisfaction functions. Note I believe these functions should still be provided because a user may for some reason have either blocktime data or height data and not have the other. Requires some work to the errors, elect to just remove the original field that held the function argument. For now remove the examples in rustdocs, we can circle back to these once the dust settles. --- primitives/src/locktime/relative.rs | 96 ++++++++++------------------- 1 file changed, 31 insertions(+), 65 deletions(-) diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs index 2c9668d1c..eee8f0baf 100644 --- a/primitives/src/locktime/relative.rs +++ b/primitives/src/locktime/relative.rs @@ -332,61 +332,41 @@ impl LockTime { } } - /// Returns true if this [`relative::LockTime`] is satisfied by [`NumberOfBlocks`]. + /// Returns true if an output with this locktime can be spent in the next block. /// /// # Errors /// /// Returns an error if this lock is not lock-by-height. - /// - /// # Examples - /// - /// ```rust - /// # use bitcoin_primitives::Sequence; - /// # use bitcoin_primitives::relative; - /// - /// let required_height: u16 = 100; - /// let lock = Sequence::from_height(required_height).to_relative_lock_time().expect("valid height"); - /// assert!(lock.is_satisfied_by_height(relative::NumberOfBlocks::from(required_height + 1)).expect("a height")); - /// ``` #[inline] pub fn is_satisfied_by_height( self, - height: NumberOfBlocks, + chain_tip: BlockHeight, + utxo_mined_at: BlockHeight, ) -> Result { use LockTime as L; match self { - L::Blocks(required_height) => Ok(required_height <= height), - L::Time(time) => Err(IncompatibleHeightError { height, time }), + L::Blocks(blocks) => Ok(blocks.is_satisfied_by(chain_tip, utxo_mined_at)), + L::Time(time) => Err(IncompatibleHeightError { time }), } } - /// Returns true if this [`relative::LockTime`] is satisfied by [`Time`]. + /// Returns true if an output with this locktime can be spent in the next block. /// /// # Errors /// /// Returns an error if this lock is not lock-by-time. - /// - /// # Examples - /// - /// ```rust - /// # use bitcoin_primitives::Sequence; - /// # use bitcoin_primitives::relative; - /// - /// let intervals: u16 = 70; // approx 10 hours; - /// let lock = Sequence::from_512_second_intervals(intervals).to_relative_lock_time().expect("valid time"); - /// assert!(lock.is_satisfied_by_time(relative::Time::from_512_second_intervals(intervals + 10)).expect("a time")); - /// ``` #[inline] pub fn is_satisfied_by_time( self, - time: NumberOf512Seconds, + chain_tip: BlockMtp, + utxo_mined_at: BlockMtp, ) -> Result { use LockTime as L; match self { - L::Time(ref t) => Ok(t.to_512_second_intervals() <= time.to_512_second_intervals()), - L::Blocks(height) => Err(IncompatibleTimeError { time, height }), + L::Time(time) => Ok(time.is_satisfied_by(chain_tip, utxo_mined_at)), + L::Blocks(blocks) => Err(IncompatibleTimeError { blocks }), } } } @@ -457,16 +437,11 @@ impl std::error::Error for DisabledLockTimeError {} /// Tried to satisfy a lock-by-blocktime lock using a height value. #[derive(Debug, Clone, PartialEq, Eq)] pub struct IncompatibleHeightError { - /// Attempted to satisfy a lock-by-blocktime lock with this height. - height: NumberOfBlocks, /// The inner time value of the lock-by-blocktime lock. time: NumberOf512Seconds, } impl IncompatibleHeightError { - /// Returns the height that was erroneously used to try and satisfy a lock-by-blocktime lock. - pub fn incompatible(&self) -> NumberOfBlocks { self.height } - /// Returns the time value of the lock-by-blocktime lock. pub fn expected(&self) -> NumberOf512Seconds { self.time } } @@ -474,11 +449,7 @@ impl IncompatibleHeightError { impl fmt::Display for IncompatibleHeightError { #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!( - f, - "tried to satisfy a lock-by-blocktime lock {} with height: {}", - self.time, self.height - ) + write!(f, "tried to satisfy a lock-by-blocktime lock {} by height", self.time,) } } @@ -488,28 +459,19 @@ impl std::error::Error for IncompatibleHeightError {} /// Tried to satisfy a lock-by-blockheight lock using a time value. #[derive(Debug, Clone, PartialEq, Eq)] pub struct IncompatibleTimeError { - /// Attempted to satisfy a lock-by-blockheight lock with this time. - time: NumberOf512Seconds, - /// The inner height value of the lock-by-blockheight lock. - height: NumberOfBlocks, + /// The inner value of the lock-by-blockheight lock. + blocks: NumberOfBlocks, } impl IncompatibleTimeError { - /// Returns the time that was erroneously used to try and satisfy a lock-by-blockheight lock. - pub fn incompatible(&self) -> NumberOf512Seconds { self.time } - /// Returns the height value of the lock-by-blockheight lock. - pub fn expected(&self) -> NumberOfBlocks { self.height } + pub fn expected(&self) -> NumberOfBlocks { self.blocks } } impl fmt::Display for IncompatibleTimeError { #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!( - f, - "tried to satisfy a lock-by-blockheight lock {} with time: {}", - self.height, self.time - ) + write!(f, "tried to satisfy a lock-by-blockheight lock {} by time", self.blocks,) } } @@ -664,25 +626,29 @@ mod tests { #[test] fn incompatible_height_error() { - let height = NumberOfBlocks::from(10); - let time = NumberOf512Seconds::from_512_second_intervals(70); - let lock_by_time = LockTime::from(time); - let err = lock_by_time.is_satisfied_by_height(height).unwrap_err(); + // This is an error test these values are not used in the error path. + let mined_at = BlockHeight::from_u32(700_000); + let chain_tip = BlockHeight::from_u32(800_000); - assert_eq!(err.incompatible(), height); - assert_eq!(err.expected(), time); + let lock_by_time = LockTime::from_512_second_intervals(70); // Arbitrary value. + let err = lock_by_time.is_satisfied_by_height(chain_tip, mined_at).unwrap_err(); + + let expected_time = NumberOf512Seconds::from_512_second_intervals(70); + assert_eq!(err.expected(), expected_time); assert!(!format!("{}", err).is_empty()); } #[test] fn incompatible_time_error() { - let height = NumberOfBlocks::from(10); - let time = NumberOf512Seconds::from_512_second_intervals(70); - let lock_by_height = LockTime::from(height); - let err = lock_by_height.is_satisfied_by_time(time).unwrap_err(); + // This is an error test these values are not used in the error path. + let mined_at = BlockMtp::from_u32(1_234_567_890); + let chain_tip = BlockMtp::from_u32(1_600_000_000); - assert_eq!(err.incompatible(), time); - assert_eq!(err.expected(), height); + let lock_by_height = LockTime::from_height(10); // Arbitrary value. + let err = lock_by_height.is_satisfied_by_time(chain_tip, mined_at).unwrap_err(); + + let expected_height = NumberOfBlocks::from(10); + assert_eq!(err.expected(), expected_height); assert!(!format!("{}", err).is_empty()); } From 3ffdc54ca5b8e38722a3b42ab69b49cadd586af3 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:17:31 +1000 Subject: [PATCH 2/8] Fix off-by-one bug in relative locktime Define 'is satisfied by' - this is a classic off-by-one problem, if a relative lock is satisfied does that mean it can go in this block or the next? Its most useful if it means 'it can go in the next' and this is how relative height and MTP are used in Core. Ramifications: - When checking a time based lock we check against the chain tip MTP, then when Core verifies a block with the output in it it uses the previous block (and this is still the chain tip). - When checking a height base lock we check against chain tip height + 1 because Core checks against height of the block being verified. Additionally we currently have a false negative in the satisfaction functions when the `crate` type (height or MTP) is to big to fit in a u16 - in this case we should return true not false because a value too big definitely is > the lock value. One final API paper cut - currently if the caller puts the args in the wrong order they get a false negative instead of an error. Fix all this by making the satisfaction functions return errors, update the docs to explicitly define 'satisfaction'. For now remove the examples in rustdocs, we can circle back to these once the dust settles. API test of Errors: Some of the errors are being 'API tested' tested in `primitives` but they should be being done in `units/tests/api.rs` - put all the new errors in the correct places. --- bitcoin/src/blockdata/mod.rs | 2 +- primitives/src/locktime/relative.rs | 262 +++++++++++++++------------- primitives/tests/api.rs | 16 +- units/src/locktime/relative.rs | 117 ++++++++----- units/tests/api.rs | 12 +- 5 files changed, 228 insertions(+), 181 deletions(-) diff --git a/bitcoin/src/blockdata/mod.rs b/bitcoin/src/blockdata/mod.rs index 9d904a1e1..13ffc6059 100644 --- a/bitcoin/src/blockdata/mod.rs +++ b/bitcoin/src/blockdata/mod.rs @@ -71,7 +71,7 @@ pub mod locktime { /// Re-export everything from the `primitives::locktime::relative` module. pub use primitives::locktime::relative::{ - DisabledLockTimeError, IncompatibleHeightError, IncompatibleTimeError, LockTime, + DisabledLockTimeError, InvalidHeightError, InvalidTimeError, LockTime, NumberOf512Seconds, NumberOfBlocks, TimeOverflowError, }; diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs index eee8f0baf..343ebd682 100644 --- a/primitives/src/locktime/relative.rs +++ b/primitives/src/locktime/relative.rs @@ -7,13 +7,15 @@ use core::{convert, fmt}; +use internals::write_err; + use crate::Sequence; #[cfg(all(doc, feature = "alloc"))] use crate::{relative, TxIn}; #[rustfmt::skip] // Keep public re-exports separate. #[doc(inline)] -pub use units::locktime::relative::{NumberOfBlocks, NumberOf512Seconds, TimeOverflowError}; +pub use units::locktime::relative::{NumberOfBlocks, NumberOf512Seconds, TimeOverflowError, InvalidHeightError, InvalidTimeError}; use units::{BlockHeight, BlockMtp}; #[deprecated(since = "TBD", note = "use `NumberOfBlocks` instead")] @@ -39,40 +41,6 @@ pub type Time = NumberOf512Seconds; /// /// * [BIP 68 Relative lock-time using consensus-enforced sequence numbers](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) /// * [BIP 112 CHECKSEQUENCEVERIFY](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) -/// -/// # Examples -/// -/// ``` -/// use bitcoin_primitives::relative; -/// use bitcoin_primitives::{BlockHeight, BlockMtp, BlockTime}; -/// let lock_by_height = relative::LockTime::from_height(144); // 144 blocks, approx 24h. -/// assert!(lock_by_height.is_block_height()); -/// -/// let lock_by_time = relative::LockTime::from_512_second_intervals(168); // 168 time intervals, approx 24h. -/// assert!(lock_by_time.is_block_time()); -/// -/// fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] { -/// let mut timestamps = [BlockTime::from_u32(0); 11]; -/// for (i, ts) in timestamps.iter_mut().enumerate() { -/// *ts = BlockTime::from_u32(start.saturating_sub((step * i as u16).into())); -/// } -/// timestamps -/// } -/// // time extracted from BlockHeader -/// let timestamps: [BlockTime; 11] = generate_timestamps(1_600_000_000, 200); -/// let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200); -/// -/// let current_height = BlockHeight::from(100); -/// let current_mtp = BlockMtp::new(timestamps); -/// -/// let utxo_height = BlockHeight::from(80); -/// let utxo_mtp = BlockMtp::new(utxo_timestamps); -/// -/// let locktime = relative::LockTime::Time(relative::NumberOf512Seconds::from_512_second_intervals(10)); -/// -/// // Check if locktime is satisfied -/// assert!(locktime.is_satisfied_by(current_height, current_mtp, utxo_height, utxo_mtp)); -/// ``` #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub enum LockTime { @@ -221,45 +189,27 @@ impl LockTime { pub const fn is_block_time(self) -> bool { !self.is_block_height() } /// Returns true if this [`relative::LockTime`] is satisfied by the given chain state. - /// # Examples /// - /// ```rust - /// # use bitcoin_primitives::relative::Time; - /// # use bitcoin_primitives::{BlockHeight, BlockMtp, BlockTime}; - /// # use bitcoin_primitives::relative::LockTime; + /// If this function returns true then an output with this locktime can be spent in the next + /// block. /// - /// fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] { - /// let mut timestamps = [BlockTime::from_u32(0); 11]; - /// for (i, ts) in timestamps.iter_mut().enumerate() { - /// *ts = BlockTime::from_u32(start.saturating_sub((step * i as u16).into())); - /// } - /// timestamps - /// } - /// // time extracted from BlockHeader - /// let timestamps: [BlockTime; 11] = generate_timestamps(1_600_000_000, 200); - /// let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200); + /// # Errors /// - /// let current_height = BlockHeight::from_u32(100); - /// let current_mtp = BlockMtp::new(timestamps); - /// let utxo_height = BlockHeight::from_u32(80); - /// let utxo_mtp = BlockMtp::new(utxo_timestamps); - /// - /// let locktime = LockTime::Time(Time::from_512_second_intervals(10)); - /// - /// // Check if locktime is satisfied - /// assert!(locktime.is_satisfied_by(current_height, current_mtp, utxo_height, utxo_mtp)); - /// ``` + /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up. pub fn is_satisfied_by( self, chain_tip_height: BlockHeight, chain_tip_mtp: BlockMtp, utxo_mined_at_height: BlockHeight, utxo_mined_at_mtp: BlockMtp, - ) -> bool { + ) -> Result { match self { - LockTime::Blocks(blocks) => - blocks.is_satisfied_by(chain_tip_height, utxo_mined_at_height), - LockTime::Time(time) => time.is_satisfied_by(chain_tip_mtp, utxo_mined_at_mtp), + LockTime::Blocks(blocks) => blocks + .is_satisfied_by(chain_tip_height, utxo_mined_at_height) + .map_err(IsSatisfiedByError::Blocks), + LockTime::Time(time) => time + .is_satisfied_by(chain_tip_mtp, utxo_mined_at_mtp) + .map_err(IsSatisfiedByError::Time), } } @@ -334,6 +284,9 @@ impl LockTime { /// Returns true if an output with this locktime can be spent in the next block. /// + /// If this function returns true then an output with this locktime can be spent in the next + /// block. + /// /// # Errors /// /// Returns an error if this lock is not lock-by-height. @@ -342,17 +295,22 @@ impl LockTime { self, chain_tip: BlockHeight, utxo_mined_at: BlockHeight, - ) -> Result { + ) -> Result { use LockTime as L; match self { - L::Blocks(blocks) => Ok(blocks.is_satisfied_by(chain_tip, utxo_mined_at)), - L::Time(time) => Err(IncompatibleHeightError { time }), + L::Blocks(blocks) => blocks + .is_satisfied_by(chain_tip, utxo_mined_at) + .map_err(IsSatisfiedByHeightError::Satisfaction), + L::Time(time) => Err(IsSatisfiedByHeightError::Incompatible(time)), } } /// Returns true if an output with this locktime can be spent in the next block. /// + /// If this function returns true then an output with this locktime can be spent in the next + /// block. + /// /// # Errors /// /// Returns an error if this lock is not lock-by-time. @@ -361,12 +319,14 @@ impl LockTime { self, chain_tip: BlockMtp, utxo_mined_at: BlockMtp, - ) -> Result { + ) -> Result { use LockTime as L; match self { - L::Time(time) => Ok(time.is_satisfied_by(chain_tip, utxo_mined_at)), - L::Blocks(blocks) => Err(IncompatibleTimeError { blocks }), + L::Time(time) => time + .is_satisfied_by(chain_tip, utxo_mined_at) + .map_err(IsSatisfiedByTimeError::Satisfaction), + L::Blocks(blocks) => Err(IsSatisfiedByTimeError::Incompatible(blocks)), } } } @@ -434,49 +394,108 @@ impl fmt::Display for DisabledLockTimeError { #[cfg(feature = "std")] impl std::error::Error for DisabledLockTimeError {} -/// Tried to satisfy a lock-by-blocktime lock using a height value. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct IncompatibleHeightError { - /// The inner time value of the lock-by-blocktime lock. - time: NumberOf512Seconds, +/// Error returned when attempting to satisfy lock fails. +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum IsSatisfiedByError { + /// Error when attempting to satisfy lock by height. + Blocks(InvalidHeightError), + /// Error when attempting to satisfy lock by time. + Time(InvalidTimeError), } -impl IncompatibleHeightError { - /// Returns the time value of the lock-by-blocktime lock. - pub fn expected(&self) -> NumberOf512Seconds { self.time } -} - -impl fmt::Display for IncompatibleHeightError { +impl fmt::Display for IsSatisfiedByError { #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "tried to satisfy a lock-by-blocktime lock {} by height", self.time,) + use IsSatisfiedByError as E; + + match *self { + E::Blocks(ref e) => write_err!(f, "blocks"; e), + E::Time(ref e) => write_err!(f, "time"; e), + } } } #[cfg(feature = "std")] -impl std::error::Error for IncompatibleHeightError {} +impl std::error::Error for IsSatisfiedByError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use IsSatisfiedByError as E; -/// Tried to satisfy a lock-by-blockheight lock using a time value. + match *self { + E::Blocks(ref e) => Some(e), + E::Time(ref e) => Some(e), + } + } +} + +/// Error returned when `is_satisfied_by_height` fails. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct IncompatibleTimeError { - /// The inner value of the lock-by-blockheight lock. - blocks: NumberOfBlocks, +pub enum IsSatisfiedByHeightError { + /// Satisfaction of the lock height value failed. + Satisfaction(InvalidHeightError), + /// Tried to satisfy a lock-by-height locktime using seconds. + // TODO: Hide inner value in a new struct error type. + Incompatible(NumberOf512Seconds), } -impl IncompatibleTimeError { - /// Returns the height value of the lock-by-blockheight lock. - pub fn expected(&self) -> NumberOfBlocks { self.blocks } -} - -impl fmt::Display for IncompatibleTimeError { +impl fmt::Display for IsSatisfiedByHeightError { #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "tried to satisfy a lock-by-blockheight lock {} by time", self.blocks,) + use IsSatisfiedByHeightError as E; + + match *self { + E::Satisfaction(ref e) => write_err!(f, "satisfaction"; e), + E::Incompatible(time) => + write!(f, "tried to satisfy a lock-by-height locktime using seconds {}", time), + } } } #[cfg(feature = "std")] -impl std::error::Error for IncompatibleTimeError {} +impl std::error::Error for IsSatisfiedByHeightError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use IsSatisfiedByHeightError as E; + + match *self { + E::Satisfaction(ref e) => Some(e), + E::Incompatible(_) => None, + } + } +} + +/// Error returned when `is_satisfied_by_time` fails. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum IsSatisfiedByTimeError { + /// Satisfaction of the lock time value failed. + Satisfaction(InvalidTimeError), + /// Tried to satisfy a lock-by-time locktime using number of blocks. + // TODO: Hide inner value in a new struct error type. + Incompatible(NumberOfBlocks), +} + +impl fmt::Display for IsSatisfiedByTimeError { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use IsSatisfiedByTimeError as E; + + match *self { + E::Satisfaction(ref e) => write_err!(f, "satisfaction"; e), + E::Incompatible(blocks) => + write!(f, "tried to satisfy a lock-by-height locktime using blocks {}", blocks), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for IsSatisfiedByTimeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use IsSatisfiedByTimeError as E; + + match *self { + E::Satisfaction(ref e) => Some(e), + E::Incompatible(_) => None, + } + } +} #[cfg(test)] mod tests { @@ -634,7 +653,7 @@ mod tests { let err = lock_by_time.is_satisfied_by_height(chain_tip, mined_at).unwrap_err(); let expected_time = NumberOf512Seconds::from_512_second_intervals(70); - assert_eq!(err.expected(), expected_time); + assert_eq!(err, IsSatisfiedByHeightError::Incompatible(expected_time)); assert!(!format!("{}", err).is_empty()); } @@ -648,7 +667,7 @@ mod tests { let err = lock_by_height.is_satisfied_by_time(chain_tip, mined_at).unwrap_err(); let expected_height = NumberOfBlocks::from(10); - assert_eq!(err.expected(), expected_height); + assert_eq!(err, IsSatisfiedByTimeError::Incompatible(expected_height)); assert!(!format!("{}", err).is_empty()); } @@ -671,49 +690,46 @@ mod tests { let utxo_mtp = BlockMtp::new(utxo_timestamps); let lock1 = LockTime::Blocks(NumberOfBlocks::from(10)); - assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap()); let lock2 = LockTime::Blocks(NumberOfBlocks::from(21)); - assert!(!lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap()); let lock3 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(10)); - assert!(lock3.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(lock3.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap()); let lock4 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(20000)); - assert!(!lock4.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(!lock4.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap()); - assert!(LockTime::ZERO.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); - assert!(LockTime::from_512_second_intervals(0).is_satisfied_by( - chain_height, - chain_mtp, - utxo_height, - utxo_mtp - )); + assert!(LockTime::ZERO + .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp) + .unwrap()); + assert!(LockTime::from_512_second_intervals(0) + .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp) + .unwrap()); let lock6 = LockTime::from_seconds_floor(5000).unwrap(); - assert!(lock6.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(lock6.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap()); let max_height_lock = LockTime::Blocks(NumberOfBlocks::MAX); - assert!(!max_height_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(!max_height_lock + .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp) + .unwrap()); let max_time_lock = LockTime::Time(NumberOf512Seconds::MAX); - assert!(!max_time_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp)); + assert!(!max_time_lock + .is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp) + .unwrap()); let max_chain_height = BlockHeight::from_u32(u32::MAX); let max_chain_mtp = BlockMtp::new(generate_timestamps(u32::MAX, 100)); let max_utxo_height = BlockHeight::MAX; let max_utxo_mtp = max_chain_mtp; - assert!(!max_height_lock.is_satisfied_by( - max_chain_height, - max_chain_mtp, - max_utxo_height, - max_utxo_mtp - )); - assert!(!max_time_lock.is_satisfied_by( - max_chain_height, - max_chain_mtp, - max_utxo_height, - max_utxo_mtp - )); + assert!(!max_height_lock + .is_satisfied_by(max_chain_height, max_chain_mtp, max_utxo_height, max_utxo_mtp) + .unwrap()); + assert!(!max_time_lock + .is_satisfied_by(max_chain_height, max_chain_mtp, max_utxo_height, max_utxo_mtp) + .unwrap()); } } diff --git a/primitives/tests/api.rs b/primitives/tests/api.rs index 913089cdb..b03a7611b 100644 --- a/primitives/tests/api.rs +++ b/primitives/tests/api.rs @@ -163,14 +163,12 @@ struct Default { #[derive(Debug, Clone, PartialEq, Eq)] // All public types implement Debug (C-DEBUG). struct Errors { a: transaction::ParseOutPointError, - b: relative::IncompatibleHeightError, - c: relative::IncompatibleTimeError, - d: relative::IncompatibleHeightError, - e: relative::IncompatibleTimeError, - f: relative::DisabledLockTimeError, - g: relative::DisabledLockTimeError, - h: script::RedeemScriptSizeError, - i: script::WitnessScriptSizeError, + b: relative::DisabledLockTimeError, + c: relative::IsSatisfiedByError, + d: relative::IsSatisfiedByHeightError, + e: relative::IsSatisfiedByTimeError, + f: script::RedeemScriptSizeError, + g: script::WitnessScriptSizeError, } #[test] @@ -212,7 +210,7 @@ fn api_can_use_types_from_crate_root() { #[test] fn api_can_use_all_types_from_module_locktime() { use bitcoin_primitives::locktime::relative::{ - DisabledLockTimeError, IncompatibleHeightError, IncompatibleTimeError, LockTime, + DisabledLockTimeError, InvalidHeightError, InvalidTimeError, LockTime, }; use bitcoin_primitives::locktime::{absolute, relative}; } diff --git a/units/src/locktime/relative.rs b/units/src/locktime/relative.rs index 6bd5e6c13..1fb3db1db 100644 --- a/units/src/locktime/relative.rs +++ b/units/src/locktime/relative.rs @@ -55,29 +55,27 @@ impl NumberOfBlocks { self.0 as u32 // cast safety: u32 is wider than u16 on all architectures } - /// Determines whether a relative‐height locktime has matured, taking into account - /// both the chain tip and the height at which the UTXO was confirmed. + /// Returns true if an output locked by height can be spent in the next block. /// - /// If you have two height intervals `x` and `y`, and want to know whether `x` - /// is satisfied by `y`, use `x >= y`. + /// # Errors /// - /// # Parameters - /// - `self` – the relative block‐height delay (`h`) required after confirmation. - /// - `chain_tip` – the height of the current chain tip - /// - `utxo_mined_at` – the height of the UTXO’s confirmation block - /// - /// # Returns - /// - `true` if a UTXO locked by `self` can be spent in a block after `chain_tip`. - /// - `false` if the UTXO is still locked at `chain_tip`. + /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up. pub fn is_satisfied_by( self, chain_tip: crate::BlockHeight, utxo_mined_at: crate::BlockHeight, - ) -> bool { - chain_tip - .checked_sub(utxo_mined_at) - .and_then(|diff: crate::BlockHeightInterval| diff.try_into().ok()) - .map_or(false, |diff: Self| diff >= self) + ) -> Result { + match chain_tip.checked_sub(utxo_mined_at) { + Some(diff) => { + if diff.to_u32() == u32::MAX { + // Weird but ok none the less - protects against overflow below. + return Ok(true); + } + // +1 because the next block will have height 1 higher than `chain_tip`. + Ok(u32::from(self.to_height()) <= diff.to_u32() + 1) + } + None => Err(InvalidHeightError { chain_tip, utxo_mined_at }), + } } } @@ -181,29 +179,24 @@ impl NumberOf512Seconds { (1u32 << 22) | self.0 as u32 // cast safety: u32 is wider than u16 on all architectures } - /// Determines whether a relative‑time lock has matured, taking into account both - /// the UTXO’s Median Time Past at confirmation and the required delay. + /// Returns true if an output locked by time can be spent in the next block. /// - /// If you have two MTP intervals `x` and `y`, and want to know whether `x` - /// is satisfied by `y`, use `x >= y`. + /// # Errors /// - /// # Parameters - /// - `self` – the relative time delay (`t`) in 512‑second intervals. - /// - `chain_tip` – the MTP of the current chain tip - /// - `utxo_mined_at` – the MTP of the UTXO’s confirmation block - /// - /// # Returns - /// - `true` if the relative‐time lock has expired by the tip’s MTP - /// - `false` if the lock has not yet expired by the tip’s MTP + /// If `chain_tip` as not _after_ `utxo_mined_at` i.e., if you get the args mixed up. pub fn is_satisfied_by( self, chain_tip: crate::BlockMtp, utxo_mined_at: crate::BlockMtp, - ) -> bool { - chain_tip - .checked_sub(utxo_mined_at) - .and_then(|diff: crate::BlockMtpInterval| diff.to_relative_mtp_interval_floor().ok()) - .map_or(false, |diff: Self| diff >= self) + ) -> Result { + match chain_tip.checked_sub(utxo_mined_at) { + Some(diff) => { + // The locktime check in Core during block validation uses the MTP of the previous + // block - which is `chain_tip` here. + Ok(self.to_seconds() <= diff.to_u32()) + } + None => Err(InvalidTimeError { chain_tip, utxo_mined_at }), + } } } @@ -246,6 +239,44 @@ impl fmt::Display for TimeOverflowError { #[cfg(feature = "std")] impl std::error::Error for TimeOverflowError {} +/// Error returned when `NumberOfBlocks::is_satisfied_by` is incorrectly called. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidHeightError { + /// The `chain_tip` argument. + pub(crate) chain_tip: crate::BlockHeight, + /// The `utxo_mined_at` argument. + pub(crate) utxo_mined_at: crate::BlockHeight, +} + +impl fmt::Display for InvalidHeightError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "is_satisfied_by arguments invalid (probably the wrong way around) chain_tip: {} utxo_mined_at: {}", self.chain_tip, self.utxo_mined_at + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidHeightError {} + +/// Error returned when `NumberOf512Seconds::is_satisfied_by` is incorrectly called. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidTimeError { + /// The `chain_tip` argument. + pub(crate) chain_tip: crate::BlockMtp, + /// The `utxo_mined_at` argument. + pub(crate) utxo_mined_at: crate::BlockMtp, +} + +impl fmt::Display for InvalidTimeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "is_satisfied_by arguments invalid (probably the wrong way around) chain_tip: {} utxo_mined_at: {}", self.chain_tip, self.utxo_mined_at + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidTimeError {} + #[cfg(feature = "arbitrary")] impl<'a> Arbitrary<'a> for NumberOfBlocks { fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result { @@ -383,24 +414,24 @@ mod tests { let time_lock = NumberOf512Seconds::from_512_second_intervals(10); let chain_state1 = BlockMtp::new(timestamps); let utxo_state1 = BlockMtp::new(utxo_timestamps); - assert!(time_lock.is_satisfied_by(chain_state1, utxo_state1)); + assert!(time_lock.is_satisfied_by(chain_state1, utxo_state1).unwrap()); // Test case 2: Not satisfied (current_mtp < utxo_mtp + required_seconds) let chain_state2 = BlockMtp::new(timestamps2); let utxo_state2 = BlockMtp::new(utxo_timestamps2); - assert!(!time_lock.is_satisfied_by(chain_state2, utxo_state2)); + assert!(!time_lock.is_satisfied_by(chain_state2, utxo_state2).unwrap()); // Test case 3: Test with a larger value (100 intervals = 51200 seconds) let larger_lock = NumberOf512Seconds::from_512_second_intervals(100); let chain_state3 = BlockMtp::new(timestamps3); let utxo_state3 = BlockMtp::new(utxo_timestamps3); - assert!(larger_lock.is_satisfied_by(chain_state3, utxo_state3)); + assert!(larger_lock.is_satisfied_by(chain_state3, utxo_state3).unwrap()); // Test case 4: Overflow handling - tests that is_satisfied_by handles overflow gracefully let max_time_lock = NumberOf512Seconds::MAX; let chain_state4 = BlockMtp::new(timestamps); let utxo_state4 = BlockMtp::new(utxo_timestamps); - assert!(!max_time_lock.is_satisfied_by(chain_state4, utxo_state4)); + assert!(!max_time_lock.is_satisfied_by(chain_state4, utxo_state4).unwrap()); } #[test] @@ -410,19 +441,19 @@ mod tests { let height_lock = NumberOfBlocks(10); // Test case 1: Satisfaction (current_height >= utxo_height + required) - let chain_state1 = BlockHeight::from_u32(100); + let chain_state1 = BlockHeight::from_u32(89); let utxo_state1 = BlockHeight::from_u32(80); - assert!(height_lock.is_satisfied_by(chain_state1, utxo_state1)); + assert!(height_lock.is_satisfied_by(chain_state1, utxo_state1).unwrap()); // Test case 2: Not satisfied (current_height < utxo_height + required) - let chain_state2 = BlockHeight::from_u32(89); + let chain_state2 = BlockHeight::from_u32(88); let utxo_state2 = BlockHeight::from_u32(80); - assert!(!height_lock.is_satisfied_by(chain_state2, utxo_state2)); + assert!(!height_lock.is_satisfied_by(chain_state2, utxo_state2).unwrap()); // Test case 3: Overflow handling - tests that is_satisfied_by handles overflow gracefully let max_height_lock = NumberOfBlocks::MAX; let chain_state3 = BlockHeight::from_u32(1000); let utxo_state3 = BlockHeight::from_u32(80); - assert!(!max_height_lock.is_satisfied_by(chain_state3, utxo_state3)); + assert!(!max_height_lock.is_satisfied_by(chain_state3, utxo_state3).unwrap()); } } diff --git a/units/tests/api.rs b/units/tests/api.rs index 33b60bc7b..3e6dd97f6 100644 --- a/units/tests/api.rs +++ b/units/tests/api.rs @@ -139,11 +139,13 @@ struct Errors { x: locktime::absolute::ConversionError, y: locktime::absolute::Height, z: locktime::absolute::ParseHeightError, - _a: locktime::absolute::ParseTimeError, - _b: locktime::relative::TimeOverflowError, - _e: parse::ParseIntError, - _f: parse::PrefixedHexError, - _g: parse::UnprefixedHexError, + aa: locktime::absolute::ParseTimeError, + ab: locktime::relative::TimeOverflowError, + ac: locktime::relative::InvalidHeightError, + ad: locktime::relative::InvalidTimeError, + ae: parse::ParseIntError, + af: parse::PrefixedHexError, + ag: parse::UnprefixedHexError, } #[test] From 727047bd393f4616392a1966fd997526add35e33 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sun, 11 May 2025 10:56:13 +1000 Subject: [PATCH 3/8] Fix off-by-one-bug in absolute locktime When checking a locktime against block height we add 1 because when the next block is being validated that is what the height will be and `is_satisfied_by` is defined to return true if a transaction with this locktime can be included in the next block. As we have in `relative`, and for the same reasons, add an API to the absolute `Height` and `MedianTimePast`: `is_satisfied_by`. Also add `is_satisfied_by_{height, time}` variants to `absolute::LockTime`. Call through to the new functions in `units`. --- primitives/src/locktime/absolute.rs | 109 +++++++++++++++++++++++++++- units/src/locktime/absolute.rs | 23 ++++++ 2 files changed, 128 insertions(+), 4 deletions(-) diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs index c10d2ff79..0ec07fb14 100644 --- a/primitives/src/locktime/absolute.rs +++ b/primitives/src/locktime/absolute.rs @@ -233,13 +233,21 @@ impl LockTime { /// is satisfied if a transaction with `nLockTime` ([`Transaction::lock_time`]) set to /// `height`/`time` is valid. /// + /// If `height` and `mtp` represent the current chain tip then a transaction with this + /// locktime can be broadcast for inclusion in the next block. + /// + /// If you do not have, or do not wish to calculate, both parameters consider using: + /// + /// * [`is_satisfied_by_height()`](absolute::LockTime::is_satisfied_by_height) + /// * [`is_satisfied_by_time()`](absolute::LockTime::is_satisfied_by_time) + /// /// # Examples /// /// ```no_run /// # use bitcoin_primitives::absolute; /// // Can be implemented if block chain data is available. /// fn get_height() -> absolute::Height { todo!("return the current block height") } - /// fn get_time() -> absolute::MedianTimePast { todo!("return the current block time") } + /// fn get_time() -> absolute::MedianTimePast { todo!("return the current block MTP") } /// /// let n = absolute::LockTime::from_consensus(741521); // `n OP_CHEKCLOCKTIMEVERIFY`. /// if n.is_satisfied_by(get_height(), get_time()) { @@ -247,12 +255,43 @@ impl LockTime { /// } /// ```` #[inline] - pub fn is_satisfied_by(self, height: Height, time: MedianTimePast) -> bool { + pub fn is_satisfied_by(self, height: Height, mtp: MedianTimePast) -> bool { + match self { + LockTime::Blocks(blocks) => blocks.is_satisfied_by(height), + LockTime::Seconds(time) => time.is_satisfied_by(mtp), + } + } + + /// Returns true if a transaction with this locktime can be spent in the next block. + /// + /// If `height` is the current block height of the chain then a transaction with this locktime + /// can be broadcast for inclusion in the next block. + /// + /// # Errors + /// + /// Returns an error if this lock is not lock-by-height. + #[inline] + pub fn is_satisfied_by_height(self, height: Height) -> Result { use LockTime as L; match self { - L::Blocks(n) => n <= height, - L::Seconds(n) => n <= time, + L::Blocks(blocks) => Ok(blocks.is_satisfied_by(height)), + L::Seconds(time) => Err(IncompatibleHeightError { lock: time, incompatible: height }), + } + } + + /// Returns true if a transaction with this locktime can be included in the next block. + /// + /// # Errors + /// + /// Returns an error if this lock is not lock-by-time. + #[inline] + pub fn is_satisfied_by_time(self, mtp: MedianTimePast) -> Result { + use LockTime as L; + + match self { + L::Seconds(time) => Ok(time.is_satisfied_by(mtp)), + L::Blocks(blocks) => Err(IncompatibleTimeError { lock: blocks, incompatible: mtp }), } } @@ -407,6 +446,68 @@ impl<'de> serde::Deserialize<'de> for LockTime { } } +/// Tried to satisfy a lock-by-time lock using a height value. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IncompatibleHeightError { + /// The inner value of the lock-by-time lock. + lock: MedianTimePast, + /// Attempted to satisfy a lock-by-time lock with this height. + incompatible: Height, +} + +impl IncompatibleHeightError { + /// Returns the value of the lock-by-time lock. + pub fn lock(&self) -> MedianTimePast { self.lock } + + /// Returns the height that was erroneously used to try and satisfy a lock-by-time lock. + pub fn incompatible(&self) -> Height { self.incompatible } +} + +impl fmt::Display for IncompatibleHeightError { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!( + f, + "tried to satisfy a lock-by-time lock {} with height: {}", + self.lock, self.incompatible + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for IncompatibleHeightError {} + +/// Tried to satisfy a lock-by-height lock using a height value. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IncompatibleTimeError { + /// The inner value of the lock-by-height lock. + lock: Height, + /// Attempted to satisfy a lock-by-height lock with this MTP. + incompatible: MedianTimePast, +} + +impl IncompatibleTimeError { + /// Returns the value of the lock-by-height lock. + pub fn lock(&self) -> Height { self.lock } + + /// Returns the MTP that was erroneously used to try and satisfy a lock-by-height lock. + pub fn incompatible(&self) -> MedianTimePast { self.incompatible } +} + +impl fmt::Display for IncompatibleTimeError { + #[inline] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!( + f, + "tried to satisfy a lock-by-height lock {} with MTP: {}", + self.lock, self.incompatible + ) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for IncompatibleTimeError {} + #[cfg(feature = "arbitrary")] impl<'a> Arbitrary<'a> for LockTime { fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result { diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index f80ccdfee..c256df2e0 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -83,6 +83,17 @@ impl Height { /// Converts this [`Height`] to a raw `u32` value. #[inline] pub const fn to_u32(self) -> u32 { self.0 } + + /// Returns true if a transaction with this locktime can be included in the next block. + /// + /// `self` is value of the `LockTime` and if `height` is the current chain tip then + /// a transaction with this lock can be broadcast for inclusion in the next block. + #[inline] + pub fn is_satisfied_by(self, height: Height) -> bool { + // Use u64 so that there can be no overflow. + let next_block_height = u64::from(height.to_u32()) + 1; + u64::from(self.to_u32()) <= next_block_height + } } impl fmt::Display for Height { @@ -217,6 +228,18 @@ impl MedianTimePast { /// Converts this [`MedianTimePast`] to a raw `u32` value. #[inline] pub const fn to_u32(self) -> u32 { self.0 } + + /// Returns true if a transaction with this locktime can be included in the next block. + /// + /// `self`is the value of the `LockTime` and if `time` is the median time past of the block at + /// the chain tip then a transaction with this lock can be broadcast for inclusion in the next + /// block. + #[inline] + pub fn is_satisfied_by(self, time: MedianTimePast) -> bool { + // The locktime check in Core during block validation uses the MTP + // of the previous block - which is the expected to be `time` here. + self <= time + } } impl fmt::Display for MedianTimePast { From f9d6453d5b2893fe6b025f05485ae71b04c3052e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:26:06 +1000 Subject: [PATCH 4/8] Shorten locktime type term Use lock-by-time and lock-by-height instead of lock-by-blocktime and lock-by-blockheight respectively with no loss of clarity. --- bitcoin/src/blockdata/mod.rs | 4 ++-- primitives/src/locktime/absolute.rs | 2 +- primitives/src/locktime/relative.rs | 2 +- units/src/locktime/absolute.rs | 10 +++++----- units/src/locktime/relative.rs | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/bitcoin/src/blockdata/mod.rs b/bitcoin/src/blockdata/mod.rs index 13ffc6059..7974ba807 100644 --- a/bitcoin/src/blockdata/mod.rs +++ b/bitcoin/src/blockdata/mod.rs @@ -32,7 +32,7 @@ pub mod locktime { pub mod absolute { //! Provides type [`LockTime`] that implements the logic around nLockTime/OP_CHECKLOCKTIMEVERIFY. //! - //! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by + //! There are two types of lock time: lock-by-height and lock-by-time, distinguished by //! whether `LockTime < LOCKTIME_THRESHOLD`. use io::{BufRead, Write}; @@ -66,7 +66,7 @@ pub mod locktime { pub mod relative { //! Provides type [`LockTime`] that implements the logic around nSequence/OP_CHECKSEQUENCEVERIFY. //! - //! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by + //! There are two types of lock time: lock-by-height and lock-by-time, distinguished by //! whether bit 22 of the `u32` consensus value is set. /// Re-export everything from the `primitives::locktime::relative` module. diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs index 0ec07fb14..4971aadb6 100644 --- a/primitives/src/locktime/absolute.rs +++ b/primitives/src/locktime/absolute.rs @@ -2,7 +2,7 @@ //! Provides type [`LockTime`] that implements the logic around `nLockTime`/`OP_CHECKLOCKTIMEVERIFY`. //! -//! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by +//! There are two types of lock time: lock-by-height and lock-by-time, distinguished by //! whether `LockTime < LOCKTIME_THRESHOLD`. use core::fmt; diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs index 343ebd682..56edad60b 100644 --- a/primitives/src/locktime/relative.rs +++ b/primitives/src/locktime/relative.rs @@ -2,7 +2,7 @@ //! Provides type [`LockTime`] that implements the logic around `nSequence`/`OP_CHECKSEQUENCEVERIFY`. //! -//! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by +//! There are two types of lock time: lock-by-height and lock-by-time, distinguished by //! whether bit 22 of the `u32` consensus value is set. use core::{convert, fmt}; diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index c256df2e0..96e1bf1fd 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -349,7 +349,7 @@ impl std::error::Error for ConversionError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } -/// Describes the two types of locking, lock-by-blockheight and lock-by-blocktime. +/// Describes the two types of locking, lock-by-height and lock-by-time. #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] enum LockTimeUnit { /// Lock by blockheight. @@ -364,9 +364,9 @@ impl fmt::Display for LockTimeUnit { match *self { L::Blocks => - write!(f, "expected lock-by-blockheight (must be < {})", LOCK_TIME_THRESHOLD), + write!(f, "expected lock-by-height (must be < {})", LOCK_TIME_THRESHOLD), L::Seconds => - write!(f, "expected lock-by-blocktime (must be >= {})", LOCK_TIME_THRESHOLD), + write!(f, "expected lock-by-time (must be >= {})", LOCK_TIME_THRESHOLD), } } } @@ -580,8 +580,8 @@ mod tests { let blocks = LockTimeUnit::Blocks; let seconds = LockTimeUnit::Seconds; - assert_eq!(format!("{}", blocks), "expected lock-by-blockheight (must be < 500000000)"); - assert_eq!(format!("{}", seconds), "expected lock-by-blocktime (must be >= 500000000)"); + assert_eq!(format!("{}", blocks), "expected lock-by-height (must be < 500000000)"); + assert_eq!(format!("{}", seconds), "expected lock-by-time (must be >= 500000000)"); } #[test] diff --git a/units/src/locktime/relative.rs b/units/src/locktime/relative.rs index 1fb3db1db..e3d9f8d69 100644 --- a/units/src/locktime/relative.rs +++ b/units/src/locktime/relative.rs @@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize}; #[doc(hidden)] pub type Height = NumberOfBlocks; -/// A relative lock time lock-by-blockheight value. +/// A relative lock time lock-by-height value. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct NumberOfBlocks(u16); @@ -94,9 +94,9 @@ impl fmt::Display for NumberOfBlocks { #[doc(hidden)] pub type Time = NumberOf512Seconds; -/// A relative lock time lock-by-blocktime value. +/// A relative lock time lock-by-time value. /// -/// For BIP 68 relative lock-by-blocktime locks, time is measured in 512 second intervals. +/// For BIP 68 relative lock-by-time locks, time is measured in 512 second intervals. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct NumberOf512Seconds(u16); From 480a2cd62aa5ddfea15cc1ccbf0e03f92bc663d9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:29:33 +1000 Subject: [PATCH 5/8] Favour new function `from_mtp` over deprecated Use the new function and not the deprecated on in rustdcos and tests. --- bitcoin/tests/serde.rs | 2 +- primitives/src/locktime/absolute.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bitcoin/tests/serde.rs b/bitcoin/tests/serde.rs index f9e7e4a6a..3bc1c3deb 100644 --- a/bitcoin/tests/serde.rs +++ b/bitcoin/tests/serde.rs @@ -64,7 +64,7 @@ fn serde_regression_absolute_lock_time_height() { #[test] fn serde_regression_absolute_lock_time_time() { let seconds: u32 = 1653195600; // May 22nd, 5am UTC. - let t = absolute::LockTime::from_time(seconds).expect("valid time"); + let t = absolute::LockTime::from_mtp(seconds).expect("valid time"); let got = serialize(&t).unwrap(); let want = include_bytes!("data/serde/absolute_lock_time_seconds_bincode") as &[_]; diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs index 4971aadb6..48b231810 100644 --- a/primitives/src/locktime/absolute.rs +++ b/primitives/src/locktime/absolute.rs @@ -79,7 +79,7 @@ pub enum LockTime { /// use bitcoin_primitives::absolute; /// /// let seconds: u32 = 1653195600; // May 22nd, 5am UTC. - /// let n = absolute::LockTime::from_time(seconds).expect("valid time"); + /// let n = absolute::LockTime::from_mtp(seconds).expect("valid time"); /// assert!(n.is_block_time()); /// assert_eq!(n.to_consensus_u32(), seconds); /// ``` @@ -197,8 +197,8 @@ impl LockTime { /// /// ```rust /// # use bitcoin_primitives::absolute; - /// assert!(absolute::LockTime::from_time(1653195600).is_ok()); - /// assert!(absolute::LockTime::from_time(741521).is_err()); + /// assert!(absolute::LockTime::from_mtp(1653195600).is_ok()); + /// assert!(absolute::LockTime::from_mtp(741521).is_err()); /// ``` #[inline] pub fn from_mtp(n: u32) -> Result { From 40bb177bc2b75f976f24885ca69fc20591884cb4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:35:01 +1000 Subject: [PATCH 6/8] Put is_satisfied_by functions together Move the `_by_{height,time}` functions to be underneath the `is_satisfied_by` function. Code move only, no logic change. --- primitives/src/locktime/relative.rs | 96 ++++++++++++++--------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs index 56edad60b..fd58ff1db 100644 --- a/primitives/src/locktime/relative.rs +++ b/primitives/src/locktime/relative.rs @@ -213,6 +213,54 @@ impl LockTime { } } + /// Returns true if an output with this locktime can be spent in the next block. + /// + /// If this function returns true then an output with this locktime can be spent in the next + /// block. + /// + /// # Errors + /// + /// Returns an error if this lock is not lock-by-height. + #[inline] + pub fn is_satisfied_by_height( + self, + chain_tip: BlockHeight, + utxo_mined_at: BlockHeight, + ) -> Result { + use LockTime as L; + + match self { + L::Blocks(blocks) => blocks + .is_satisfied_by(chain_tip, utxo_mined_at) + .map_err(IsSatisfiedByHeightError::Satisfaction), + L::Time(time) => Err(IsSatisfiedByHeightError::Incompatible(time)), + } + } + + /// Returns true if an output with this locktime can be spent in the next block. + /// + /// If this function returns true then an output with this locktime can be spent in the next + /// block. + /// + /// # Errors + /// + /// Returns an error if this lock is not lock-by-time. + #[inline] + pub fn is_satisfied_by_time( + self, + chain_tip: BlockMtp, + utxo_mined_at: BlockMtp, + ) -> Result { + use LockTime as L; + + match self { + L::Time(time) => time + .is_satisfied_by(chain_tip, utxo_mined_at) + .map_err(IsSatisfiedByTimeError::Satisfaction), + L::Blocks(blocks) => Err(IsSatisfiedByTimeError::Incompatible(blocks)), + } + } + /// Returns true if satisfaction of `other` lock time implies satisfaction of this /// [`relative::LockTime`]. /// @@ -281,54 +329,6 @@ impl LockTime { false } } - - /// Returns true if an output with this locktime can be spent in the next block. - /// - /// If this function returns true then an output with this locktime can be spent in the next - /// block. - /// - /// # Errors - /// - /// Returns an error if this lock is not lock-by-height. - #[inline] - pub fn is_satisfied_by_height( - self, - chain_tip: BlockHeight, - utxo_mined_at: BlockHeight, - ) -> Result { - use LockTime as L; - - match self { - L::Blocks(blocks) => blocks - .is_satisfied_by(chain_tip, utxo_mined_at) - .map_err(IsSatisfiedByHeightError::Satisfaction), - L::Time(time) => Err(IsSatisfiedByHeightError::Incompatible(time)), - } - } - - /// Returns true if an output with this locktime can be spent in the next block. - /// - /// If this function returns true then an output with this locktime can be spent in the next - /// block. - /// - /// # Errors - /// - /// Returns an error if this lock is not lock-by-time. - #[inline] - pub fn is_satisfied_by_time( - self, - chain_tip: BlockMtp, - utxo_mined_at: BlockMtp, - ) -> Result { - use LockTime as L; - - match self { - L::Time(time) => time - .is_satisfied_by(chain_tip, utxo_mined_at) - .map_err(IsSatisfiedByTimeError::Satisfaction), - L::Blocks(blocks) => Err(IsSatisfiedByTimeError::Incompatible(blocks)), - } - } } impl From for LockTime { From caebb1bf73ec34f0ca87c54a072a79f154ef853a Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:48:05 +1000 Subject: [PATCH 7/8] units: relative: Do minor rustdocs fixes Slightly improve grammar and fix column width to 100. --- units/src/locktime/relative.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/units/src/locktime/relative.rs b/units/src/locktime/relative.rs index e3d9f8d69..e50fe8f93 100644 --- a/units/src/locktime/relative.rs +++ b/units/src/locktime/relative.rs @@ -111,7 +111,8 @@ impl NumberOf512Seconds { /// The maximum relative block time (33,554,432 seconds or approx 388 days). pub const MAX: Self = NumberOf512Seconds(u16::MAX); - /// Constructs a new [`NumberOf512Seconds`] using time intervals where each interval is equivalent to 512 seconds. + /// Constructs a new [`NumberOf512Seconds`] using time intervals where each interval is + /// equivalent to 512 seconds. /// /// Encoding finer granularity of time for relative lock-times is not supported in Bitcoin. #[inline] @@ -122,8 +123,8 @@ impl NumberOf512Seconds { #[must_use] pub const fn to_512_second_intervals(self) -> u16 { self.0 } - /// Constructs a new [`NumberOf512Seconds`] from seconds, converting the seconds into 512 second - /// interval with truncating division. + /// Constructs a new [`NumberOf512Seconds`] from seconds, converting the seconds into a 512 + /// second interval using truncating division. /// /// # Errors /// @@ -139,8 +140,8 @@ impl NumberOf512Seconds { } } - /// Constructs a new [`NumberOf512Seconds`] from seconds, converting the seconds into 512 second - /// intervals with ceiling division. + /// Constructs a new [`NumberOf512Seconds`] from seconds, converting the seconds into a 512 + /// second interval using ceiling division. /// /// # Errors /// From 4ccecf5decfead9818b74fbdee73115c349e2f3e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 12 May 2025 12:57:08 +1000 Subject: [PATCH 8/8] Fix stale Height type link `units::locktime::relative::Height` type is now deprecated, use the new name in rustdoc. --- units/src/block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/units/src/block.rs b/units/src/block.rs index 778fb1bea..4c832f18e 100644 --- a/units/src/block.rs +++ b/units/src/block.rs @@ -127,7 +127,7 @@ impl_u32_wrapper! { /// Block interval is an integer type representing a difference between the heights of two blocks. /// /// This type is not meant for constructing relative height based timelocks. It is a general - /// purpose block interval abstraction. For locktimes please see [`locktime::relative::Height`]. + /// purpose block interval abstraction. For locktimes please see [`locktime::relative::NumberOfBlocks`]. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] // Public to try and make it really clear that there are no invariants.