Change HeightDiff to i64 and make Height.sub(Height) return HeightDif…

…f (no Option)

zebra-chain/src/block/height.rs

@@ -65,7 +65,10 @@ 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;
@@ -84,57 +87,47 @@ impl TryFrom<u32> for Height {
 // 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 `None` if:
-    /// - either height is outside the valid `i32` range ([`Height::MAX`] is currently [`i32::MAX`])
-    /// - the result is outside the valid `i32` range
+    /// 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 height is outside the valid `i32` range ([`Height::MAX`] is currently [`i32::MAX`])
-    /// - the resulting height is outside the valid `Height` range,
-    ///   this also checks the result is non-negative
+    /// 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()?;
+        // 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.
+        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 height is outside the valid `i32` range ([`Height::MAX`] is currently [`i32::MAX`])
-    /// - the resulting height is outside the valid `Height` range,
-    ///   this also checks the result is non-negative
+    /// 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()?;
+        // 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()
     }
 }
@@ -199,12 +192,12 @@ fn operator_tests() {
     assert_eq!(None, Height(i32::MAX as u32) - -1);
     assert_eq!(None, Height(u32::MAX) - -1);
 
-    assert_eq!(Some(1), (Height(2) - Height(1)));
-    assert_eq!(Some(0), (Height(1) - Height(1)));
-    assert_eq!(Some(-1), Height(0) - Height(1));
-    assert_eq!(Some(-5), Height(2) - Height(7));
-    assert_eq!(Some(Height::MAX), (Height::MAX - 0));
-    assert_eq!(Some(1), (Height::MAX - Height(Height::MAX_AS_U32 - 1)));
-    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/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/checkpoint/list/tests.rs

@@ -1,15 +1,15 @@
 //! Tests for CheckpointList
 
-use super::*;
-
 use std::sync::Arc;
 
-use zebra_chain::parameters::{Network, Network::*};
 use zebra_chain::{
-    block::{self, Block},
+    block::{self, Block, HeightDiff},
+    parameters::{Network, Network::*},
     serialization::ZcashDeserialize,
 };
 
+use super::*;
+
 /// Make a checkpoint list containing only the genesis block
 #[test]
 fn checkpoint_list_genesis() -> Result<(), BoxError> {
@@ -290,7 +290,8 @@ fn checkpoint_list_hard_coded_max_gap(network: Network) -> Result<(), BoxError>
     assert_eq!(heights.next(), Some(&previous_height));
 
     for height in heights {
-        let height_limit = (previous_height + (crate::MAX_CHECKPOINT_HEIGHT_GAP as i32)).unwrap();
+        let height_limit =
+            (previous_height + (crate::MAX_CHECKPOINT_HEIGHT_GAP as HeightDiff)).unwrap();
         assert!(
             height <= &height_limit,
             "Checkpoint gaps must be within MAX_CHECKPOINT_HEIGHT_GAP"

zebra-consensus/src/parameters/subsidy.rs

@@ -41,7 +41,7 @@ pub const PRE_BLOSSOM_HALVING_INTERVAL: HeightDiff = 840_000;
 
 /// After Blossom the block time is reduced to 75 seconds but halving period should remain around 4 years.
 pub const POST_BLOSSOM_HALVING_INTERVAL: HeightDiff =
-    PRE_BLOSSOM_HALVING_INTERVAL * (BLOSSOM_POW_TARGET_SPACING_RATIO as i32);
+    PRE_BLOSSOM_HALVING_INTERVAL * (BLOSSOM_POW_TARGET_SPACING_RATIO as HeightDiff);
 
 /// The first halving height in the testnet is at block height `1_116_000`
 /// as specified in [protocol specification §7.10.1][7.10.1]

zebra-state/src/constants.rs

@@ -15,6 +15,8 @@ pub use zebra_chain::transparent::MIN_TRANSPARENT_COINBASE_MATURITY;
 /// early non-finalized blocks, or finalized blocks. But if that chain becomes
 /// the best chain, all non-finalized blocks past the [`MAX_BLOCK_REORG_HEIGHT`]
 /// will be finalized. This includes all mature coinbase outputs.
+//
+// TODO: change to HeightDiff
 pub const MAX_BLOCK_REORG_HEIGHT: u32 = MIN_TRANSPARENT_COINBASE_MATURITY - 1;
 
 /// The database format version, incremented each time the database format changes.

zebra-state/src/service.rs

@@ -42,7 +42,7 @@ use tracing::{instrument, Instrument, Span};
 use tower::buffer::Buffer;
 
 use zebra_chain::{
-    block::{self, CountedHeader},
+    block::{self, CountedHeader, HeightDiff},
     diagnostic::CodeTimer,
     parameters::{Network, NetworkUpgrade},
 };
@@ -391,7 +391,8 @@ impl StateService {
         );
 
         let full_verifier_utxo_lookahead = max_checkpoint_height
-            - i32::try_from(checkpoint_verify_concurrency_limit).expect("fits in i32");
+            - HeightDiff::try_from(checkpoint_verify_concurrency_limit)
+                .expect("fits in HeightDiff");
         let full_verifier_utxo_lookahead =
             full_verifier_utxo_lookahead.expect("unexpected negative height");
 

zebrad/src/components/inbound/downloads.rs

@@ -2,7 +2,6 @@
 
 use std::{
     collections::HashMap,
-    convert::TryFrom,
     pin::Pin,
     task::{Context, Poll},
 };
@@ -17,7 +16,10 @@ use tokio::{sync::oneshot, task::JoinHandle};
 use tower::{Service, ServiceExt};
 use tracing_futures::Instrument;
 
-use zebra_chain::{block, chain_tip::ChainTip};
+use zebra_chain::{
+    block::{self, HeightDiff},
+    chain_tip::ChainTip,
+};
 use zebra_network as zn;
 use zebra_state as zs;
 
@@ -281,7 +283,8 @@ where
             let tip_height = latest_chain_tip.best_tip_height();
 
             let max_lookahead_height = if let Some(tip_height) = tip_height {
-                let lookahead = i32::try_from(full_verify_concurrency_limit).expect("fits in i32");
+                let lookahead = HeightDiff::try_from(full_verify_concurrency_limit)
+                    .expect("fits in HeightDiff");
                 (tip_height + lookahead).expect("tip is much lower than Height::MAX")
             } else {
                 let genesis_lookahead =

zebrad/src/components/sync.rs

@@ -15,7 +15,7 @@ use tower::{
 };
 
 use zebra_chain::{
-    block::{self, Height},
+    block::{self, Height, HeightDiff},
     chain_tip::ChainTip,
     parameters::genesis_hash,
 };
@@ -165,7 +165,7 @@ const FINAL_CHECKPOINT_BLOCK_VERIFY_TIMEOUT: Duration = Duration::from_secs(2 *
 /// The number of blocks after the final checkpoint that get the shorter timeout.
 ///
 /// We've only seen this error on the first few blocks after the final checkpoint.
-const FINAL_CHECKPOINT_BLOCK_VERIFY_TIMEOUT_LIMIT: i32 = 100;
+const FINAL_CHECKPOINT_BLOCK_VERIFY_TIMEOUT_LIMIT: HeightDiff = 100;
 
 /// Controls how long we wait to restart syncing after finishing a sync run.
 ///

zebrad/src/components/sync/downloads.rs

@@ -24,7 +24,7 @@ use tower::{hedge, Service, ServiceExt};
 use tracing_futures::Instrument;
 
 use zebra_chain::{
-    block::{self, Height},
+    block::{self, Height, HeightDiff},
     chain_tip::ChainTip,
 };
 use zebra_network as zn;
@@ -56,7 +56,7 @@ pub const VERIFICATION_PIPELINE_SCALING_MULTIPLIER: usize = 2;
 
 /// The maximum height difference between Zebra's state tip and a downloaded block.
 /// Blocks higher than this will get dropped and return an error.
-pub const VERIFICATION_PIPELINE_DROP_LIMIT: i32 = 50_000;
+pub const VERIFICATION_PIPELINE_DROP_LIMIT: HeightDiff = 50_000;
 
 #[derive(Copy, Clone, Debug)]
 pub(super) struct AlwaysHedge;
@@ -388,10 +388,10 @@ where
                 let (lookahead_drop_height, lookahead_pause_height, lookahead_reset_height) = if let Some(tip_height) = tip_height {
                     // Scale the height limit with the lookahead limit,
                     // so users with low capacity or under DoS can reduce them both.
-                    let lookahead_pause = i32::try_from(
+                    let lookahead_pause = HeightDiff::try_from(
                         lookahead_limit + lookahead_limit * VERIFICATION_PIPELINE_SCALING_MULTIPLIER,
                     )
-                        .expect("fits in i32");
+                        .expect("fits in HeightDiff");
 
 
                     ((tip_height + VERIFICATION_PIPELINE_DROP_LIMIT).expect("tip is much lower than Height::MAX"),

zebrad/src/components/sync/progress.rs

@@ -6,7 +6,7 @@ use chrono::Utc;
 use num_integer::div_ceil;
 
 use zebra_chain::{
-    block::Height,
+    block::{Height, HeightDiff},
     chain_sync_status::ChainSyncStatus,
     chain_tip::ChainTip,
     fmt::humantime_seconds,
@@ -23,14 +23,14 @@ const LOG_INTERVAL: Duration = Duration::from_secs(60);
 /// The number of blocks we consider to be close to the tip.
 ///
 /// Most chain forks are 1-7 blocks long.
-const MAX_CLOSE_TO_TIP_BLOCKS: i32 = 1;
+const MAX_CLOSE_TO_TIP_BLOCKS: HeightDiff = 1;
 
 /// Skip slow sync warnings when we are this close to the tip.
 ///
 /// In testing, we've seen warnings around 30 blocks.
 ///
 /// TODO: replace with `MAX_CLOSE_TO_TIP_BLOCKS` after fixing slow syncing near tip (#3375)
-const MIN_SYNC_WARNING_BLOCKS: i32 = 60;
+const MIN_SYNC_WARNING_BLOCKS: HeightDiff = 60;
 
 /// The number of fractional digits in sync percentages.
 const SYNC_PERCENT_FRAC_DIGITS: usize = 3;
@@ -49,6 +49,8 @@ const SYNC_PERCENT_FRAC_DIGITS: usize = 3;
 ///
 /// We might add tests that sync from a cached tip state,
 /// so we only allow a few extra blocks here.
+//
+// TODO: change to HeightDiff?
 const MIN_BLOCKS_MINED_AFTER_CHECKPOINT_UPDATE: u32 = 10;
 
 /// Logs Zebra's estimated progress towards the chain tip every minute or so.
@@ -67,9 +69,7 @@ pub async fn show_block_chain_progress(
     //   and the automated tests for that update.
     let min_after_checkpoint_blocks =
         MAX_BLOCK_REORG_HEIGHT + MIN_BLOCKS_MINED_AFTER_CHECKPOINT_UPDATE;
-    let min_after_checkpoint_blocks: i32 = min_after_checkpoint_blocks
-        .try_into()
-        .expect("constant fits in i32");
+    let min_after_checkpoint_blocks: HeightDiff = min_after_checkpoint_blocks.into();
 
     // The minimum height of the valid best chain, based on:
     // - the hard-coded checkpoint height,