Merge rust-bitcoin#4458: locktimes: replace MtpAndHeight type with pair of BlockMtp and BlockHeight

47c77af units: delete MtpAndHeight type (Andrew Poelstra)
d82b8c0 primitives: stop using MtpAndHeight (Andrew Poelstra)
72d5fba units: stop using MtpAndHeight in locktime::relative is_satisfied_by methods (Andrew Poelstra)
d933c75 units: change type of MtpHeight::to_mtp to BlockMtp (Andrew Poelstra)
dcbdb7c units: add checked arithmetic to Block{Height,Mtp}{Interval,} (Andrew Poelstra)
4300271 units: add constructor for absolute::Mtp from timestamps (Andrew Poelstra)
4e4601b units: rename BlockInterval to BlockHeightInterval (Andrew Poelstra)
cb882c5 units: add global `BlockMtpInterval` type (Andrew Poelstra)
4e3af51 units: add global `BlockMtp` type (Andrew Poelstra)
a3228d4 units: pull u32 conversions for BlockHeight/BlockInterval into macro (Andrew Poelstra)

Pull request description:

  This is a more involved PR than I'd expected but hopefully the individual commits make sense and are well-motivated. Essentially, my goal was to replace `MtpAndHeight` as used by relative locktimes with a pair of `Mtp` and `Height`.

  However, relative locktimes, when given a MTP/Height for the UTXO creation and the chain tip, are roughly modeled as "take a diff of MTPs to get a `relative::MtpInterval`, a diff of heights to get a `relative::HeightInterval`, and compare to the locktimes". *However*, we have no standalone MTP type to "take a diff of", and also there are failure modes when creating the diffs (e.g. if the diff would exceed the range of `MtpInterval` or `HeightInterval`).

  So I backed up and decided to use the existing `BlockHeight`/`BlockInterval` as the type to "take a diff of". I needed to introduce a `BlockMtp`/`BlockMtpInterval` to work with MTPs. These types have full-u32 range, unlike the similarly-named types in `units::locktimes::absolute`. I then needed to add some conversion methods. Along the way, I cleaned up the APIs and documentation, added checked arithmetic, etc., as needed.

  See the individual commit messages for more detail.

  I believe the resulting API is much more consistent and discoverable, even though it has more surface than the old API.

  I considered splitting this into 2 PRs but I think the first half of the changes aren't well-motivated with out the second half. Let me know.

ACKs for top commit:
  tcharding:
    ACK 47c77af

Tree-SHA512: ebe19a5b1684db8c2d913274347c994026aaa0dcdd79349c237920a82fe55560777278efdbbc7f1b1424c9391d9bbd891ae844db885deea75288000437a8a287

Author: apoelstra

bitcoin/src/blockdata/block.rs

@@ -30,7 +30,13 @@ use crate::transaction::{Transaction, TransactionExt as _, Wtxid};
 #[doc(inline)]
 pub use primitives::block::{Block, Checked, Unchecked, Validation, Version, BlockHash, Header, WitnessCommitment};
 #[doc(inline)]
-pub use units::block::{BlockHeight, BlockInterval, TooBigForRelativeBlockHeightIntervalError};
+pub use units::block::{
+    BlockHeight, BlockHeightInterval, TooBigForRelativeBlockHeightIntervalError,
+};
+
+#[deprecated(since = "TBD", note = "use `BlockHeightInterval` instead")]
+#[doc(hidden)]
+pub type BlockInterval = BlockHeightInterval;
 
 impl_hashencode!(BlockHash);
 

bitcoin/src/lib.rs

@@ -135,12 +135,16 @@ pub use primitives::{
 #[doc(inline)]
 pub use units::{
     amount::{Amount, Denomination, SignedAmount},
-    block::{BlockHeight, BlockInterval},
+    block::{BlockHeight, BlockHeightInterval, BlockMtp},
     fee_rate::FeeRate,
     time::{self, BlockTime},
     weight::Weight,
 };
 
+#[deprecated(since = "TBD", note = "use `BlockHeightInterval` instead")]
+#[doc(hidden)]
+pub type BlockInterval = BlockHeightInterval;
+
 #[doc(inline)]
 pub use crate::{
     address::{Address, AddressType, KnownHrp},
@@ -243,7 +247,7 @@ mod encode_impls {
     //! Encodable/Decodable implementations.
     // While we are deprecating, re-exporting, and generally moving things around just put these here.
 
-    use units::{BlockHeight, BlockInterval};
+    use units::{BlockHeight, BlockHeightInterval};
 
     use crate::consensus::{encode, Decodable, Encodable};
     use crate::io::{BufRead, Write};
@@ -275,5 +279,5 @@ mod encode_impls {
     }
 
     impl_encodable_for_u32_wrapper!(BlockHeight);
-    impl_encodable_for_u32_wrapper!(BlockInterval);
+    impl_encodable_for_u32_wrapper!(BlockHeightInterval);
 }

bitcoin/src/network/params.rs

@@ -67,7 +67,7 @@
 //! # }
 //! ```
 
-use units::{BlockHeight, BlockInterval};
+use units::{BlockHeight, BlockHeightInterval};
 
 use crate::network::Network;
 #[cfg(doc)]
@@ -92,9 +92,9 @@ pub struct Params {
     /// Minimum blocks including miner confirmation of the total of 2016 blocks in a retargeting period,
     /// (nPowTargetTimespan / nPowTargetSpacing) which is also used for BIP9 deployments.
     /// Examples: 1916 for 95%, 1512 for testchains.
-    pub rule_change_activation_threshold: BlockInterval,
+    pub rule_change_activation_threshold: BlockHeightInterval,
     /// Number of blocks with the same set of rules.
-    pub miner_confirmation_window: BlockInterval,
+    pub miner_confirmation_window: BlockHeightInterval,
     /// Proof of work limit value. It contains the lowest possible difficulty.
     #[deprecated(since = "0.32.0", note = "use `max_attainable_target` instead")]
     pub pow_limit: Target,
@@ -152,8 +152,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(227931), // 000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8
         bip65_height: BlockHeight::from_u32(388381), // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
         bip66_height: BlockHeight::from_u32(363725), // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931
-        rule_change_activation_threshold: BlockInterval::from_u32(1916), // 95%
-        miner_confirmation_window: BlockInterval::from_u32(2016),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(1916), // 95%
+        miner_confirmation_window: BlockHeightInterval::from_u32(2016),
         pow_limit: Target::MAX_ATTAINABLE_MAINNET,
         max_attainable_target: Target::MAX_ATTAINABLE_MAINNET,
         pow_target_spacing: 10 * 60,            // 10 minutes.
@@ -170,8 +170,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(21111), // 0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8
         bip65_height: BlockHeight::from_u32(581885), // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
         bip66_height: BlockHeight::from_u32(330776), // 000000002104c8c45e99a8853285a3b592602a3ccde2b832481da85e9e4ba182
-        rule_change_activation_threshold: BlockInterval::from_u32(1512), // 75%
-        miner_confirmation_window: BlockInterval::from_u32(2016),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(1512), // 75%
+        miner_confirmation_window: BlockHeightInterval::from_u32(2016),
         pow_limit: Target::MAX_ATTAINABLE_TESTNET,
         max_attainable_target: Target::MAX_ATTAINABLE_TESTNET,
         pow_target_spacing: 10 * 60,            // 10 minutes.
@@ -187,8 +187,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(21111), // 0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8
         bip65_height: BlockHeight::from_u32(581885), // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
         bip66_height: BlockHeight::from_u32(330776), // 000000002104c8c45e99a8853285a3b592602a3ccde2b832481da85e9e4ba182
-        rule_change_activation_threshold: BlockInterval::from_u32(1512), // 75%
-        miner_confirmation_window: BlockInterval::from_u32(2016),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(1512), // 75%
+        miner_confirmation_window: BlockHeightInterval::from_u32(2016),
         pow_limit: Target::MAX_ATTAINABLE_TESTNET,
         max_attainable_target: Target::MAX_ATTAINABLE_TESTNET,
         pow_target_spacing: 10 * 60,            // 10 minutes.
@@ -204,8 +204,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(1),
         bip65_height: BlockHeight::from_u32(1),
         bip66_height: BlockHeight::from_u32(1),
-        rule_change_activation_threshold: BlockInterval::from_u32(1512), // 75%
-        miner_confirmation_window: BlockInterval::from_u32(2016),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(1512), // 75%
+        miner_confirmation_window: BlockHeightInterval::from_u32(2016),
         pow_limit: Target::MAX_ATTAINABLE_TESTNET,
         max_attainable_target: Target::MAX_ATTAINABLE_TESTNET,
         pow_target_spacing: 10 * 60,            // 10 minutes.
@@ -221,8 +221,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(1),
         bip65_height: BlockHeight::from_u32(1),
         bip66_height: BlockHeight::from_u32(1),
-        rule_change_activation_threshold: BlockInterval::from_u32(1916), // 95%
-        miner_confirmation_window: BlockInterval::from_u32(2016),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(1916), // 95%
+        miner_confirmation_window: BlockHeightInterval::from_u32(2016),
         pow_limit: Target::MAX_ATTAINABLE_SIGNET,
         max_attainable_target: Target::MAX_ATTAINABLE_SIGNET,
         pow_target_spacing: 10 * 60,            // 10 minutes.
@@ -238,8 +238,8 @@ impl Params {
         bip34_height: BlockHeight::from_u32(100000000), // not activated on regtest
         bip65_height: BlockHeight::from_u32(1351),
         bip66_height: BlockHeight::from_u32(1251), // used only in rpc tests
-        rule_change_activation_threshold: BlockInterval::from_u32(108), // 75%
-        miner_confirmation_window: BlockInterval::from_u32(144),
+        rule_change_activation_threshold: BlockHeightInterval::from_u32(108), // 75%
+        miner_confirmation_window: BlockHeightInterval::from_u32(144),
         pow_limit: Target::MAX_ATTAINABLE_REGTEST,
         max_attainable_target: Target::MAX_ATTAINABLE_REGTEST,
         pow_target_spacing: 10 * 60,            // 10 minutes.

primitives/src/lib.rs

@@ -52,12 +52,16 @@ pub mod witness;
 #[doc(inline)]
 pub use units::{
     amount::{self, Amount, SignedAmount},
-    block::{BlockHeight, BlockInterval},
+    block::{BlockHeight, BlockHeightInterval, BlockMtp, BlockMtpInterval},
     fee_rate::{self, FeeRate},
     time::{self, BlockTime},
     weight::{self, Weight},
 };
 
+#[deprecated(since = "TBD", note = "use `BlockHeightInterval` instead")]
+#[doc(hidden)]
+pub type BlockInterval = BlockHeightInterval;
+
 #[doc(inline)]
 #[cfg(feature = "alloc")]
 pub use self::{

primitives/src/locktime/relative.rs

@@ -14,7 +14,7 @@ use crate::{relative, TxIn};
 #[rustfmt::skip]                // Keep public re-exports separate.
 #[doc(inline)]
 pub use units::locktime::relative::{HeightInterval, MtpInterval, TimeOverflowError};
-use units::mtp_height::MtpAndHeight;
+use units::{BlockHeight, BlockMtp};
 
 #[deprecated(since = "TBD", note = "use `Mtp` instead")]
 #[doc(hidden)]
@@ -44,9 +44,7 @@ pub type Time = MtpInterval;
 ///
 /// ```
 /// use bitcoin_primitives::relative;
-/// use bitcoin_primitives::BlockTime;
-/// use bitcoin_primitives::BlockHeight;
-/// use units::mtp_height::MtpAndHeight;
+/// 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());
 ///
@@ -65,14 +63,15 @@ pub type Time = MtpInterval;
 /// 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 chain_tip = MtpAndHeight::new(current_height, timestamps);
-/// let utxo_mined_at = MtpAndHeight::new(utxo_height, utxo_timestamps);
 /// let locktime = relative::LockTime::Time(relative::MtpInterval::from_512_second_intervals(10));
 ///
 /// // Check if locktime is satisfied
-/// assert!(locktime.is_satisfied_by(chain_tip, utxo_mined_at));
+/// 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))]
@@ -226,10 +225,8 @@ impl LockTime {
     ///
     /// ```rust
     /// # use bitcoin_primitives::relative::Time;
-    /// # use units::mtp_height::MtpAndHeight;
-    /// # use bitcoin_primitives::BlockHeight;
+    /// # use bitcoin_primitives::{BlockHeight, BlockMtp, BlockTime};
     /// # use bitcoin_primitives::relative::LockTime;
-    /// # use bitcoin_primitives::BlockTime;
     ///
     /// fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] {
     ///     let mut timestamps = [BlockTime::from_u32(0); 11];
@@ -243,19 +240,26 @@ impl LockTime {
     /// let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200);
     ///
     /// 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 chain_tip = MtpAndHeight::new(current_height, timestamps);
-    /// let utxo_mined_at = MtpAndHeight::new(utxo_height, utxo_timestamps);
     /// let locktime = LockTime::Time(Time::from_512_second_intervals(10));
     ///
     /// // Check if locktime is satisfied
-    /// assert!(locktime.is_satisfied_by(chain_tip, utxo_mined_at));
+    /// assert!(locktime.is_satisfied_by(current_height, current_mtp, utxo_height, utxo_mtp));
     /// ```
-    pub fn is_satisfied_by(self, chain_tip: MtpAndHeight, utxo_mined_at: MtpAndHeight) -> bool {
+    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 {
         match self {
-            LockTime::Blocks(blocks) => blocks.is_satisfied_by(chain_tip, utxo_mined_at),
-            LockTime::Time(time) => time.is_satisfied_by(chain_tip, utxo_mined_at),
+            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),
         }
     }
 
@@ -695,38 +699,55 @@ mod tests {
         let timestamps: [BlockTime; 11] = generate_timestamps(1_600_000_000, 200);
         let utxo_timestamps: [BlockTime; 11] = generate_timestamps(1_599_000_000, 200);
 
-        let chain_tip = MtpAndHeight::new(BlockHeight::from_u32(100), timestamps);
-        let utxo_mined_at = MtpAndHeight::new(BlockHeight::from_u32(80), utxo_timestamps);
+        let chain_height = BlockHeight::from_u32(100);
+        let chain_mtp = BlockMtp::new(timestamps);
+        let utxo_height = BlockHeight::from_u32(80);
+        let utxo_mtp = BlockMtp::new(utxo_timestamps);
 
         let lock1 = LockTime::Blocks(HeightInterval::from(10));
-        assert!(lock1.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
         let lock2 = LockTime::Blocks(HeightInterval::from(21));
-        assert!(!lock2.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(!lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
         let lock3 = LockTime::Time(MtpInterval::from_512_second_intervals(10));
-        assert!(lock3.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(lock3.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
         let lock4 = LockTime::Time(MtpInterval::from_512_second_intervals(20000));
-        assert!(!lock4.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(!lock4.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
-        assert!(LockTime::ZERO.is_satisfied_by(chain_tip, utxo_mined_at));
-        assert!(LockTime::from_512_second_intervals(0).is_satisfied_by(chain_tip, utxo_mined_at));
+        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
+        ));
 
         let lock6 = LockTime::from_seconds_floor(5000).unwrap();
-        assert!(lock6.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(lock6.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
         let max_height_lock = LockTime::Blocks(HeightInterval::MAX);
-        assert!(!max_height_lock.is_satisfied_by(chain_tip, utxo_mined_at));
+        assert!(!max_height_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
 
         let max_time_lock = LockTime::Time(MtpInterval::MAX);
-        assert!(!max_time_lock.is_satisfied_by(chain_tip, utxo_mined_at));
-
-        let max_chain_tip =
-            MtpAndHeight::new(BlockHeight::from_u32(u32::MAX), generate_timestamps(u32::MAX, 100));
-        let max_utxo_mined_at =
-            MtpAndHeight::new(BlockHeight::MAX, generate_timestamps(u32::MAX, 100));
-        assert!(!max_height_lock.is_satisfied_by(max_chain_tip, max_utxo_mined_at));
-        assert!(!max_time_lock.is_satisfied_by(max_chain_tip, max_utxo_mined_at));
+        assert!(!max_time_lock.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp));
+
+        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
+        ));
     }
 }