diff --git a/zebra-chain/src/block/height.rs b/zebra-chain/src/block/height.rs index 4af86ff9c38..c08234ddf1a 100644 --- a/zebra-chain/src/block/height.rs +++ b/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 for Height { type Error = &'static str; @@ -84,37 +87,31 @@ impl TryFrom for Height { // We don't implement Add or Sub, because they cause type inference issues for integer constants. impl Sub for Height { - type Output = Option; + 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 for Height { type Output = Option; - /// 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 { - // 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() } } @@ -122,19 +119,15 @@ impl Sub for Height { impl Add for Height { type Output = Option; - /// 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 { - // 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); } diff --git a/zebra-chain/src/parameters/network.rs b/zebra-chain/src/parameters/network.rs index d23abe94417..6c6319ce765 100644 --- a/zebra-chain/src/parameters/network.rs +++ b/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)] diff --git a/zebra-consensus/src/checkpoint/list/tests.rs b/zebra-consensus/src/checkpoint/list/tests.rs index 29bc16928d1..60b4ec9ef99 100644 --- a/zebra-consensus/src/checkpoint/list/tests.rs +++ b/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" diff --git a/zebra-consensus/src/parameters/subsidy.rs b/zebra-consensus/src/parameters/subsidy.rs index b62b6a64ce6..cca22955339 100644 --- a/zebra-consensus/src/parameters/subsidy.rs +++ b/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] diff --git a/zebra-state/src/constants.rs b/zebra-state/src/constants.rs index b91fe29d755..166d93dd6a3 100644 --- a/zebra-state/src/constants.rs +++ b/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. diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index 0fe50c8380c..ea4a63b6aa5 100644 --- a/zebra-state/src/service.rs +++ b/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"); diff --git a/zebrad/src/components/inbound/downloads.rs b/zebrad/src/components/inbound/downloads.rs index a8315b75812..aa8b2cf6c25 100644 --- a/zebrad/src/components/inbound/downloads.rs +++ b/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 = diff --git a/zebrad/src/components/sync.rs b/zebrad/src/components/sync.rs index e74be242077..c0cc3889e56 100644 --- a/zebrad/src/components/sync.rs +++ b/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. /// diff --git a/zebrad/src/components/sync/downloads.rs b/zebrad/src/components/sync/downloads.rs index 4b6b5f00256..fde200f7fc8 100644 --- a/zebrad/src/components/sync/downloads.rs +++ b/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"), diff --git a/zebrad/src/components/sync/progress.rs b/zebrad/src/components/sync/progress.rs index 8befd233484..840436c1c1b 100644 --- a/zebrad/src/components/sync/progress.rs +++ b/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,