From c4630cd1f5baacab319399453a001cf3b0ce5272 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 13 Oct 2020 07:33:32 +1000 Subject: [PATCH] Improve error messages for header.time validation --- zebra-chain/src/block.rs | 5 +--- zebra-chain/src/block/header.rs | 36 +++++++++++++++++++++----- zebra-chain/src/block/tests/vectors.rs | 8 ++++-- zebra-consensus/src/block.rs | 5 ++-- zebra-consensus/src/block/check.rs | 16 +++++++----- zebra-consensus/src/block/tests.rs | 11 ++++++-- 6 files changed, 59 insertions(+), 22 deletions(-) diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs index 8786695ac..be009544c 100644 --- a/zebra-chain/src/block.rs +++ b/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; - use serde::{Deserialize, Serialize}; use crate::{parameters::Network, transaction::Transaction, transparent}; diff --git a/zebra-chain/src/block/header.rs b/zebra-chain/src/block/header.rs index 0dac000b4..078cb653f 100644 --- a/zebra-chain/src/block/header.rs +++ b/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, + crate::block::Height, + crate::block::Hash, + DateTime, + ), +} + 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) -> 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, + 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, + ))? } } } diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index 16b2dba76..1860a81e7 100644 --- a/zebra-chain/src/block/tests/vectors.rs +++ b/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, now: DateTime) -> Result<(), Error> { +fn node_time_check( + block_header_time: DateTime, + now: DateTime, +) -> 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] diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index c341906c9..c7c1ae10e 100644 --- a/zebra-consensus/src/block.rs +++ b/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)?; diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 5d7f8e021..1ec64de22 100644 --- a/zebra-consensus/src/block/check.rs +++ b/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) -> 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, + 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` diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 184d76e3f..1d10f0f4c 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/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]