Repository: ZcashFoundation/zebra
Pull request #1289 (2020-11-12 23:49:20)

Replace Target<block::Height> with TargetHeight
## Motivation

We'd like to remove an unused generic, to simplify the CheckpointVerifier code.

## Solution

Replace `Target<block::Height>` with `TargetHeight`, and used a fixed `block::Height` type for the inner type.

## Review

@hdevalence asked for this change.
It's not urgent at all.



Patch:
diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs
index 4e1b8e822..c9a407eff 100644
--- a/zebra-consensus/src/checkpoint.rs
+++ b/zebra-consensus/src/checkpoint.rs
@@ -44,7 +44,7 @@ mod tests;
 
 pub(crate) use list::CheckpointList;
 use types::{Progress, Progress::*};
-use types::{Target, Target::*};
+use types::{TargetHeight, TargetHeight::*};
 
 /// An unverified block, which is in the queue for checkpoint verification.
 #[derive(Debug)]
@@ -262,7 +262,7 @@ where
     /// `height` increases as checkpoints are verified.
     ///
     /// If verification has finished, returns `FinishedVerifying`.
-    fn target_checkpoint_height(&self) -> Target<block::Height> {
+    fn target_checkpoint_height(&self) -> TargetHeight {
         // Find the height we want to start searching at
         let start_height = match self.previous_checkpoint_height() {
             // Check if we have the genesis block as a special case, to simplify the loop
diff --git a/zebra-consensus/src/checkpoint/types.rs b/zebra-consensus/src/checkpoint/types.rs
index 6ca0dcac8..db295d926 100644
--- a/zebra-consensus/src/checkpoint/types.rs
+++ b/zebra-consensus/src/checkpoint/types.rs
@@ -5,7 +5,7 @@ use std::cmp::Ordering;
 use zebra_chain::block;
 
 use Progress::*;
-use Target::*;
+use TargetHeight::*;
 
 /// A `CheckpointVerifier`'s current progress verifying the chain.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -62,16 +62,17 @@ impl PartialOrd for Progress<block::Height> {
     }
 }
 
-/// A `CheckpointVerifier`'s target checkpoint, based on the current queue.
+/// A `CheckpointVerifier`'s target checkpoint height, based on the current
+/// queue.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub enum Target<HeightOrHash> {
+pub enum TargetHeight {
     /// We need more blocks before we can choose a target checkpoint.
     WaitingForBlocks,
     /// We want to verify this checkpoint.
     ///
     /// The target checkpoint can be multiple checkpoints ahead of the previous
     /// checkpoint.
-    Checkpoint(HeightOrHash),
+    Checkpoint(block::Height),
     /// We have finished verifying, there will be no more targets.
     FinishedVerifying,
 }
@@ -79,7 +80,7 @@ pub enum Target<HeightOrHash> {
 /// Block height target, in chain order.
 ///
 /// `WaitingForBlocks` is incomparable with itself and `Checkpoint(_)`.
-impl PartialOrd for Target<block::Height> {
+impl PartialOrd for TargetHeight {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         match (self, other) {
             // FinishedVerifying is the final state
diff --git a/zebra-state/src/sled_state.rs b/zebra-state/src/sled_state.rs
index 4b3ce9e88..8e49e14c4 100644
--- a/zebra-state/src/sled_state.rs
+++ b/zebra-state/src/sled_state.rs
@@ -38,6 +38,7 @@ use self::sled_format::TransactionLocation;
 pub struct FinalizedState {
     /// Queued blocks that arrived out of order, indexed by their parent block hash.
     queued_by_prev_hash: HashMap<block::Hash, QueuedBlock>,
+    max_queued_height: i64,
 
     hash_by_height: sled::Tree,
     height_by_hash: sled::Tree,
@@ -67,6 +68,7 @@ impl FinalizedState {
 
         let new_state = Self {
             queued_by_prev_hash: HashMap::new(),
+            max_queued_height: -1,
             hash_by_height: db.open_tree(b"hash_by_height").unwrap(),
             height_by_hash: db.open_tree(b"height_by_hash").unwrap(),
             block_by_height: db.open_tree(b"block_by_height").unwrap(),
@@ -173,6 +175,7 @@ impl FinalizedState {
     /// queued block (and any of its descendants) can be committed to the state.
     pub fn queue_and_commit_finalized_blocks(&mut self, queued_block: QueuedBlock) {
         let prev_hash = queued_block.block.header.previous_block_hash;
+        let height = queued_block.block.coinbase_height().unwrap();
         self.queued_by_prev_hash.insert(prev_hash, queued_block);
 
         while let Some(queued_block) = self.queued_by_prev_hash.remove(&self.finalized_tip_hash()) {
@@ -181,12 +184,21 @@ impl FinalizedState {
                 .coinbase_height()
                 .expect("valid blocks must have a height");
             self.commit_finalized(queued_block);
-            metrics::counter!("state.committed.block.count", 1);
-            metrics::gauge!("state.committed.block.height", height.0 as _);
+            metrics::counter!("state.finalized.committed.block.count", 1);
+            metrics::gauge!("state.finalized.committed.block.height", height.0 as _);
         }
 
+        if self.queued_by_prev_hash.is_empty() {
+            // use -1 as a sentinel value for "None", because 0 is a valid height
+            self.max_queued_height = -1;
+        } else {
+            self.max_queued_height = std::cmp::max(self.max_queued_height, height.0 as _);
+        }
+
+        metrics::gauge!("state.finalized.queued.max.height", self.max_queued_height);
+
         metrics::gauge!(
-            "state.queued.block.count",
+            "state.finalized.queued.block.count",
             self.queued_by_prev_hash.len() as _
         );
     }


Test patch:
diff --git a/zebra-consensus/src/checkpoint/tests.rs b/zebra-consensus/src/checkpoint/tests.rs
index 6f2c99d8d..4b8eb8039 100644
--- a/zebra-consensus/src/checkpoint/tests.rs
+++ b/zebra-consensus/src/checkpoint/tests.rs
@@ -3,7 +3,7 @@
 use super::*;
 
 use super::types::Progress::*;
-use super::types::Target::*;
+use super::types::TargetHeight::*;
 
 use color_eyre::eyre::{eyre, Report};
 use futures::{future::TryFutureExt, stream::FuturesUnordered};