Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject future blocks #492

Merged
merged 11 commits into from
Jun 24, 2020
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions zebra-chain/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ 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, and our block verification time
// checks should detect any truncation.
writer.write_u32::<LittleEndian>(self.time.timestamp() as u32)?;
yaahc marked this conversation as resolved.
Show resolved Hide resolved
writer.write_u32::<LittleEndian>(self.bits)?;
writer.write_all(&self.nonce[..])?;
Expand Down Expand Up @@ -190,6 +193,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::<LittleEndian>()? as i64, 0),
bits: reader.read_u32::<LittleEndian>()?,
nonce: reader.read_32_bytes()?,
Expand Down
6 changes: 4 additions & 2 deletions zebra-chain/src/block/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<BlockHeaderHash>(),
any::<MerkleTreeRootHash>(),
any::<SaplingNoteTreeRootHash>(),
(0i64..4_294_967_296i64),
// time is interpreted as u32 in the spec, but rust timestamps are i64
(0i64..(u32::MAX as i64)),
any::<u32>(),
any::<[u8; 32]>(),
any::<EquihashSolution>(),
Expand Down
11 changes: 11 additions & 0 deletions zebra-chain/src/transaction/serialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,17 @@ impl ZcashDeserialize for Output {

impl ZcashSerialize for Transaction {
fn zcash_serialize<W: io::Write>(&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,
Expand Down
1 change: 1 addition & 0 deletions zebra-consensus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
106 changes: 89 additions & 17 deletions zebra-consensus/src/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -16,10 +17,11 @@ use std::{
sync::Arc,
task::{Context, Poll},
};
use tower::{buffer::Buffer, Service};
use tower::{buffer::Buffer, Service, ServiceExt};

use zebra_chain::block::{Block, BlockHeaderHash};

mod block;
mod script;
mod transaction;

Expand All @@ -36,31 +38,45 @@ type Error = Box<dyn error::Error + Send + Sync + 'static>;
/// After verification, blocks are added to the underlying state service.
impl<S> Service<Arc<Block>> for BlockVerifier<S>
where
S: Service<zebra_state::Request, Response = zebra_state::Response, Error = Error>,
S: Service<zebra_state::Request, Response = zebra_state::Response, Error = Error>
+ Send
+ Clone
+ 'static,
S::Future: Send + 'static,
{
type Response = BlockHeaderHash;
type Error = Error;
type Future =
Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send + 'static>>;

fn poll_ready(&mut self, context: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
self.state_service.poll_ready(context)
fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
// 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<Block>) -> 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)

// `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 });
// - handle chain reorgs
// - adjust state_service "unique block height" conditions
let mut state_service = self.state_service.clone();

async move {
// Since errors cause an early exit, try to do the
// quick checks first.

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
// `call()`.
let add_block = state_service
.ready_and()
.await?
.call(zebra_state::Request::AddBlock { block });

match add_block.await? {
zebra_state::Response::Added { hash } => Ok(hash),
_ => Err("adding block to zebra-state failed".into()),
Expand Down Expand Up @@ -96,6 +112,7 @@ pub fn init<S>(
where
S: Service<zebra_state::Request, Response = zebra_state::Response, Error = Error>
+ Send
+ Clone
+ 'static,
S::Future: Send + 'static,
{
Expand All @@ -105,6 +122,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};
Expand Down Expand Up @@ -248,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
Copy link
Contributor

@yaahc yaahc Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't default to doing string comparisons, if there's ever a specific error that we need to check for that is being constructed from str -> Box<dyn Error> we should instead change the error to be a proper error type that we can downcast to, using string's content as a proxy for a typed error is a recipe for disaster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix these comments, possibly in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #538 in 5f96b32 for verify/block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #538 in 609de23 for checkpoint

verify_result.is_err(),
"unexpected result kind: {:?}",
verify_result
);
Expand All @@ -275,4 +291,60 @@ mod tests {

Ok(())
}

#[tokio::test]
#[spandoc::spandoc]
async fn verify_fail_future_time() -> Result<(), Report> {
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
let mut block =
<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
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
// 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> = block.into();

// Try to add the block, and expect failure
let verify_result = block_verifier
.ready_and()
.await
.map_err(|e| eyre!(e))?
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
.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(())
}
}
Loading