Add some assertions about outliers (#10773)

I think I have finally teased apart the codepaths which handle outliers, and those that handle non-outliers. 
Let's add some assertions to demonstrate my newfound knowledge.

Author: richvdh

changelog.d/10773.misc

@@ -0,0 +1 @@
+Clean up some of the federation event authentication code for clarity.

synapse/handlers/federation_event.py

@@ -173,6 +173,9 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None:
             pdu: received PDU
         """
 
+        # We should never see any outliers here.
+        assert not pdu.internal_metadata.outlier
+
         room_id = pdu.room_id
         event_id = pdu.event_id
 
@@ -232,77 +235,71 @@ async def on_receive_pdu(self, origin: str, pdu: EventBase) -> None:
             )
             return None
 
-        # Check that the event passes auth based on the state at the event. This is
-        # done for events that are to be added to the timeline (non-outliers).
-        #
-        # Get missing pdus if necessary:
-        #  - Fetching any missing prev events to fill in gaps in the graph
-        #  - Fetching state if we have a hole in the graph
-        if not pdu.internal_metadata.is_outlier():
-            prevs = set(pdu.prev_event_ids())
-            seen = await self._store.have_events_in_timeline(prevs)
-            missing_prevs = prevs - seen
+        # Try to fetch any missing prev events to fill in gaps in the graph
+        prevs = set(pdu.prev_event_ids())
+        seen = await self._store.have_events_in_timeline(prevs)
+        missing_prevs = prevs - seen
 
-            if missing_prevs:
-                # We only backfill backwards to the min depth.
-                min_depth = await self.get_min_depth_for_context(pdu.room_id)
-                logger.debug("min_depth: %d", min_depth)
+        if missing_prevs:
+            # We only backfill backwards to the min depth.
+            min_depth = await self.get_min_depth_for_context(pdu.room_id)
+            logger.debug("min_depth: %d", min_depth)
 
-                if min_depth is not None and pdu.depth > min_depth:
-                    # If we're missing stuff, ensure we only fetch stuff one
-                    # at a time.
+            if min_depth is not None and pdu.depth > min_depth:
+                # If we're missing stuff, ensure we only fetch stuff one
+                # at a time.
+                logger.info(
+                    "Acquiring room lock to fetch %d missing prev_events: %s",
+                    len(missing_prevs),
+                    shortstr(missing_prevs),
+                )
+                with (await self._room_pdu_linearizer.queue(pdu.room_id)):
                     logger.info(
-                        "Acquiring room lock to fetch %d missing prev_events: %s",
+                        "Acquired room lock to fetch %d missing prev_events",
                         len(missing_prevs),
-                        shortstr(missing_prevs),
                     )
-                    with (await self._room_pdu_linearizer.queue(pdu.room_id)):
-                        logger.info(
-                            "Acquired room lock to fetch %d missing prev_events",
-                            len(missing_prevs),
+
+                    try:
+                        await self._get_missing_events_for_pdu(
+                            origin, pdu, prevs, min_depth
                         )
+                    except Exception as e:
+                        raise Exception(
+                            "Error fetching missing prev_events for %s: %s"
+                            % (event_id, e)
+                        ) from e
 
-                        try:
-                            await self._get_missing_events_for_pdu(
-                                origin, pdu, prevs, min_depth
-                            )
-                        except Exception as e:
-                            raise Exception(
-                                "Error fetching missing prev_events for %s: %s"
-                                % (event_id, e)
-                            ) from e
-
-                    # Update the set of things we've seen after trying to
-                    # fetch the missing stuff
-                    seen = await self._store.have_events_in_timeline(prevs)
-                    missing_prevs = prevs - seen
-
-                    if not missing_prevs:
-                        logger.info("Found all missing prev_events")
-
-                if missing_prevs:
-                    # since this event was pushed to us, it is possible for it to
-                    # become the only forward-extremity in the room, and we would then
-                    # trust its state to be the state for the whole room. This is very
-                    # bad. Further, if the event was pushed to us, there is no excuse
-                    # for us not to have all the prev_events. (XXX: apart from
-                    # min_depth?)
-                    #
-                    # We therefore reject any such events.
-                    logger.warning(
-                        "Rejecting: failed to fetch %d prev events: %s",
-                        len(missing_prevs),
-                        shortstr(missing_prevs),
-                    )
-                    raise FederationError(
-                        "ERROR",
-                        403,
-                        (
-                            "Your server isn't divulging details about prev_events "
-                            "referenced in this event."
-                        ),
-                        affected=pdu.event_id,
-                    )
+                # Update the set of things we've seen after trying to
+                # fetch the missing stuff
+                seen = await self._store.have_events_in_timeline(prevs)
+                missing_prevs = prevs - seen
+
+                if not missing_prevs:
+                    logger.info("Found all missing prev_events")
+
+            if missing_prevs:
+                # since this event was pushed to us, it is possible for it to
+                # become the only forward-extremity in the room, and we would then
+                # trust its state to be the state for the whole room. This is very
+                # bad. Further, if the event was pushed to us, there is no excuse
+                # for us not to have all the prev_events. (XXX: apart from
+                # min_depth?)
+                #
+                # We therefore reject any such events.
+                logger.warning(
+                    "Rejecting: failed to fetch %d prev events: %s",
+                    len(missing_prevs),
+                    shortstr(missing_prevs),
+                )
+                raise FederationError(
+                    "ERROR",
+                    403,
+                    (
+                        "Your server isn't divulging details about prev_events "
+                        "referenced in this event."
+                    ),
+                    affected=pdu.event_id,
+                )
 
         await self._process_received_pdu(origin, pdu, state=None)
 
@@ -885,8 +882,13 @@ async def _process_received_pdu(
         state: Optional[Iterable[EventBase]],
         backfilled: bool = False,
     ) -> None:
-        """Called when we have a new pdu. We need to do auth checks and put it
-        through the StateHandler.
+        """Called when we have a new non-outlier event.
+
+        This is called when we have a new event to add to the room DAG - either directly
+        via a /send request, retrieved via get_missing_events after a /send request, or
+        backfilled after a client request.
+
+        We need to do auth checks and put it through the StateHandler.
 
         Args:
             origin: server sending the event
@@ -901,6 +903,7 @@ async def _process_received_pdu(
                 notification to clients, and validation of device keys.)
         """
         logger.debug("Processing event: %s", event)
+        assert not event.internal_metadata.outlier
 
         try:
             context = await self._state_handler.compute_event_context(
@@ -1263,11 +1266,13 @@ async def _auth_and_persist_event(
                 Possibly incomplete, and possibly including events that are not yet
                 persisted, or authed, or in the right room.
 
-                Only populated where we may not already have persisted these events -
-                for example, when populating outliers.
+                Only populated when populating outliers.
 
             backfilled: True if the event was backfilled.
         """
+        # claimed_auth_event_map should be given iff the event is an outlier
+        assert bool(claimed_auth_event_map) == event.internal_metadata.outlier
+
         context = await self._check_event_auth(
             origin,
             event,
@@ -1306,15 +1311,16 @@ async def _check_event_auth(
                 Possibly incomplete, and possibly including events that are not yet
                 persisted, or authed, or in the right room.
 
-                Only populated where we may not already have persisted these events -
-                for example, when populating outliers, or the state for a backwards
-                extremity.
+                Only populated when populating outliers.
 
             backfilled: True if the event was backfilled.
 
         Returns:
             The updated context object.
         """
+        # claimed_auth_event_map should be given iff the event is an outlier
+        assert bool(claimed_auth_event_map) == event.internal_metadata.outlier
+
         room_version = await self._store.get_room_version_id(event.room_id)
         room_version_obj = KNOWN_ROOM_VERSIONS[room_version]