State writing concurrency improvement (upstream issue: commit blocks …

…to state using a separate task ZcashFoundation#4937), added HeightDiff and height ops fixed, several read requests forwarded to ReadStateService

Cargo.toml

@@ -16,7 +16,7 @@ members = [
 ]
 
 [profile.dev]
-panic = "abort"
+panic = "unwind"
 
 # Speed up tests by optimizing performance-critical crates
 

zebra-chain/src/block.rs

@@ -36,7 +36,7 @@ pub use commitment::{
 };
 pub use hash::Hash;
 pub use header::{BlockTimeError, CountedHeader, Header};
-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

@@ -1,24 +1,23 @@
 //! Block height.
 
-use crate::serialization::{SerializationError, ZcashDeserialize};
+use std::ops::{Add, Sub};
 
-use byteorder::{LittleEndian, ReadBytesExt};
-
-use std::{
-    convert::TryFrom,
-    io,
-    ops::{Add, Sub},
-};
+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
 ///
 /// Users should not construct block heights greater than `Height::MAX`.
+///
+/// # Consensus
+///
+/// There are multiple formats for serializing a height, so we don't implement
+/// `ZcashSerialize` or `ZcashDeserialize` for `Height`.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub struct Height(pub u32);
 
@@ -65,106 +64,95 @@ 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")
         }
     }
 }
 
-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")
-    }
-}
-
 // We don't implement Add<u32> or Sub<u32>, because they cause type inference issues for integer constants.
 
-impl Add<i32> for Height {
-    type Output = Option<Height>;
+impl Sub<Height> for Height {
+    type Output = HeightDiff;
 
-    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,
-        }
+    /// 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
     }
 }
 
-impl Sub<i32> for Height {
-    type Output = Option<Height>;
+impl Sub<HeightDiff> for Height {
+    type Output = Option<Self>;
 
-    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,
-        }
+    /// 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;
+
+        // Check the bounds.
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
-impl ZcashDeserialize for Height {
-    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        let height = reader.read_u32::<LittleEndian>()?;
+impl Add<HeightDiff> for Height {
+    type Output = Option<Height>;
 
-        if height > Self::MAX.0 {
-            return Err(SerializationError::Parse("Height exceeds maximum 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 [`i64`] so we can add negative [`HeightDiff`]s.
+        let lhs = i64::from(self.0);
+        let res = lhs + rhs;
 
-        Ok(Self(height))
+        // Check the bounds.
+        let res = u32::try_from(res).ok()?;
+        Height::try_from(res).ok()
     }
 }
 
 #[test]
 fn operator_tests() {
-    zebra_test::init();
+    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);
@@ -175,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);
@@ -195,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);
 }