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

consensus: Add a checkpoint verifier stub #502

Merged
merged 6 commits into from
Jun 21, 2020
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 52 additions & 52 deletions zebra-consensus/src/checkpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::{
sync::Arc,
task::{Context, Poll},
};
use tower::{buffer::Buffer, Service};
use tower::{Service, ServiceExt};

use zebra_chain::block::{Block, BlockHeaderHash};
use zebra_chain::types::BlockHeight;
Expand All @@ -35,7 +35,7 @@ struct CheckpointVerifier<S> {
/// which only happen in the last few hundred blocks in the chain.
/// (zcashd allows chain reorganizations up to 99 blocks, and prunes
/// orphaned side-chains after 288 blocks.)
checkpoint_list: Option<HashMap<BlockHeight, BlockHeaderHash>>,
checkpoint_list: Arc<HashMap<BlockHeight, BlockHeaderHash>>,
}

/// The error type for the CheckpointVerifier Service.
Expand All @@ -47,63 +47,64 @@ 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 CheckpointVerifier<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.

// These checks are cheap, so we can do them in the call()

let checkpoints = match &self.checkpoint_list {
Some(checkpoints) => checkpoints,
None => return async { Err("the checkpoint list is empty".into()) }.boxed(),
};

let block_height = match block.coinbase_height() {
Some(height) => height,
None => {
return async { Err("the block does not have a coinbase height".into()) }.boxed()
}
};

// TODO(teor):
// - implement chaining from checkpoints to their ancestors
// - if chaining is expensive, move this check to the Future
// - should the state contain a mapping from previous_block_hash to block?
let checkpoint_hash_ref = match checkpoints.get(&block_height) {
Some(hash) => hash,
None => {
return async { Err("the block's height is not a checkpoint height".into()) }
.boxed()
}
};

// Avoid moving a reference into the future.
let checkpoint_hash = *checkpoint_hash_ref;

// `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: block.clone(),
});
let mut state_service = self.state_service.clone();
let checkpoint_list = self.checkpoint_list.clone();

async move {
// Hashing is expensive, so we do it in the Future
if checkpoint_list.is_empty() {
return Err("the checkpoint list is empty".into());
};

let block_height = match block.coinbase_height() {
Some(height) => height,
None => return Err("the block does not have a coinbase height".into()),
};
Comment on lines +77 to +80
Copy link
Contributor

@yaahc yaahc Jun 19, 2020

Choose a reason for hiding this comment

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

You can use ok_or and ok_or_else to convert an option to a result which then lets you use ? on it

Suggested change
let block_height = match block.coinbase_height() {
Some(height) => height,
None => return Err("the block does not have a coinbase height".into()),
};
let block_height = block
.coinbase_height()
.ok_or("the block does not have a coinbase height")?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll have a look at that for my next PR, once I've changed the internals of CheckpointVerifier.


// TODO(teor):
// - implement chaining from checkpoints to their ancestors
// - if chaining is expensive, move this check to the Future
// - should the state contain a mapping from previous_block_hash to block?
let checkpoint_hash = match checkpoint_list.get(&block_height) {
Some(&hash) => hash,
None => return Err("the block's height is not a checkpoint height".into()),
};

// Hashing is expensive, so we do it as late as possible
if BlockHeaderHash::from(block.as_ref()) != checkpoint_hash {
// The block is on a side-chain
return Err("the block hash does not match the checkpoint hash".into());
}

// `Tower::Buffer` requires a 1:1 relationship between `poll()`s
// and `call()`s, because it reserves a buffer slot in each
// `call()`.
// TODO(teor): what happens if the await fails?
Copy link
Contributor

Choose a reason for hiding this comment

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

If ready_and().await resolves to an Err (signaling failure of the state service), the error will be bubbled up by ? and verification of the block will fail with the error from the state service, i.e., block verification won't succeed unless the block can be written back to the state, which I think is what we want?

Copy link
Contributor

Choose a reason for hiding this comment

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

(That said, I think that this service shouldn't be committing anything to the state, just returning a verification result, so that we can have separation of concerns and factor out the state-commitment behavior from this checkpoint verifier on the one hand and from the full verifier on the other hand).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the buffer slot will be used up, regardless of whether the error is in the buffer or state service?

(I'll factor out the state commitment once we have a HybridVerifier service.)

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 guess if the error is in the buffer, it won't reserve a slot. And if the error is in the state service, then it will use up the buffer slot.

let add_block = state_service
.ready_and()
.await?
.call(zebra_state::Request::AddBlock {
block: block.clone(),
});

match add_block.await? {
zebra_state::Response::Added { hash } => Ok(hash),
_ => Err("adding block to zebra-state failed".into()),
Expand All @@ -113,8 +114,10 @@ where
}
}

// TODO(teor): add a function for the maximum checkpoint height
// We can pre-calculate the result in init(), if we want.
// TODO(teor):
// - add a function for the maximum checkpoint height
// (We can pre-calculate the result in init(), if we want.)
// - check that block.coinbase_height() <= max_checkpoint_height

/// Return a checkpoint verification service, using the provided state service.
///
Expand All @@ -131,28 +134,25 @@ where
/// backed by the same state layer.
pub fn init<S>(
state_service: S,
checkpoint_list: Option<HashMap<BlockHeight, BlockHeaderHash>>,
checkpoint_list: impl Into<Arc<HashMap<BlockHeight, BlockHeaderHash>>>,
) -> impl Service<
Arc<Block>,
Response = BlockHeaderHash,
Error = Error,
Future = impl Future<Output = Result<BlockHeaderHash, Error>>,
> + Send
+ Clone
+ 'static
where
S: Service<zebra_state::Request, Response = zebra_state::Response, Error = Error>
+ Send
+ Clone
+ 'static,
S::Future: Send + 'static,
{
Buffer::new(
CheckpointVerifier {
state_service,
checkpoint_list,
},
1,
)
CheckpointVerifier {
state_service,
checkpoint_list: checkpoint_list.into(),
}
}

// TODO(teor): tests