From a1a3e4db5aa90037e882bcf6d1c73194b6ae00d5 Mon Sep 17 00:00:00 2001
From: Henry de Valence <hdevalence@hdevalence.ca>
Date: Wed, 21 Oct 2020 21:06:21 -0700
Subject: [PATCH] consensus: simplify block verify tracing output

The previous debug output printed a message that the chain verifier had
recieved a block.  But this provides no additional information compared
to printing no message in chain::Verifier and a message in whichever
verifier the block was sent to, since the resulting spans indicate where
the block was dispatched.

This commit also removes the "unexpected high block" detection; this was
an artefact of the original sync algorithm failing to handle block
advertisements, but we don't have that problem any more, so we can
simplify the code by eliminating that logic.
---
 zebra-consensus/src/chain.rs      | 73 ++++++-------------------------
 zebra-consensus/src/checkpoint.rs | 12 +----
 2 files changed, 15 insertions(+), 70 deletions(-)

diff --git a/zebra-consensus/src/chain.rs b/zebra-consensus/src/chain.rs
index fa76f7ac..3e3722ae 100644
--- a/zebra-consensus/src/chain.rs
+++ b/zebra-consensus/src/chain.rs
@@ -27,11 +27,6 @@ use crate::{
     BoxError, Config,
 };
 
-/// The maximum expected gap between blocks.
-///
-/// Used to identify unexpected out of order blocks.
-const MAX_EXPECTED_BLOCK_GAP: u32 = 100_000;
-
 /// The chain verifier routes requests to either the checkpoint verifier or the
 /// block verifier, depending on the maximum checkpoint height.
 struct ChainVerifier<S>
@@ -42,7 +37,6 @@ where
     block: BlockVerifier<S>,
     checkpoint: CheckpointVerifier<S>,
     max_checkpoint_height: block::Height,
-    last_block_height: Option<block::Height>,
 }
 
 #[derive(Debug, Display, Error)]
@@ -76,60 +70,20 @@ where
     }
 
     fn call(&mut self, block: Arc<Block>) -> Self::Future {
-        let height = block.coinbase_height();
-        let span = tracing::info_span!("ChainVerifier::call", ?height);
-        let _entered = span.enter();
-        tracing::debug!("verifying new block");
-
-        // TODO: do we still need this logging?
-        // 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)
-                    || (height + MAX_EXPECTED_BLOCK_GAP < last_block_height) =>
-            {
-                self.last_block_height = Some(block::Height(height));
-                true
-            }
-            (Some(height), _) => {
-                self.last_block_height = Some(height);
-                false
-            }
-            // The other cases are covered by the verifiers
-            _ => false,
-        };
-        // 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 {
-            tracing::debug!(
-                "large block height gap: this block or the previous block is out of order"
-            );
+        match block.coinbase_height() {
+            Some(height) if height <= self.max_checkpoint_height => self
+                .checkpoint
+                .call(block)
+                .map_err(VerifyChainError::Checkpoint)
+                .boxed(),
+            // This also covers blocks with no height, which the block verifier
+            // will reject immediately.
+            _ => self
+                .block
+                .call(block)
+                .map_err(VerifyChainError::Block)
+                .boxed(),
         }
-
-        self.last_block_height = height;
-
-        if let Some(height) = height {
-            if height <= self.max_checkpoint_height {
-                return self
-                    .checkpoint
-                    .call(block)
-                    .map_err(VerifyChainError::Checkpoint)
-                    .boxed();
-            }
-        }
-
-        // For the purposes of routing requests, we can send blocks
-        // with no height to the block verifier, which will reject them.
-        //
-        // We choose the block verifier because it doesn't have any
-        // internal state, so it will always return the same error for a
-        // block with no height.
-        self.block
-            .call(block)
-            .map_err(VerifyChainError::Block)
-            .boxed()
     }
 }
 
@@ -177,7 +131,6 @@ where
             block,
             checkpoint,
             max_checkpoint_height,
-            last_block_height: None,
         }),
         3,
     )
diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs
index bcb93a98..6151b07a 100644
--- a/zebra-consensus/src/checkpoint.rs
+++ b/zebra-consensus/src/checkpoint.rs
@@ -499,15 +499,7 @@ where
         );
 
         let is_checkpoint = self.checkpoint_list.contains(height);
-        tracing::trace!(?height, ?hash, ?is_checkpoint, "Queued block");
-
-        // TODO(teor):
-        //   - Remove this log once the CheckpointVerifier is working?
-        //   - Modify the default filter or add another log, so users see
-        //     regular download progress info (vs verification info)
-        if is_checkpoint {
-            tracing::info!(?height, ?hash, ?is_checkpoint, "Queued checkpoint block");
-        }
+        tracing::debug!(?height, ?hash, ?is_checkpoint, "queued block");
 
         rx
     }
@@ -808,7 +800,7 @@ where
         Poll::Ready(Ok(()))
     }
 
-    #[instrument(name = "checkpoint_call", skip(self, block))]
+    #[instrument(name = "checkpoint", skip(self, block))]
     fn call(&mut self, block: Arc<Block>) -> Self::Future {
         // Immediately reject all incoming blocks that arrive after we've finished.
         if let FinalCheckpoint = self.previous_checkpoint_height() {