Check for bad heights or hashes in checkpoint lists (#639)

* Add MIN and MAX for BlockHeight and LockTime
* Remove duplicate test cases
* fix a comment about the minimum lock time

The minimum LockTime::Time is 5 November 1985 00:53:20 UTC, so the first
day that only contains valid times is 6 November 1985 (in all timezones).

Similarly, the maximum LockTime::Time is 7 February 2106 06::28::15 UTC,
so the last day that only contains valid times in all time zones is
5 February 2106.

* fix: Reject checkpoint lists with bad hashes or heights

Reject the all-zeroes hash, because it is the parent hash of the genesis
block, and should never appear in a checkpoint list.

Reject checkpoint heights that are greater than the maximum block
height.

zebra-chain/src/block/tests.rs

@@ -1,11 +1,14 @@
 use super::*;
+
 use crate::equihash_solution::EquihashSolution;
 use crate::merkle_tree::MerkleTreeRootHash;
 use crate::note_commitment_tree::SaplingNoteTreeRootHash;
 use crate::serialization::{
     SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize,
 };
+use crate::types::LockTime;
 use crate::{sha256d_writer::Sha256dWriter, test::generate};
+
 use chrono::{DateTime, Duration, LocalResult, TimeZone, Utc};
 use proptest::{
     arbitrary::{any, Arbitrary},
@@ -299,20 +302,15 @@ static BLOCK_HEADER_VALID_TIMESTAMPS: &[i64] = &[
     i64::MIN,
     i64::MIN + 1,
     // These times are valid DateTimes
-    (u32::MIN as i64) - 1,
-    (u32::MIN as i64),
-    (u32::MIN as i64) + 1,
     (i32::MIN as i64) - 1,
     (i32::MIN as i64),
     (i32::MIN as i64) + 1,
     -1,
     0,
     1,
-    // maximum nExpiryHeight or lock_time, in blocks
+    LockTime::MIN_TIMESTAMP - 1,
-    499_999_999,
+    LockTime::MIN_TIMESTAMP,
-    // minimum lock_time, in seconds
+    LockTime::MIN_TIMESTAMP + 1,
-    500_000_000,
-    500_000_001,
 ];
 
 /// Invalid unix epoch timestamps for blocks, in seconds
@@ -320,9 +318,9 @@ static BLOCK_HEADER_INVALID_TIMESTAMPS: &[i64] = &[
     (i32::MAX as i64) - 1,
     (i32::MAX as i64),
     (i32::MAX as i64) + 1,
-    (u32::MAX as i64) - 1,
+    LockTime::MAX_TIMESTAMP - 1,
-    (u32::MAX as i64),
+    LockTime::MAX_TIMESTAMP,
-    (u32::MAX as i64) + 1,
+    LockTime::MAX_TIMESTAMP + 1,
     // These times are currently invalid DateTimes, but they could
     // become valid in future chrono versions
     i64::MAX - 1,

zebra-chain/src/transaction/serialize.rs

@@ -89,7 +89,7 @@ fn parse_coinbase_height(
                 + ((data[2] as u32) << 8)
                 + ((data[3] as u32) << 16)
                 + ((data[4] as u32) << 24);
-            if h < 500_000_000 {
+            if h <= BlockHeight::MAX.0 {
                 Ok((BlockHeight(h), CoinbaseData(data.split_off(5))))
             } else {
                 Err(SerializationError::Parse("Invalid block height"))
@@ -114,7 +114,7 @@ fn coinbase_height_len(height: BlockHeight) -> usize {
         3
     } else if let _h @ 65536..=16_777_215 = height.0 {
         4
-    } else if let _h @ 16_777_216..=499_999_999 = height.0 {
+    } else if let _h @ 16_777_216..=BlockHeight::MAX_AS_U32 = height.0 {
         5
     } else {
         panic!("Invalid coinbase height");
@@ -139,7 +139,7 @@ fn write_coinbase_height<W: io::Write>(height: BlockHeight, mut w: W) -> Result<
         w.write_u8(h as u8)?;
         w.write_u8((h >> 8) as u8)?;
         w.write_u8((h >> 16) as u8)?;
-    } else if let h @ 16_777_216..=499_999_999 = height.0 {
+    } else if let h @ 16_777_216..=BlockHeight::MAX_AS_U32 = height.0 {
         w.write_u8(0x04)?;
         w.write_u32::<LittleEndian>(h)?;
     } else {

zebra-chain/src/types.rs

@@ -16,7 +16,7 @@ pub mod amount;
 ///
 /// # Invariants
 ///
-/// Users should not construct block heights greater than or equal to `500_000_000`.
+/// Users should not construct block heights greater than `BlockHeight::MAX`.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub struct BlockHeight(pub u32);
 
@@ -25,7 +25,9 @@ impl Arbitrary for BlockHeight {
     type Parameters = ();
 
     fn arbitrary_with(_args: ()) -> Self::Strategy {
-        (0u32..500_000_000_u32).prop_map(BlockHeight).boxed()
+        (BlockHeight::MIN.0..=BlockHeight::MAX.0)
+            .prop_map(BlockHeight)
+            .boxed()
     }
 
     type Strategy = BoxedStrategy<Self>;
@@ -36,9 +38,12 @@ impl Arbitrary for BlockHeight {
 ///
 /// # Invariants
 ///
-/// Users should not construct a `LockTime` with `BlockHeight` greater than or
+/// Users should not construct a `LockTime` with:
-/// equal to `500_000_000` or a timestamp before 4 November 1985 (Unix timestamp
+///   - a `BlockHeight` greater than MAX_BLOCK_HEIGHT,
-/// less than `500_000_000`).
+///   - a timestamp before 6 November 1985
+///     (Unix timestamp less than MIN_LOCK_TIMESTAMP), or
+///   - a timestamp after 5 February 2106
+///     (Unix timestamp greater than MAX_LOCK_TIMESTAMP).
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
 pub enum LockTime {
     /// Unlock at a particular block height.
@@ -47,6 +52,61 @@ pub enum LockTime {
     Time(DateTime<Utc>),
 }
 
+impl BlockHeight {
+    /// The minimum BlockHeight.
+    ///
+    /// Due to the underlying type, it is impossible to construct block heights
+    /// less than `BlockHeight::MIN`.
+    ///
+    /// Style note: Sometimes, `BlockHeight::MIN` is less readable than
+    /// `BlockHeight(0)`. Use whichever makes sense in context.
+    pub const MIN: BlockHeight = BlockHeight(0);
+
+    /// The maximum BlockHeight.
+    ///
+    /// Users should not construct block heights greater than `BlockHeight::MAX`.
+    pub const MAX: BlockHeight = BlockHeight(499_999_999);
+
+    /// The maximum BlockHeight as a u32, for range patterns.
+    ///
+    /// `BlockHeight::MAX.0` can't be used in match range patterns, use this
+    /// alias instead.
+    pub const MAX_AS_U32: u32 = Self::MAX.0;
+}
+
+impl LockTime {
+    /// The minimum LockTime::Time, as a timestamp in seconds.
+    ///
+    /// Users should not construct lock times less than `MIN_TIMESTAMP`.
+    pub const MIN_TIMESTAMP: i64 = 500_000_000;
+
+    /// 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.
+    pub const MAX_TIMESTAMP: i64 = u32::MAX as i64;
+
+    /// Returns the minimum LockTime::Time, as a LockTime.
+    ///
+    /// Users should not construct lock times less than `min_lock_timestamp`.
+    //
+    // When `Utc.timestamp` stabilises as a const function, we can make this a
+    // const function.
+    pub fn min_lock_time() -> LockTime {
+        LockTime::Time(Utc.timestamp(Self::MIN_TIMESTAMP, 0))
+    }
+
+    /// Returns the maximum LockTime::Time, as a LockTime.
+    ///
+    /// Users should not construct lock times greater than `max_lock_timestamp`.
+    //
+    // When `Utc.timestamp` stabilises as a const function, we can make this a
+    // const function.
+    pub fn max_lock_time() -> LockTime {
+        LockTime::Time(Utc.timestamp(Self::MAX_TIMESTAMP, 0))
+    }
+}
+
 impl ZcashSerialize for LockTime {
     fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
         // This implementation does not check the invariants on `LockTime` so that the
@@ -64,7 +124,7 @@ 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 < 500_000_000 {
+        if n <= BlockHeight::MAX.0 {
             Ok(LockTime::Height(BlockHeight(n)))
         } else {
             Ok(LockTime::Time(Utc.timestamp(n as i64, 0)))
@@ -78,9 +138,10 @@ impl Arbitrary for LockTime {
 
     fn arbitrary_with(_args: ()) -> Self::Strategy {
         prop_oneof![
-            (0u32..500_000_000_u32).prop_map(|n| LockTime::Height(BlockHeight(n))),
+            (BlockHeight::MIN.0..=BlockHeight::MAX.0)
-            // LockTime is u32 in the spec, so we are limited to u32::MAX here
+                .prop_map(|n| LockTime::Height(BlockHeight(n))),
-            (500_000_000..u32::MAX).prop_map(|n| { LockTime::Time(Utc.timestamp(n as i64, 0)) })
+            (LockTime::MIN_TIMESTAMP..=LockTime::MAX_TIMESTAMP)
+                .prop_map(|n| { LockTime::Time(Utc.timestamp(n as i64, 0)) })
         ]
         .boxed()
     }

zebra-consensus/src/checkpoint/list.rs

@@ -64,7 +64,18 @@ impl CheckpointList {
             Err("checkpoint hashes must be unique")?;
         }
 
-        Ok(CheckpointList(checkpoints))
+        // Make sure all the hashes are valid. In Bitcoin, [0; 32] is the null
+        // hash. It is also used as the parent hash of genesis blocks.
+        if block_hashes.contains(&BlockHeaderHash([0; 32])) {
+            Err("checkpoint list contains invalid checkpoint hash: found null hash")?;
+        }
+
+        let checkpoints = CheckpointList(checkpoints);
+        if checkpoints.max_height() > BlockHeight::MAX {
+            Err("checkpoint list contains invalid checkpoint: checkpoint height is greater than the maximum block height")?;
+        }
+
+        Ok(checkpoints)
     }
 
     /// Return true if there is a checkpoint at `height`.
@@ -186,6 +197,46 @@ mod tests {
         Ok(())
     }
 
+    /// Make sure a checkpoint list that contains a null hash fails
+    #[test]
+    fn checkpoint_list_null_hash_fail() -> Result<(), Error> {
+        let checkpoint_data = vec![(BlockHeight(0), BlockHeaderHash([0; 32]))];
+
+        // Make a checkpoint list containing the non-genesis block
+        let checkpoint_list: BTreeMap<BlockHeight, BlockHeaderHash> =
+            checkpoint_data.iter().cloned().collect();
+        let _ = CheckpointList::new(checkpoint_list)
+            .expect_err("a checkpoint list with a null block hash should fail");
+
+        Ok(())
+    }
+
+    /// Make sure a checkpoint list that contains an invalid block height fails
+    #[test]
+    fn checkpoint_list_bad_height_fail() -> Result<(), Error> {
+        let checkpoint_data = vec![(
+            BlockHeight(BlockHeight::MAX.0 + 1),
+            BlockHeaderHash([1; 32]),
+        )];
+
+        // Make a checkpoint list containing the non-genesis block
+        let checkpoint_list: BTreeMap<BlockHeight, BlockHeaderHash> =
+            checkpoint_data.iter().cloned().collect();
+        let _ = CheckpointList::new(checkpoint_list).expect_err(
+            "a checkpoint list with an invalid block height (BlockHeight::MAX + 1) should fail",
+        );
+
+        let checkpoint_data = vec![(BlockHeight(u32::MAX), BlockHeaderHash([1; 32]))];
+
+        // Make a checkpoint list containing the non-genesis block
+        let checkpoint_list: BTreeMap<BlockHeight, BlockHeaderHash> =
+            checkpoint_data.iter().cloned().collect();
+        let _ = CheckpointList::new(checkpoint_list)
+            .expect_err("a checkpoint list with an invalid block height (u32::MAX) should fail");
+
+        Ok(())
+    }
+
     /// Make sure that a checkpoint list containing duplicate blocks fails
     #[test]
     fn checkpoint_list_duplicate_blocks_fail() -> Result<(), Error> {