Repository: ZcashFoundation/zebra
Pull request #1288 (2020-11-12 22:01:24)

reorganize stop check
closes https://github.com/ZcashFoundation/zebra/issues/1205


Patch:
diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs
index d4018c434..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
@@ -400,6 +400,14 @@ where
 
     /// Increase the current checkpoint height to `verified_height`,
     fn update_progress(&mut self, verified_height: block::Height) {
+        if let Some(max_height) = self.queued.keys().next_back() {
+            metrics::gauge!("checkpoint.queued.max.height", max_height.0 as i64);
+        } else {
+            // use -1 as a sentinel value for "None", because 0 is a valid height
+            metrics::gauge!("checkpoint.queued.max.height", -1);
+        }
+        metrics::gauge!("checkpoint.queued_slots", self.queued.len() as i64);
+
         // Ignore blocks that are below the previous checkpoint, or otherwise
         // have invalid heights.
         //
@@ -837,8 +845,6 @@ where
         // should be rare.
         self.process_checkpoint_range();
 
-        metrics::gauge!("checkpoint.queued_slots", self.queued.len() as i64);
-
         let mut state_service = self.state_service.clone();
         async move {
             let hash = rx
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..002f2b80e 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,
@@ -52,21 +53,13 @@ pub struct FinalizedState {
     debug_stop_at_height: Option<block::Height>,
 }
 
-/// Where is the stop check being performed?
-#[derive(Debug, Copy, Clone, Eq, PartialEq)]
-enum StopCheckContext {
-    /// Checking when the state is loaded
-    OnLoad,
-    /// Checking when a block is committed
-    OnCommit,
-}
-
 impl FinalizedState {
     pub fn new(config: &Config, network: Network) -> Self {
         let db = config.sled_config(network).open().unwrap();
 
         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(),
@@ -78,11 +71,32 @@ impl FinalizedState {
         };
 
         if let Some(tip_height) = new_state.finalized_tip_height() {
-            new_state.stop_if_at_height_limit(
-                StopCheckContext::OnLoad,
-                tip_height,
-                new_state.finalized_tip_hash(),
-            );
+            if new_state.is_at_stop_height(tip_height) {
+                let debug_stop_at_height = new_state
+                    .debug_stop_at_height
+                    .expect("true from `is_at_stop_height` implies `debug_stop_at_height` is Some");
+
+                let tip_hash = new_state.finalized_tip_hash();
+
+                if tip_height > debug_stop_at_height {
+                    tracing::error!(
+                        ?debug_stop_at_height,
+                        ?tip_height,
+                        ?tip_hash,
+                        "previous state height is greater than the stop height",
+                    );
+                }
+
+                tracing::info!(
+                    ?debug_stop_at_height,
+                    ?tip_height,
+                    ?tip_hash,
+                    "state is already at the configured height"
+                );
+
+                // There's no need to sync before exit, because the trees have just been opened
+                std::process::exit(0);
+            }
         }
 
         new_state
@@ -112,59 +126,17 @@ impl FinalizedState {
     /// Flushes sled trees before exiting.
     ///
     /// `called_from` and `block_hash` are used for assertions and logging.
-    fn stop_if_at_height_limit(
-        &self,
-        called_from: StopCheckContext,
-        block_height: block::Height,
-        block_hash: block::Hash,
-    ) {
+    fn is_at_stop_height(&self, block_height: block::Height) -> bool {
         let debug_stop_at_height = match self.debug_stop_at_height {
             Some(debug_stop_at_height) => debug_stop_at_height,
-            None => return,
+            None => return false,
         };
 
         if block_height < debug_stop_at_height {
-            return;
-        }
-
-        // this error is expected on load, but unexpected on commit
-        if block_height > debug_stop_at_height {
-            if called_from == StopCheckContext::OnLoad {
-                tracing::error!(
-                    ?debug_stop_at_height,
-                    ?called_from,
-                    ?block_height,
-                    ?block_hash,
-                    "previous state height is greater than the stop height",
-                );
-            } else {
-                unreachable!("committed blocks must be committed in order");
-            }
+            return false;
         }
 
-        // Don't sync when the trees have just been opened
-        if called_from == StopCheckContext::OnCommit {
-            if let Err(e) = self.flush() {
-                tracing::error!(
-                    ?e,
-                    ?debug_stop_at_height,
-                    ?called_from,
-                    ?block_height,
-                    ?block_hash,
-                    "error flushing sled state before stopping"
-                );
-            }
-        }
-
-        tracing::info!(
-            ?debug_stop_at_height,
-            ?called_from,
-            ?block_height,
-            ?block_hash,
-            "stopping at configured height"
-        );
-
-        std::process::exit(0);
+        true
     }
 
     /// Queue a finalized block to be committed to the state.
@@ -173,6 +145,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 +154,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 _
         );
     }
@@ -295,8 +277,19 @@ impl FinalizedState {
                 },
             );
 
-        if result.is_ok() {
-            self.stop_if_at_height_limit(StopCheckContext::OnCommit, height, hash);
+        if result.is_ok() && self.is_at_stop_height(height) {
+            if let Err(e) = self.flush() {
+                tracing::error!(
+                    ?e,
+                    ?height,
+                    ?hash,
+                    "error flushing sled state before stopping"
+                );
+            }
+
+            tracing::info!(?height, ?hash, "stopping at configured height");
+
+            std::process::exit(0);
         }
 
         result.map_err(Into::into)


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};
diff --git a/zebrad/tests/acceptance.rs b/zebrad/tests/acceptance.rs
index 3aedb972d..227d94354 100644
--- a/zebrad/tests/acceptance.rs
+++ b/zebrad/tests/acceptance.rs
@@ -539,7 +539,7 @@ fn restart_stop_at_height() -> Result<()> {
     sync_until(
         Height(0),
         Mainnet,
-        "called_from=OnLoad",
+        "state is already at the configured height",
         STOP_ON_LOAD_TIMEOUT,
         Some(reuse_tempdir),
     )?;