Fix lookups of the block at oldest_block_slot (#7693)

Closes:

- https://github.com/sigp/lighthouse/issues/7690


  Another checkpoint sync related fix! See issue for a description of the bug.

We fix it by just loading the block root of the `oldest_block_slot`, rather than trying to load the slot prior, which will always fail.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -991,6 +991,14 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             return Ok(root_opt);
         }
 
+        // Do not try to access the previous slot if it's older than the oldest block root
+        // stored in the database. Instead, load just the block root at `oldest_block_slot`,
+        // under the assumption that the `oldest_block_slot` *is not* a skipped slot (should be
+        // true because it is set by the oldest *block*).
+        if request_slot == self.store.get_anchor_info().oldest_block_slot {
+            return self.block_root_at_slot_skips_prev(request_slot);
+        }
+
         if let Some(((prev_root, _), (curr_root, curr_slot))) = process_results(
             self.forwards_iter_block_roots_until(prev_slot, request_slot)?,
             |iter| iter.tuple_windows().next(),

beacon_node/beacon_chain/tests/store_tests.rs

@@ -2238,7 +2238,15 @@ async fn weak_subjectivity_sync_easy() {
     let num_initial_slots = E::slots_per_epoch() * 11;
     let checkpoint_slot = Slot::new(E::slots_per_epoch() * 9);
     let slots = (1..num_initial_slots).map(Slot::new).collect();
-    weak_subjectivity_sync_test(slots, checkpoint_slot).await
+    weak_subjectivity_sync_test(slots, checkpoint_slot, None).await
+}
+
+#[tokio::test]
+async fn weak_subjectivity_sync_single_block_batches() {
+    let num_initial_slots = E::slots_per_epoch() * 11;
+    let checkpoint_slot = Slot::new(E::slots_per_epoch() * 9);
+    let slots = (1..num_initial_slots).map(Slot::new).collect();
+    weak_subjectivity_sync_test(slots, checkpoint_slot, Some(1)).await
 }
 
 #[tokio::test]
@@ -2252,7 +2260,7 @@ async fn weak_subjectivity_sync_unaligned_advanced_checkpoint() {
             slot <= checkpoint_slot - 3 || slot > checkpoint_slot
         })
         .collect();
-    weak_subjectivity_sync_test(slots, checkpoint_slot).await
+    weak_subjectivity_sync_test(slots, checkpoint_slot, None).await
 }
 
 #[tokio::test]
@@ -2266,7 +2274,7 @@ async fn weak_subjectivity_sync_unaligned_unadvanced_checkpoint() {
             slot <= checkpoint_slot || slot > checkpoint_slot + 3
         })
         .collect();
-    weak_subjectivity_sync_test(slots, checkpoint_slot).await
+    weak_subjectivity_sync_test(slots, checkpoint_slot, None).await
 }
 
 // Regression test for https://github.com/sigp/lighthouse/issues/4817
@@ -2278,7 +2286,7 @@ async fn weak_subjectivity_sync_skips_at_genesis() {
     let end_slot = E::slots_per_epoch() * 4;
     let slots = (start_slot..end_slot).map(Slot::new).collect();
     let checkpoint_slot = Slot::new(E::slots_per_epoch() * 2);
-    weak_subjectivity_sync_test(slots, checkpoint_slot).await
+    weak_subjectivity_sync_test(slots, checkpoint_slot, None).await
 }
 
 // Checkpoint sync from the genesis state.
@@ -2291,10 +2299,14 @@ async fn weak_subjectivity_sync_from_genesis() {
     let end_slot = E::slots_per_epoch() * 2;
     let slots = (start_slot..end_slot).map(Slot::new).collect();
     let checkpoint_slot = Slot::new(0);
-    weak_subjectivity_sync_test(slots, checkpoint_slot).await
+    weak_subjectivity_sync_test(slots, checkpoint_slot, None).await
 }
 
-async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
+async fn weak_subjectivity_sync_test(
+    slots: Vec<Slot>,
+    checkpoint_slot: Slot,
+    backfill_batch_size: Option<usize>,
+) {
     // Build an initial chain on one harness, representing a synced node with full history.
     let num_final_blocks = E::slots_per_epoch() * 2;
 
@@ -2557,30 +2569,57 @@ async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
                 .unwrap_err(),
             HistoricalBlockError::InvalidSignature
         ));
-
-        let available_blocks_slots = available_blocks
-            .iter()
-            .map(|block| (block.block().slot(), block.block().canonical_root()))
-            .collect::<Vec<_>>();
-        info!(
-            ?available_blocks_slots,
-            "wss_block_slot" = wss_block.slot().as_usize(),
-            "Importing historical block batch"
-        );
-
-        // Importing the batch with valid signatures should succeed.
-        let available_blocks_dup = available_blocks.iter().map(clone_block).collect::<Vec<_>>();
         assert_eq!(beacon_chain.store.get_oldest_block_slot(), wss_block.slot());
-        beacon_chain
-            .import_historical_block_batch(available_blocks_dup)
-            .unwrap();
-        assert_eq!(beacon_chain.store.get_oldest_block_slot(), 0);
 
-        // Resupplying the blocks should not fail, they can be safely ignored.
-        beacon_chain
-            .import_historical_block_batch(available_blocks)
-            .unwrap();
+        let batch_size = backfill_batch_size.unwrap_or(available_blocks.len());
+
+        for batch in available_blocks.rchunks(batch_size) {
+            let available_blocks_slots = batch
+                .iter()
+                .map(|block| (block.block().slot(), block.block().canonical_root()))
+                .collect::<Vec<_>>();
+            info!(
+                ?available_blocks_slots,
+                "wss_block_slot" = wss_block.slot().as_usize(),
+                "Importing historical block batch"
+            );
+
+            // Importing the batch with valid signatures should succeed.
+            let available_blocks_batch1 = batch.iter().map(clone_block).collect::<Vec<_>>();
+            beacon_chain
+                .import_historical_block_batch(available_blocks_batch1)
+                .unwrap();
+
+            // We should be able to load the block root at the `oldest_block_slot`.
+            //
+            // This is a regression test for: https://github.com/sigp/lighthouse/issues/7690
+            let oldest_block_imported = &batch[0];
+            let (oldest_block_slot, oldest_block_root) =
+                if oldest_block_imported.block().parent_root() == beacon_chain.genesis_block_root {
+                    (Slot::new(0), beacon_chain.genesis_block_root)
+                } else {
+                    available_blocks_slots[0]
+                };
+            assert_eq!(
+                beacon_chain.store.get_oldest_block_slot(),
+                oldest_block_slot
+            );
+            assert_eq!(
+                beacon_chain
+                    .block_root_at_slot(oldest_block_slot, WhenSlotSkipped::None)
+                    .unwrap()
+                    .unwrap(),
+                oldest_block_root
+            );
+
+            // Resupplying the blocks should not fail, they can be safely ignored.
+            let available_blocks_batch2 = batch.iter().map(clone_block).collect::<Vec<_>>();
+            beacon_chain
+                .import_historical_block_batch(available_blocks_batch2)
+                .unwrap();
+        }
     }
+    assert_eq!(beacon_chain.store.get_oldest_block_slot(), 0);
 
     // Sanity check for non-aligned WSS starts, to make sure the WSS block is persisted properly
     if wss_block_slot != wss_state_slot {