change(chain): Refactor the handling of height differences (#6330)

* Unify the `impl`s of `Sub` and `Add` for `Height`

* Adjust tests for `Height` subtraction

* Use `Height` instead of `i32`

* Use `block:Height` in RPC tests

* Use `let .. else` statement

Co-authored-by: Arya <[email protected]>

* Update zebra-consensus/src/block/subsidy/general.rs

* Refactor the handling of height differences

* Remove a redundant comment

* Update zebrad/src/components/sync/progress.rs

Co-authored-by: Arya <[email protected]>

* Update progress.rs

* impl TryFrom<u32> for Height

* Make some test assertions clearer

* Refactor estimate_up_to()

* Restore a comment that was accidentally removed

* Document when estimate_distance_to_network_chain_tip() returns None

* Change HeightDiff to i64 and make Height.sub(Height) return HeightDiff (no Option)

* Update chain tip estimates for HeightDiff i64

* Update subsidy for HeightDiff i64

* Fix some height calculation test edge cases

* Fix the funding stream interval calculation

---------

Co-authored-by: Arya <[email protected]>
Co-authored-by: teor <[email protected]>

zebra-chain/src/block.rs

@@ -35,7 +35,7 @@ pub use commitment::{
 };
 pub use hash::Hash;
 pub use header::{BlockTimeError, CountedHeader, Header, ZCASH_BLOCK_VERSION};
-pub use height::Height;
+pub use height::{Height, HeightDiff};
 pub use serialize::{SerializedBlock, MAX_BLOCK_BYTES};
 
 #[cfg(any(test, feature = "proptest-impl"))]

zebra-chain/src/block/height.rs

@@ -6,9 +6,9 @@ use crate::serialization::SerializationError;
 
 /// The length of the chain back to the genesis block.
 ///
-/// Block heights can't be added, but they can be *subtracted*,
-/// to get a difference of block heights, represented as an `i32`,
-/// and height differences can be added to block heights to get new heights.
+/// Two [`Height`]s can't be added, but they can be *subtracted* to get their difference,
+/// represented as an [`HeightDiff`]. This difference can then be added to or subtracted from a
+/// [`Height`]. Note the similarity with `chrono::DateTime` and `chrono::Duration`.
 ///
 /// # Invariants
 ///
@@ -64,70 +64,71 @@ impl Height {
     pub const MAX_EXPIRY_HEIGHT: Height = Height(499_999_999);
 }
 
-impl Add<Height> for Height {
-    type Output = Option<Height>;
-
-    fn add(self, rhs: Height) -> Option<Height> {
-        // We know that both values are positive integers. Therefore, the result is
-        // positive, and we can skip the conversions. The checked_add is required,
-        // because the result may overflow.
-        let height = self.0.checked_add(rhs.0)?;
-        let height = Height(height);
-
-        if height <= Height::MAX && height >= Height::MIN {
-            Some(height)
+/// A difference between two [`Height`]s, possibly negative.
+///
+/// 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 {
-            None
+            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 = i32;
-
-    /// Panics if the inputs or result are outside the valid i32 range.
-    fn sub(self, rhs: Height) -> i32 {
-        // We construct heights from integers without any checks,
-        // so the inputs or result could be out of range.
-        let lhs = i32::try_from(self.0)
-            .expect("out of range input `self`: inputs should be valid Heights");
-        let rhs =
-            i32::try_from(rhs.0).expect("out of range input `rhs`: inputs should be valid Heights");
-        lhs.checked_sub(rhs)
-            .expect("out of range result: valid input heights should yield a valid result")
+    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 {
+        // 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
     }
 }
 
-// We don't implement Add<u32> or Sub<u32>, because they cause type inference issues for integer constants.
+impl Sub<HeightDiff> for Height {
+    type Output = Option<Self>;
 
-impl Add<i32> for Height {
-    type Output = Option<Height>;
+    /// 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 [`i64`] so we can subtract negative [`HeightDiff`]s.
+        let lhs = HeightDiff::from(self.0);
+        let res = lhs - rhs;
 
-    fn add(self, rhs: i32) -> Option<Height> {
-        // Because we construct heights from integers without any checks,
-        // the input values could be outside the valid range for i32.
-        let lhs = i32::try_from(self.0).ok()?;
-        let result = lhs.checked_add(rhs)?;
-        let result = u32::try_from(result).ok()?;
-        match result {
-            h if (Height(h) <= Height::MAX && Height(h) >= Height::MIN) => Some(Height(h)),
-            _ => None,
-        }
+        // Check the bounds.
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
-impl Sub<i32> for Height {
+impl Add<HeightDiff> for Height {
     type Output = Option<Height>;
 
-    fn sub(self, rhs: i32) -> Option<Height> {
-        // These checks are required, see above for details.
-        let lhs = i32::try_from(self.0).ok()?;
-        let result = lhs.checked_sub(rhs)?;
-        let result = u32::try_from(result).ok()?;
-        match result {
-            h if (Height(h) <= Height::MAX && Height(h) >= Height::MIN) => Some(Height(h)),
-            _ => None,
-        }
+    /// 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 [`i64`] so we can add negative [`HeightDiff`]s.
+        let lhs = i64::from(self.0);
+        let res = lhs + rhs;
+
+        // Check the bounds.
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
@@ -136,22 +137,22 @@ fn operator_tests() {
     let _init_guard = zebra_test::init();
 
     // Elementary checks.
-    assert_eq!(Some(Height(2)), Height(1) + Height(1));
-    assert_eq!(None, Height::MAX + Height(1));
+    assert_eq!(Some(Height(2)), Height(1) + 1);
+    assert_eq!(None, Height::MAX + 1);
 
     let height = Height(u32::pow(2, 31) - 2);
     assert!(height < Height::MAX);
 
-    let max_height = (height + Height(1)).expect("this addition should produce the max height");
+    let max_height = (height + 1).expect("this addition should produce the max height");
     assert!(height < max_height);
     assert!(max_height <= Height::MAX);
     assert_eq!(Height::MAX, max_height);
-    assert_eq!(None, max_height + Height(1));
+    assert_eq!(None, max_height + 1);
 
     // Bad heights aren't caught at compile-time or runtime, until we add or subtract
-    assert_eq!(None, Height(Height::MAX_AS_U32 + 1) + Height(0));
-    assert_eq!(None, Height(i32::MAX as u32) + Height(1));
-    assert_eq!(None, Height(u32::MAX) + Height(0));
+    assert_eq!(None, Height(Height::MAX_AS_U32 + 1) + 0);
+    assert_eq!(None, Height(i32::MAX as u32) + 1);
+    assert_eq!(None, Height(u32::MAX) + 0);
 
     assert_eq!(Some(Height(2)), Height(1) + 1);
     assert_eq!(None, Height::MAX + 1);
@@ -162,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);
@@ -182,22 +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);
 
-    // Sub<Height> panics on out of range errors
-    assert_eq!(1, Height(2) - Height(1));
-    assert_eq!(0, Height(1) - Height(1));
+    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_AS_U32 as i32, Height::MAX - Height(0));
-    assert_eq!(1, Height::MAX - Height(Height::MAX_AS_U32 - 1));
+    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 i32), Height(0) - Height::MAX);
+    assert_eq!(-(Height::MAX_AS_U32 as HeightDiff), Height(0) - Height::MAX);
 }

zebra-chain/src/chain_tip.rs

@@ -95,26 +95,31 @@ pub trait ChainTip {
         Some(estimator.estimate_height_at(now))
     }
 
-    /// Return an estimate of how many blocks there are ahead of Zebra's best chain tip
-    /// until the network chain tip, and Zebra's best chain tip height.
+    /// Return an estimate of how many blocks there are ahead of Zebra's best chain tip until the
+    /// network chain tip, and Zebra's best chain tip height.
+    ///
+    /// The first element in the returned tuple is the estimate.
+    /// The second element in the returned tuple is the current best chain tip.
     ///
     /// The estimate is calculated based on the current local time, the block time of the best tip
     /// and the height of the best tip.
     ///
-    /// This estimate may be negative if the current local time is behind the chain tip block's timestamp.
+    /// 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,
-    ) -> Option<(i32, block::Height)> {
+    ) -> Option<(block::HeightDiff, block::Height)> {
         let (current_height, current_block_time) = self.best_tip_height_and_block_time()?;
 
         let estimator =
             NetworkChainTipHeightEstimator::new(current_block_time, current_height, network);
 
-        Some((
-            estimator.estimate_height_at(Utc::now()) - current_height,
-            current_height,
-        ))
+        let distance_to_tip = estimator.estimate_height_at(Utc::now()) - current_height;
+
+        Some((distance_to_tip, current_height))
     }
 }
 

zebra-chain/src/chain_tip/mock.rs

@@ -27,7 +27,7 @@ pub struct MockChainTipSender {
     best_tip_block_time: watch::Sender<Option<DateTime<Utc>>>,
 
     /// A sender that sets the `estimate_distance_to_network_chain_tip` of a [`MockChainTip`].
-    estimated_distance_to_network_chain_tip: watch::Sender<Option<i32>>,
+    estimated_distance_to_network_chain_tip: watch::Sender<Option<block::HeightDiff>>,
 }
 
 /// A mock [`ChainTip`] implementation that allows setting the `best_tip_height` externally.
@@ -43,7 +43,7 @@ pub struct MockChainTip {
     best_tip_block_time: watch::Receiver<Option<DateTime<Utc>>>,
 
     /// A mocked `estimate_distance_to_network_chain_tip` value set by the [`MockChainTipSender`].
-    estimated_distance_to_network_chain_tip: watch::Receiver<Option<i32>>,
+    estimated_distance_to_network_chain_tip: watch::Receiver<Option<block::HeightDiff>>,
 }
 
 impl MockChainTip {
@@ -112,7 +112,7 @@ impl ChainTip for MockChainTip {
     fn estimate_distance_to_network_chain_tip(
         &self,
         _network: Network,
-    ) -> Option<(i32, block::Height)> {
+    ) -> Option<(block::HeightDiff, block::Height)> {
         self.estimated_distance_to_network_chain_tip
             .borrow()
             .and_then(|estimated_distance| {
@@ -179,7 +179,10 @@ impl MockChainTipSender {
     }
 
     /// Send a new estimated distance to network chain tip to the [`MockChainTip`].
-    pub fn send_estimated_distance_to_network_chain_tip(&self, distance: impl Into<Option<i32>>) {
+    pub fn send_estimated_distance_to_network_chain_tip(
+        &self,
+        distance: impl Into<Option<block::HeightDiff>>,
+    ) {
         self.estimated_distance_to_network_chain_tip
             .send(distance.into())
             .expect("attempt to send a best tip height to a dropped `MockChainTip`");

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,12 +87,11 @@ 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) {
-        let remaining_blocks = i64::from(max_height - self.current_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;
         }
@@ -119,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 = i64::from(end_height - start_height);
-
-    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)]