Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Clean up _update_auth_events_and_context_for_auth #11122

Merged
merged 9 commits into from
Oct 20, 2021
Merged

Clean up _update_auth_events_and_context_for_auth #11122

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4a8b5cc
remove redundant code in _update_auth_events_and_context_for_auth
richvdh Oct 19, 2021
53db7c2
_update_auth_events_and_context_for_auth: delay copy of auth_events
richvdh Oct 4, 2021
535aec7
`_update_auth_events_and_context_for_auth`: remove redundant `origin`…
richvdh Oct 4, 2021
5e971ee
replace room_id check with an assertion
richvdh Oct 19, 2021
d1389f0
`_update_auth_events_and_context_for_auth`: bail out earlier
richvdh Oct 19, 2021
6d17246
`_update_auth_events_and_context_for_auth`: just return auth map
richvdh Oct 4, 2021
f5674f7
changelog
richvdh Oct 19, 2021
bb07db5
drop redundant type hint
richvdh Oct 20, 2021
8b3dfd9
lint
richvdh Oct 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 10 additions & 76 deletions synapse/handlers/federation_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,83 +1592,15 @@ async def _update_auth_events_and_context_for_auth(
"""
assert not event.internal_metadata.outlier

# take a copy of calculated_auth_event_map before we modify it.
auth_events: MutableStateMap[EventBase] = dict(calculated_auth_event_map)

# check for events which are in the event's claimed auth_events, but not
# in our calculated event map.
event_auth_events = set(event.auth_event_ids())

# missing_auth is the set of the event's auth_events which we don't yet have
# in auth_events.
missing_auth = event_auth_events.difference(
e.event_id for e in auth_events.values()
)

# if we have missing events, we need to fetch those events from somewhere.
#
# we start by checking if they are in the store, and then try calling /event_auth/.
#
# TODO: this code is now redundant, since it should be impossible for us to
# get here without already having the auth events.
if missing_auth:
have_events = await self._store.have_seen_events(
event.room_id, missing_auth
)
logger.debug("Events %s are in the store", have_events)
missing_auth.difference_update(have_events)
Copy link
Member Author

@richvdh richvdh Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point, missing_auth must be an empty set, because we all of the events in event_auth_events must be in our database (otherwise we would have rejected the event sooner, thanks to the changes in #11001).

(I toyed with the idea of changing this to an assertion, but the have_seen_events lookup isn't entirely negligible and I figured it was better just to clear it out.)


# missing_auth is now the set of event_ids which:
# a. are listed in event.auth_events, *and*
# b. are *not* part of our calculated auth events based on room state, *and*
# c. are *not* yet in our database.

if missing_auth:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... which means that this is never true.

# If we don't have all the auth events, we need to get them.
logger.info("auth_events contains unknown events: %s", missing_auth)
try:
await self._get_remote_auth_chain_for_event(
origin, event.room_id, event.event_id
)
except Exception:
logger.exception("Failed to get auth chain")
else:
# load any auth events we might have persisted from the database. This
# has the side-effect of correctly setting the rejected_reason on them.
auth_events.update(
{
(ae.type, ae.state_key): ae
for ae in await self._store.get_events_as_list(
missing_auth, allow_rejected=True
)
}
)

# auth_events now contains
# 1. our *calculated* auth events based on the room state, plus:
# 2. any events which:
# a. are listed in `event.auth_events`, *and*
# b. are not part of our calculated auth events, *and*
# c. were not in our database before the call to /event_auth
# d. have since been added to our database (most likely by /event_auth).

different_auth = event_auth_events.difference(
e.event_id for e in auth_events.values()
e.event_id for e in calculated_auth_event_map.values()
)

# different_auth is the set of events which *are* in `event.auth_events`, but
# which are *not* in `auth_events`. Comparing with (2.) above, this means
# exclusively the set of `event.auth_events` which we already had in our
# database before any call to /event_auth.
#
# I'm reasonably sure that the fact that events returned by /event_auth are
# blindly added to auth_events (and hence excluded from different_auth) is a bug
# - though it's a very long-standing one (see
# https://github.com/matrix-org/synapse/commit/78015948a7febb18e000651f72f8f58830a55b93#diff-0bc92da3d703202f5b9be2d3f845e375f5b1a6bc6ba61705a8af9be1121f5e42R786
# from Jan 2015 which seems to add it, though it actually just moves it from
# elsewhere (before that, it gets lost in a mess of huge "various bug fixes"
# PRs).

if not different_auth:
return context, auth_events
return context, calculated_auth_event_map

logger.info(
"auth_events refers to events which are not in our calculated auth "
Expand All @@ -1694,13 +1626,13 @@ async def _update_auth_events_and_context_for_auth(
# XXX: should we reject the event in this case? It feels like we should,
# but then shouldn't we also do so if we've failed to fetch any of the
# auth events?
return context, auth_events
return context, calculated_auth_event_map

# now we state-resolve between our own idea of the auth events, and the remote's
# idea of them.

local_state = auth_events.values()
remote_auth_events = dict(auth_events)
local_state = calculated_auth_event_map.values()
remote_auth_events = dict(calculated_auth_event_map)
remote_auth_events.update({(d.type, d.state_key): d for d in different_events})
remote_state = remote_auth_events.values()

Expand All @@ -1714,10 +1646,12 @@ async def _update_auth_events_and_context_for_auth(
{
d
for d in new_state.values()
if auth_events.get((d.type, d.state_key)) != d
if calculated_auth_event_map.get((d.type, d.state_key)) != d
},
)

# take a copy of calculated_auth_event_map before we modify it.
auth_events: MutableStateMap[EventBase] = dict(calculated_auth_event_map)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that this needs a type hint and remote_auth_events doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. removed.

auth_events.update(new_state)

context = await self._update_context_for_auth_events(
Expand Down