diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 5234103c9..4fd16cc78 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -84,21 +84,36 @@ where // height for parsed blocks when we deserialize them. let height = block .coinbase_height() - .ok_or("Invalid block: missing block height.")?; + .ok_or_else(|| format!("invalid block {:?}: missing block height", + hash))?; if height > block::Height::MAX { - Err("Invalid block height: greater than the maximum height.")?; + Err(format!("invalid block height {:?} in {:?}: greater than the maximum height {:?}", + height, + hash, + block::Height::MAX))?; } // Check that this block is actually a new block - if BlockVerifier::get_block(&mut state, hash).await?.is_some() { - Err(format!("Block has already been verified. {:?} {:?}", height, hash))?; + if BlockVerifier::get_block(&mut state_service, hash).await?.is_some() { + Err(format!("duplicate block {:?} {:?}: block has already been verified", + height, + hash))?; } // Do the difficulty checks first, to raise the threshold for // attacks that use any other fields. - let difficulty_threshold = block.header.difficulty_threshold.to_expanded().ok_or("Invalid difficulty threshold in block header.")?; + let difficulty_threshold = block + .header + .difficulty_threshold + .to_expanded() + .ok_or_else(|| format!("invalid difficulty threshold in block header {:?} {:?}", + height, + hash))?; if hash > difficulty_threshold { - Err("Block failed the difficulty filter: hash must be less than or equal to the difficulty threshold.")?; + Err(format!("block {:?} failed the difficulty filter: hash {:?} must be less than or equal to the difficulty threshold {:?}", + height, + hash, + difficulty_threshold))?; } check::is_equihash_solution_valid(&block.header)?; @@ -110,21 +125,20 @@ where check::is_time_valid_at(&block.header, now)?; check::is_coinbase_first(&block)?; - // TODO: - // - context-free header verification: merkle root - // - contextual verification + // TODO: context-free header verification: merkle root // As a temporary solution for chain gaps, wait for the previous block, // and check its height. - // TODO: - // - Add a previous block height and hash constraint to the AddBlock request, - // so that we can verify in parallel, then check constraints before committing + // + // TODO: replace this check with the contextual verification RFC design // // Skip contextual checks for the genesis block let previous_block_hash = block.header.previous_block_hash; if previous_block_hash != crate::parameters::GENESIS_PREVIOUS_BLOCK_HASH { if height == block::Height(0) { - Err("Invalid block: height is 0, but previous block hash is not null.")?; + Err(format!("invalid block {:?}: height is 0, but previous block hash {:?} is not null", + hash, + previous_block_hash))?; } let expected_height = block::Height(height.0 - 1); @@ -141,11 +155,15 @@ where let previous_height = previous_block.coinbase_height().unwrap(); if previous_height != expected_height { - Err("Invalid block height: must be 1 more than the previous block height.")?; + Err(format!("invalid block height {:?} for {:?}: must be 1 more than the previous block height {:?} in {:?}", + height, + hash, + previous_height, + previous_block_hash))?; } } - tracing::trace!(?height, ?hash, "Verified block"); + tracing::trace!("verified block"); metrics::gauge!( "block.verified.block.height", height.0 as _ @@ -202,9 +220,7 @@ where match BlockVerifier::get_block(state, hash).await? { Some(block) => return Ok(block), // Busy-waiting is only a temporary solution to waiting for blocks. - // TODO: - // - Get an AwaitBlock future from the state - // - Replace with AddBlock constraints + // Replace with the contextual verification RFC design None => { tracing::debug!(?height, ?hash, "Waiting for state to have block"); time::delay_for(Duration::from_millis(50)).await diff --git a/zebra-consensus/src/chain.rs b/zebra-consensus/src/chain.rs index 4d18e9665..1cf0e87ea 100644 --- a/zebra-consensus/src/chain.rs +++ b/zebra-consensus/src/chain.rs @@ -33,7 +33,7 @@ use zebra_chain::parameters::{Network, NetworkUpgrade::Sapling}; /// The maximum expected gap between blocks. /// -/// Used to identify unexpected high blocks. +/// Used to identify unexpected out of order blocks. const MAX_EXPECTED_BLOCK_GAP: u32 = 100_000; /// A wrapper type that holds the `ChainVerifier`'s `CheckpointVerifier`, and @@ -120,10 +120,11 @@ where let checkpoint_verifier = self.checkpoint.clone().map(|c| c.verifier); let max_checkpoint_height = self.checkpoint.clone().map(|c| c.max_height); - // Log an info-level message on unexpected high blocks - let is_unexpected_high_block = match (height, self.last_block_height) { + // Log an info-level message on unexpected out of order blocks + let is_unexpected_gap = match (height, self.last_block_height) { (Some(block::Height(height)), Some(block::Height(last_block_height))) - if (height > last_block_height + MAX_EXPECTED_BLOCK_GAP) => + if (height > last_block_height + MAX_EXPECTED_BLOCK_GAP) + || (height + MAX_EXPECTED_BLOCK_GAP < last_block_height) => { self.last_block_height = Some(block::Height(height)); true @@ -137,20 +138,14 @@ where }; async move { - // TODO(teor): in the post-sapling checkpoint range, allow callers - // to use BlockVerifier, CheckpointVerifier, or both. - // Call a verifier based on the block height and checkpoints. if is_higher_than_max_checkpoint(height, max_checkpoint_height) { - // Log a message on early high blocks. + // Log a message on unexpected out of order blocks. + // // The sync service rejects most of these blocks, but we // still want to know if a large number get through. - // - // This message can also happen if we keep getting unexpected - // low blocks. (We can't distinguish between these cases, until - // we've verified the blocks.) - if is_unexpected_high_block { - tracing::debug!(?height, "unexpected high block, or recent unexpected low blocks"); + if is_unexpected_gap { + tracing::debug!("large block height gap: this block or the previous block is out of order"); } block_verifier @@ -160,17 +155,17 @@ where .await?; } else { checkpoint_verifier - .expect("Missing checkpoint verifier: verifier must be Some if max checkpoint height is Some") + .expect("missing checkpoint verifier: verifier must be Some if max checkpoint height is Some") .ready_and() .await? .call(block.clone()) .await?; } - tracing::trace!(?height, ?hash, "Verified block"); + tracing::trace!(?height, ?hash, "verified block"); metrics::gauge!( "chain.verified.block.height", - height.expect("Valid blocks have a block height").0 as _ + height.expect("valid blocks have a block height").0 as _ ); metrics::counter!("chain.verified.block.count", 1); diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index 9a65d1229..cb5dd3148 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -653,7 +653,7 @@ impl CheckpointVerifier { ); let block_count = rev_valid_blocks.len(); - tracing::info!(?block_count, ?current_range, "Verified checkpoint range"); + tracing::info!(?block_count, ?current_range, "verified checkpoint range"); metrics::gauge!( "checkpoint.verified.block.height", target_checkpoint_height.0 as _