From bf88680deef8c33cbe4a52ef588bdb383fcb8299 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jun 2020 21:54:41 +1000 Subject: [PATCH 01/11] chain: Add a comment about transaction sizes We don't need to check transaction sizes yet, because we aren't parsing or generating transactions outside of blocks. Part of #483. --- zebra-chain/src/transaction/serialize.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/zebra-chain/src/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index 7e40b6fa346..88d991e4d79 100644 --- a/zebra-chain/src/transaction/serialize.rs +++ b/zebra-chain/src/transaction/serialize.rs @@ -359,6 +359,17 @@ impl ZcashDeserialize for Output { impl ZcashSerialize for Transaction { fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { + // Post-Sapling, transaction size is limited to MAX_BLOCK_BYTES. + // (Strictly, the maximum transaction size is about 1.5 kB less, + // because blocks also include a block header.) + // + // Currently, all transaction structs are parsed as part of a + // block. So we don't need to check transaction size here, until + // we start parsing mempool transactions, or generating our own + // transactions (see #483). + // + // Since we checkpoint on Sapling activation, we won't ever need + // to check the smaller pre-Sapling transaction size limit. match self { Transaction::V1 { inputs, From 4342abdfb14473765e8d7f0c92467c7680cd599a Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jun 2020 23:29:00 +1000 Subject: [PATCH 02/11] consensus: Add a block time check function Add a function that checks the block header miner times against the node's local clock. (We'll use this function in the next commit.) Part of #477. --- Cargo.lock | 1 + zebra-consensus/Cargo.toml | 1 + zebra-consensus/src/verify.rs | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 61b7d4bcfe8..fcd9dcd3ba9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2327,6 +2327,7 @@ version = "0.1.0" name = "zebra-consensus" version = "0.1.0" dependencies = [ + "chrono", "color-eyre", "futures-util", "spandoc", diff --git a/zebra-consensus/Cargo.toml b/zebra-consensus/Cargo.toml index 681d4c485a5..061fdc7518a 100644 --- a/zebra-consensus/Cargo.toml +++ b/zebra-consensus/Cargo.toml @@ -10,6 +10,7 @@ edition = "2018" [dependencies] zebra-chain = { path = "../zebra-chain" } zebra-state = { path = "../zebra-state" } +chrono = "0.4.11" futures-util = "0.3.5" tower = "0.3.1" diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 428b7b3717c..2a34940291a 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -31,6 +31,43 @@ struct BlockVerifier { // TODO(jlusby): Error = Report ? type Error = Box; +/// Block validity checks +mod block { + use super::Error; + + use chrono::{Duration, Utc}; + use std::sync::Arc; + + use zebra_chain::block::Block; + + /// Check if the block header time is less than or equal to + /// 2 hours in the future, according to the node's local clock. + /// + /// 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(super) fn node_time_check(block: Arc) -> Result<(), Error> { + let now = Utc::now(); + 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()) + } + } +} + /// The BlockVerifier service implementation. /// /// After verification, blocks are added to the underlying state service. From 4cea5bfc9434224665529e5e0c20c43b501e3cf3 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jun 2020 23:32:44 +1000 Subject: [PATCH 03/11] consensus: Check block times against the local clock Part of #477. --- zebra-consensus/src/verify.rs | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 2a34940291a..88f080e9ed6 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -88,16 +88,35 @@ where fn call(&mut self, block: Arc) -> Self::Future { // TODO(jlusby): Error = Report, handle errors from state_service. // TODO(teor): - // - handle chain reorgs, adjust state_service "unique block height" conditions - // - handle block validation errors (including errors in the block's transactions) + // - handle chain reorgs + // - adjust state_service "unique block height" conditions + // - move expensive checks into the async block + // Create an AddBlock Future, but don't run it yet. + // // `state_service.call` is OK here because we already called // `state_service.poll_ready` in our `poll_ready`. - let add_block = self - .state_service - .call(zebra_state::Request::AddBlock { block }); + // + // `tower::Buffer` expects exactly one `call` for each + // `poll_ready`. So we unconditionally create the AddBlock + // Future using `state_service.call`. If verification fails, + // we return an error, and implicitly cancel the future. + let add_block = self.state_service.call(zebra_state::Request::AddBlock { + block: block.clone(), + }); async move { + // If verification fails, return an error result. + // The AddBlock Future is implicitly cancelled by the + // error return in `?`. + + // Since errors cause an early exit, try to do the + // quick checks first. + block::node_time_check(block)?; + + // Verification was successful. + // Add the block to the state by awaiting the AddBlock + // Future, and return its result. match add_block.await? { zebra_state::Response::Added { hash } => Ok(hash), _ => Err("adding block to zebra-state failed".into()), From 5486f7933ae77130acf79165bd2e82fdc39445d7 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jun 2020 21:10:02 +1000 Subject: [PATCH 04/11] chain: Tweak the version and time test bounds Use MAX constants for the block header version and time arbitrary test ranges. Reduces the block header time arbitrary test range from 2**32 to 2**32-1 (u32::MAX). (2**32 is an invalid time value, which gets truncated to 0 during serialization.) Also add some comments about DateTime conversions. Part of #477. --- zebra-chain/src/block.rs | 3 +++ zebra-chain/src/block/tests.rs | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs index 15ee3c202c9..e2058fc465b 100644 --- a/zebra-chain/src/block.rs +++ b/zebra-chain/src/block.rs @@ -145,6 +145,8 @@ impl ZcashSerialize for BlockHeader { self.previous_block_hash.zcash_serialize(&mut writer)?; writer.write_all(&self.merkle_root_hash.0[..])?; writer.write_all(&self.final_sapling_root_hash.0[..])?; + // this is a truncating cast, rather than a saturating cast + // but u32 times are valid until 2106 writer.write_u32::(self.time.timestamp() as u32)?; writer.write_u32::(self.bits)?; writer.write_all(&self.nonce[..])?; @@ -190,6 +192,7 @@ impl ZcashDeserialize for BlockHeader { previous_block_hash: BlockHeaderHash::zcash_deserialize(&mut reader)?, merkle_root_hash: MerkleTreeRootHash(reader.read_32_bytes()?), final_sapling_root_hash: SaplingNoteTreeRootHash(reader.read_32_bytes()?), + // This can't panic, because all u32 values are valid `Utc.timestamp`s time: Utc.timestamp(reader.read_u32::()? as i64, 0), bits: reader.read_u32::()?, nonce: reader.read_32_bytes()?, diff --git a/zebra-chain/src/block/tests.rs b/zebra-chain/src/block/tests.rs index 7c633b9f11f..98c2fd7761c 100644 --- a/zebra-chain/src/block/tests.rs +++ b/zebra-chain/src/block/tests.rs @@ -16,11 +16,13 @@ impl Arbitrary for BlockHeader { fn arbitrary_with(_args: ()) -> Self::Strategy { ( - (4u32..2_147_483_647u32), + // version is interpreted as i32 in the spec, so we are limited to i32::MAX here + (4u32..(i32::MAX as u32)), any::(), any::(), any::(), - (0i64..4_294_967_296i64), + // time is interpreted as u32 in the spec, but rust timestamps are i64 + (0i64..(u32::MAX as i64)), any::(), any::<[u8; 32]>(), any::(), From b3e95d2de70ba6047bcff6c5f2fe59efe6e7f9dc Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jun 2020 21:15:42 +1000 Subject: [PATCH 05/11] consensus: Refactor the node time check for testing Part of #477. --- zebra-consensus/src/verify.rs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 88f080e9ed6..1f389dacbe0 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -35,11 +35,27 @@ type Error = Box; mod block { use super::Error; - use chrono::{Duration, Utc}; + use chrono::{DateTime, Duration, Utc}; use std::sync::Arc; use zebra_chain::block::Block; + /// Helper function for `node_time_check()`, see that function for details. + fn node_time_check_helper( + 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()) + } + } + /// Check if the block header time is less than or equal to /// 2 hours in the future, according to the node's local clock. /// @@ -55,16 +71,7 @@ mod block { /// /// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader pub(super) fn node_time_check(block: Arc) -> Result<(), Error> { - let now = Utc::now(); - 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()) - } + node_time_check_helper(block.header.time, Utc::now()) } } From 087d4c16cdabeed197809e22cb46998735ec87c0 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jun 2020 21:18:31 +1000 Subject: [PATCH 06/11] consensus: move verify::block to its own file Part of #477. --- zebra-consensus/src/verify.rs | 45 +---------------------------- zebra-consensus/src/verify/block.rs | 42 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 44 deletions(-) create mode 100644 zebra-consensus/src/verify/block.rs diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 1f389dacbe0..3ba763839a9 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -20,6 +20,7 @@ use tower::{buffer::Buffer, Service}; use zebra_chain::block::{Block, BlockHeaderHash}; +mod block; mod script; mod transaction; @@ -31,50 +32,6 @@ struct BlockVerifier { // TODO(jlusby): Error = Report ? type Error = Box; -/// Block validity checks -mod block { - use super::Error; - - use chrono::{DateTime, Duration, Utc}; - use std::sync::Arc; - - use zebra_chain::block::Block; - - /// Helper function for `node_time_check()`, see that function for details. - fn node_time_check_helper( - 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()) - } - } - - /// Check if the block header time is less than or equal to - /// 2 hours in the future, according to the node's local clock. - /// - /// 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(super) fn node_time_check(block: Arc) -> Result<(), Error> { - node_time_check_helper(block.header.time, Utc::now()) - } -} - /// The BlockVerifier service implementation. /// /// After verification, blocks are added to the underlying state service. diff --git a/zebra-consensus/src/verify/block.rs b/zebra-consensus/src/verify/block.rs new file mode 100644 index 00000000000..4543cff9672 --- /dev/null +++ b/zebra-consensus/src/verify/block.rs @@ -0,0 +1,42 @@ +//! Block validity checks + +use super::Error; + +use chrono::{DateTime, Duration, Utc}; +use std::sync::Arc; + +use zebra_chain::block::Block; + +/// Helper function for `node_time_check()`, see that function for details. +fn node_time_check_helper( + 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()) + } +} + +/// Check if the block header time is less than or equal to +/// 2 hours in the future, according to the node's local clock. +/// +/// 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(super) fn node_time_check(block: Arc) -> Result<(), Error> { + node_time_check_helper(block.header.time, Utc::now()) +} From 69ce8cc2df25a8a20b0055fffe4339062f30509a Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jun 2020 21:19:23 +1000 Subject: [PATCH 07/11] consensus: Add tests for block node time checks Part of #477. --- zebra-consensus/src/verify/block.rs | 136 ++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/zebra-consensus/src/verify/block.rs b/zebra-consensus/src/verify/block.rs index 4543cff9672..6f27c16be4b 100644 --- a/zebra-consensus/src/verify/block.rs +++ b/zebra-consensus/src/verify/block.rs @@ -40,3 +40,139 @@ fn node_time_check_helper( pub(super) fn node_time_check(block: Arc) -> Result<(), Error> { node_time_check_helper(block.header.time, Utc::now()) } + +#[cfg(test)] +mod tests { + use super::*; + use chrono::offset::{LocalResult, TimeZone}; + use zebra_chain::serialization::ZcashDeserialize; + + #[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"); + + // 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).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_helper(now, now) + .expect("the current time should be valid as a block header time"); + node_time_check_helper(three_hours_in_the_past, now) + .expect("a past time should be valid as a block header time"); + node_time_check_helper(two_hours_in_the_future, now) + .expect("2 hours in the future should be valid as a block header time"); + node_time_check_helper(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_helper(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_helper(now, two_hours_in_the_future) + .expect("2 hours in the past should be valid as a block header time"); + node_time_check_helper(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_helper(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_helper(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_helper(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_helper(now, block_header_time) + .expect("the inverse comparison should be valid"); + } + } +} From 5f18ed52d15a5c3e7ce2d26cb9b29cecfc4cb70b Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 22 Jun 2020 10:01:34 +1000 Subject: [PATCH 08/11] consensus: Make BlockVerifier only call AddBlock on success We don't want to call the state's AddBlock until we know the block is valid. This avoids subtle bugs if the state is modified in call(). (in_memory currently modifies the state in call(), on_disk modifies the state in the async block.) Part of #477. --- zebra-consensus/src/verify.rs | 104 +++++++++++++++++++++++++--------- 1 file changed, 78 insertions(+), 26 deletions(-) diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 3ba763839a9..51a83793493 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -16,7 +16,7 @@ use std::{ sync::Arc, task::{Context, Poll}, }; -use tower::{buffer::Buffer, Service}; +use tower::{buffer::Buffer, Service, ServiceExt}; use zebra_chain::block::{Block, BlockHeaderHash}; @@ -37,7 +37,10 @@ type Error = Box; /// After verification, blocks are added to the underlying state service. impl Service> for BlockVerifier where - S: Service, + S: Service + + Send + + Clone + + 'static, S::Future: Send + 'static, { type Response = BlockHeaderHash; @@ -45,8 +48,10 @@ where type Future = Pin> + Send + 'static>>; - fn poll_ready(&mut self, context: &mut Context<'_>) -> Poll> { - self.state_service.poll_ready(context) + fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll> { + // We don't expect the state to exert backpressure on verifier users, + // so we don't need to call `state_service.poll_ready()` here. + Poll::Ready(Ok(())) } fn call(&mut self, block: Arc) -> Self::Future { @@ -54,33 +59,21 @@ where // TODO(teor): // - handle chain reorgs // - adjust state_service "unique block height" conditions - // - move expensive checks into the async block - - // Create an AddBlock Future, but don't run it yet. - // - // `state_service.call` is OK here because we already called - // `state_service.poll_ready` in our `poll_ready`. - // - // `tower::Buffer` expects exactly one `call` for each - // `poll_ready`. So we unconditionally create the AddBlock - // Future using `state_service.call`. If verification fails, - // we return an error, and implicitly cancel the future. - let add_block = self.state_service.call(zebra_state::Request::AddBlock { - block: block.clone(), - }); + let mut state_service = self.state_service.clone(); async move { - // If verification fails, return an error result. - // The AddBlock Future is implicitly cancelled by the - // error return in `?`. - // Since errors cause an early exit, try to do the // quick checks first. - block::node_time_check(block)?; + block::node_time_check(block.clone())?; + + // `Tower::Buffer` requires a 1:1 relationship between `poll()`s + // and `call()`s, because it reserves a buffer slot in each + // `call()`. + let add_block = state_service + .ready_and() + .await? + .call(zebra_state::Request::AddBlock { block }); - // Verification was successful. - // Add the block to the state by awaiting the AddBlock - // Future, and return its result. match add_block.await? { zebra_state::Response::Added { hash } => Ok(hash), _ => Err("adding block to zebra-state failed".into()), @@ -116,6 +109,7 @@ pub fn init( where S: Service + Send + + Clone + 'static, S::Future: Send + 'static, { @@ -125,6 +119,8 @@ where #[cfg(test)] mod tests { use super::*; + + use chrono::{Duration, Utc}; use color_eyre::eyre::Report; use color_eyre::eyre::{bail, ensure, eyre}; use tower::{util::ServiceExt, Service}; @@ -295,4 +291,60 @@ mod tests { Ok(()) } + + #[tokio::test] + #[spandoc::spandoc] + async fn verify_fail_future_time() -> Result<(), Report> { + let mut block = + ::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_415000_BYTES[..])?; + + let mut state_service = zebra_state::in_memory::init(); + let mut block_verifier = super::init(state_service.clone()); + + // Modify the block's time + // Changing the block header also invalidates the header hashes, but + // those checks should be performed later in validation, because they + // are more expensive. + let three_hours_in_the_future = Utc::now() + .checked_add_signed(Duration::hours(3)) + .ok_or("overflow when calculating 3 hours in the future") + .map_err(|e| eyre!(e))?; + block.header.time = three_hours_in_the_future; + + let arc_block: Arc = block.into(); + + // Try to add the block, and expect failure + let verify_result = block_verifier + .ready_and() + .await + .map_err(|e| eyre!(e))? + .call(arc_block.clone()) + .await; + + ensure!( + // TODO(teor || jlusby): check error string + verify_result.is_err(), + "unexpected result kind: {:?}", + verify_result + ); + + // Now make sure the block isn't in the state + let state_result = state_service + .ready_and() + .await + .map_err(|e| eyre!(e))? + .call(zebra_state::Request::GetBlock { + hash: arc_block.as_ref().into(), + }) + .await; + + ensure!( + // TODO(teor || jlusby): check error string + state_result.is_err(), + "unexpected result kind: {:?}", + verify_result + ); + + Ok(()) + } } From 6708f29d360d32ee743eaf62ad8a82eb1233b15d Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 22 Jun 2020 10:05:41 +1000 Subject: [PATCH 09/11] consensus: Inline node_time_check() into callers node_time_check() is a small function, so we inline it into its callers. (And then rename node_time_check_helper() to node_time_check().) Part of #477. --- zebra-consensus/src/verify.rs | 5 ++- zebra-consensus/src/verify/block.rs | 68 ++++++++++++++--------------- 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 51a83793493..6507b2de056 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -8,6 +8,7 @@ //! Verification is provided via a `tower::Service`, to support backpressure and batch //! verification. +use chrono::Utc; use futures_util::FutureExt; use std::{ error, @@ -64,7 +65,9 @@ where async move { // Since errors cause an early exit, try to do the // quick checks first. - block::node_time_check(block.clone())?; + + let now = Utc::now(); + block::node_time_check(block.header.time, now)?; // `Tower::Buffer` requires a 1:1 relationship between `poll()`s // and `call()`s, because it reserves a buffer slot in each diff --git a/zebra-consensus/src/verify/block.rs b/zebra-consensus/src/verify/block.rs index 6f27c16be4b..3a3ec03c27c 100644 --- a/zebra-consensus/src/verify/block.rs +++ b/zebra-consensus/src/verify/block.rs @@ -3,28 +3,9 @@ use super::Error; use chrono::{DateTime, Duration, Utc}; -use std::sync::Arc; -use zebra_chain::block::Block; - -/// Helper function for `node_time_check()`, see that function for details. -fn node_time_check_helper( - 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()) - } -} - -/// Check if the block header time is less than or equal to -/// 2 hours in the future, according to the node's local clock. +/// 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. @@ -37,14 +18,28 @@ fn node_time_check_helper( /// accepted."[S 7.5][7.5] /// /// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader -pub(super) fn node_time_check(block: Arc) -> Result<(), Error> { - node_time_check_helper(block.header.time, Utc::now()) +pub(super) 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()) + } } #[cfg(test)] mod tests { use super::*; use chrono::offset::{LocalResult, TimeZone}; + use std::sync::Arc; + + use zebra_chain::block::Block; use zebra_chain::serialization::ZcashDeserialize; #[test] @@ -54,12 +49,14 @@ mod 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).expect("the header time from a mainnet block should be valid"); + node_time_check(block.header.time, now) + .expect("the header time from a mainnet block should be valid"); } #[test] @@ -72,24 +69,23 @@ mod tests { let two_hours_and_one_second_in_the_future = now + Duration::hours(2) + Duration::seconds(1); - node_time_check_helper(now, now) - .expect("the current time should be valid as a block header time"); - node_time_check_helper(three_hours_in_the_past, now) + 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_helper(two_hours_in_the_future, now) + 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_helper(two_hours_and_one_second_in_the_future, now).expect_err( + 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_helper(now, three_hours_in_the_past) + 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_helper(now, two_hours_in_the_future) + 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_helper(now, two_hours_and_one_second_in_the_future) + 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"); } @@ -149,10 +145,10 @@ mod tests { unreachable!(); } }; - node_time_check_helper(block_header_time, now) + 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_helper(now, block_header_time) + node_time_check(now, block_header_time) .expect_err("the inverse comparison should be invalid"); } @@ -168,10 +164,10 @@ mod tests { unreachable!(); } }; - node_time_check_helper(block_header_time, now) + 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_helper(now, block_header_time) + node_time_check(now, block_header_time) .expect("the inverse comparison should be valid"); } } From f2629b5bef6b7ba78c6c670f7ddce02dc383b957 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 22 Jun 2020 10:07:16 +1000 Subject: [PATCH 10/11] consensus: Simplify error handling in a test Part of #477. --- zebra-consensus/src/verify.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/zebra-consensus/src/verify.rs b/zebra-consensus/src/verify.rs index 6507b2de056..17673a5f71a 100644 --- a/zebra-consensus/src/verify.rs +++ b/zebra-consensus/src/verify.rs @@ -267,11 +267,8 @@ mod tests { .await; ensure!( - match verify_result { - Ok(_) => false, - // TODO(teor || jlusby): check error string - _ => true, - }, + // TODO(teor || jlusby): check error string + verify_result.is_err(), "unexpected result kind: {:?}", verify_result ); From 20dacb9ae292be138852460da37e3b868f244cd4 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 22 Jun 2020 10:17:34 +1000 Subject: [PATCH 11/11] chain: Update the note about time truncation --- zebra-chain/src/block.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs index e2058fc465b..4392db124ff 100644 --- a/zebra-chain/src/block.rs +++ b/zebra-chain/src/block.rs @@ -146,7 +146,8 @@ impl ZcashSerialize for BlockHeader { writer.write_all(&self.merkle_root_hash.0[..])?; writer.write_all(&self.final_sapling_root_hash.0[..])?; // this is a truncating cast, rather than a saturating cast - // but u32 times are valid until 2106 + // but u32 times are valid until 2106, and our block verification time + // checks should detect any truncation. writer.write_u32::(self.time.timestamp() as u32)?; writer.write_u32::(self.bits)?; writer.write_all(&self.nonce[..])?;