change(chain): Refactor the handling of height differences

zebra-chain/src/block/height.rs

@@ -65,60 +65,70 @@ impl Height {
 }
 
 /// A difference between two [`Height`]s, possibly negative.
-pub type HeightDiff = i32;
+///
+/// This can represent the difference between any height values,
+/// even if they are outside the valid height range (for example, in buggy RPC code).
+pub type HeightDiff = i64;
+
+impl TryFrom<u32> for Height {
+    type Error = &'static str;
+
+    /// Checks that the `height` is within the valid [`Height`] range.
+    fn try_from(height: u32) -> Result<Self, Self::Error> {
+        // Check the bounds.
+        if Height::MIN.0 <= height && height <= Height::MAX.0 {
+            Ok(Height(height))
+        } else {
+            Err("heights must be less than or equal to Height::MAX")
+        }
+    }
+}
+
+// We don't implement Add<u32> or Sub<u32>, because they cause type inference issues for integer constants.
 
 impl Sub<Height> for Height {
-    type Output = Option<HeightDiff>;
+    type Output = HeightDiff;
 
+    /// Subtract two heights, returning the result, which can be negative.
+    /// Since [`HeightDiff`] is `i64` and [`Height`] is `u32`, the result is always correct.
     fn sub(self, rhs: Height) -> Self::Output {
-        // We convert the heights from [`u32`] to [`i32`] because `u32::checked_sub` returns
-        // [`None`] for negative results. We must check the conversion since it's possible to
-        // construct heights outside the valid range for [`i32`].
-        let lhs = i32::try_from(self.0).ok()?;
-        let rhs = i32::try_from(rhs.0).ok()?;
-        lhs.checked_sub(rhs)
+        // All these conversions are exact, and the subtraction can't overflow or underflow.
+        let lhs = HeightDiff::from(self.0);
+        let rhs = HeightDiff::from(rhs.0);
+
+        lhs - rhs
     }
 }
 
 impl Sub<HeightDiff> for Height {
     type Output = Option<Self>;
 
+    /// Subtract a height difference from a height, returning `None` if the resulting height is
+    /// outside the valid `Height` range (this also checks the result is non-negative).
     fn sub(self, rhs: HeightDiff) -> Option<Self> {
-        // We need to convert the height to [`i32`] so we can subtract negative [`HeightDiff`]s. We
-        // must check the conversion since it's possible to construct heights outside the valid
-        // range for [`i32`].
-        let lhs = i32::try_from(self.0).ok()?;
-        let res = lhs.checked_sub(rhs)?;
-        let res = u32::try_from(res).ok()?;
-        let height = Height(res);
+        // We need to convert the height to [`i64`] so we can subtract negative [`HeightDiff`]s.
+        let lhs = HeightDiff::from(self.0);
+        let res = lhs - rhs;
 
         // Check the bounds.
-        if Height::MIN <= height && height <= Height::MAX {
-            Some(height)
-        } else {
-            None
-        }
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
 impl Add<HeightDiff> for Height {
     type Output = Option<Height>;
 
+    /// Add a height difference to a height, returning `None` if the resulting height is outside
+    /// the valid `Height` range (this also checks the result is non-negative).
     fn add(self, rhs: HeightDiff) -> Option<Height> {
-        // We need to convert the height to [`i32`] so we can subtract negative [`HeightDiff`]s. We
-        // must check the conversion since it's possible to construct heights outside the valid
-        // range for [`i32`].
-        let lhs = i32::try_from(self.0).ok()?;
-        let res = lhs.checked_add(rhs)?;
-        let res = u32::try_from(res).ok()?;
-        let height = Height(res);
+        // We need to convert the height to [`i64`] so we can add negative [`HeightDiff`]s.
+        let lhs = i64::from(self.0);
+        let res = lhs + rhs;
 
         // Check the bounds.
-        if Height::MIN <= height && height <= Height::MAX {
-            Some(height)
-        } else {
-            None
-        }
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
@@ -153,14 +163,14 @@ fn operator_tests() {
     assert_eq!(None, Height(0) + -1);
     assert_eq!(Some(Height(Height::MAX_AS_U32 - 1)), Height::MAX + -1);
 
-    // Bad heights aren't caught at compile-time or runtime, until we add or subtract
-    // `+ 0` would also cause an error here, but it triggers a spurious clippy lint
+    // Bad heights aren't caught at compile-time or runtime, until we add or subtract,
+    // and the result is invalid
     assert_eq!(None, Height(Height::MAX_AS_U32 + 1) + 1);
     assert_eq!(None, Height(i32::MAX as u32) + 1);
     assert_eq!(None, Height(u32::MAX) + 1);
 
     // Adding negative numbers
-    assert_eq!(None, Height(i32::MAX as u32 + 1) + -1);
+    assert_eq!(Some(Height::MAX), Height(i32::MAX as u32 + 1) + -1);
     assert_eq!(None, Height(u32::MAX) + -1);
 
     assert_eq!(Some(Height(1)), Height(2) - 1);
@@ -173,21 +183,22 @@ fn operator_tests() {
     assert_eq!(Some(Height::MAX), Height(Height::MAX_AS_U32 - 1) - -1);
     assert_eq!(None, Height::MAX - -1);
 
-    // Bad heights aren't caught at compile-time or runtime, until we add or subtract
-    assert_eq!(None, Height(i32::MAX as u32 + 1) - 1);
+    // Bad heights aren't caught at compile-time or runtime, until we add or subtract,
+    // and the result is invalid
+    assert_eq!(Some(Height::MAX), Height(i32::MAX as u32 + 1) - 1);
     assert_eq!(None, Height(u32::MAX) - 1);
 
     // Subtracting negative numbers
     assert_eq!(None, Height(Height::MAX_AS_U32 + 1) - -1);
     assert_eq!(None, Height(i32::MAX as u32) - -1);
     assert_eq!(None, Height(u32::MAX) - -1);
 
-    assert_eq!(1, (Height(2) - Height(1)).unwrap());
-    assert_eq!(0, (Height(1) - Height(1)).unwrap());
-    assert_eq!(Some(-1), Height(0) - Height(1));
-    assert_eq!(Some(-5), Height(2) - Height(7));
-    assert_eq!(Height::MAX, (Height::MAX - 0).unwrap());
-    assert_eq!(1, (Height::MAX - Height(Height::MAX_AS_U32 - 1)).unwrap());
-    assert_eq!(Some(-1), Height(Height::MAX_AS_U32 - 1) - Height::MAX);
-    assert_eq!(Some(-(Height::MAX_AS_U32 as i32)), Height(0) - Height::MAX);
+    assert_eq!(1, (Height(2) - Height(1)));
+    assert_eq!(0, (Height(1) - Height(1)));
+    assert_eq!(-1, Height(0) - Height(1));
+    assert_eq!(-5, Height(2) - Height(7));
+    assert_eq!(Height::MAX.0 as HeightDiff, (Height::MAX - Height(0)));
+    assert_eq!(1, (Height::MAX - Height(Height::MAX_AS_U32 - 1)));
+    assert_eq!(-1, Height(Height::MAX_AS_U32 - 1) - Height::MAX);
+    assert_eq!(-(Height::MAX_AS_U32 as HeightDiff), Height(0) - Height::MAX);
 }

zebra-chain/src/chain_tip.rs

@@ -106,6 +106,8 @@ pub trait ChainTip {
     ///
     /// This estimate may be negative if the current local time is behind the chain tip block's
     /// timestamp.
+    ///
+    /// Returns `None` if the state is empty.
     fn estimate_distance_to_network_chain_tip(
         &self,
         network: Network,
@@ -117,7 +119,7 @@ pub trait ChainTip {
 
         let distance_to_tip = estimator.estimate_height_at(Utc::now()) - current_height;
 
-        distance_to_tip.map(|distance_to_tip| (distance_to_tip, current_height))
+        Some((distance_to_tip, current_height))
     }
 }
 

zebra-chain/src/chain_tip/network_chain_tip_height_estimator.rs

@@ -5,7 +5,7 @@ use std::vec;
 use chrono::{DateTime, Duration, Utc};
 
 use crate::{
-    block,
+    block::{self, HeightDiff},
     parameters::{Network, NetworkUpgrade},
 };
 
@@ -87,17 +87,14 @@ impl NetworkChainTipHeightEstimator {
     /// The amount of blocks advanced is then used to extrapolate the amount to advance the
     /// `current_block_time`.
     fn estimate_up_to(&mut self, max_height: block::Height) {
-        (max_height - self.current_height).and_then(|remaining_blocks| {
-            remaining_blocks.is_positive().then(|| {
-                let remaining_blocks = i64::from(remaining_blocks);
-                let target_spacing_seconds = self.current_target_spacing.num_seconds();
-                let time_to_activation =
-                    Duration::seconds(remaining_blocks * target_spacing_seconds);
-
-                self.current_block_time += time_to_activation;
-                self.current_height = max_height;
-            })
-        });
+        let remaining_blocks = max_height - self.current_height;
+
+        if remaining_blocks > 0 {
+            let target_spacing_seconds = self.current_target_spacing.num_seconds();
+            let time_to_activation = Duration::seconds(remaining_blocks * target_spacing_seconds);
+            self.current_block_time += time_to_activation;
+            self.current_height = max_height;
+        }
     }
 
     /// Calculate an estimate for the chain height using the `current_target_spacing`.
@@ -121,21 +118,25 @@ impl NetworkChainTipHeightEstimator {
             time_difference_seconds -= 1;
         }
 
-        let block_difference = i32::try_from(
-            // Euclidean division is used so that the number is rounded towards negative infinity,
-            // so that fractionary values always round down to the previous height when going back
-            // in time (i.e., when the dividend is negative). This works because the divisor (the
-            // target spacing) is always positive.
-            time_difference_seconds.div_euclid(self.current_target_spacing.num_seconds()),
-        )
-        .expect("time difference is too large");
+        // Euclidean division is used so that the number is rounded towards negative infinity,
+        // so that fractionary values always round down to the previous height when going back
+        // in time (i.e., when the dividend is negative). This works because the divisor (the
+        // target spacing) is always positive.
+        let block_difference: HeightDiff =
+            time_difference_seconds.div_euclid(self.current_target_spacing.num_seconds());
+
+        let current_height_as_diff = HeightDiff::from(self.current_height.0);
 
-        if -(block_difference as i64) > self.current_height.0 as i64 {
+        if let Some(height_estimate) = self.current_height + block_difference {
+            height_estimate
+        } else if current_height_as_diff + block_difference < 0 {
             // Gracefully handle attempting to estimate a block before genesis. This can happen if
             // the local time is set incorrectly to a time too far in the past.
             block::Height(0)
         } else {
-            (self.current_height + block_difference).expect("block difference is too large")
+            // Gracefully handle attempting to estimate a block at a very large height. This can
+            // happen if the local time is set incorrectly to a time too far in the future.
+            block::Height::MAX
         }
     }
 }

zebra-chain/src/chain_tip/tests/prop.rs

@@ -71,10 +71,13 @@ fn estimate_time_difference(
     active_network_upgrade: NetworkUpgrade,
 ) -> Duration {
     let spacing_seconds = active_network_upgrade.target_spacing().num_seconds();
+    let height_difference = end_height - start_height;
 
-    let height_difference = (end_height - start_height).map_or(0, |height| height.into());
-
-    Duration::seconds(height_difference * spacing_seconds)
+    if height_difference > 0 {
+        Duration::seconds(height_difference * spacing_seconds)
+    } else {
+        Duration::zero()
+    }
 }
 
 /// Use `displacement` to get a displacement duration between zero and the target spacing of the

zebra-chain/src/parameters/network.rs

@@ -4,7 +4,10 @@ use std::{fmt, str::FromStr};
 
 use thiserror::Error;
 
-use crate::{block::Height, parameters::NetworkUpgrade::Canopy};
+use crate::{
+    block::{Height, HeightDiff},
+    parameters::NetworkUpgrade::Canopy,
+};
 
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
@@ -46,7 +49,7 @@ mod tests;
 /// period. Therefore Zebra must validate those blocks during the grace period using checkpoints.
 /// Therefore the mandatory checkpoint height ([`Network::mandatory_checkpoint_height`]) must be
 /// after the grace period.
-const ZIP_212_GRACE_PERIOD_DURATION: i32 = 32_256;
+const ZIP_212_GRACE_PERIOD_DURATION: HeightDiff = 32_256;
 
 /// An enum describing the possible network choices.
 #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, Serialize, Deserialize)]

zebra-consensus/src/block/check.rs

@@ -154,7 +154,7 @@ pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockErro
         .activation_height(network)
         .expect("Canopy activation height is known");
 
-    if (height - SLOW_START_INTERVAL).is_none() {
+    if height < SLOW_START_INTERVAL {
         unreachable!(
             "unsupported block height: callers should handle blocks below {:?}",
             SLOW_START_INTERVAL

zebra-consensus/src/block/subsidy/funding_streams.rs

@@ -69,13 +69,23 @@ pub fn height_for_first_halving(network: Network) -> Height {
 ///
 /// [7.10]: https://zips.z.cash/protocol/protocol.pdf#fundingstreams
 fn funding_stream_address_period(height: Height, network: Network) -> u32 {
-    // - Spec equation: `address_period = floor((height - height_for_halving(1) - post_blossom_halving_interval)/funding_stream_address_change_interval)`:
-    //   https://zips.z.cash/protocol/protocol.pdf#fundingstreams
-    // - In Rust, "integer division rounds towards zero":
-    //   https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators
+    // Spec equation: `address_period = floor((height - (height_for_halving(1) - post_blossom_halving_interval))/funding_stream_address_change_interval)`,
+    // <https://zips.z.cash/protocol/protocol.pdf#fundingstreams>
+    //
+    // Note that the brackets make it so the post blossom halving interval is added to the total.
+    //
+    // In Rust, "integer division rounds towards zero":
+    // <https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators>
     //   This is the same as `floor()`, because these numbers are all positive.
-    (height.0 + (POST_BLOSSOM_HALVING_INTERVAL as u32) - (height_for_first_halving(network).0))
-        / (FUNDING_STREAM_ADDRESS_CHANGE_INTERVAL.0)
+
+    let height_after_first_halving = height - height_for_first_halving(network);
+
+    let address_period = (height_after_first_halving + POST_BLOSSOM_HALVING_INTERVAL)
+        / FUNDING_STREAM_ADDRESS_CHANGE_INTERVAL;
+
+    address_period
+        .try_into()
+        .expect("all values are positive and smaller than the input height")
 }
 
 /// Returns the position in the address slice for each funding stream