-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC4218: Improving performance of profile changes #4218
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
# MSC4218: Improving performance of profile changes | ||
|
||
Many users in the Matrix ecosystem have seen how slow it is to change either their display name or their avatar. It is slow because it causes O(n) updates, one for each room the user is joined to. This MSC details a mechanism to improve performance by not sending O(n) updates. It does this by introducing a new concept called "Synthetic Events". This new concept can also be conveniently applied to [communicate state resolution deltas accurately to clients](https://github.com/matrix-org/matrix-spec/issues/1209). Also conveniently, decoupling profile changes from the membership of the user decreases the length of the auth chain for that user, which decreases the amount of data that is required to be fetched in order to join a room, thus improving performance on room joins as well. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please wrap lines to ~100 characters |
||
|
||
## Proposal | ||
|
||
This MSC borrows and expands upon the ideas in MSC3883, specifically: | ||
|
||
> Displayname/Avatar updates should be EDUs that trigger a /profile query. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts on what this would imply for turning off federation profile lookups? https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html?highlight=profile#allow_profile_lookup_over_federation MSC4170 (merged) says that profile info is required for shared rooms and public rooms, which seems to be enough to cover the usecases of these changes, but i'm not sure if it's been implemented that way yet in server implementations What should happen if the server isn't allowed to look at the profile of a user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is only for the CS API. SS profile look-ups can be turned off entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that doesn't make sense really though. If the CSAPI demands to get profile info for users who have shared rooms, then the SS API must also do that as that is the only way you can get that information for users on other servers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree but I think the difficulty is that with the current SS endpoint for profile look-up, the receiving server cannot verify that condition. It only sees the requesting server but not the requesting user. For reference, here's the spec PR for MSC4170 in case we want to refine (or roll back) the wording before Matrix v1.13. |
||
|
||
For unfederated rooms, no EDUs need to be sent. So how do users see profile changes in rooms? The server creates a "synthetic event" which is effectively a fake member event based on the real member event, decorated with the latest profile data at the time the EDU is received / the profile was updated. As an example, if a user has no profile then sets one, the new profile fields will appear in the synthetic event e.g `content: { membership: join }` becomes `content: { membership, join, displayname: Alice }`. The synthesised event MUST have a unique event ID as clients often deduplicate data based on the event ID. This proposal proposes a format of `{original_member_event_id}_{iteration}` e.g `$A5z9GLL8ABCYg4SKujf_ehtW52yGoGhZuQGc0nspEeU_2`. | ||
|
||
As a result, clients will see no differences except: | ||
- profile change `m.room.member` events are synthetic so they don't exist in the room DAG, so the event ID isn't real. This can break clients which ask for events in federation format as this event won't have `prev_events`, etc. The response to `GET /state/m.room.member/@user:id` would also return the _synthetic event content_, not the federation form (as would `/members`), on the basis that we care about profiles more than accurate DAG reproducibility. This could be configured in another MSC. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would synthetic events be in the timeline section of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They would function the same as normal m.room.member profile changes, so they would be in the timeline. You make an excellent point regarding receipts which would be inconsistent over federation (as the marker would be for a synthetic event which doesn't exist on other servers). I'd suggest the pragmatic approach of just not sending receipts for synthetic events, as by their definition of a state synchronisation primitive they aren't intentional messages sent which require intentional acknowledgement via receipts. For SSS I would suggest putting synthetic events in the state section only, as that matches their intended purpose better. It does however mean you'd lose profile changes in the timeline, so that might be a controversial move. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would that mean an old client can't mark a room as read if the last timeline event is a synthetic one? Or would the receipt be saved locally, just not sent over federation? |
||
- `/myroomnick` (per-room nicks) isn't possible anymore without modifications. | ||
|
||
### Per-room nicks | ||
|
||
To support per-room nicks, there exists a new endpoint: `POST /rooms/{roomID}/user_profile` which sends a new state event type `m.room.user_profile` into the room, with the `state_key` matching the logged in user's ID. This new event has the relevant `displayname` and `avatar_url` fields. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can i send this as a room admin or similar? Usually spammers use abusive nicknames / profile pics, but sometimes the username is abusive too, to combat this what i do currently is
This usually results in clients primarily displaying the fake display name i set, which is nice since mxid's are currently picked by the user. Do you think it makes sense to let room mods send this event with the state key of a banned user? I believe this could be accomplished via MSC3757 and owned state events MSC3779. If it was based on those proposals it would also be possible to use a state key like |
||
|
||
We don't want to send `m.room.member` events because then they would form part of the auth chain. We don't want to put per-room nicks in global profile data (in a room ID to profile info map) because it would leak the rooms the user is joined to. We want a new endpoint and not `PUT /state/m.room.user_profile/{userID}` because that event is never sent to the client, so the lack of symmetry would be confusing. See below. | ||
|
||
On receipt of `m.room.user_profile`, the contents of this event _overrides_ the global profile info for that user in that room only, _even if the global profile is updated after this point_. Servers should treat this event _as if_ it were the global profile response for this room, meaning the way the data is delivered to clients should remain the same. This implies the new event type is NOT sent to clients, which is intentional as we do not wish to introduce needless breaking API changes to all clients, AND we want to preserve the existing property of seeing a new `m.room.member` join the room and immediately knowing that member's name/avatar: this atomicity requirement was why they were coupled in the first place. Because of these reasons, we do not go "all in" on the new event type and synthesise fake versions of this event to communicate profile updates, even though this may initially seem like a more consistent alternative. | ||
|
||
### Redactions | ||
|
||
`m.room.member` events can be redacted to remove abusive display names and avatars. This proposal changes how profile information is stored so it has implications on the redaction logic. It is more complex now because there is an impedance mismatch between clients and federation: clients will redact _synthetic events_ which have no representation over federation, so how is this redaction communicated to other servers? | ||
- As today, moderators will redact the `m.room.member` event, however the semantics server-side changes. | ||
- Redacting a member event automatically redacts any corresponding `m.room.user_profile` event if they exist **now OR in the future**. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may have redacted some member events a few times, either because they thought it was funny to include the bee movie script in their display name, or just because it filled too much of my screen, or because i simply thought it was funny Maybe it woud be nice to have a way to lift this automatic redaction manually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Automatically kick and reinvite? It's going to be janky no matter what as we aren't optimising for this behaviour, so rather than have a new API we can get by with the existing APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it won’t be applied to old rooms I’m fine with this, will just have to find a new practical joke to use in the future :p |
||
- Redacted member events are communicated to other servers as they are today. | ||
- Receiving a redacted member event will cause servers to create a new synthetic event with just `content: {membership: join}`, with no profile fields. This synthetic event will then be sent to clients. | ||
- After this point, global profile updates have no effect for this user in this room: the state is terminal. This means that if your member event is redacted and then you send an `m.room.user_profile` event, it is ignored and immediately redacted by the receiving server. This immediate redaction is required in order to prevent servers from storing abusive profile fields, even if they aren't communicated to the clients. | ||
- Due to synthetic events, redacting a member event will cause both the redaction event _and the new synthetic event_ to come down /sync. The synthetic event should be decorated to indicate they the fields were removed via redaction e.g via `unsigned.redacted_because`. This allows clients to differentiate between redactions and genuinely unsetting the display name / avatar. | ||
|
||
Effectively, this mechanism means we are using the redaction of the `m.room.member `event as a signal to redact all profile info for that user in that room. This has tradeoffs, notably that if a profile is redacted, it cannot be set again unless that user changes their membership e.g by leaving and rejoining the room. Typically, spammers will have their display names redacted then they will be kicked from the room, so this wouldn't change the behaviour for this use case at all. | ||
|
||
The entire flow chart looks like: | ||
|
||
<img width="994" alt="Screenshot 2024-10-20 at 15 17 46" src="https://github.com/user-attachments/assets/ac227603-b635-4ecc-bd60-d3446074facf"> | ||
|
||
|
||
### Synthetic Events | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. synth events also make an appearance in msc #2228 (thankfully looking the same) as a way of letting servers tell clients if an event has destructed. |
||
|
||
These are events created by a server which aren't sent to any other server, but _are sent to clients_. Clients treat them as real events but they aren't real over federation and do not form part of the room DAG. The purpose of this is to help state synchronisation between clients and server. They can be detected because all synthetic events MUST have `synthetic: true` at the top-level of the event JSON. Other uses for synthetic events include communicating state deltas to clients when state resolution needs to back out some state it had previously told clients, see https://github.com/matrix-org/matrix-spec/issues/1209 | ||
|
||
## Potential issues | ||
|
||
If the receiving server cannot get the profile info for a user due to network issues, it must retry periodically. This can introduce a delay between when the user actually changed their name and it taking effect. | ||
|
||
## Alternatives | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative missing here is creating a new PDU specifically for user profiles, separate from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I literally cover this in the MSC :D
A new event breaks atomicity. |
||
|
||
- Do nothing, and deal with slow performance and longer auth chains. | ||
- Don't send synthetic events on profile updates. We could pass the EDU directly to clients and expect them to re-query, but this would then break the ability to have historical display names, and it would cause much more traffic to get the profile as every client would ask for the profile rather than every server. In addition, client uptime isn't as good as servers, and the lag time between the EDU being sent and the profile being retrieved would be much higher than this proposal. | ||
- Remove support for per-room nicks. This would remove the need for `m.room.user_profile` events and arguably simplify this MSC. | ||
|
||
|
||
## Security considerations | ||
|
||
Profile changes are no longer part of the room DAG. This means malicious servers can split-brain the room for a user's profile: users on different servers may see different profile info for the same user. This can be done by accident due to network partitions or maliciously, by only sending innocuous display names to servers with admins but abusive names to servers without admins. Users in a room typically ping admins to remove bad users, so this could cause confusion if the admin doesn't see the abusive name/avatar. Given this relies on global profile data which is public, we could do transparency logs to ensure a server responds with the same data to all servers, but this is likely overkill so it doesn't form part of this MSC. It is worth noting per-room nicks ARE part of the room DAG and as such are not vulnerable to this. | ||
|
||
## Unstable prefix | ||
|
||
- Room version is `org.matrix.msc4218` based off room version 11. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to deploy this to existing room versions? I guess main problem would be that people wouldn't be allowed to set per-room profiles without admin intervention, and some out of date servers wouldn't know about the new profile mechanism, but this might be worth the tradeoff? Personally I would prefer a drastically shorter auth chain than not being totally sure if someone is out of office for the weekend :p There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It technically could be done on existing room versions, but there is a change in semantics which you have already pointed out so it's polite to actually bump the version. If we wanted to enforce that you can't do member event profile change events then this would need auth rules changing, which would absolutely need a new version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good point, enforcing it in the auth rules seems like a great idea, and yeah i agree that it’s polite to bump here anyway Would that then be done via ensuring that the m.room.member content doesn’t contain anything but the membership info? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
- `m.room.user_profile` events are `org.matrix.msc4218.room.user_profile`. | ||
- `synthetic` flag is `org.matrix.msc4218.synthetic`. | ||
- `POST /rooms/{roomID}/user_profile` is `POST /rooms/{roomID}/org.matrix.msc4218.user_profile` | ||
|
||
## Dependencies | ||
|
||
This MSC has no dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements: