Fix one schema test and add a new one that's broken 😎

Author: michaelsproul

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -6778,13 +6778,21 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     #[allow(clippy::type_complexity)]
     pub fn chain_dump(
         &self,
+    ) -> Result<Vec<BeaconSnapshot<T::EthSpec, BlindedPayload<T::EthSpec>>>, Error> {
+        self.chain_dump_from_slot(Slot::new(0))
+    }
+
+    /// As for `chain_dump` but dumping only the portion of the chain newer than `from_slot`.
+    pub fn chain_dump_from_slot(
+        &self,
+        from_slot: Slot,
     ) -> Result<Vec<BeaconSnapshot<T::EthSpec, BlindedPayload<T::EthSpec>>>, Error> {
         let mut dump = vec![];
 
         let mut prev_block_root = None;
         let mut prev_beacon_state = None;
 
-        for res in self.forwards_iter_block_roots(Slot::new(0))? {
+        for res in self.forwards_iter_block_roots(from_slot)? {
             let (beacon_block_root, _) = res?;
 
             // Do not include snapshots at skipped slots.

beacon_node/beacon_chain/src/schema_change/migration_schema_v24.rs

@@ -177,6 +177,14 @@ pub fn upgrade_to_v24<T: BeaconChainTypes>(
         // If the node is already an archive node, we can set the anchor slot to 0 and copy
         // snapshots and diffs from the freezer DB to the hot DB in order to establish an initial
         // hot grid that is aligned/"perfect" (no `start_slot`/`anchor_slot` to worry about).
+        //
+        // This only works if all of the following are true:
+        //
+        // - We have the previous snapshot for the split state stored in the freezer DB, i.e.
+        //   if `previous_snapshot_slot >= state_upper_limit`.
+        // - The split state itself will be stored as a diff or snapshot in the new grid. We choose
+        //   not to support a split state that requires block replay, because computing its previous
+        //   state root from the DAG is not straight-forward.
         let dummy_start_slot = Slot::new(0);
         let closest_layer_points = db
             .hierarchy
@@ -191,9 +199,12 @@ pub fn upgrade_to_v24<T: BeaconChainTypes>(
                     "closest_layer_points must not be empty".to_string(),
                 ))?;
 
-        // If we have the previous snapshot stored in the freezer DB, then we can use this
-        // optimisation.
-        if previous_snapshot_slot >= anchor_info.state_upper_limit {
+        if previous_snapshot_slot >= anchor_info.state_upper_limit
+            && db
+                .hierarchy
+                .storage_strategy(split.slot, dummy_start_slot)
+                .is_ok_and(|strategy| !strategy.is_replay_from())
+        {
             info!(
                 %previous_snapshot_slot,
                 split_slot = %split.slot,
@@ -310,11 +321,13 @@ pub fn upgrade_to_v24<T: BeaconChainTypes>(
                     StorageStrategy::DiffFrom(_) | StorageStrategy::Snapshot => {
                         // We choose to not compute states during the epoch with block replay for
                         // simplicity. Therefore we can only support a hot heriarchy config where the
-                        // lowest layer is >= 5.
+                        // lowest layer is >= log2(SLOTS_PER_EPOCH).
+                        // FIXME(tree-states): we need to remove this restriction.
                         if slot % T::EthSpec::slots_per_epoch() != 0 {
-                            return Err(Error::MigrationError(
-                                "Hot hierarchy config lowest value must be >= 5".to_owned(),
-                            ));
+                            return Err(Error::MigrationError(format!(
+                                "Hot hierarchy config lowest value must be >= {}",
+                                T::EthSpec::slots_per_epoch().trailing_zeros()
+                            )));
                         }
 
                         // Load the full state and re-store it as a snapshot or diff.
@@ -344,7 +357,7 @@ pub fn upgrade_to_v24<T: BeaconChainTypes>(
                         // 2. Convert the summary to the new format.
                         if state_root == split.state_root {
                             return Err(Error::MigrationError(
-                                "unreachable: split state should be stored as a snapshot"
+                                "unreachable: split state should be stored as a snapshot or diff"
                                     .to_string(),
                             ));
                         }

beacon_node/beacon_chain/tests/store_tests.rs

@@ -3056,12 +3056,27 @@ async fn revert_minority_fork_on_resume() {
 // version is correct. This is the easiest schema test to write without historic versions of
 // Lighthouse on-hand, but has the disadvantage that the min version needs to be adjusted manually
 // as old downgrades are deprecated.
-#[tokio::test]
-async fn schema_downgrade_to_min_version() {
+async fn schema_downgrade_to_min_version(
+    store_config: StoreConfig,
+    reconstruct_historic_states: bool,
+) {
     let num_blocks_produced = E::slots_per_epoch() * 4;
     let db_path = tempdir().unwrap();
-    let store = get_store(&db_path);
-    let harness = get_harness(store.clone(), LOW_VALIDATOR_COUNT);
+    let spec = test_spec::<E>();
+
+    let chain_config = ChainConfig {
+        reconstruct_historic_states,
+        ..ChainConfig::default()
+    };
+    let import_all_data_columns = false;
+
+    let store = get_store_generic(&db_path, store_config.clone(), spec.clone());
+    let harness = get_harness_generic(
+        store.clone(),
+        LOW_VALIDATOR_COUNT,
+        chain_config.clone(),
+        import_all_data_columns,
+    );
 
     harness
         .extend_chain(
@@ -3082,7 +3097,7 @@ async fn schema_downgrade_to_min_version() {
     drop(harness);
 
     // Re-open the store.
-    let store = get_store(&db_path);
+    let store = get_store_generic(&db_path, store_config, spec);
 
     // Downgrade.
     migrate_schema::<DiskHarnessType<E>>(
@@ -3105,16 +3120,28 @@ async fn schema_downgrade_to_min_version() {
     // Recreate the harness.
     let harness = BeaconChainHarness::builder(MinimalEthSpec)
         .default_spec()
+        .chain_config(chain_config)
         .keypairs(KEYPAIRS[0..LOW_VALIDATOR_COUNT].to_vec())
         .testing_slot_clock(slot_clock)
         .resumed_disk_store(store.clone())
         .mock_execution_layer()
         .build();
 
+    // Check chain dump for appropriate range depending on whether this is an archive node.
+    let chain_dump_start_slot = if reconstruct_historic_states {
+        Slot::new(0)
+    } else {
+        store.get_split_slot()
+    };
+
     check_finalization(&harness, num_blocks_produced);
     check_split_slot(&harness, store.clone());
-    check_chain_dump(&harness, num_blocks_produced + 1);
-    check_iterators(&harness);
+    check_chain_dump_from_slot(
+        &harness,
+        chain_dump_start_slot,
+        num_blocks_produced + 1 - chain_dump_start_slot.as_u64(),
+    );
+    check_iterators_from_slot(&harness, chain_dump_start_slot);
 
     // Check that downgrading beyond the minimum version fails (bound is *tight*).
     let min_version_sub_1 = SchemaVersion(min_version.as_u64().checked_sub(1).unwrap());
@@ -3127,6 +3154,67 @@ async fn schema_downgrade_to_min_version() {
     .expect_err("should not downgrade below minimum version");
 }
 
+// Schema upgrade/downgrade on an archive node where the optimised migration does apply due
+// to the split state being aligned to a diff layer.
+#[tokio::test]
+async fn schema_downgrade_to_min_version_archive_node_grid_aligned() {
+    // Need to use 3 as the hierarchy exponent to get diffs on every epoch boundary with minimal
+    // spec.
+    schema_downgrade_to_min_version(
+        StoreConfig {
+            hierarchy_config: HierarchyConfig::from_str("3,4,5").unwrap(),
+            prune_payloads: false,
+            ..StoreConfig::default()
+        },
+        true,
+    )
+    .await
+}
+
+// Schema upgrade/downgrade on an archive node where the optimised migration DOES NOT apply
+// due to the split state NOT being aligned to a diff layer.
+#[tokio::test]
+async fn schema_downgrade_to_min_version_archive_node_grid_unaligned() {
+    schema_downgrade_to_min_version(
+        StoreConfig {
+            hierarchy_config: HierarchyConfig::from_str("7").unwrap(),
+            prune_payloads: false,
+            ..StoreConfig::default()
+        },
+        true,
+    )
+    .await
+}
+
+// Schema upgrade/downgrade on a full node with a fairly normal per-epoch diff config.
+#[tokio::test]
+async fn schema_downgrade_to_min_version_full_node_per_epoch_diffs() {
+    schema_downgrade_to_min_version(
+        StoreConfig {
+            hierarchy_config: HierarchyConfig::from_str("3,4,5").unwrap(),
+            prune_payloads: false,
+            ..StoreConfig::default()
+        },
+        false,
+    )
+    .await
+}
+
+// Schema upgrade/downgrade on a full node with dense per-slot diffs.
+// FIXME(tree-states): this will panic
+#[tokio::test]
+async fn schema_downgrade_to_min_version_full_node_dense_diffs() {
+    schema_downgrade_to_min_version(
+        StoreConfig {
+            hierarchy_config: HierarchyConfig::from_str("0,3,4,5").unwrap(),
+            prune_payloads: false,
+            ..StoreConfig::default()
+        },
+        true,
+    )
+    .await
+}
+
 /// Check that blob pruning prunes blobs older than the data availability boundary.
 #[tokio::test]
 async fn deneb_prune_blobs_happy_case() {
@@ -3700,7 +3788,11 @@ fn check_split_slot(
 
 /// Check that all the states in a chain dump have the correct tree hash.
 fn check_chain_dump(harness: &TestHarness, expected_len: u64) {
-    let mut chain_dump = harness.chain.chain_dump().unwrap();
+    check_chain_dump_from_slot(harness, Slot::new(0), expected_len)
+}
+
+fn check_chain_dump_from_slot(harness: &TestHarness, from_slot: Slot, expected_len: u64) {
+    let mut chain_dump = harness.chain.chain_dump_from_slot(from_slot).unwrap();
 
     assert_eq!(chain_dump.len() as u64, expected_len);
 
@@ -3748,7 +3840,7 @@ fn check_chain_dump(harness: &TestHarness, expected_len: u64) {
 
     let mut forward_block_roots = harness
         .chain
-        .forwards_iter_block_roots(Slot::new(0))
+        .forwards_iter_block_roots(from_slot)
         .expect("should get iter")
         .map(Result::unwrap)
         .collect::<Vec<_>>();
@@ -3769,10 +3861,14 @@ fn check_chain_dump(harness: &TestHarness, expected_len: u64) {
 /// Check that every state from the canonical chain is in the database, and that the
 /// reverse state and block root iterators reach genesis.
 fn check_iterators(harness: &TestHarness) {
+    check_iterators_from_slot(harness, Slot::new(0))
+}
+
+fn check_iterators_from_slot(harness: &TestHarness, slot: Slot) {
     let mut max_slot = None;
     for (state_root, slot) in harness
         .chain
-        .forwards_iter_state_roots(Slot::new(0))
+        .forwards_iter_state_roots(slot)
         .expect("should get iter")
         .map(Result::unwrap)
     {
@@ -3794,7 +3890,7 @@ fn check_iterators(harness: &TestHarness) {
     assert_eq!(
         harness
             .chain
-            .forwards_iter_block_roots(Slot::new(0))
+            .forwards_iter_block_roots(slot)
             .expect("should get iter")
             .last()
             .map(Result::unwrap)

beacon_node/store/src/hdiff.rs

@@ -788,6 +788,18 @@ impl StorageStrategy {
             Self::Snapshot => None,
         }
     }
+
+    pub fn is_replay_from(&self) -> bool {
+        matches!(self, Self::ReplayFrom(_))
+    }
+
+    pub fn is_diff_from(&self) -> bool {
+        matches!(self, Self::DiffFrom(_))
+    }
+
+    pub fn is_snapshot(&self) -> bool {
+        matches!(self, Self::Snapshot)
+    }
 }
 
 #[cfg(test)]