Support large block heights (#3401)

* Support large block heights

* Document consensus rules referring to expiry heights

* Refactor the docs

* Apply suggestions from code review

Co-authored-by: teor <teor@riseup.net>

* Fix the formatting of an error message

* refactor: Simplify coinbase expiry code so the consensus rule is clear (#3408)

* Fix some outdated TODO comments

* refactor(coinbase expiry): Simplify the code so consensus rule is clear

* Fix the formatting of an error message

* Remove a redundant comment

Co-authored-by: Marek <mail@marek.onl>

Co-authored-by: Marek <mail@marek.onl>

* Check the max expiry height at parse time

* Test that 2^31 - 1 is the last valid height

* Add tests for nExpiryHeight

* Add tests for expiry heights of V4 transactions

* Add tests for V5 transactions

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

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,44 +132,73 @@ 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))
+    }
+}
+
 #[test]
 fn operator_tests() {
     zebra_test::init();
 
+    // Elementary checks.
     assert_eq!(Some(Height(2)), Height(1) + Height(1));
     assert_eq!(None, Height::MAX + Height(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");
+    assert!(height < max_height);
+    assert!(max_height <= Height::MAX);
+    assert_eq!(Height::MAX, max_height);
+    assert_eq!(None, max_height + 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);
     assert_eq!(None, Height::MAX + 1);
+
     // Adding negative numbers
     assert_eq!(Some(Height(1)), Height(2) + -1);
     assert_eq!(Some(Height(0)), Height(1) + -1);
     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
     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);
+    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);
     assert_eq!(Some(Height(0)), Height(1) - 1);
     assert_eq!(None, Height(0) - 1);
     assert_eq!(Some(Height(Height::MAX_AS_U32 - 1)), Height::MAX - 1);
+
     // Subtracting negative numbers
     assert_eq!(Some(Height(2)), Height(1) - -1);
     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);
     assert_eq!(None, Height(i32::MAX as u32) - -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/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(),
@@ -302,7 +302,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/block.rs

@@ -153,8 +153,8 @@ where
                 .coinbase_height()
                 .ok_or(BlockError::MissingHeight(hash))?;
 
-            // TODO: support block heights up to u32::MAX (#1113)
-            // In practice, these blocks are invalid anyway, because their parent block doesn't exist.
+            // Zebra does not support heights greater than
+            // [`block::Height::MAX`].
             if height > block::Height::MAX {
                 Err(BlockError::MaxHeight(height, hash, block::Height::MAX))?;
             }