Improve error messages for header.time validation

zebra-chain/src/block.rs

@@ -17,14 +17,11 @@ mod tests;
 use std::fmt;
 
 pub use hash::Hash;
+pub use header::BlockTimeError;
 pub use header::Header;
 pub use height::Height;
 pub use root_hash::RootHash;
 
-/// The error type for Block checks.
-// XXX try to remove this -- block checks should be done in zebra-consensus
-type Error = Box<dyn std::error::Error + Send + Sync + 'static>;
-
 use serde::{Deserialize, Serialize};
 
 use crate::{parameters::Network, transaction::Transaction, transparent};

zebra-chain/src/block/header.rs

@@ -1,8 +1,9 @@
 use chrono::{DateTime, Duration, Utc};
+use thiserror::Error;
 
 use crate::work::{difficulty::CompactDifficulty, equihash::Solution};
 
-use super::{merkle, Error, Hash};
+use super::{merkle, Hash, Height};
 
 /// A block header, containing metadata about a block.
 ///
@@ -67,17 +68,40 @@ pub struct Header {
     pub solution: Solution,
 }
 
+/// TODO: Inline this error into zebra_consensus::error::BlockError.
+/// See https://github.com/ZcashFoundation/zebra/issues/1021 for more details.
+#[derive(Error, Debug)]
+pub enum BlockTimeError {
+    #[error("invalid time {0:?} in block header {1:?} {2:?}: must be less than 2 hours in the future ({3:?}). Hint: check your machine's date, time, and time zone.")]
+    InvalidBlockTime(
+        DateTime<Utc>,
+        crate::block::Height,
+        crate::block::Hash,
+        DateTime<Utc>,
+    ),
+}
+
 impl Header {
-    /// TODO Inline this function in zebra_consensus::block::check see
-    /// https://github.com/ZcashFoundation/zebra/issues/1021 for more details
-    pub fn is_time_valid_at(&self, now: DateTime<Utc>) -> Result<(), Error> {
+    /// TODO: Inline this function into zebra_consensus::block::check::is_time_valid_at.
+    /// See https://github.com/ZcashFoundation/zebra/issues/1021 for more details.
+    pub fn is_time_valid_at(
+        &self,
+        now: DateTime<Utc>,
+        height: &Height,
+        hash: &Hash,
+    ) -> Result<(), BlockTimeError> {
         let two_hours_in_the_future = now
             .checked_add_signed(Duration::hours(2))
-            .ok_or("overflow when calculating 2 hours in the future")?;
+            .expect("calculating 2 hours in the future does not overflow");
         if self.time <= two_hours_in_the_future {
             Ok(())
         } else {
-            Err("block header time is more than 2 hours in the future".into())
+            Err(BlockTimeError::InvalidBlockTime(
+                self.time,
+                *height,
+                *hash,
+                two_hours_in_the_future,
+            ))?
         }
     }
 }

zebra-chain/src/block/tests/vectors.rs

@@ -190,10 +190,14 @@ fn block_limits_single_tx() {
 ///
 /// Generates a block header, sets its `time` to `block_header_time`, then
 /// calls `is_time_valid_at`.
-fn node_time_check(block_header_time: DateTime<Utc>, now: DateTime<Utc>) -> Result<(), Error> {
+fn node_time_check(
+    block_header_time: DateTime<Utc>,
+    now: DateTime<Utc>,
+) -> Result<(), BlockTimeError> {
     let mut header = generate::block_header();
     header.time = block_header_time;
-    header.is_time_valid_at(now)
+    // pass a zero height and hash - they are only used in the returned error
+    header.is_time_valid_at(now, &Height(0), &Hash([0; 32]))
 }
 
 #[test]

zebra-consensus/src/block.rs

@@ -65,7 +65,7 @@ pub enum VerifyBlockError {
         source: equihash::Error,
     },
     #[error(transparent)]
-    Time(BoxError),
+    Time(zebra_chain::block::BlockTimeError),
     #[error("unable to commit block after semantic verification")]
     Commit(#[source] BoxError),
 }
@@ -143,7 +143,8 @@ where
 
             // Field validity and structure checks
             let now = Utc::now();
-            check::time_is_valid_at(&block.header, now).map_err(VerifyBlockError::Time)?;
+            check::time_is_valid_at(&block.header, now, &height, &hash)
+                .map_err(VerifyBlockError::Time)?;
             check::coinbase_is_first(&block)?;
             check::subsidy_is_correct(network, &block)?;
 

zebra-consensus/src/block/check.rs

@@ -6,15 +6,12 @@ use zebra_chain::{
     block::Hash,
     block::Height,
     block::{Block, Header},
-    parameters::NetworkUpgrade,
+    parameters::{Network, NetworkUpgrade},
     work::equihash,
 };
 
-use crate::BoxError;
 use crate::{error::*, parameters::SLOW_START_INTERVAL};
 
-use zebra_chain::parameters::Network;
-
 use super::subsidy;
 
 /// Returns `Ok(())` if there is exactly one coinbase transaction in `Block`,
@@ -89,8 +86,15 @@ pub fn equihash_solution_is_valid(header: &Header) -> Result<(), equihash::Error
 /// accepted." [§7.5][7.5]
 ///
 /// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader
-pub fn time_is_valid_at(header: &Header, now: DateTime<Utc>) -> Result<(), BoxError> {
-    header.is_time_valid_at(now)
+///
+/// If the header time is invalid, returns an error containing `height` and `hash`.
+pub fn time_is_valid_at(
+    header: &Header,
+    now: DateTime<Utc>,
+    height: &Height,
+    hash: &Hash,
+) -> Result<(), zebra_chain::block::BlockTimeError> {
+    header.is_time_valid_at(now, height, hash)
 }
 
 /// Returns `Ok(())` if the block subsidy and miner fees in `block` are valid for `network`

zebra-consensus/src/block/tests.rs

@@ -142,8 +142,15 @@ fn time_check_past_block() {
     // 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.
-    check::time_is_valid_at(&block.header, now)
-        .expect("the header time from a mainnet block should be valid");
+    check::time_is_valid_at(
+        &block.header,
+        now,
+        &block
+            .coinbase_height()
+            .expect("block test vector height should be valid"),
+        &block.hash(),
+    )
+    .expect("the header time from a mainnet block should be valid");
 }
 
 #[test]