Support large block heights

zebra-chain/src/block/height.rs

@@ -1,11 +1,16 @@
-use crate::serialization::SerializationError;
+//! Block height.
+
+use crate::serialization::{SerializationError, ZcashDeserialize};
+
+use byteorder::{LittleEndian, ReadBytesExt};
 
 use std::{
     convert::TryFrom,
+    io,
     ops::{Add, Sub},
 };
 
-/// The height of a block is the length of the chain back to the genesis block.
+/// 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`,
@@ -29,32 +34,34 @@ impl std::str::FromStr for Height {
 }
 
 impl Height {
-    /// The minimum Height.
+    /// The minimum [`Height`].
     ///
     /// Due to the underlying type, it is impossible to construct block heights
-    /// less than `Height::MIN`.
+    /// less than [`Height::MIN`].
     ///
-    /// Style note: Sometimes, `Height::MIN` is less readable than
+    /// Style note: Sometimes, [`Height::MIN`] is less readable than
     /// `Height(0)`. Use whichever makes sense in context.
     pub const MIN: Height = Height(0);
 
-    /// The maximum Height.
+    /// The maximum [`Height`].
+    ///
+    /// Users should not construct block heights greater than [`Height::MAX`].
     ///
-    /// Users should not construct block heights greater than `Height::MAX`.
-    pub const MAX: Height = Height(499_999_999);
+    /// The spec says *"Implementations MUST support block heights up to and
+    /// including 2^31 − 1"*.
+    ///
+    /// Note that `u32::MAX / 2 == 2^31 - 1 == i32::MAX`.
+    pub const MAX: Height = Height(u32::MAX / 2);
 
-    /// The maximum Height as a u32, for range patterns.
+    /// The maximum [`Height`] as a [`u32`], for range patterns.
     ///
     /// `Height::MAX.0` can't be used in match range patterns, use this
     /// alias instead.
     pub const MAX_AS_U32: u32 = Self::MAX.0;
 
-    /// The maximum expiration Height that is allowed in all transactions
-    /// previous to Nu5 and in non-coinbase transactions from Nu5 activation height
-    /// and above.
-    ///
-    /// TODO: This is currently the same as `Height::MAX` but that change in #1113.
-    /// Remove this TODO when that happens.
+    /// The maximum expiration [`Height`] that is allowed in all transactions
+    /// previous to Nu5 and in non-coinbase transactions from Nu5 activation
+    /// height and above.
     pub const MAX_EXPIRY_HEIGHT: Height = Height(499_999_999);
 }
 
@@ -125,6 +132,18 @@ impl Sub<i32> for Height {
     }
 }
 
+impl ZcashDeserialize for Height {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        let height = reader.read_u32::<LittleEndian>()?;
+
+        if height > Self::MAX.0 {
+            return Err(SerializationError::Parse("Height exceeds maximum height"));
+        }
+
+        Ok(Self(height))
+    }
+}
+
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest::prelude::*;
 #[cfg(any(test, feature = "proptest-impl"))]
@@ -146,7 +165,7 @@ fn operator_tests() {
     assert_eq!(None, Height::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(0));
+    assert_eq!(None, Height(i32::MAX as u32) + Height(1));
     assert_eq!(None, Height(u32::MAX) + Height(0));
 
     assert_eq!(Some(Height(2)), Height(1) + 1);
@@ -162,7 +181,7 @@ fn operator_tests() {
     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);
+    assert_eq!(None, Height(i32::MAX as u32 + 1) + -1);
     assert_eq!(None, Height(u32::MAX) + -1);
 
     assert_eq!(Some(Height(1)), Height(2) - 1);
@@ -174,7 +193,7 @@ 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);
+    assert_eq!(None, 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);

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

@@ -1,3 +1,5 @@
+//! Sapling prop tests.
+
 use proptest::prelude::*;
 
 use crate::{
@@ -120,7 +122,7 @@ proptest! {
         let tx = Transaction::V4 {
             inputs: Vec::new(),
             outputs: Vec::new(),
-            lock_time: LockTime::min_lock_time(),
+            lock_time: LockTime::min_lock_time_timestamp(),
             expiry_height: block::Height(0),
             joinsplit_data: None,
             sapling_shielded_data: Some(shielded_v4),
@@ -182,7 +184,7 @@ proptest! {
         let tx = Transaction::V4 {
             inputs: Vec::new(),
             outputs: Vec::new(),
-            lock_time: LockTime::min_lock_time(),
+            lock_time: LockTime::min_lock_time_timestamp(),
             expiry_height: block::Height(0),
             joinsplit_data: None,
             sapling_shielded_data: Some(shielded_v4),

zebra-chain/src/serialization/error.rs

@@ -1,3 +1,5 @@
+//! Errors for transaction serialization.
+
 use std::{array::TryFromSliceError, io, num::TryFromIntError, str::Utf8Error};
 
 use thiserror::Error;

zebra-chain/src/transaction/arbitrary.rs

@@ -524,12 +524,13 @@ impl Arbitrary for Memo {
     type Strategy = BoxedStrategy<Self>;
 }
 
+/// Generates arbitrary [`LockTime`]s.
 impl Arbitrary for LockTime {
     type Parameters = ();
 
     fn arbitrary_with(_args: ()) -> Self::Strategy {
         prop_oneof![
-            (block::Height::MIN.0..=block::Height::MAX.0)
+            (block::Height::MIN.0..=LockTime::MAX_HEIGHT.0)
                 .prop_map(|n| LockTime::Height(block::Height(n))),
             (LockTime::MIN_TIMESTAMP..=LockTime::MAX_TIMESTAMP)
                 .prop_map(|n| { LockTime::Time(Utc.timestamp(n as i64, 0)) })

zebra-chain/src/transaction/lock_time.rs

@@ -1,66 +1,86 @@
+//! Transaction LockTime.
+
 use std::{convert::TryInto, io};
 
 use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
 use chrono::{DateTime, TimeZone, Utc};
 
-use crate::block;
+use crate::block::{self, Height};
 use crate::serialization::{SerializationError, ZcashDeserialize, ZcashSerialize};
 
 /// A Bitcoin-style `locktime`, representing either a block height or an epoch
 /// time.
 ///
 /// # Invariants
 ///
-/// Users should not construct a `LockTime` with:
-///   - a `block::Height` greater than MAX_BLOCK_HEIGHT,
+/// Users should not construct a [`LockTime`] with:
+///   - a [`block::Height`] greater than [`LockTime::MAX_HEIGHT`],
 ///   - a timestamp before 6 November 1985
-///     (Unix timestamp less than MIN_LOCK_TIMESTAMP), or
+///     (Unix timestamp less than [`LockTime::MIN_TIMESTAMP`]), or
 ///   - a timestamp after 5 February 2106
-///     (Unix timestamp greater than MAX_LOCK_TIMESTAMP).
+///     (Unix timestamp greater than [`LockTime::MAX_TIMESTAMP`]).
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
 pub enum LockTime {
-    /// Unlock at a particular block height.
+    /// The transaction can only be included in a block if the block height is
+    /// strictly greater than this height
     Height(block::Height),
-    /// Unlock at a particular time.
+    /// The transaction can only be included in a block if the block time is
+    /// strictly greater than this timestamp
     Time(DateTime<Utc>),
 }
 
 impl LockTime {
-    /// The minimum LockTime::Time, as a timestamp in seconds.
+    /// The minimum [`LockTime::Time`], as a Unix timestamp in seconds.
+    ///
+    /// Users should not construct [`LockTime`]s with [`LockTime::Time`] lower
+    /// than [`LockTime::MIN_TIMESTAMP`].
     ///
-    /// Users should not construct lock times less than `MIN_TIMESTAMP`.
+    /// If a [`LockTime`] is supposed to be lower than
+    /// [`LockTime::MIN_TIMESTAMP`], then a particular [`LockTime::Height`]
+    /// applies instead, as described in the spec.
     pub const MIN_TIMESTAMP: i64 = 500_000_000;
 
-    /// The maximum LockTime::Time, as a timestamp in seconds.
+    /// The maximum [`LockTime::Time`], as a timestamp in seconds.
     ///
-    /// Users should not construct lock times greater than `MAX_TIMESTAMP`.
-    /// LockTime is u32 in the spec, so times are limited to u32::MAX.
+    /// Users should not construct lock times with timestamps greater than
+    /// [`LockTime::MAX_TIMESTAMP`]. LockTime is [`u32`] in the spec, so times
+    /// are limited to [`u32::MAX`].
     pub const MAX_TIMESTAMP: i64 = u32::MAX as i64;
 
+    /// The maximum [`LockTime::Height`], as a block height.
+    ///
+    /// Users should not construct lock times with a block height greater than
+    /// [`LockTime::MAX_TIMESTAMP`].
+    ///
+    /// If a [`LockTime`] is supposed to be greater than
+    /// [`LockTime::MAX_HEIGHT`], then a particular [`LockTime::Time`] applies
+    /// instead, as described in the spec.
+    pub const MAX_HEIGHT: Height = Height((Self::MIN_TIMESTAMP - 1) as u32);
+
     /// Returns a [`LockTime`] that is always unlocked.
     ///
     /// The lock time is set to the block height of the genesis block.
     pub fn unlocked() -> Self {
         LockTime::Height(block::Height(0))
     }
 
-    /// Returns the minimum LockTime::Time, as a LockTime.
+    /// Returns the minimum [`LockTime::Time`], as a [`LockTime`].
     ///
-    /// Users should not construct lock times less than `min_lock_timestamp`.
+    /// Users should not construct lock times with timestamps lower than the
+    /// value returned by this function.
     //
-    // When `Utc.timestamp` stabilises as a const function, we can make this a
-    // const function.
-    pub fn min_lock_time() -> LockTime {
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
+    pub fn min_lock_time_timestamp() -> LockTime {
         LockTime::Time(Utc.timestamp(Self::MIN_TIMESTAMP, 0))
     }
 
-    /// Returns the maximum LockTime::Time, as a LockTime.
+    /// Returns the maximum [`LockTime::Time`], as a [`LockTime`].
     ///
-    /// Users should not construct lock times greater than `max_lock_timestamp`.
+    /// Users should not construct lock times with timestamps greater than the
+    /// value returned by this function.
     //
-    // When `Utc.timestamp` stabilises as a const function, we can make this a
-    // const function.
-    pub fn max_lock_time() -> LockTime {
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
+    pub fn max_lock_time_timestamp() -> LockTime {
         LockTime::Time(Utc.timestamp(Self::MAX_TIMESTAMP, 0))
     }
 }
@@ -82,10 +102,10 @@ impl ZcashSerialize for LockTime {
 impl ZcashDeserialize for LockTime {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         let n = reader.read_u32::<LittleEndian>()?;
-        if n <= block::Height::MAX.0 {
+        if n < Self::MIN_TIMESTAMP.try_into().expect("fits in u32") {
             Ok(LockTime::Height(block::Height(n)))
         } else {
-            // This can't panic, because all u32 values are valid `Utc.timestamp`s
+            // This can't panic, because all u32 values are valid `Utc.timestamp`s.
             Ok(LockTime::Time(Utc.timestamp(n.into(), 0)))
         }
     }

zebra-chain/src/transaction/serialize.rs

@@ -583,7 +583,7 @@ impl ZcashDeserialize for Transaction {
                     inputs: Vec::zcash_deserialize(&mut limited_reader)?,
                     outputs: Vec::zcash_deserialize(&mut limited_reader)?,
                     lock_time: LockTime::zcash_deserialize(&mut limited_reader)?,
-                    expiry_height: block::Height(limited_reader.read_u32::<LittleEndian>()?),
+                    expiry_height: block::Height::zcash_deserialize(&mut limited_reader)?,
                     joinsplit_data: OptV3Jsd::zcash_deserialize(&mut limited_reader)?,
                 })
             }
@@ -615,7 +615,7 @@ impl ZcashDeserialize for Transaction {
                 let inputs = Vec::zcash_deserialize(&mut limited_reader)?;
                 let outputs = Vec::zcash_deserialize(&mut limited_reader)?;
                 let lock_time = LockTime::zcash_deserialize(&mut limited_reader)?;
-                let expiry_height = block::Height(limited_reader.read_u32::<LittleEndian>()?);
+                let expiry_height = block::Height::zcash_deserialize(&mut limited_reader)?;
 
                 let value_balance = (&mut limited_reader).zcash_deserialize_into()?;
                 let shielded_spends = Vec::zcash_deserialize(&mut limited_reader)?;
@@ -684,9 +684,9 @@ impl ZcashDeserialize for Transaction {
                             "expected a valid network upgrade from the consensus branch id",
                         ))?;
 
-                // transaction validity time and height limits
+                // Transaction lock time and expiry height.
                 let lock_time = LockTime::zcash_deserialize(&mut limited_reader)?;
-                let expiry_height = block::Height(limited_reader.read_u32::<LittleEndian>()?);
+                let expiry_height = block::Height::zcash_deserialize(&mut limited_reader)?;
 
                 // transparent
                 let inputs = Vec::zcash_deserialize(&mut limited_reader)?;

zebra-chain/src/transaction/tests/vectors.rs

@@ -23,7 +23,7 @@ use super::super::*;
 lazy_static! {
     pub static ref EMPTY_V5_TX: Transaction = Transaction::V5 {
         network_upgrade: NetworkUpgrade::Nu5,
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: block::Height(0),
         inputs: Vec::new(),
         outputs: Vec::new(),
@@ -292,7 +292,7 @@ fn empty_v4_round_trip() {
     let tx = Transaction::V4 {
         inputs: Vec::new(),
         outputs: Vec::new(),
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: block::Height(0),
         joinsplit_data: None,
         sapling_shielded_data: None,

zebra-consensus/src/transaction.rs

@@ -293,7 +293,7 @@ where
             // (approximately 50,000 for a 2 MB transaction)
             let (utxo_sender, mut utxo_receiver) = mpsc::unbounded_channel();
 
-            // Do basic checks first
+            // Check the `lock_time` field of the transaction.
             if let Some(block_time) = req.block_time() {
                 check::lock_time_has_passed(&tx, req.height(), block_time)?;
             }
@@ -308,7 +308,7 @@ where
                 check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?;
             }
 
-            // Validate `nExpiryHeight` consensus rules
+            // Check the `nExpiryHeight` field of the transaction.
             if tx.has_any_coinbase_inputs() {
                 check::coinbase_expiry_height(&req.height(), &tx, network)?;
             } else {