From 8ba9d0114b70ddbd64e56af603f44e126a72335d Mon Sep 17 00:00:00 2001
From: Jane Lusby <jlusby42@gmail.com>
Date: Thu, 12 Nov 2020 20:26:16 -0800
Subject: [PATCH] Add consensus critical check for sequential heights (#1291)

* Add consensus critical check for sequential heights
* document the check module
* Add unit tests for consensus checks
---
 zebra-state/src/error.rs         |  4 ++
 zebra-state/src/service.rs       | 20 ++++----
 zebra-state/src/service/check.rs | 87 ++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 9 deletions(-)
 create mode 100644 zebra-state/src/service/check.rs

diff --git a/zebra-state/src/error.rs b/zebra-state/src/error.rs
index 8bdf83769..2b2bdb794 100644
--- a/zebra-state/src/error.rs
+++ b/zebra-state/src/error.rs
@@ -38,4 +38,8 @@ pub enum ValidateContextError {
     /// block.height is lower than the current finalized height
     #[non_exhaustive]
     OrphanedBlock,
+
+    /// block.height is not one greater than its parent block's height
+    #[non_exhaustive]
+    NonSequentialBlock,
 }
diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs
index f2c6b0e7c..2a6430e30 100644
--- a/zebra-state/src/service.rs
+++ b/zebra-state/src/service.rs
@@ -25,6 +25,7 @@ use crate::{
     ValidateContextError,
 };
 
+mod check;
 mod memory_state;
 mod utxo;
 
@@ -182,17 +183,18 @@ impl StateService {
     /// Check that `block` is contextually valid based on the committed finalized
     /// and non-finalized state.
     fn check_contextual_validity(&mut self, block: &Block) -> Result<(), ValidateContextError> {
-        use ValidateContextError::*;
+        let finalized_tip_height = self.sled.finalized_tip_height().expect(
+            "finalized state must contain at least one block to use the non-finalized state",
+        );
+        check::block_is_not_orphaned(finalized_tip_height, block)?;
 
-        if block
+        let parent_block = self
+            .block(block.hash().into())
+            .expect("the parent's presence has already been checked");
+        let parent_height = parent_block
             .coinbase_height()
-            .expect("valid blocks have a coinbase height")
-            <= self.sled.finalized_tip_height().expect(
-                "finalized state must contain at least one block to use the non-finalized state",
-            )
-        {
-            Err(OrphanedBlock)?;
-        }
+            .expect("valid blocks have a coinbase height");
+        check::height_one_more_than_parent_height(parent_height, block)?;
 
         // TODO: contextual validation design and implementation
         Ok(())
diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs
new file mode 100644
index 000000000..b62cb61da
--- /dev/null
+++ b/zebra-state/src/service/check.rs
@@ -0,0 +1,87 @@
+//! Consensus critical contextual checks
+
+use zebra_chain::block::{self, Block};
+
+use crate::ValidateContextError;
+
+/// Returns `ValidateContextError::OrphanedBlock` if the height of the given
+/// block is less than or equal to the finalized tip height.
+pub(super) fn block_is_not_orphaned(
+    finalized_tip_height: block::Height,
+    block: &Block,
+) -> Result<(), ValidateContextError> {
+    if block
+        .coinbase_height()
+        .expect("valid blocks have a coinbase height")
+        <= finalized_tip_height
+    {
+        Err(ValidateContextError::OrphanedBlock)
+    } else {
+        Ok(())
+    }
+}
+
+/// Returns `ValidateContextError::NonSequentialBlock` if the block height isn't
+/// equal to the parent_height+1.
+pub(super) fn height_one_more_than_parent_height(
+    parent_height: block::Height,
+    block: &Block,
+) -> Result<(), ValidateContextError> {
+    let height = block
+        .coinbase_height()
+        .expect("valid blocks have a coinbase height");
+
+    if parent_height + 1 != Some(height) {
+        Err(ValidateContextError::NonSequentialBlock)
+    } else {
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Arc;
+
+    use zebra_chain::serialization::ZcashDeserializeInto;
+
+    use super::*;
+
+    #[test]
+    fn test_orphan_consensus_check() {
+        zebra_test::init();
+
+        let block = zebra_test::vectors::BLOCK_MAINNET_347499_BYTES
+            .zcash_deserialize_into::<Arc<Block>>()
+            .unwrap();
+
+        block_is_not_orphaned(block::Height(0), &block).expect("tip is lower so it should be fine");
+        block_is_not_orphaned(block::Height(347498), &block)
+            .expect("tip is lower so it should be fine");
+        block_is_not_orphaned(block::Height(347499), &block)
+            .expect_err("tip is equal so it should error");
+        block_is_not_orphaned(block::Height(500000), &block)
+            .expect_err("tip is higher so it should error");
+    }
+
+    #[test]
+    fn test_sequential_height_check() {
+        zebra_test::init();
+
+        let block = zebra_test::vectors::BLOCK_MAINNET_347499_BYTES
+            .zcash_deserialize_into::<Arc<Block>>()
+            .unwrap();
+
+        height_one_more_than_parent_height(block::Height(0), &block)
+            .expect_err("block is much lower, should panic");
+        height_one_more_than_parent_height(block::Height(347497), &block)
+            .expect_err("parent height is 2 less, should panic");
+        height_one_more_than_parent_height(block::Height(347498), &block)
+            .expect("parent height is 1 less, should be good");
+        height_one_more_than_parent_height(block::Height(347499), &block)
+            .expect_err("parent height is equal, should panic");
+        height_one_more_than_parent_height(block::Height(347500), &block)
+            .expect_err("parent height is way more, should panic");
+        height_one_more_than_parent_height(block::Height(500000), &block)
+            .expect_err("parent height is way more, should panic");
+    }
+}