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

Draft: Add proper state and state_groups to historical events so they return state from /context (avatar/displayname) (MSC2716) #10948

Closed
wants to merge 12 commits into from
Closed

Draft: Add proper state and state_groups to historical events so they return state from /context (avatar/displayname) (MSC2716) #10948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
64448b3
Fix event context for outliers in important MSC2716 spot
MadLittleMods Sep 29, 2021
f3174cd
Add changelog
MadLittleMods Sep 29, 2021
6713a2a
Update changelog.d/10938.bugfix
MadLittleMods Sep 29, 2021
97fa9a2
Update synapse/handlers/message.py
MadLittleMods Sep 29, 2021
fa4f20d
Only generate context when we need to (it's not free to throw away)
MadLittleMods Sep 29, 2021
4fea37e
Add proper state and state_groups so historical events return state f…
MadLittleMods Sep 29, 2021
8fb4d6f
Force /context to return state for the given historical event
MadLittleMods Sep 30, 2021
96d9d11
Set full state for each historical event like others
MadLittleMods Sep 30, 2021
b20fd16
Fix boolean logic for testing whether msc2716 is enabled
MadLittleMods Sep 30, 2021
0362887
Merge branch 'develop' into madlittlemods/msc2716-resolve-state-for-a…
MadLittleMods Oct 1, 2021
cafb1dc
Remove debug logs
MadLittleMods Oct 1, 2021
487754f
Restore back to what was before
MadLittleMods Oct 1, 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
5 changes: 2 additions & 3 deletions synapse/handlers/federation_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,9 +1010,8 @@ async def _handle_marker_event(self, origin: str, marker_event: EventBase) -> No
room_version = await self._store.get_room_version(marker_event.room_id)
create_event = await self._store.get_create_event_for_room(marker_event.room_id)
room_creator = create_event.content.get(EventContentFields.ROOM_CREATOR)
if (
not room_version.msc2716_historical
or not self._config.experimental.msc2716_enabled
if not room_version.msc2716_historical and (
not self._config.experimental.msc2716_enabled
or marker_event.sender != room_creator
):
return
Expand Down
57 changes: 33 additions & 24 deletions synapse/handlers/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,29 +610,6 @@ async def create_event(

builder.internal_metadata.historical = historical

# Strip down the auth_event_ids to only what we need to auth the event.
# For example, we don't need extra m.room.member that don't match event.sender
if auth_event_ids is not None:
# If auth events are provided, prev events must be also.
assert prev_event_ids is not None

temp_event = await builder.build(
prev_event_ids=prev_event_ids,
auth_event_ids=auth_event_ids,
depth=depth,
)
auth_events = await self.store.get_events_as_list(auth_event_ids)
# Create a StateMap[str]
auth_event_state_map = {
(e.type, e.state_key): e.event_id for e in auth_events
}
# Actually strip down and use the necessary auth events
auth_event_ids = self._event_auth_handler.compute_auth_events(
event=temp_event,
current_state_ids=auth_event_state_map,
for_verification=False,
)

event, context = await self.create_new_client_event(
builder=builder,
requester=requester,
Expand Down Expand Up @@ -939,6 +916,32 @@ async def create_new_client_event(
Tuple of created event, context
"""

# Strip down the auth_event_ids to only what we need to auth the event.
# For example, we don't need extra m.room.member that don't match event.sender
if auth_event_ids is not None:
# If auth events are provided, prev events must be also.
assert prev_event_ids is not None

# Copy the full auth state before it stripped down
full_state_ids_at_event = auth_event_ids.copy()

temp_event = await builder.build(
prev_event_ids=prev_event_ids,
auth_event_ids=auth_event_ids,
depth=depth,
)
auth_events = await self.store.get_events_as_list(auth_event_ids)
# Create a StateMap[str]
auth_event_state_map = {
(e.type, e.state_key): e.event_id for e in auth_events
}
# Actually strip down and use the necessary auth events
auth_event_ids = self._event_auth_handler.compute_auth_events(
event=temp_event,
current_state_ids=auth_event_state_map,
for_verification=False,
)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Moved this auth state stripping into create_new_client_event so we can copy full_state_ids_at_event before we strip and use it for the old_state for state_group calculation during compute_event_context


if prev_event_ids is not None:
assert (
len(prev_event_ids) <= 10
Expand Down Expand Up @@ -969,7 +972,13 @@ async def create_new_client_event(
event.internal_metadata.outlier = True
context = EventContext.for_outlier()
else:
context = await self.state.compute_event_context(event)
old_state = None
# Define the state for historical messages while we know to get all of
# state_groups setup properly when we `compute_event_context`.
if builder.internal_metadata.is_historical() and full_state_ids_at_event:
old_state = await self.store.get_events_as_list(full_state_ids_at_event)

context = await self.state.compute_event_context(event, old_state=old_state)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When you pull state from a state_group, it grabs the full resolved state from the all of the state_group deltas.

So we want to define the state for the historical event as all of the state at that point in time(m.room.create, m.room.power_levels, m.room.member, etc), including member events not matching the sender of the event. This way when /messages fetches the state for the first event in the batch, it most likely grabs from some historical event with all of the member events necessary for that batch.

This does have the flaw where if you just insert a single historical event somewhere, it probably won't resolve the state correctly from /messages or /context since it will grab a non historical event above or below with resolved state which never included the historical state back then.

Might be good to look at /mesages and /context to additionally add the state for any historical messages in that batch.

Also pretty inefficient not to share the same state_group for every historical event in the batch 🤔. This will currently create a new state_group for every historical event because compute_event_context creates a new state_group with old_state every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sharing state_groups and resolving state in v2, #10975


if requester:
context.app_service = requester.app_service
Expand Down
10 changes: 4 additions & 6 deletions synapse/storage/databases/main/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1763,9 +1763,8 @@ def _handle_insertion_event(self, txn: LoggingTransaction, event: EventBase):
retcol="creator",
allow_none=True,
)
if (
not room_version.msc2716_historical
or not self.hs.config.experimental.msc2716_enabled
if not room_version.msc2716_historical and (
not self.hs.config.experimental.msc2716_enabled
or event.sender != room_creator
):
return
Expand Down Expand Up @@ -1825,9 +1824,8 @@ def _handle_batch_event(self, txn: LoggingTransaction, event: EventBase):
retcol="creator",
allow_none=True,
)
if (
not room_version.msc2716_historical
or not self.hs.config.experimental.msc2716_enabled
if not room_version.msc2716_historical and (
not self.hs.config.experimental.msc2716_enabled
or event.sender != room_creator
):
return
Expand Down