Fix off-by-one in backfill sig verification (#5120)

* Fix off-by-one in backfill sig verification

* Add self-referential PR link

beacon_node/beacon_chain/src/data_availability_checker.rs

@@ -545,6 +545,18 @@ pub struct AvailableBlock<E: EthSpec> {
 }
 
 impl<E: EthSpec> AvailableBlock<E> {
+    pub fn __new_for_testing(
+        block_root: Hash256,
+        block: Arc<SignedBeaconBlock<E>>,
+        blobs: Option<BlobSidecarList<E>>,
+    ) -> Self {
+        Self {
+            block_root,
+            block,
+            blobs,
+        }
+    }
+
     pub fn block(&self) -> &SignedBeaconBlock<E> {
         &self.block
     }

beacon_node/beacon_chain/src/historical_blocks.rs

@@ -135,20 +135,20 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
             prev_block_slot = block.slot();
             expected_block_root = block.message().parent_root();
+            signed_blocks.push(block);
 
             // If we've reached genesis, add the genesis block root to the batch for all slots
             // between 0 and the first block slot, and set the anchor slot to 0 to indicate
             // completion.
             if expected_block_root == self.genesis_block_root {
                 let genesis_slot = self.spec.genesis_slot;
-                for slot in genesis_slot.as_usize()..block.slot().as_usize() {
+                for slot in genesis_slot.as_usize()..prev_block_slot.as_usize() {
                     chunk_writer.set(slot, self.genesis_block_root, &mut cold_batch)?;
                 }
                 prev_block_slot = genesis_slot;
                 expected_block_root = Hash256::zero();
                 break;
             }
-            signed_blocks.push(block);
         }
         chunk_writer.write(&mut cold_batch)?;
         // these were pushed in reverse order so we reverse again

beacon_node/beacon_chain/tests/store_tests.rs

@@ -3,6 +3,7 @@
 use beacon_chain::attestation_verification::Error as AttnError;
 use beacon_chain::block_verification_types::RpcBlock;
 use beacon_chain::builder::BeaconChainBuilder;
+use beacon_chain::data_availability_checker::AvailableBlock;
 use beacon_chain::schema_change::migrate_schema;
 use beacon_chain::test_utils::{
     mock_execution_layer_from_parts, test_spec, AttestationStrategy, BeaconChainHarness,
@@ -2547,6 +2548,25 @@ async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
         }
     }
 
+    // Corrupt the signature on the 1st block to ensure that the backfill processor is checking
+    // signatures correctly. Regression test for https://github.com/sigp/lighthouse/pull/5120.
+    let mut batch_with_invalid_first_block = available_blocks.clone();
+    batch_with_invalid_first_block[0] = {
+        let (block_root, block, blobs) = available_blocks[0].clone().deconstruct();
+        let mut corrupt_block = (*block).clone();
+        *corrupt_block.signature_mut() = Signature::empty();
+        AvailableBlock::__new_for_testing(block_root, Arc::new(corrupt_block), blobs)
+    };
+
+    // Importing the invalid batch should error.
+    assert!(matches!(
+        beacon_chain
+            .import_historical_block_batch(batch_with_invalid_first_block)
+            .unwrap_err(),
+        BeaconChainError::HistoricalBlockError(HistoricalBlockError::InvalidSignature)
+    ));
+
+    // Importing the batch with valid signatures should succeed.
     beacon_chain
         .import_historical_block_batch(available_blocks.clone())
         .unwrap();