From c2e4f7b0a552c67b98cb61eb2ea52a727ea60f14 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 15 Jul 2020 13:43:26 +1000 Subject: [PATCH] refactor: Move the local clock check to a BlockHeader method --- zebra-chain/src/block/header.rs | 33 ++++++- zebra-chain/src/block/tests.rs | 141 ++++++++++++++++++++++++++++- zebra-consensus/src/block.rs | 33 +------ zebra-consensus/src/block/tests.rs | 128 -------------------------- 4 files changed, 171 insertions(+), 164 deletions(-) diff --git a/zebra-chain/src/block/header.rs b/zebra-chain/src/block/header.rs index 3a1ccba20..b536775a3 100644 --- a/zebra-chain/src/block/header.rs +++ b/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) -> 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")] diff --git a/zebra-chain/src/block/tests.rs b/zebra-chain/src/block/tests.rs index e0d2f2f93..eb357c367 100644 --- a/zebra-chain/src/block/tests.rs +++ b/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::::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, now: DateTime) -> 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"); + } +} diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index b2c15b90c..3bb662c26 100644 --- a/zebra-consensus/src/block.rs +++ b/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, - now: DateTime, -) -> 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 { /// 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()?; diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 643c95733..53296129b 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/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::::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