fix: Make sure checkpoint heights and hashes are unique

Previously, CheckpointList would silently ignore duplicate heights or
hashes.

Closes #632.

zebra-consensus/src/checkpoint/list.rs

@@ -5,7 +5,11 @@
 //! Checkpoints can be used to verify their ancestors, by chaining backwards
 //! to another checkpoint, via each block's parent block hash.
 
-use std::{collections::BTreeMap, error, ops::RangeBounds};
+use std::{
+    collections::{BTreeMap, HashSet},
+    error,
+    ops::RangeBounds,
+};
 
 use zebra_chain::block::BlockHeaderHash;
 use zebra_chain::types::BlockHeight;
@@ -20,19 +24,27 @@ type Error = Box<dyn error::Error + Send + Sync + 'static>;
 /// which only happen in the last few hundred blocks in the chain.
 /// (zcashd allows chain reorganizations up to 99 blocks, and prunes
 /// orphaned side-chains after 288 blocks.)
-///
-/// There must be a checkpoint for the genesis block at BlockHeight 0.
-/// (All other checkpoints are optional.)
 #[derive(Debug)]
 pub struct CheckpointList(BTreeMap<BlockHeight, BlockHeaderHash>);
 
 impl CheckpointList {
     /// Create a new checkpoint list from `checkpoint_list`.
+    ///
+    /// Checkpoint heights and checkpoint hashes must be unique.
+    ///
+    /// There must be a checkpoint for the genesis block at BlockHeight 0.
+    /// (All other checkpoints are optional.)
     pub fn new(
         checkpoint_list: impl IntoIterator<Item = (BlockHeight, BlockHeaderHash)>,
     ) -> Result<Self, Error> {
-        let checkpoints: BTreeMap<BlockHeight, BlockHeaderHash> =
+        // BTreeMap silently ignores duplicates, so we count the checkpoints
+        // before adding them to the map
+        let original_checkpoints: Vec<(BlockHeight, BlockHeaderHash)> =
             checkpoint_list.into_iter().collect();
+        let original_len = original_checkpoints.len();
+
+        let checkpoints: BTreeMap<BlockHeight, BlockHeaderHash> =
+            original_checkpoints.into_iter().collect();
 
         // An empty checkpoint list can't actually verify any blocks.
         match checkpoints.keys().next() {
@@ -41,6 +53,17 @@ impl CheckpointList {
             _ => Err("checkpoints must start at the genesis block height 0")?,
         };
 
+        // This check rejects duplicate heights, whether they have the same or
+        // different hashes
+        if checkpoints.len() != original_len {
+            Err("checkpoint heights must be unique")?;
+        }
+
+        let block_hashes: HashSet<&BlockHeaderHash> = checkpoints.values().collect();
+        if block_hashes.len() != original_len {
+            Err("checkpoint hashes must be unique")?;
+        }
+
         Ok(CheckpointList(checkpoints))
     }
 
@@ -81,3 +104,167 @@ impl CheckpointList {
         self.0.range(range).map(|(height, _)| *height).next_back()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use std::sync::Arc;
+
+    use zebra_chain::{block::Block, serialization::ZcashDeserialize};
+
+    /// Make a checkpoint list containing only the genesis block
+    #[test]
+    fn checkpoint_list_genesis() -> Result<(), Error> {
+        // Parse the genesis block
+        let mut checkpoint_data = Vec::new();
+        let block =
+            Arc::<Block>::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES[..])?;
+        let hash: BlockHeaderHash = block.as_ref().into();
+        checkpoint_data.push((
+            block.coinbase_height().expect("test block has height"),
+            hash,
+        ));
+
+        // Make a checkpoint list containing the genesis block
+        let checkpoint_list: BTreeMap<BlockHeight, BlockHeaderHash> =
+            checkpoint_data.iter().cloned().collect();
+        let _ = CheckpointList::new(checkpoint_list)?;
+
+        Ok(())
+    }
+
+    /// Make a checkpoint list containing multiple blocks
+    #[test]
+    fn checkpoint_list_multiple() -> Result<(), Error> {
+        // Parse all the blocks
+        let mut checkpoint_data = Vec::new();
+        for b in &[
+            &zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES[..],
+            &zebra_test::vectors::BLOCK_MAINNET_1_BYTES[..],
+            &zebra_test::vectors::BLOCK_MAINNET_415000_BYTES[..],
+            &zebra_test::vectors::BLOCK_MAINNET_434873_BYTES[..],
+        ] {
+            let block = Arc::<Block>::zcash_deserialize(*b)?;
+            let hash: BlockHeaderHash = block.as_ref().into();
+            checkpoint_data.push((
+                block.coinbase_height().expect("test block has height"),
+                hash,
+            ));
+        }
+
+        // Make a checkpoint list containing all the blocks
+        let checkpoint_list: BTreeMap<BlockHeight, BlockHeaderHash> =
+            checkpoint_data.iter().cloned().collect();
+        let _ = CheckpointList::new(checkpoint_list)?;
+
+        Ok(())
+    }
+
+    /// Make sure that an empty checkpoint list fails
+    #[test]
+    fn checkpoint_list_empty_fail() -> Result<(), Error> {
+        let _ = CheckpointList::new(Vec::new()).expect_err("empty checkpoint lists should fail");
+
+        Ok(())
+    }
+
+    /// Make sure a checkpoint list that doesn't contain the genesis block fails
+    #[test]
+    fn checkpoint_list_no_genesis_fail() -> Result<(), Error> {
+        // Parse a non-genesis block
+        let mut checkpoint_data = Vec::new();
+        let block =
+            Arc::<Block>::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_1_BYTES[..])?;
+        let hash: BlockHeaderHash = block.as_ref().into();
+        checkpoint_data.push((
+            block.coinbase_height().expect("test block has height"),
+            hash,
+        ));
+
+        // 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 no genesis block should fail");
+
+        Ok(())
+    }
+
+    /// Make sure that a checkpoint list containing duplicate blocks fails
+    #[test]
+    fn checkpoint_list_duplicate_blocks_fail() -> Result<(), Error> {
+        // Parse some blocks twice
+        let mut checkpoint_data = Vec::new();
+        for b in &[
+            &zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES[..],
+            &zebra_test::vectors::BLOCK_MAINNET_1_BYTES[..],
+            &zebra_test::vectors::BLOCK_MAINNET_1_BYTES[..],
+        ] {
+            let block = Arc::<Block>::zcash_deserialize(*b)?;
+            let hash: BlockHeaderHash = block.as_ref().into();
+            checkpoint_data.push((
+                block.coinbase_height().expect("test block has height"),
+                hash,
+            ));
+        }
+
+        // Make a checkpoint list containing some duplicate blocks
+        let _ = CheckpointList::new(checkpoint_data)
+            .expect_err("checkpoint lists with duplicate blocks should fail");
+
+        Ok(())
+    }
+
+    /// Make sure that a checkpoint list containing duplicate heights
+    /// (with different hashes) fails
+    #[test]
+    fn checkpoint_list_duplicate_heights_fail() -> Result<(), Error> {
+        // Parse the genesis block
+        let mut checkpoint_data = Vec::new();
+        for b in &[&zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES[..]] {
+            let block = Arc::<Block>::zcash_deserialize(*b)?;
+            let hash: BlockHeaderHash = block.as_ref().into();
+            checkpoint_data.push((
+                block.coinbase_height().expect("test block has height"),
+                hash,
+            ));
+        }
+
+        // Then add some fake entries with duplicate heights
+        checkpoint_data.push((BlockHeight(1), BlockHeaderHash([0xaa; 32])));
+        checkpoint_data.push((BlockHeight(1), BlockHeaderHash([0xbb; 32])));
+
+        // Make a checkpoint list containing some duplicate blocks
+        let _ = CheckpointList::new(checkpoint_data)
+            .expect_err("checkpoint lists with duplicate heights should fail");
+
+        Ok(())
+    }
+
+    /// Make sure that a checkpoint list containing duplicate hashes
+    /// (at different heights) fails
+    #[test]
+    fn checkpoint_list_duplicate_hashes_fail() -> Result<(), Error> {
+        // Parse the genesis block
+        let mut checkpoint_data = Vec::new();
+        for b in &[&zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES[..]] {
+            let block = Arc::<Block>::zcash_deserialize(*b)?;
+            let hash: BlockHeaderHash = block.as_ref().into();
+            checkpoint_data.push((
+                block.coinbase_height().expect("test block has height"),
+                hash,
+            ));
+        }
+
+        // Then add some fake entries with duplicate hashes
+        checkpoint_data.push((BlockHeight(1), BlockHeaderHash([0xcc; 32])));
+        checkpoint_data.push((BlockHeight(2), BlockHeaderHash([0xcc; 32])));
+
+        // Make a checkpoint list containing some duplicate blocks
+        let _ = CheckpointList::new(checkpoint_data)
+            .expect_err("checkpoint lists with duplicate hashes should fail");
+
+        Ok(())
+    }
+}