Don't log extra diffs as roots

dapplion · dapplion · commit ceb59e39df31 · 2025-03-07T01:32:51.000-03:00
diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs
@@ -8,7 +8,7 @@ use std::sync::{mpsc, Arc};
 use std::thread;
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
 use store::hot_cold_store::{migrate_database, HotColdDBError};
-use store::{Error, ItemStore, StoreOp};
+use store::{Error, ItemStore, Split, StoreOp};
 pub use store::{HotColdDB, MemoryStore};
 use types::{BeaconState, BeaconStateHash, Checkpoint, Epoch, EthSpec, Hash256, Slot};
 
@@ -319,19 +319,24 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
             }
         };
 
-        match migrate_database(
+        let split_prior_to_migration = match migrate_database(
             db.clone(),
             finalized_state_root.into(),
             finalized_block_root,
             &finalized_state,
         ) {
-            Ok(()) => {}
+            Ok(split_change) => {
+                // Migration run, return the split before the migration
+                split_change.previous
+            }
             Err(Error::HotColdDBError(HotColdDBError::FreezeSlotUnaligned(slot))) => {
                 debug!(
                     log,
                     "Database migration postponed, unaligned finalized block";
                     "slot" => slot.as_u64()
                 );
+                // Migration did not run, return the current split info
+                db.get_split_info()
             }
             Err(e) => {
                 warn!(
@@ -348,6 +353,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
             finalized_state_root.into(),
             &finalized_state,
             notif.finalized_checkpoint,
+            split_prior_to_migration,
             log,
         ) {
             Ok(PruningOutcome::Successful {
@@ -457,6 +463,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
         new_finalized_state_root: Hash256,
         new_finalized_state: &BeaconState<E>,
         new_finalized_checkpoint: Checkpoint,
+        split_prior_to_migration: Split,
         log: &Logger,
     ) -> Result<PruningOutcome, BeaconChainError> {
         let new_finalized_slot = new_finalized_checkpoint
@@ -483,6 +490,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
             "Starting database pruning";
             "new_finalized_checkpoint" => ?new_finalized_checkpoint,
             "new_finalized_state_root" => ?new_finalized_state_root,
+            "split_prior_to_migration" => ?split_prior_to_migration,
         );
 
         let state_summaries_dag = {
@@ -521,13 +529,21 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
         // To debug faulty trees log if we unexpectedly have more than one root. These trees may not
         // result in an error, as they may not be queried in the codepaths below.
         let state_summaries_dag_roots = state_summaries_dag.tree_roots();
-        if state_summaries_dag_roots.len() > 1 {
+        let state_summaries_dag_roots_post_split = state_summaries_dag_roots
+            .iter()
+            .filter(|(_, s)| s.slot >= split_prior_to_migration.slot)
+            .collect::<Vec<_>>();
+        // Because of the additional HDiffs kept for the grid prior to finalization the tree_roots
+        // function will consider them roots. Those are expected. We just want to assert that the
+        // relevant tree of states (post-split) is well-formed.
+        if state_summaries_dag_roots_post_split.len() > 1 {
             warn!(
                 log,
                 "State summaries DAG found more than one root";
                 "location" => "pruning",
                 "new_finalized_state_root" => ?new_finalized_state_root,
-                "state_summaries_dag_roots" => ?state_summaries_dag_roots
+                "split_prior_to_migration_slot" => split_prior_to_migration.slot,
+                "state_summaries_dag_roots_post_split" => ?state_summaries_dag_roots_post_split
             );
         }
 
@@ -693,6 +709,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
             "Extra pruning information";
             "new_finalized_checkpoint" => ?new_finalized_checkpoint,
             "new_finalized_state_root" => ?new_finalized_state_root,
+            "split_prior_to_migration" => ?split_prior_to_migration,
             "newly_finalized_blocks" => newly_finalized_blocks.len(),
             "newly_finalized_state_roots" => newly_finalized_state_roots.len(),
             "newly_finalized_states_min_slot" => newly_finalized_states_min_slot,
diff --git a/beacon_node/beacon_chain/src/summaries_dag.rs b/beacon_node/beacon_chain/src/summaries_dag.rs
@@ -238,10 +238,14 @@ impl StateSummariesDAG {
         self.state_summaries_by_state_root
             .iter()
             .filter_map(|(state_root, summary)| {
-                if summary.previous_state_root == Hash256::ZERO {
-                    Some((*state_root, *summary))
-                } else {
+                if self
+                    .state_summaries_by_state_root
+                    .contains_key(&summary.previous_state_root)
+                {
+                    // Summaries with a known parent are not roots
                     None
+                } else {
+                    Some((*state_root, *summary))
                 }
             })
             .collect()
diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs
@@ -3302,7 +3302,7 @@ pub fn migrate_database<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>(
     finalized_state_root: Hash256,
     finalized_block_root: Hash256,
     finalized_state: &BeaconState<E>,
-) -> Result<(), Error> {
+) -> Result<SplitChange, Error> {
     debug!(
         store.log,
         "Freezer migration started";
@@ -3312,12 +3312,12 @@ pub fn migrate_database<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>(
     // 0. Check that the migration is sensible.
     // The new finalized state must increase the current split slot, and lie on an epoch
     // boundary (in order for the hot state summary scheme to work).
-    let current_split_slot = store.split.read_recursive().slot;
+    let current_split = *store.split.read_recursive();
     let anchor_info = store.anchor_info.read_recursive().clone();
 
-    if finalized_state.slot() < current_split_slot {
+    if finalized_state.slot() < current_split.slot {
         return Err(HotColdDBError::FreezeSlotError {
-            current_split_slot,
+            current_split_slot: current_split.slot,
             proposed_split_slot: finalized_state.slot(),
         }
         .into());
@@ -3334,7 +3334,7 @@ pub fn migrate_database<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>(
     // Iterate in descending order until the current split slot
     let state_roots: Vec<_> =
         process_results(RootsIterator::new(&store, finalized_state), |iter| {
-            iter.take_while(|(_, _, slot)| *slot >= current_split_slot)
+            iter.take_while(|(_, _, slot)| *slot >= current_split.slot)
                 .collect()
         })?;
 
@@ -3392,41 +3392,42 @@ pub fn migrate_database<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>(
     // in the worst case we will restart with the old split and re-run the migration.
     store.cold_db.do_atomically(cold_db_block_ops)?;
     store.cold_db.sync()?;
-    {
+    let new_split = {
         let mut split_guard = store.split.write();
-        let latest_split_slot = split_guard.slot;
+        let latest_split = *split_guard;
 
         // Detect a situation where the split point is (erroneously) changed from more than one
         // place in code.
-        if latest_split_slot != current_split_slot {
+        if latest_split.slot != current_split.slot {
             error!(
                 store.log,
                 "Race condition detected: Split point changed while copying states to the freezer";
-                "previous split slot" => current_split_slot,
-                "current split slot" => latest_split_slot,
+                "previous split slot" => current_split.slot,
+                "current split slot" => latest_split.slot,
             );
 
             // Assume the freezing procedure will be retried in case this happens.
             return Err(Error::SplitPointModified(
-                current_split_slot,
-                latest_split_slot,
+                current_split.slot,
+                latest_split.slot,
             ));
         }
 
         // Before updating the in-memory split value, we flush it to disk first, so that should the
         // OS process die at this point, we pick up from the right place after a restart.
-        let split = Split {
+        let new_split = Split {
             slot: finalized_state.slot(),
             state_root: finalized_state_root,
             block_root: finalized_block_root,
         };
-        store.hot_db.put_sync(&SPLIT_KEY, &split)?;
+        store.hot_db.put_sync(&SPLIT_KEY, &new_split)?;
 
         // Split point is now persisted in the hot database on disk. The in-memory split point
         // hasn't been modified elsewhere since we keep a write lock on it. It's safe to update
         // the in-memory split point now.
-        *split_guard = split;
-    }
+        *split_guard = new_split;
+        new_split
+    };
 
     // Update the cache's view of the finalized state.
     store.update_finalized_state(
@@ -3441,7 +3442,16 @@ pub fn migrate_database<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>>(
         "slot" => finalized_state.slot()
     );
 
-    Ok(())
+    Ok(SplitChange {
+        previous: current_split,
+        new: new_split,
+    })
+}
+
+#[derive(Debug)]
+pub struct SplitChange {
+    pub previous: Split,
+    pub new: Split,
 }
 
 /// Struct for storing the split slot and state root in the database.
