refactor: Move the local clock check to a BlockHeader method

zebra-chain/src/block/header.rs

@@ -1,9 +1,9 @@
-use super::BlockHeaderHash;
+use super::{BlockHeaderHash, Error};
 use crate::equihash_solution::EquihashSolution;
 use crate::merkle_tree::MerkleTreeRootHash;
 use crate::note_commitment_tree::SaplingNoteTreeRootHash;
 use crate::serialization::ZcashSerialize;
-use chrono::{DateTime, Utc};
+use chrono::{DateTime, Duration, Utc};
 
 /// Block header.
 ///
@@ -67,7 +67,7 @@ pub struct BlockHeader {
 
 impl BlockHeader {
     /// Returns true if the header is valid based on its `EquihashSolution`
-    pub fn is_equihash_solution_valid(&self) -> Result<(), Error> {
+    pub fn is_equihash_solution_valid(&self) -> Result<(), EquihashError> {
         let n = 200;
         let k = 9;
         let nonce = &self.nonce;
@@ -81,11 +81,36 @@ impl BlockHeader {
 
         Ok(())
     }
+
+    /// Check if `self.time` is less than or equal to
+    /// 2 hours in the future, according to the node's local clock (`now`).
+    ///
+    /// This is a non-deterministic rule, as clocks vary over time, and
+    /// between different nodes.
+    ///
+    /// "In addition, a full validator MUST NOT accept blocks with nTime
+    /// more than two hours in the future according to its clock. This
+    /// is not strictly a consensus rule because it is nondeterministic,
+    /// and clock time varies between nodes. Also note that a block that
+    /// is rejected by this rule at a given point in time may later be
+    /// accepted."[S 7.5][7.5]
+    ///
+    /// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader
+    pub fn is_time_valid_local_clock(&self, now: DateTime<Utc>) -> Result<(), Error> {
+        let two_hours_in_the_future = now
+            .checked_add_signed(Duration::hours(2))
+            .ok_or("overflow when calculating 2 hours in the future")?;
+        if self.time <= two_hours_in_the_future {
+            Ok(())
+        } else {
+            Err("block header time is more than 2 hours in the future")?
+        }
+    }
 }
 
 #[non_exhaustive]
 #[derive(Debug, thiserror::Error)]
-pub enum Error {
+pub enum EquihashError {
     #[error("invalid equihash solution for BlockHeader")]
     EquihashInvalid(#[from] equihash::Error),
     #[error("cannot reserialize header for equihash verification")]

zebra-chain/src/block/tests.rs

@@ -6,7 +6,7 @@ use crate::serialization::{
     SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize,
 };
 use crate::{sha256d_writer::Sha256dWriter, test::generate};
-use chrono::{TimeZone, Utc};
+use chrono::{DateTime, Duration, LocalResult, TimeZone, Utc};
 use proptest::{
     arbitrary::{any, Arbitrary},
     prelude::*,
@@ -234,3 +234,142 @@ proptest! {
     }
 
 }
+
+#[test]
+fn time_check_past_block() {
+    // This block is also verified as part of the BlockVerifier service
+    // tests.
+    let block =
+        Arc::<Block>::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_415000_BYTES[..])
+            .expect("block should deserialize");
+    let now = Utc::now();
+
+    // This check is non-deterministic, but BLOCK_MAINNET_415000 is
+    // a long time in the past. So it's unlikely that the test machine
+    // will have a clock that's far enough in the past for the test to
+    // fail.
+    block
+        .header
+        .is_time_valid_local_clock(now)
+        .expect("the header time from a mainnet block should be valid");
+}
+
+/// Test wrapper for `BlockHeader.is_time_valid_local_clock`.
+///
+/// Generates a block header, sets its `time` to `block_header_time`, then
+/// calls `is_time_valid_local_clock`.
+fn node_time_check(block_header_time: DateTime<Utc>, now: DateTime<Utc>) -> Result<(), Error> {
+    let mut header = generate::block_header();
+    header.time = block_header_time;
+    header.is_time_valid_local_clock(now)
+}
+
+#[test]
+fn time_check_now() {
+    // These checks are deteministic, because all the times are offset
+    // from the current time.
+    let now = Utc::now();
+    let three_hours_in_the_past = now - Duration::hours(3);
+    let two_hours_in_the_future = now + Duration::hours(2);
+    let two_hours_and_one_second_in_the_future = now + Duration::hours(2) + Duration::seconds(1);
+
+    node_time_check(now, now).expect("the current time should be valid as a block header time");
+    node_time_check(three_hours_in_the_past, now)
+        .expect("a past time should be valid as a block header time");
+    node_time_check(two_hours_in_the_future, now)
+        .expect("2 hours in the future should be valid as a block header time");
+    node_time_check(two_hours_and_one_second_in_the_future, now)
+        .expect_err("2 hours and 1 second in the future should be invalid as a block header time");
+
+    // Now invert the tests
+    // 3 hours in the future should fail
+    node_time_check(now, three_hours_in_the_past)
+        .expect_err("3 hours in the future should be invalid as a block header time");
+    // The past should succeed
+    node_time_check(now, two_hours_in_the_future)
+        .expect("2 hours in the past should be valid as a block header time");
+    node_time_check(now, two_hours_and_one_second_in_the_future)
+        .expect("2 hours and 1 second in the past should be valid as a block header time");
+}
+
+/// Valid unix epoch timestamps for blocks, in seconds
+static BLOCK_HEADER_VALID_TIMESTAMPS: &[i64] = &[
+    // These times are currently invalid DateTimes, but they could
+    // become valid in future chrono versions
+    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
+    499_999_999,
+    // minimum lock_time, in seconds
+    500_000_000,
+    500_000_001,
+];
+
+/// Invalid unix epoch timestamps for blocks, in seconds
+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,
+    (u32::MAX as i64),
+    (u32::MAX as i64) + 1,
+    // These times are currently invalid DateTimes, but they could
+    // become valid in future chrono versions
+    i64::MAX - 1,
+    i64::MAX,
+];
+
+#[test]
+fn time_check_fixed() {
+    // These checks are non-deterministic, but the times are all in the
+    // distant past or far future. So it's unlikely that the test
+    // machine will have a clock that makes these tests fail.
+    let now = Utc::now();
+
+    for valid_timestamp in BLOCK_HEADER_VALID_TIMESTAMPS {
+        let block_header_time = match Utc.timestamp_opt(*valid_timestamp, 0) {
+            LocalResult::Single(time) => time,
+            LocalResult::None => {
+                // Skip the test if the timestamp is invalid
+                continue;
+            }
+            LocalResult::Ambiguous(_, _) => {
+                // Utc doesn't have ambiguous times
+                unreachable!();
+            }
+        };
+        node_time_check(block_header_time, now)
+            .expect("the time should be valid as a block header time");
+        // Invert the check, leading to an invalid time
+        node_time_check(now, block_header_time)
+            .expect_err("the inverse comparison should be invalid");
+    }
+
+    for invalid_timestamp in BLOCK_HEADER_INVALID_TIMESTAMPS {
+        let block_header_time = match Utc.timestamp_opt(*invalid_timestamp, 0) {
+            LocalResult::Single(time) => time,
+            LocalResult::None => {
+                // Skip the test if the timestamp is invalid
+                continue;
+            }
+            LocalResult::Ambiguous(_, _) => {
+                // Utc doesn't have ambiguous times
+                unreachable!();
+            }
+        };
+        node_time_check(block_header_time, now)
+            .expect_err("the time should be invalid as a block header time");
+        // Invert the check, leading to a valid time
+        node_time_check(now, block_header_time).expect("the inverse comparison should be valid");
+    }
+}

zebra-consensus/src/block.rs

@@ -11,7 +11,7 @@
 #[cfg(test)]
 mod tests;
 
-use chrono::{DateTime, Duration, Utc};
+use chrono::Utc;
 use futures_util::FutureExt;
 use std::{
     error,
@@ -24,35 +24,6 @@ use tower::{buffer::Buffer, Service, ServiceExt};
 
 use zebra_chain::block::{Block, BlockHeaderHash};
 
-/// Check if `block_header_time` is less than or equal to
-/// 2 hours in the future, according to the node's local clock (`now`).
-///
-/// This is a non-deterministic rule, as clocks vary over time, and
-/// between different nodes.
-///
-/// "In addition, a full validator MUST NOT accept blocks with nTime
-/// more than two hours in the future according to its clock. This
-/// is not strictly a consensus rule because it is nondeterministic,
-/// and clock time varies between nodes. Also note that a block that
-/// is rejected by this rule at a given point in time may later be
-/// accepted."[S 7.5][7.5]
-///
-/// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader
-pub(crate) fn node_time_check(
-    block_header_time: DateTime<Utc>,
-    now: DateTime<Utc>,
-) -> Result<(), Error> {
-    let two_hours_in_the_future = now
-        .checked_add_signed(Duration::hours(2))
-        .ok_or("overflow when calculating 2 hours in the future")?;
-
-    if block_header_time <= two_hours_in_the_future {
-        Ok(())
-    } else {
-        Err("block header time is more than 2 hours in the future".into())
-    }
-}
-
 struct BlockVerifier<S> {
     /// The underlying `ZebraState`, possibly wrapped in other services.
     state_service: S,
@@ -96,7 +67,7 @@ where
             // quick checks first.
 
             let now = Utc::now();
-            node_time_check(block.header.time, now)?;
+            block.header.is_time_valid_local_clock(now)?;
             block.header.is_equihash_solution_valid()?;
             block.is_coinbase_first()?;
 

zebra-consensus/src/block/tests.rs

@@ -2,7 +2,6 @@
 
 use super::*;
 
-use chrono::offset::{LocalResult, TimeZone};
 use chrono::{Duration, Utc};
 use color_eyre::eyre::Report;
 use color_eyre::eyre::{bail, eyre};
@@ -14,133 +13,6 @@ use zebra_chain::block::BlockHeader;
 use zebra_chain::serialization::ZcashDeserialize;
 use zebra_chain::transaction::Transaction;
 
-#[test]
-fn time_check_past_block() {
-    // This block is also verified as part of the BlockVerifier service
-    // tests.
-    let block =
-        Arc::<Block>::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_415000_BYTES[..])
-            .expect("block should deserialize");
-    let now = Utc::now();
-
-    // This check is non-deterministic, but BLOCK_MAINNET_415000 is
-    // a long time in the past. So it's unlikely that the test machine
-    // will have a clock that's far enough in the past for the test to
-    // fail.
-    node_time_check(block.header.time, now)
-        .expect("the header time from a mainnet block should be valid");
-}
-
-#[test]
-fn time_check_now() {
-    // These checks are deteministic, because all the times are offset
-    // from the current time.
-    let now = Utc::now();
-    let three_hours_in_the_past = now - Duration::hours(3);
-    let two_hours_in_the_future = now + Duration::hours(2);
-    let two_hours_and_one_second_in_the_future = now + Duration::hours(2) + Duration::seconds(1);
-
-    node_time_check(now, now).expect("the current time should be valid as a block header time");
-    node_time_check(three_hours_in_the_past, now)
-        .expect("a past time should be valid as a block header time");
-    node_time_check(two_hours_in_the_future, now)
-        .expect("2 hours in the future should be valid as a block header time");
-    node_time_check(two_hours_and_one_second_in_the_future, now)
-        .expect_err("2 hours and 1 second in the future should be invalid as a block header time");
-
-    // Now invert the tests
-    // 3 hours in the future should fail
-    node_time_check(now, three_hours_in_the_past)
-        .expect_err("3 hours in the future should be invalid as a block header time");
-    // The past should succeed
-    node_time_check(now, two_hours_in_the_future)
-        .expect("2 hours in the past should be valid as a block header time");
-    node_time_check(now, two_hours_and_one_second_in_the_future)
-        .expect("2 hours and 1 second in the past should be valid as a block header time");
-}
-
-/// Valid unix epoch timestamps for blocks, in seconds
-static BLOCK_HEADER_VALID_TIMESTAMPS: &[i64] = &[
-    // These times are currently invalid DateTimes, but they could
-    // become valid in future chrono versions
-    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
-    499_999_999,
-    // minimum lock_time, in seconds
-    500_000_000,
-    500_000_001,
-];
-
-/// Invalid unix epoch timestamps for blocks, in seconds
-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,
-    (u32::MAX as i64),
-    (u32::MAX as i64) + 1,
-    // These times are currently invalid DateTimes, but they could
-    // become valid in future chrono versions
-    i64::MAX - 1,
-    i64::MAX,
-];
-
-#[test]
-fn time_check_fixed() {
-    // These checks are non-deterministic, but the times are all in the
-    // distant past or far future. So it's unlikely that the test
-    // machine will have a clock that makes these tests fail.
-    let now = Utc::now();
-
-    for valid_timestamp in BLOCK_HEADER_VALID_TIMESTAMPS {
-        let block_header_time = match Utc.timestamp_opt(*valid_timestamp, 0) {
-            LocalResult::Single(time) => time,
-            LocalResult::None => {
-                // Skip the test if the timestamp is invalid
-                continue;
-            }
-            LocalResult::Ambiguous(_, _) => {
-                // Utc doesn't have ambiguous times
-                unreachable!();
-            }
-        };
-        node_time_check(block_header_time, now)
-            .expect("the time should be valid as a block header time");
-        // Invert the check, leading to an invalid time
-        node_time_check(now, block_header_time)
-            .expect_err("the inverse comparison should be invalid");
-    }
-
-    for invalid_timestamp in BLOCK_HEADER_INVALID_TIMESTAMPS {
-        let block_header_time = match Utc.timestamp_opt(*invalid_timestamp, 0) {
-            LocalResult::Single(time) => time,
-            LocalResult::None => {
-                // Skip the test if the timestamp is invalid
-                continue;
-            }
-            LocalResult::Ambiguous(_, _) => {
-                // Utc doesn't have ambiguous times
-                unreachable!();
-            }
-        };
-        node_time_check(block_header_time, now)
-            .expect_err("the time should be invalid as a block header time");
-        // Invert the check, leading to a valid time
-        node_time_check(now, block_header_time).expect("the inverse comparison should be valid");
-    }
-}
-
 #[tokio::test]
 async fn verify_test() -> Result<(), Report> {
     verify().await