Log message and comment cleanups

Also:
* Clarify the out of order block debug message

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() {
+            if BlockVerifier::get_block(&mut state_service, hash).await?.is_some() {
-                Err(format!("Block has already been verified. {:?} {:?}", height, hash))?;
+                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:
+            // TODO: context-free header verification: merkle root
-            //   - context-free header verification: merkle root
-            //   - contextual verification
 
             // 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,
+            // TODO: replace this check with the contextual verification RFC design
-            //     so that we can verify in parallel, then check constraints before committing
             //
             // 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:
+                // Replace with the contextual verification RFC design
-                //   - Get an AwaitBlock future from the state
-                //   - Replace with AddBlock constraints
                 None => {
                     tracing::debug!(?height, ?hash, "Waiting for state to have block");
                     time::delay_for(Duration::from_millis(50)).await

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
+        // Log an info-level message on unexpected out of order blocks
-        let is_unexpected_high_block = match (height, self.last_block_height) {
+        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.
-                //
+                if is_unexpected_gap {
-                // This message can also happen if we keep getting unexpected
+                    tracing::debug!("large block height gap: this block or the previous block is out of order");
-                // 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");
                 }
 
                 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);
 

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 _