Fix ancestor to return most recent root in the case of skip slot (#6242)

* Revert "remove excess copies (#6142)"

This reverts commit c956905.
* Update ancestor function to handle skip slot
* Test
* Revert "Revert "remove excess copies (#6142)""

This reverts commit f1222b5.
* Merge refs/heads/master into fix-ancestor

beacon-chain/blockchain/process_block_helpers.go

@@ -380,15 +380,15 @@ func (s *Service) filterBlockRoots(ctx context.Context, roots [][32]byte) ([][32
 // ancestor returns the block root of an ancestry block from the input block root.
 //
 // Spec pseudocode definition:
-//   def get_ancestor(store: Store, root: Hash, slot: Slot) -> Hash:
+//   def get_ancestor(store: Store, root: Root, slot: Slot) -> Root:
 //    block = store.blocks[root]
 //    if block.slot > slot:
-//      return get_ancestor(store, block.parent_root, slot)
+//        return get_ancestor(store, block.parent_root, slot)
 //    elif block.slot == slot:
-//      return root
+//        return root
 //    else:
-//      # root is older than queried slot, thus a skip slot. Return most recent root prior to slot.
-//      return root
+//        # root is older than queried slot, thus a skip slot. Return most recent root prior to slot
+//        return root
 func (s *Service) ancestor(ctx context.Context, root []byte, slot uint64) ([]byte, error) {
 	ctx, span := trace.StartSpan(ctx, "forkchoice.ancestor")
 	defer span.End()
@@ -412,13 +412,7 @@ func (s *Service) ancestor(ctx context.Context, root []byte, slot uint64) ([]byt
 	}
 	b := signed.Block
 
-	// If we dont have the ancestor in the DB, simply return nil so rest of fork choice
-	// operation can proceed. This is not an error condition.
-	if b == nil || b.Slot < slot {
-		return nil, nil
-	}
-
-	if b.Slot == slot {
+	if b.Slot == slot || b.Slot < slot {
 		return root, nil
 	}
 

beacon-chain/blockchain/process_block_test.go

@@ -779,3 +779,57 @@ func TestCurrentSlot_HandlesOverflow(t *testing.T) {
 		t.Fatalf("Expected slot to be 0, got %d", slot)
 	}
 }
+
+func TestAncestor_HandleSkipSlot(t *testing.T) {
+	ctx := context.Background()
+	db := testDB.SetupDB(t)
+
+	cfg := &Config{BeaconDB: db}
+	service, err := NewService(ctx, cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	b1 := &ethpb.BeaconBlock{Slot: 1, ParentRoot: []byte{'a'}}
+	r1, err := ssz.HashTreeRoot(b1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	b100 := &ethpb.BeaconBlock{Slot: 100, ParentRoot: r1[:]}
+	r100, err := ssz.HashTreeRoot(b100)
+	if err != nil {
+		t.Fatal(err)
+	}
+	b200 := &ethpb.BeaconBlock{Slot: 200, ParentRoot: r100[:]}
+	r200, err := ssz.HashTreeRoot(b200)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, b := range []*ethpb.BeaconBlock{b1, b100, b200} {
+		beaconBlock := testutil.NewBeaconBlock()
+		beaconBlock.Block.Slot = b.Slot
+		beaconBlock.Block.ParentRoot = bytesutil.PadTo(b.ParentRoot, 32)
+		beaconBlock.Block.Body = &ethpb.BeaconBlockBody{}
+		if err := db.SaveBlock(context.Background(), beaconBlock); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Slots 100 to 200 are skip slots. Requesting root at 150 will yield root at 100. The last physical block.
+	r, err := service.ancestor(context.Background(), r200[:], 150)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytesutil.ToBytes32(r) != r100 {
+		t.Error("Did not get correct root")
+	}
+
+	// Slots 1 to 100 are skip slots. Requesting root at 50 will yield root at 1. The last physical block.
+	r, err = service.ancestor(context.Background(), r200[:], 50)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytesutil.ToBytes32(r) != r1 {
+		t.Error("Did not get correct root")
+	}
+}