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
+    /// The maximum expiration [`Height`] that is allowed in all transactions
-    /// previous to Nu5 and in non-coinbase transactions from Nu5 activation height
+    /// previous to Nu5 and in non-coinbase transactions from Nu5 activation
-    /// and above.
+    /// height and above.
-    ///
-    /// TODO: This is currently the same as `Height::MAX` but that change in #1113.
-    /// Remove this TODO when that happens.
     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,9 +1,11 @@
+//! 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
@@ -11,32 +13,50 @@ use crate::serialization::{SerializationError, ZcashDeserialize, ZcashSerialize}
 ///
 /// # Invariants
 ///
-/// Users should not construct a `LockTime` with:
+/// Users should not construct a [`LockTime`] with:
-///   - a `block::Height` greater than MAX_BLOCK_HEIGHT,
+///   - 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 lock times less than `MIN_TIMESTAMP`.
+    /// Users should not construct [`LockTime`]s with [`LockTime::Time`] lower
+    /// than [`LockTime::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`.
+    /// Users should not construct lock times with timestamps greater than
-    /// LockTime is u32 in the spec, so times are limited to u32::MAX.
+    /// [`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.
@@ -44,23 +64,23 @@ impl LockTime {
         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
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
-    // const function.
+    pub fn min_lock_time_timestamp() -> LockTime {
-    pub fn min_lock_time() -> 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
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
-    // const function.
+    pub fn max_lock_time_timestamp() -> LockTime {
-    pub fn max_lock_time() -> 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)
+            // Zebra does not support heights greater than
-            // In practice, these blocks are invalid anyway, because their parent block doesn't exist.
+            // [`block::Height::MAX`].
             if height > block::Height::MAX {
                 Err(BlockError::MaxHeight(height, hash, block::Height::MAX))?;
             }

zebra-consensus/src/transaction/check.rs

@@ -310,8 +310,8 @@ pub fn coinbase_outputs_are_decryptable(
 /// Returns `Ok(())` if the expiry height for the coinbase transaction is valid
 /// according to specifications [7.1] and [ZIP-203].
 ///
-/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+/// [7.1]: <https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus>
-/// [ZIP-203]: https://zips.z.cash/zip-0203
+/// [ZIP-203]: <https://zips.z.cash/zip-0203>
 pub fn coinbase_expiry_height(
     block_height: &Height,
     coinbase: &Transaction,
@@ -319,51 +319,41 @@ pub fn coinbase_expiry_height(
 ) -> Result<(), TransactionError> {
     let expiry_height = coinbase.expiry_height();
 
-    match NetworkUpgrade::Nu5.activation_height(network) {
+    // TODO: replace `if let` with `expect` after NU5 mainnet activation
-        // If Nu5 does not have a height, apply the pre-Nu5 rule.
+    if let Some(nu5_activation_height) = NetworkUpgrade::Nu5.activation_height(network) {
-        None => validate_expiry_height_max(expiry_height, true, block_height, coinbase),
+        // # Consensus
-        Some(activation_height) => {
+        //
-            // # Consensus
+        // > [NU5 onward] The nExpiryHeight field of a coinbase transaction
-            //
+        // > MUST be equal to its block height.
-            // > [NU5 onward] The nExpiryHeight field of a coinbase transaction MUST be equal
+        //
-            // > to its block height.
+        // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
-            //
+        if *block_height >= nu5_activation_height {
-            // https://zips.z.cash/protocol/protocol.pdf#txnconsensus
+            if expiry_height != Some(*block_height) {
-            if *block_height >= activation_height {
+                return Err(TransactionError::CoinbaseExpiryBlockHeight {
-                match expiry_height {
+                    expiry_height,
-                    None => Err(TransactionError::CoinbaseExpiryBlockHeight {
+                    block_height: *block_height,
-                        expiry_height,
+                    transaction_hash: coinbase.hash(),
-                        block_height: *block_height,
+                });
-                        transaction_hash: coinbase.hash(),
+            } else {
-                    })?,
-                    Some(expiry) => {
-                        if expiry != *block_height {
-                            return Err(TransactionError::CoinbaseExpiryBlockHeight {
-                                expiry_height,
-                                block_height: *block_height,
-                                transaction_hash: coinbase.hash(),
-                            })?;
-                        }
-                    }
-                }
                 return Ok(());
             }
-            // # Consensus
-            //
-            // > [Overwinter to Canopy inclusive, pre-NU5] `nExpiryHeight` MUST be less than
-            // > or equal to 499999999.
-            //
-            // https://zips.z.cash/protocol/protocol.pdf#txnconsensus
-            validate_expiry_height_max(expiry_height, true, block_height, coinbase)
         }
     }
+
+    // # Consensus
+    //
+    // > [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight MUST be less than
+    // > or equal to 499999999.
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
+    validate_expiry_height_max(expiry_height, true, block_height, coinbase)
 }
 
-/// Returns `Ok(())` if the expiry height for a non coinbase transaction is valid
+/// Returns `Ok(())` if the expiry height for a non coinbase transaction is
-/// according to specifications [7.1] and [ZIP-203].
+/// valid according to specifications [7.1] and [ZIP-203].
 ///
-/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+/// [7.1]: <https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus>
-/// [ZIP-203]: https://zips.z.cash/zip-0203
+/// [ZIP-203]: <https://zips.z.cash/zip-0203>
 pub fn non_coinbase_expiry_height(
     block_height: &Height,
     transaction: &Transaction,
@@ -379,21 +369,27 @@ pub fn non_coinbase_expiry_height(
         // > [NU5 onward] nExpiryHeight MUST be less than or equal to 499999999
         // > for non-coinbase transactions.
         //
-        // > [Overwinter onward] If a transaction is not a coinbase transaction and its
+        // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
-        // > nExpiryHeight field is nonzero, then it MUST NOT be mined at a block height
-        // > greater than its nExpiryHeight.
-        //
-        // https://zips.z.cash/protocol/protocol.pdf#txnconsensus
         validate_expiry_height_max(expiry_height, false, block_height, transaction)?;
+
+        // # Consensus
+        //
+        // > [Overwinter onward] If a transaction is not a coinbase transaction and its
+        // > nExpiryHeight field is nonzero, then it MUST NOT be mined at a block
+        // > height greater than its nExpiryHeight.
+        //
+        // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
         validate_expiry_height_mined(expiry_height, block_height, transaction)?;
     }
     Ok(())
 }
 
-/// Validate the consensus rule: nExpiryHeight MUST be less than or equal to 499999999.
+/// Checks that the expiry height of a transaction does not exceed the maximal
+/// value.
 ///
-/// The remaining arguments are not used for validation,
+/// Only the `expiry_height` parameter is used for the check. The
-/// they are only used to create errors.
+/// remaining parameters are used to give details about the error when the check
+/// fails.
 fn validate_expiry_height_max(
     expiry_height: Option<Height>,
     is_coinbase: bool,
@@ -414,11 +410,10 @@ fn validate_expiry_height_max(
     Ok(())
 }
 
-/// Validate the consensus rule: If a transaction is not a coinbase transaction
+/// Checks that a transaction does not exceed its expiry height.
-/// and its nExpiryHeight field is nonzero, then it MUST NOT be mined at a block
-/// height greater than its nExpiryHeight.
 ///
-/// The `transaction` is only used to create errors.
+/// The `transaction` parameter is only used to give details about the error
+/// when the check fails.
 fn validate_expiry_height_mined(
     expiry_height: Option<Height>,
     block_height: &Height,

zebra-consensus/src/transaction/tests.rs

@@ -1,3 +1,5 @@
+//! Tests for Zcash transaction consensus checks.
+
 use std::{
     cmp::max,
     collections::HashMap,
@@ -380,6 +382,227 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() {
     );
 }
 
+/// Tests if a non-coinbase V4 transaction with the last valid expiry height is
+/// accepted.
+#[tokio::test]
+async fn v4_transaction_with_last_valid_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = NetworkUpgrade::Canopy
+        .activation_height(Network::Mainnet)
+        .expect("Canopy activation height is specified");
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // Create a non-coinbase V4 tx with the last valid expiry height.
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height: block_height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result.expect("unexpected error response").tx_id(),
+        transaction.unmined_id()
+    );
+}
+
+/// Tests if a coinbase V4 transaction with an expiry height lower than the
+/// block height is accepted.
+///
+/// Note that an expiry height lower than the block height is considered
+/// *expired* for *non-coinbase* transactions.
+#[tokio::test]
+async fn v4_coinbase_transaction_with_low_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = NetworkUpgrade::Canopy
+        .activation_height(Network::Mainnet)
+        .expect("Canopy activation height is specified");
+
+    let (input, output) = mock_coinbase_transparent_output(block_height);
+
+    // This is a correct expiry height for coinbase V4 transactions.
+    let expiry_height = (block_height - 1).expect("original block height is too small");
+
+    // Create a coinbase V4 tx.
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result.expect("unexpected error response").tx_id(),
+        transaction.unmined_id()
+    );
+}
+
+/// Tests if an expired non-coinbase V4 transaction is rejected.
+#[tokio::test]
+async fn v4_transaction_with_too_low_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = NetworkUpgrade::Canopy
+        .activation_height(Network::Mainnet)
+        .expect("Canopy activation height is specified");
+
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // This expiry height is too low so that the tx should seem expired to the verifier.
+    let expiry_height = (block_height - 1).expect("original block height is too small");
+
+    // Create a non-coinbase V4 tx.
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::ExpiredTransaction {
+            expiry_height,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
+/// Tests if a non-coinbase V4 transaction with an expiry height exceeding the
+/// maximum is rejected.
+#[tokio::test]
+async fn v4_transaction_with_exceeding_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = block::Height::MAX;
+
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // This expiry height exceeds the maximum defined by the specification.
+    let expiry_height = block::Height(500_000_000);
+
+    // Create a non-coinbase V4 tx.
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::MaximumExpiryHeight {
+            expiry_height,
+            is_coinbase: false,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
+/// Tests if a coinbase V4 transaction with an expiry height exceeding the
+/// maximum is rejected.
+#[tokio::test]
+async fn v4_coinbase_transaction_with_exceeding_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = block::Height::MAX;
+
+    let (input, output) = mock_coinbase_transparent_output(block_height);
+
+    // This expiry height exceeds the maximum defined by the specification.
+    let expiry_height = block::Height(500_000_000);
+
+    // Create a coinbase V4 tx.
+    let transaction = Transaction::V4 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        joinsplit_data: None,
+        sapling_shielded_data: None,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::MaximumExpiryHeight {
+            expiry_height,
+            is_coinbase: true,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
 /// Test if V4 coinbase transaction is accepted.
 #[tokio::test]
 async fn v4_coinbase_transaction_is_accepted() {
@@ -717,6 +940,277 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() {
     );
 }
 
+/// Tests if a non-coinbase V5 transaction with the last valid expiry height is
+/// accepted.
+#[tokio::test]
+async fn v5_transaction_with_last_valid_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Testnet, state_service);
+
+    let block_height = NetworkUpgrade::Nu5
+        .activation_height(Network::Testnet)
+        .expect("Nu5 activation height for testnet is specified");
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // Create a non-coinbase V5 tx with the last valid expiry height.
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height: block_height,
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade: NetworkUpgrade::Nu5,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result.expect("unexpected error response").tx_id(),
+        transaction.unmined_id()
+    );
+}
+
+/// Tests that a coinbase V5 transaction is accepted only if its expiry height
+/// is equal to the height of the block the transaction belongs to.
+#[tokio::test]
+async fn v5_coinbase_transaction_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Testnet, state_service);
+
+    let block_height = NetworkUpgrade::Nu5
+        .activation_height(Network::Testnet)
+        .expect("Nu5 activation height for testnet is specified");
+
+    let (input, output) = mock_coinbase_transparent_output(block_height);
+
+    // Create a coinbase V5 tx with an expiry height that matches the height of
+    // the block. Note that this is the only valid expiry height for a V5
+    // coinbase tx.
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height: block_height,
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade: NetworkUpgrade::Nu5,
+    };
+
+    let result = verifier
+        .clone()
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result.expect("unexpected error response").tx_id(),
+        transaction.unmined_id()
+    );
+
+    // Increment the expiry height so that it becomes invalid.
+    let new_expiry_height = (block_height + 1).expect("transaction block height is too large");
+    let mut new_transaction = transaction.clone();
+
+    *new_transaction.expiry_height_mut() = new_expiry_height;
+
+    let result = verifier
+        .clone()
+        .oneshot(Request::Block {
+            transaction: Arc::new(new_transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::CoinbaseExpiryBlockHeight {
+            expiry_height: Some(new_expiry_height),
+            block_height,
+            transaction_hash: new_transaction.hash(),
+        })
+    );
+
+    // Decrement the expiry height so that it becomes invalid.
+    let new_expiry_height = (block_height - 1).expect("transaction block height is too low");
+    let mut new_transaction = transaction.clone();
+
+    *new_transaction.expiry_height_mut() = new_expiry_height;
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(new_transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::CoinbaseExpiryBlockHeight {
+            expiry_height: Some(new_expiry_height),
+            block_height,
+            transaction_hash: new_transaction.hash(),
+        })
+    );
+}
+
+/// Tests if an expired non-coinbase V5 transaction is rejected.
+#[tokio::test]
+async fn v5_transaction_with_too_low_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Testnet, state_service);
+
+    let block_height = NetworkUpgrade::Nu5
+        .activation_height(Network::Testnet)
+        .expect("Nu5 activation height for testnet is specified");
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // This expiry height is too low so that the tx should seem expired to the verifier.
+    let expiry_height = (block_height - 1).expect("original block height is too small");
+
+    // Create a non-coinbase V5 tx.
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade: NetworkUpgrade::Nu5,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::ExpiredTransaction {
+            expiry_height,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
+/// Tests if a non-coinbase V5 transaction with an expiry height exceeding the
+/// maximum is rejected.
+#[tokio::test]
+async fn v5_transaction_with_exceeding_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = block::Height::MAX;
+
+    let fund_height = (block_height - 1).expect("fake source fund block height is too small");
+    let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0);
+
+    // This expiry height exceeds the maximum defined by the specification.
+    let expiry_height = block::Height(500_000_000);
+
+    // Create a non-coinbase V5 tx.
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade: NetworkUpgrade::Nu5,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(known_utxos),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::MaximumExpiryHeight {
+            expiry_height,
+            is_coinbase: false,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
+/// Tests if a coinbase V5 transaction with an expiry height exceeding the
+/// maximum is rejected.
+#[tokio::test]
+async fn v5_coinbase_transaction_with_exceeding_expiry_height() {
+    let state_service =
+        service_fn(|_| async { unreachable!("State service should not be called") });
+    let verifier = Verifier::new(Network::Mainnet, state_service);
+
+    let block_height = block::Height::MAX;
+
+    let (input, output) = mock_coinbase_transparent_output(block_height);
+
+    // This expiry height exceeds the maximum defined by the specification.
+    let expiry_height = block::Height(500_000_000);
+
+    // Create a coinbase V4 tx.
+    let transaction = Transaction::V5 {
+        inputs: vec![input],
+        outputs: vec![output],
+        lock_time: LockTime::unlocked(),
+        expiry_height,
+        sapling_shielded_data: None,
+        orchard_shielded_data: None,
+        network_upgrade: NetworkUpgrade::Nu5,
+    };
+
+    let result = verifier
+        .oneshot(Request::Block {
+            transaction: Arc::new(transaction.clone()),
+            known_utxos: Arc::new(HashMap::new()),
+            height: block_height,
+            time: chrono::MAX_DATETIME,
+        })
+        .await;
+
+    assert_eq!(
+        result,
+        Err(TransactionError::MaximumExpiryHeight {
+            expiry_height,
+            is_coinbase: true,
+            block_height,
+            transaction_hash: transaction.hash(),
+        })
+    );
+}
+
 /// Test if V5 coinbase transaction is accepted.
 #[tokio::test]
 async fn v5_coinbase_transaction_is_accepted() {

zebra-consensus/src/transaction/tests/prop.rs

@@ -229,13 +229,14 @@ proptest! {
     }
 }
 
-/// Generate an arbitrary block height after the Sapling activation height on an arbitrary network.
+/// Generates an arbitrary [`block::Height`] after the Sapling activation height
+/// on an arbitrary network.
 ///
-/// A proptest [`Strategy`] that generates random tuples with
+/// A proptest [`Strategy`] that generates random tuples with:
 ///
-/// - a network (mainnet or testnet)
+/// - a network (mainnet or testnet);
-/// - a block height between the Sapling activation height (inclusive) on that network and the
+/// - a block height between the Sapling activation height (inclusive) on that
-///   maximum block height.
+///   network and the maximum transaction expiry height.
 fn sapling_onwards_strategy() -> impl Strategy<Value = (Network, block::Height)> {
     any::<Network>().prop_flat_map(|network| {
         let start_height_value = NetworkUpgrade::Sapling
@@ -243,7 +244,7 @@ fn sapling_onwards_strategy() -> impl Strategy<Value = (Network, block::Height)>
             .expect("Sapling to have an activation height")
             .0;
 
-        let end_height_value = block::Height::MAX.0;
+        let end_height_value = block::Height::MAX_EXPIRY_HEIGHT.0;
 
         (start_height_value..=end_height_value)
             .prop_map(move |height_value| (network, block::Height(height_value)))

zebra-state/src/service/check/tests/anchors.rs

@@ -117,7 +117,7 @@ fn prepare_sprout_block(mut block_to_prepare: Block, reference_block: Block) ->
                 .push(Arc::new(Transaction::V4 {
                     inputs: Vec::new(),
                     outputs: Vec::new(),
-                    lock_time: LockTime::min_lock_time(),
+                    lock_time: LockTime::min_lock_time_timestamp(),
                     expiry_height: Height(0),
                     joinsplit_data,
                     sapling_shielded_data: None,
@@ -173,7 +173,7 @@ fn check_sapling_anchors() {
             block1.transactions.push(Arc::new(Transaction::V4 {
                 inputs: Vec::new(),
                 outputs: Vec::new(),
-                lock_time: LockTime::min_lock_time(),
+                lock_time: LockTime::min_lock_time_timestamp(),
                 expiry_height: Height(0),
                 joinsplit_data: None,
                 sapling_shielded_data,
@@ -220,7 +220,7 @@ fn check_sapling_anchors() {
             block2.transactions.push(Arc::new(Transaction::V4 {
                 inputs: Vec::new(),
                 outputs: Vec::new(),
-                lock_time: LockTime::min_lock_time(),
+                lock_time: LockTime::min_lock_time_timestamp(),
                 expiry_height: Height(0),
                 joinsplit_data: None,
                 sapling_shielded_data,

zebra-state/src/service/check/tests/nullifier.rs

@@ -934,7 +934,7 @@ fn transaction_v4_with_joinsplit_data(
     Transaction::V4 {
         inputs: Vec::new(),
         outputs: Vec::new(),
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: Height(0),
         joinsplit_data,
         sapling_shielded_data: None,
@@ -1002,7 +1002,7 @@ fn transaction_v4_with_sapling_shielded_data(
     Transaction::V4 {
         inputs: Vec::new(),
         outputs: Vec::new(),
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: Height(0),
         joinsplit_data: None,
         sapling_shielded_data,
@@ -1039,7 +1039,7 @@ fn transaction_v5_with_orchard_shielded_data(
         network_upgrade: Nu5,
         inputs: Vec::new(),
         outputs: Vec::new(),
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: Height(0),
         sapling_shielded_data: None,
         orchard_shielded_data,

zebra-state/src/service/check/tests/utxo.rs

@@ -930,7 +930,7 @@ fn transaction_v4_with_transparent_data(
     let mut transaction = Transaction::V4 {
         inputs,
         outputs,
-        lock_time: LockTime::min_lock_time(),
+        lock_time: LockTime::min_lock_time_timestamp(),
         expiry_height: Height(0),
         joinsplit_data: None,
         sapling_shielded_data: None,

zebra-state/src/service/check/utxo.rs

@@ -193,7 +193,6 @@ pub fn transparent_coinbase_spend(
     match spend_restriction {
         OnlyShieldedOutputs { spend_height } => {
             let min_spend_height = utxo.height + block::Height(MIN_TRANSPARENT_COINBASE_MATURITY);
-            // TODO: allow full u32 range of block heights (#1113)
             let min_spend_height =
                 min_spend_height.expect("valid UTXOs have coinbase heights far below Height::MAX");
             if spend_height >= min_spend_height {