Fix a bug in fork pruning (#1507)

Extracted from #1380 because merging #1380 proves to be contentious.

Co-authored-by: Michael Sproul <michael@sigmaprime.io>

beacon_node/beacon_chain/src/migrate.rs

@@ -202,11 +202,6 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> Migrate<E, Hot, Cold>
         old_finalized_block_hash: SignedBeaconBlockHash,
         new_finalized_block_hash: SignedBeaconBlockHash,
     ) {
-        if let Err(e) = process_finalization(self.db.clone(), state_root, &new_finalized_state) {
-            // This migrator is only used for testing, so we just log to stderr without a logger.
-            eprintln!("Migration error: {:?}", e);
-        }
-
         if let Err(e) = Self::prune_abandoned_forks(
             self.db.clone(),
             head_tracker,
@@ -216,6 +211,11 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> Migrate<E, Hot, Cold>
         ) {
             eprintln!("Pruning error: {:?}", e);
         }
+
+        if let Err(e) = process_finalization(self.db.clone(), state_root, &new_finalized_state) {
+            // This migrator is only used for testing, so we just log to stderr without a logger.
+            eprintln!("Migration error: {:?}", e);
+        }
     }
 }
 
@@ -325,6 +325,17 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
                 new_finalized_slot,
             )) = rx.recv()
             {
+                match Self::prune_abandoned_forks(
+                    db.clone(),
+                    head_tracker,
+                    old_finalized_block_hash,
+                    new_finalized_block_hash,
+                    new_finalized_slot,
+                ) {
+                    Ok(()) => {}
+                    Err(e) => warn!(log, "Block pruning failed: {:?}", e),
+                }
+
                 match process_finalization(db.clone(), state_root, &state) {
                     Ok(()) => {}
                     Err(Error::HotColdDBError(HotColdDBError::FreezeSlotUnaligned(slot))) => {
@@ -342,17 +353,6 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
                         );
                     }
                 };
-
-                match Self::prune_abandoned_forks(
-                    db.clone(),
-                    head_tracker,
-                    old_finalized_block_hash,
-                    new_finalized_block_hash,
-                    new_finalized_slot,
-                ) {
-                    Ok(()) => {}
-                    Err(e) => warn!(log, "Block pruning failed: {:?}", e),
-                }
             }
         });
 

beacon_node/beacon_chain/tests/store_tests.rs

@@ -743,7 +743,7 @@ fn prunes_abandoned_fork_between_two_finalized_checkpoints() {
     let (stray_blocks, stray_states, _, stray_head, _) = harness.add_stray_blocks(
         harness.get_head_state(),
         slot,
-        slots_per_epoch - 1,
+        slots_per_epoch - 3,
         &faulty_validators,
     );
 

beacon_node/store/src/hot_cold_store.rs

@@ -208,6 +208,13 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> HotColdDB<E, Hot, Cold>
     }
 
     /// Fetch a state from the store.
+    ///
+    /// If `slot` is provided then it will be used as a hint as to which database should
+    /// be checked. Importantly, if the slot hint is provided and indicates a slot that lies
+    /// in the freezer database, then only the freezer database will be accessed and `Ok(None)`
+    /// will be returned if the provided `state_root` doesn't match the state root of the
+    /// frozen state at `slot`. Consequently, if a state from a non-canonical chain is desired, it's
+    /// best to set `slot` to `None`, or call `load_hot_state` directly.
     pub fn get_state(
         &self,
         state_root: &Hash256,
@@ -217,7 +224,11 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> HotColdDB<E, Hot, Cold>
 
         if let Some(slot) = slot {
             if slot < self.get_split_slot() {
-                self.load_cold_state_by_slot(slot).map(Some)
+                // Although we could avoid a DB lookup by shooting straight for the
+                // frozen state using `load_cold_state_by_slot`, that would be incorrect
+                // in the case where the caller provides a `state_root` that's off the canonical
+                // chain. This way we avoid returning a state that doesn't match `state_root`.
+                self.load_cold_state(state_root)
             } else {
                 self.load_hot_state(state_root)
             }