Fix descendant_block_roots_of

beacon_node/beacon_chain/src/errors.rs

@@ -11,7 +11,6 @@ use crate::observed_aggregates::Error as ObservedAttestationsError;
 use crate::observed_attesters::Error as ObservedAttestersError;
 use crate::observed_block_producers::Error as ObservedBlockProducersError;
 use crate::observed_data_sidecars::Error as ObservedDataSidecarsError;
-use crate::summaries_dag::Error as SummariesDagError;
 use execution_layer::PayloadStatus;
 use fork_choice::ExecutionStatus;
 use futures::channel::mpsc::TrySendError;
@@ -224,7 +223,6 @@ pub enum BeaconChainError {
     EmptyRpcCustodyColumns,
     AttestationError(AttestationError),
     AttestationCommitteeIndexNotSet,
-    SummariesDagError(SummariesDagError),
 }
 
 easy_from_to!(SlotProcessingError, BeaconChainError);
@@ -256,7 +254,6 @@ easy_from_to!(EpochCacheError, BeaconChainError);
 easy_from_to!(LightClientUpdateError, BeaconChainError);
 easy_from_to!(MilhouseError, BeaconChainError);
 easy_from_to!(AttestationError, BeaconChainError);
-easy_from_to!(SummariesDagError, BeaconChainError);
 
 #[derive(Debug)]
 pub enum BlockProductionError {

beacon_node/beacon_chain/src/migrate.rs

@@ -1,7 +1,7 @@
 use crate::beacon_chain::BEACON_CHAIN_DB_KEY;
 use crate::errors::BeaconChainError;
 use crate::persisted_beacon_chain::{PersistedBeaconChain, DUMMY_CANONICAL_HEAD_BLOCK_ROOT};
-use crate::summaries_dag::{DAGStateSummary, StateSummariesDAG};
+use crate::summaries_dag::{self, DAGStateSummary, StateSummariesDAG};
 use parking_lot::Mutex;
 use slog::{debug, error, info, warn, Logger};
 use ssz::Decode;
@@ -116,6 +116,8 @@ pub enum PruningError {
     },
     UnexpectedEqualStateRoots,
     UnexpectedUnequalStateRoots,
+    MissingSummaryForFinalizedCheckpoint(Hash256),
+    SummariesDagError(summaries_dag::Error),
 }
 
 /// Message sent to the migration thread containing the information it needs to run.
@@ -386,7 +388,7 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
                 return;
             }
             Err(e) => {
-                warn!(log, "Block pruning failed"; "error" => ?e);
+                warn!(log, "Hot DB pruning failed"; "error" => ?e);
                 return;
             }
         };
@@ -539,6 +541,15 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
                     .map(|(_, summary)| summary.latest_block_root),
             );
 
+            // Sanity check, there is at least one summary with the new finalized block root
+            if !summary_block_roots.contains(&new_finalized_checkpoint.root) {
+                return Err(BeaconChainError::PruningError(
+                    PruningError::MissingSummaryForFinalizedCheckpoint(
+                        new_finalized_checkpoint.root,
+                    ),
+                ));
+            }
+
             let mut parent_block_roots = HashMap::new();
             for block_root in summary_block_roots {
                 let blinded_block = store
@@ -554,8 +565,11 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
         // From the DAG compute the list of roots that ascend from finalized root up to the split
         // slot. And run `migrate_database` with it
 
-        let block_roots_descendant_of_finalized =
-            summaries_dag.descendant_block_roots_of(&new_finalized_checkpoint.root)?;
+        // Note: The sanity check above for existance of at least one summary with
+        // new_finalized_checkpoint.root should ensure that this call never errors
+        let block_roots_descendant_of_finalized = summaries_dag
+            .descendant_block_roots_of(&new_finalized_checkpoint.root)
+            .map_err(PruningError::SummariesDagError)?;
 
         // TODO(hdiff): Could re-use the summaries dag
         let newly_finalized_state_roots =

beacon_node/beacon_chain/src/summaries_dag.rs

@@ -49,6 +49,8 @@ impl StateSummariesDAG {
                 .entry(*parent_block_root)
                 .or_default()
                 .push(*child_block_root);
+            // Add empty entry for the child block
+            child_block_roots.entry(*child_block_root).or_default();
         }
 
         Self {
@@ -155,3 +157,41 @@ impl From<HotStateSummary> for DAGStateSummary {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{Error, StateSummariesDAG};
+    use bls::FixedBytesExtended;
+    use std::collections::HashMap;
+    use types::Hash256;
+
+    #[test]
+    fn descendant_block_roots_of() {
+        let root_1 = Hash256::from_low_u64_le(1);
+        let root_2 = Hash256::from_low_u64_le(2);
+        let root_3 = Hash256::from_low_u64_le(3);
+        //                                 (child, parent)
+        let parents = HashMap::from_iter([(root_1, root_2)]);
+        let dag = StateSummariesDAG::new(vec![], parents);
+
+        // root 1 is known and has no childs
+        assert_eq!(
+            dag.descendant_block_roots_of(&root_1).unwrap(),
+            Vec::<Hash256>::new()
+        );
+        // root 2 is known and has childs
+        assert_eq!(
+            dag.descendant_block_roots_of(&root_2).unwrap(),
+            vec![root_1]
+        );
+        // root 3 is not known
+        {
+            let err = dag.descendant_block_roots_of(&root_3).unwrap_err();
+            if let Error::MissingChildBlockRoot(_) = err {
+                // ok
+            } else {
+                panic!("unexpected err {err:?}");
+            }
+        }
+    }
+}