Add unit tests for

book/src/dev/rfcs/0005-state-updates.md

@@ -404,11 +404,11 @@ chain and updates all side chains to match.
 3. Remove the lowest height block from the best chain with
    `let finalized_block = best_chain.pop_root();`
 
-4. Add `best_chain` back to `self.chain_set`
+4. Add `best_chain` back to `self.chain_set` if it is not empty
 
 5. For each remaining `chain` in `side_chains`
     - remove the lowest height block from `chain`
-    - If that block is equal to `finalized_block` add `chain` back to `self.chain_set`
+    - If that block is equal to `finalized_block` and `chain` is not empty add `chain` back to `self.chain_set`
     - Else, drop `chain`
 
 6. Return `finalized_block`

zebra-state/src/service/memory_state/chain.rs

@@ -117,6 +117,10 @@ impl Chain {
             .next_back()
             .expect("only called while blocks is populated")
     }
+
+    pub fn is_empty(&self) -> bool {
+        self.blocks.is_empty()
+    }
 }
 
 /// Helper trait to organize inverse operations done on the `Chain` type. Used to
@@ -414,9 +418,7 @@ impl Ord for Chain {
 
 #[cfg(test)]
 mod tests {
-    use transaction::Transaction;
-
-    use std::{env, fmt, mem};
+    use std::{env, fmt};
 
     use zebra_chain::serialization::ZcashDeserializeInto;
     use zebra_chain::{
@@ -425,6 +427,8 @@ mod tests {
     };
     use zebra_test::prelude::*;
 
+    use crate::tests::FakeChainHelper;
+
     use self::assert_eq;
     use super::*;
 
@@ -436,37 +440,6 @@ mod tests {
         }
     }
 
-    /// Helper trait for constructing "valid" looking chains of blocks
-    trait FakeChainHelper {
-        fn make_fake_child(&self) -> Arc<Block>;
-    }
-
-    impl FakeChainHelper for Block {
-        fn make_fake_child(&self) -> Arc<Block> {
-            let parent_hash = self.hash();
-            let mut child = Block::clone(self);
-            let mut transactions = mem::take(&mut child.transactions);
-            let mut tx = transactions.remove(0);
-
-            let input = match Arc::make_mut(&mut tx) {
-                Transaction::V1 { inputs, .. } => &mut inputs[0],
-                Transaction::V2 { inputs, .. } => &mut inputs[0],
-                Transaction::V3 { inputs, .. } => &mut inputs[0],
-                Transaction::V4 { inputs, .. } => &mut inputs[0],
-            };
-
-            match input {
-                transparent::Input::Coinbase { height, .. } => height.0 += 1,
-                _ => panic!("block must have a coinbase height to create a child"),
-            }
-
-            child.transactions.push(tx);
-            child.header.previous_block_hash = parent_hash;
-
-            Arc::new(child)
-        }
-    }
-
     #[test]
     fn construct_empty() {
         zebra_test::init();
@@ -511,6 +484,25 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn bigger_is_greater() -> Result<()> {
+        zebra_test::init();
+        let block: Arc<Block> =
+            zebra_test::vectors::BLOCK_MAINNET_434873_BYTES.zcash_deserialize_into()?;
+
+        let mut lesser_chain = Chain::default();
+        lesser_chain.push(block.clone());
+
+        let fake_child = block.make_fake_child();
+        let mut bigger_chain = Chain::default();
+        bigger_chain.push(block);
+        bigger_chain.push(fake_child);
+
+        assert!(bigger_chain > lesser_chain);
+
+        Ok(())
+    }
+
     fn arbitrary_chain(tip_height: block::Height) -> BoxedStrategy<Vec<Arc<Block>>> {
         Block::partial_chain_strategy(LedgerState::new(tip_height, Network::Mainnet), 100)
     }

zebra-state/src/service/memory_state/non_finalized_state.rs

@@ -33,15 +33,18 @@ impl NonFinalizedState {
 
         // remove the lowest height block from the best_chain as finalized_block
         let finalized_block = best_chain.pop_root();
+
         // add best_chain back to `self.chain_set`
-        self.chain_set.insert(best_chain);
+        if !best_chain.is_empty() {
+            self.chain_set.insert(best_chain);
+        }
 
         // for each remaining chain in side_chains
         for mut chain in side_chains {
             // remove the first block from `chain`
             let chain_start = chain.pop_root();
             // if block equals finalized_block
-            if chain_start == finalized_block {
+            if !chain.is_empty() && chain_start == finalized_block {
                 // add the chain back to `self.chain_set`
                 self.chain_set.insert(chain);
             } else {
@@ -217,3 +220,66 @@ impl NonFinalizedState {
         metrics::gauge!("state.memory.best.chain.length", self.best_chain_len() as _);
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use zebra_chain::serialization::ZcashDeserializeInto;
+    use zebra_test::prelude::*;
+
+    use crate::tests::FakeChainHelper;
+
+    use self::assert_eq;
+    use super::*;
+
+    #[test]
+    fn best_chain_wins() -> Result<()> {
+        zebra_test::init();
+
+        let block2: Arc<Block> =
+            zebra_test::vectors::BLOCK_MAINNET_419201_BYTES.zcash_deserialize_into()?;
+        let block1: Arc<Block> =
+            zebra_test::vectors::BLOCK_MAINNET_419200_BYTES.zcash_deserialize_into()?;
+        // Create a random block which will have a much worse difficulty hash
+        // than an intentionally mined block from the mainnet
+        let child = block1.make_fake_child();
+
+        let expected_hash = block2.hash();
+
+        let mut state = NonFinalizedState::default();
+        state.commit_new_chain(block2);
+        state.commit_new_chain(child);
+
+        let best_chain = state.best_chain().unwrap();
+        assert!(best_chain.height_by_hash.contains_key(&expected_hash));
+
+        Ok(())
+    }
+
+    #[test]
+    fn finalize_pops_from_best_chain() -> Result<()> {
+        zebra_test::init();
+
+        let block2: Arc<Block> =
+            zebra_test::vectors::BLOCK_MAINNET_419201_BYTES.zcash_deserialize_into()?;
+        let block1: Arc<Block> =
+            zebra_test::vectors::BLOCK_MAINNET_419200_BYTES.zcash_deserialize_into()?;
+        // Create a random block which will have a much worse difficulty hash
+        // than an intentionally mined block from the mainnet
+        let child = block1.make_fake_child();
+
+        let mut state = NonFinalizedState::default();
+        state.commit_new_chain(block1.clone());
+        state.commit_block(block2.clone());
+        state.commit_block(child);
+
+        let finalized = state.finalize();
+        assert_eq!(block1, finalized);
+
+        let finalized = state.finalize();
+        assert_eq!(block2, finalized);
+
+        assert!(state.best_chain().is_none());
+
+        Ok(())
+    }
+}

zebra-state/src/tests.rs

@@ -1,7 +1,44 @@
-use zebra_chain::block;
+use std::{mem, sync::Arc};
+
+use zebra_chain::{
+    block::{self, Block},
+    transaction::Transaction,
+    transparent,
+};
 
 use super::*;
 
+/// Helper trait for constructing "valid" looking chains of blocks
+pub trait FakeChainHelper {
+    fn make_fake_child(&self) -> Arc<Block>;
+}
+
+impl FakeChainHelper for Block {
+    fn make_fake_child(&self) -> Arc<Block> {
+        let parent_hash = self.hash();
+        let mut child = Block::clone(self);
+        let mut transactions = mem::take(&mut child.transactions);
+        let mut tx = transactions.remove(0);
+
+        let input = match Arc::make_mut(&mut tx) {
+            Transaction::V1 { inputs, .. } => &mut inputs[0],
+            Transaction::V2 { inputs, .. } => &mut inputs[0],
+            Transaction::V3 { inputs, .. } => &mut inputs[0],
+            Transaction::V4 { inputs, .. } => &mut inputs[0],
+        };
+
+        match input {
+            transparent::Input::Coinbase { height, .. } => height.0 += 1,
+            _ => panic!("block must have a coinbase height to create a child"),
+        }
+
+        child.transactions.push(tx);
+        child.header.previous_block_hash = parent_hash;
+
+        Arc::new(child)
+    }
+}
+
 /// Block heights, and the expected minimum block locator height
 static BLOCK_LOCATOR_CASES: &[(u32, u32)] = &[
     (0, 0),