Resolve and share state_groups for all historical events in batch (MSC2716)

[MadLittleMods]

Resolve and share state_groups for all historical events in batch. This also helps for showing the appropriate avatar/displayname in Element and will work whenever /messages has one of the historical messages as the first message in the 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. For the same reasions, this also does not work in Element between the transition from actual messages to historical messages. In the Gitter case, this isn't really a problem since all of the historical messages are in one big lump at the beginning of the room.

For a future iteration, might be good to look at /messages and /context to additionally add the state for any historical messages in that batch.

DAG breakdown

In the before cases, the m.room.member events floated off on their own and were only used as auth_event_ids (not prev_event_ids)

Before (expected)	Before (actual, because of bug)	After
Mermaid live editor	Mermaid live editor	Mermaid live editor

How are the state_groups shared?

To illustrate the state_group sharing, see this example:

Before (new state_group for every event 😬, very inefficient):

# Tests from https://github.com/matrix-org/complement/pull/206
$ COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestBackfillingHistory/parallel/should_resolve_member_state_events_for_historical_events
create_new_client_event m.room.member event=$_JXfwUDIWS6xKGG4SmZXjSFrizhARM7QblhATVWWUcA state_group=None
create_new_client_event org.matrix.msc2716.insertion event=$1ZBfmBKEjg94d-vGYymKrVYeghwBOuGJ3wubU1-I9y0 state_group=9
create_new_client_event org.matrix.msc2716.insertion event=$Mq2JvRetTyclPuozRI682SAjYp3GqRuPc8_cH5-ezPY state_group=10
create_new_client_event m.room.message event=$MfmY4rBQkxrIp8jVwVMTJ4PKnxSigpG9E2cn7S0AtTo state_group=11
create_new_client_event m.room.message event=$uYOv6V8wiF7xHwOMt-60d1AoOIbqLgrDLz6ZIQDdWUI state_group=12
create_new_client_event m.room.message event=$PAbkJRMxb0bX4A6av463faiAhxkE3FEObM1xB4D0UG4 state_group=13
create_new_client_event org.matrix.msc2716.batch event=$Oy_S7AWN7rJQe_MYwGPEy6RtbYklrI-tAhmfiLrCaKI state_group=14

After (all events in batch sharing state_group=10) (the base insertion event has state_group=8 which matches the prev_event we're insertion next to):

# Tests from https://github.com/matrix-org/complement/pull/206
$ COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestBackfillingHistory/parallel/should_resolve_member_state_events_for_historical_events
create_new_client_event m.room.member event=$PWomJ8PwENYEYuVNoG30gqtybuQQSZ55eldBUSs0i0U state_group=None
create_new_client_event org.matrix.msc2716.insertion event=$e_mCU7Eah9ABF6nQU7lu4E1RxIWccNF05AKaTT5m3lw state_group=9
create_new_client_event org.matrix.msc2716.insertion event=$ui7A3_GdXIcJq0C8GpyrF8X7B3DTjMd_WGCjogax7xU state_group=10
create_new_client_event m.room.message event=$EnTIM5rEGVezQJiYl62uFBl6kJ7B-sMxWqe2D_4FX1I state_group=10
create_new_client_event m.room.message event=$LGx5jGONnBPuNhAuZqHeEoXChd9ryVkuTZatGisOPjk state_group=10
create_new_client_event m.room.message event=$wW0zwoN50lbLu1KoKbybVMxLbKUj7GV_olozIc5i3M0 state_group=10
create_new_client_event org.matrix.msc2716.batch event=$5ZB6dtzqFBCEuMRgpkU201Qhx3WtXZGTz_YgldL6JrQ state_group=10

Dev notes

v1: #10948

---

update_membership
update_membership_locked
_local_membership_update
create_event
create_new_client_event

handle_new_client_event
_persist_event
persist_and_notify_client_event
self.storage.persistence.persist_event -> _event_persist_queue.add_to_queue(...)
_persist_event_batch
_persist_events_and_state_updates
_persist_events_txn
_store_event_state_mappings_txn (inserts into the `event_to_state_groups` table)

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[MadLittleMods]

I think this was actually a bug. We only want the base insertion event to be tied to prev_event_ids_from_query.

Whereas previously, this attached the base and normal insertion event for the first batch (when no ?batch_id was specified)

[MadLittleMods]

(notice the extra prev_event for the insertion event in chunk0)

Before (expected)	Before (actual, because of this bug)
Mermaid live editor	Mermaid live editor

[MadLittleMods]

Changed this to an upsert to avoid colliding with a previous entry we put in via store_state_group_id_for_event_id earlier before we persisted the events.

[MadLittleMods]

Add explicit state to the insertion event so the rest of the batch can inherit the same state and state_group

[MadLittleMods]

We still have the state events float off on their own, but now they're in a floating chain instead of individual. This way we can connect the historical events to the state chain via one prev_event.

[MadLittleMods]

This is moved down here so we can get a copy of the full_state_ids_at_event to put in the insertion events below.

[MadLittleMods]

Thanks for the review @erikjohnston 🐋

I'm happy to see the more sane state with this 🙌

[DMRobertson]

Not sure what's going on here. The braces to enclose the await is legit syntax (it'll build a set, like {"hello"}). As written the function is going to return None.

[DMRobertson]

See #11310

[richvdh]

@MadLittleMods, @erikjohnston: I'm trying to figure out how this works, so that I can fit it in with my work on faster joins and partial room state.

so... how does this work? full_state_ids_at_event is set to auth_event_ids, which is absolutely not the full state at the event. Rather, as the docstring says, it is "The event ids to use as the auth_events for the new event." - ie, the subset of the state that is needed to auth the event.

[MadLittleMods]

The relevant call stacks for this are:

# For events
RoomBatchSendEventRestServlet.on_POST
handle_batch_of_events
persist_historical_events
create_event
create_new_client_event

# For `m.room.member` state events
RoomBatchSendEventRestServlet.on_POST
persist_state_events_at_start
update_membership
update_membership_locked
_local_membership_update
create_event
create_new_client_event

# For non `m.room.member` state events
RoomBatchSendEventRestServlet.on_POST
persist_state_events_at_start
create_and_send_nonmember_event
create_event
create_new_client_event

In RoomBatchSendEventRestServlet.on_POST we fetch the full state for the event we're inserting next to (specified by /batch_send?prev_event_id=xxx) as a base to start from. Then as we persist the state_events_at_start, we also add that to the auth_event_ids list.

That full auth_event_ids list is then passed down that call stack and gets stripped down to only include the necessary bits in the code here. And we use full_state_ids_at_event to "Add explicit state to the insertion event so the rest of the batch can inherit the same state and state_group"

---

Here is the context where we originally started stripping down the state: #9247 (comment)

[richvdh]

right, thanks for the explanation, but this seems extremely confusing. At the very least, if create_new_client_event and co are expecting auth_event_ids to be the full state rather than just the auth events, then the docstrings should be updated to say so - right now they are flat-out wrong.

But generally, this feels like an abuse of auth_event_ids - we shouldn't be using it to mean "normally the auth events, except under special circumstances, when it is the full state". I'm not sure I have any immediate suggestions, but adding a new parameter would be better than the current situation.

@erikjohnston I think you've been following the MSC2716 work a bit more than me - any thoughts here?

[erikjohnston]

Ah, I think I got confused when reading this as at some point the state at the insertion event was limited to the used auth events, which is no longer the case.

I agree that we should therefore be passing the full state through as a separate parameter. Though at some point I'm wondering if this code path needs refactoring to make it easier to use in the /send_batch case, as its starting to get a bit unwieldy with all the different cases.

[MadLittleMods]

@richvdh Do you want me to tackle this new full state parameter? Don't want to collide and cause conflicts with your existing refactor.

[richvdh]

You're unlikely to conflict with me here, so please do go ahead!

[MadLittleMods]

Updated via #12083