MSC 1501: Room version upgrades #1502

richvdh · 2018-08-10T15:52:59Z

Proposal for #1501

Rendered: https://github.com/matrix-org/matrix-doc/blob/main/proposals/1501-room-version-upgrades.md

(For reference: this doc superceded https://docs.google.com/document/d/1UdXG0grQsS4vhn28yquj79wZqBcFrTWvf3PoFXaKBaY)

turt2live · 2018-08-10T16:56:15Z

Alongside muting the room (demote and set default level for events to subzero), I wonder if it's a good idea to have capable servers reject events for the old room. This would give the server the ability to essentially stop bothering to try and spend resources on the room given it has been effectively frozen.

Joining users to the room is a hard problem to solve, and is yet another case of element-hq/element-web#2925. Assuming aliases are also brought over to the new room, the servers could use those to get in. However as mentioned in the proposal the case of private chats does lead to the high probability of rooms becoming unroutable. Although using the sender of the tombstone event would work shortly after the upgrade, there's no guarantee that the server is still participating in the room by the time someone tries to join the new room. This could be solved by mandating that the server stay in the room somehow, potentially scope creeping to #499.

KitsuneRal · 2018-08-12T11:08:53Z

Does "Replicates PL/privacy/topic/etc settings to the new room." means replication of respective state events to the new room or just the server inheriting the state of the new room from the old room? If it's the former (as I expect) then it's probably better to mention "events" instead of "settings".
Should we really mandate clients to represent the timeline of the two incarnations entirely separately instead of seaming the two timelines together (still with proper markup at the seams)? Isn't it a client implementation detail? You might have problems with matrix-react-sdk but I can imagine how to do it relatively easily in libQMatrixClient (though some invariants in the library API will be shaken, yes). So maybe just not spec the client UX part?
It would be great to see a bit more explanation on why auto-joining and merging don't work.

richvdh · 2018-08-13T12:02:20Z

Could I suggest that people comment on a relevant bit of the PR, where possible, so that we can maintain some semblance of threading?

richvdh · 2018-08-13T12:07:38Z

Alongside muting the room (demote and set default level for events to subzero), I wonder if it's a good idea to have capable servers reject events for the old room.

yes, this sounds sensible. We can always give that a go once the other bits land.

Although using the sender of the tombstone event would work shortly after the upgrade, there's no guarantee that the server is still participating in the room by the time someone tries to join the new room. This could be solved by mandating that the server stay in the room somehow, potentially scope creeping to #499.

Yeah; this is a more general problem. Let's try to avoid scope-creeping it for now and consider the more general problem separately.

richvdh · 2018-08-13T12:40:53Z

Does "Replicates PL/privacy/topic/etc settings to the new room." means replication of respective state events to the new room or just the server inheriting the state of the new room from the old room? If it's the former (as I expect) then it's probably better to mention "events" instead of "settings".

Indeed. Done.

Should we really mandate clients to represent the timeline of the two incarnations entirely separately instead of seaming the two timelines together (still with proper markup at the seams)? Isn't it a client implementation detail?

Yes, this is fair: the doc probably talks too strongly about things which are really client-side choices, and which shouldn't be part of the final C-S spec. Really we were just trying to make sure there was at least one plausible implementation on the client side.

I think this is basically https://github.com/matrix-org/matrix-doc/blob/rav/room_upgrades/proposals/1501-room-version-upgrades.md#have-clients-merge-old-and-new-rooms - which, as per that doc, we dismissed for Riot. There'd be nothing stopping different clients making different choices.

It would be great to see a bit more explanation on why auto-joining and merging don't work.

Auto-joining: in short we dismissed this because of the high chance of remote joins failing, and it being hard to handle. This is alluded to in the spec: do you think it needs clarifying?

Merging: assuming you mean trying to merge the rooms on the server side, https://github.com/matrix-org/matrix-doc/blob/256ad01fd231945be01e1d2e18fb597e2780c595/proposals/1501-room-version-upgrades.md#have-servers-merge-old-and-new-rooms-together talks about this. Can you say what further explanation is needed?

turt2live · 2018-08-13T14:28:12Z

Although I don't want to scope creep, I think it's important that people can get into the new room post-upgrade. Whether that be autojoining (accepting remote failure as non-fatal) or figuring out the routing problem in matrix.

dbkr · 2018-08-14T15:34:06Z

proposals/1501-room-version-upgrades.md

+
+   * Also the ability for an op to see what versions the servers in the current
+     room supports (nb via a cap API) and so how many users will get locked out
+     [XXX: really? this sounds like a pita]


Aside from knowing what versions other server in the room support, how does a client know what the latest version its own server supports / when to suggest to an admin that a room ought to be upgraded?

when discussing this with @lampholder when working through the client UX, our suggestion was that a server admin could inject room account data into the accounts of the room ops to nag them to upgrade the room to a newer version, with an optional severity and reason.

having discussed this with @lampholder more this week, we agreed to put a pin in this topic for now. Certainly we may want to consider how we will advise room ops to do their upgrades, but I think that it's not something we should delay this proposal for.

dbkr · 2018-08-17T13:16:29Z

proposals/1501-room-version-upgrades.md

+hits a new C-S api:
+
+```
+POST /_matrix/client/r0/room/{roomId}/upgrade


KitsuneRal · 2018-08-21T02:02:36Z

Auto-joining: in short we dismissed this because of the high chance of remote joins failing, and it being hard to handle. This is alluded to in the spec: do you think it needs clarifying?

I perceived "It's worth noting" as some kind of "by the way" consideration rather than a blocking argument - especially with the workaround presented in the end. The current text leaves me under impression that it wouldn't be so bad to combine the primary proposal with auto-joining (that will work on the best-effort basis but the obvious fallback will be the user clicking that link to the new room displayed for the tombstone event and trying to join the room manually). For end users the difference between the primary case and the fall-back would be being routed to the new room at once vs. a click on the link to upgrade the room.

Merging: assuming you mean trying to merge the rooms on the server side, https://github.com/matrix-org/matrix-doc/blob/256ad01fd231945be01e1d2e18fb597e2780c595/proposals/1501-room-version-upgrades.md#have-servers-merge-old-and-new-rooms-together talks about this. Can you say what further explanation is needed?

The current text is not quite clear on what becomes the showstopper; now that I think more about it. The sheer amount of duplication in the sync as the number of upgrades increases (basically, it's another batch duplicating some other for each upgrade) is the primary problem, right? Maybe that previous sentence could be added to the dismissed proposal, for clarity.

dbkr · 2018-08-22T16:35:09Z

proposals/1501-room-version-upgrades.md

+
+```json
+{
+    "replacement_room": "!QtykxKocfsgujksjgd:matrix.org"


Shouldn't this be 'successor' to match m.room.create's 'predecessor'?

possibly. It's worth noting that predecessor is a dict (with room_id and event_id) whereas this is just a room_id, so the similarity is only partial.

I'm not particularly fussed either way. WDYT?

@dbkr: I'm taking the lack of response to mean that you don't care that much either, and have implemented this as replacement_room.

I did indeed

KitsuneRal · 2018-08-25T09:57:14Z

One more comment: do we want to require new_versions to be no-less-than the current room version or is it unnecessary? I can readily imagine a case for re-incarnating room with the same version as before (it effectively already occurred in Matrix) but I'm not sure how much we want downgrades to happen.

KitsuneRal · 2018-08-25T09:58:51Z

Also, a nitpick: in one place the endpoint is defined as /upgrade but then it is referred to as /upgrade_room later on.

richvdh · 2018-08-28T10:36:09Z

do we want to require new_versions to be no-less-than the current room version or is it unnecessary?

We're hoping to use this for recreating existing rooms.

Since versions numbers are strings, it would be hard to enforce anyway.

richvdh · 2018-08-28T10:36:25Z

Also, a nitpick: in one place the endpoint is defined as /upgrade but then it is referred to as /upgrade_room later on.

Fixed, thanks

richvdh · 2018-08-28T10:53:41Z

@KitsuneRal : please can you make any future comments via comments on the diff rather than on the whole issue? I'm finding the lack of threading here makes it impossible to keep track of what remains to be addressed.

richvdh · 2018-08-28T11:33:35Z

proposals/1501-room-version-upgrades.md

+auto-join Bob to the new room, and updates any aliases it may have to point to
+the new room.
+
+It's worth noting that the join may not be successful: for example, because


@KitsuneRal wrote:

I perceived "It's worth noting" as some kind of "by the way" consideration rather than a blocking argument - especially with the workaround presented in the end. The current text leaves me under impression that it wouldn't be so bad to combine the primary proposal with auto-joining (that will work on the best-effort basis but the obvious fallback will be the user clicking that link to the new room displayed for the tombstone event and trying to join the room manually). For end users the difference between the primary case and the fall-back would be being routed to the new room at once vs. a click on the link to upgrade the room.

I am not in favour of things that work 80% of the time and need a completely different solution for the remaining 20%: they tend to confuse users and lead to them thinking things are buggy. (Of course, joins are buggy, but I'm reluctant to build more functionality on top of them instead of taking the time to fix that.)

I'm still in favour of leaving auto-joins out for now. Obviously, this behaviour of "try an auto-join, and otherwise fall back to having the user rejoin" could be implemented later.

richvdh · 2018-08-28T11:38:14Z

proposals/1501-room-version-upgrades.md

+
+#### Have servers merge old and new rooms together
+
+Instead of expecting clients to interpret tombstone events, servers could merge


@KitsuneRal wrote:

The current text is not quite clear on what becomes the showstopper; now that I think more about it. The sheer amount of duplication in the sync as the number of upgrades increases (basically, it's another batch duplicating some other for each upgrade) is the primary problem, right? Maybe that previous sentence could be added to the dismissed proposal, for clarity.

The showstopper is as per https://github.com/matrix-org/matrix-doc/pull/1502/files#diff-ce167b1ac0f01d8c81d7d9718f396207R218: the server ends up having to do the mapping forever for all rooms and for all users, which is an overhead we don't really want to support.

KitsuneRal · 2018-08-29T09:27:12Z

please can you make any future comments via comments on the diff rather than on the whole issue? I'm finding the lack of threading here makes it impossible to keep track of what remains to be addressed.

No problem; I wasn't sure what was better after all the strong words about GitHub's inline comments.

anoadragon453 · 2018-10-17T11:05:59Z

@mujx Will do, sorry!

proposals/1501-room-version-upgrades.md

turt2live · 2018-10-17T15:49:14Z

proposals/1501-room-version-upgrades.md

+       "state_key": "",
+       "room_id": "!cURbaf:matrix.org",
+       "content": {
+           "body": "This room has been replaced",


Is this always expected to be this string? can clients influence the description here?

for now, it's up to the server.

uhoreg

lgtm, with a couple of minor questions

uhoreg · 2018-10-17T16:02:30Z

proposals/1501-room-version-upgrades.md

+upgrade (ie, security problems due to state resets and the like) do not apply
+as strongly to invite-only rooms, and we have descoped them for now.
+
+In future, we will most likely deal with them as follows:


Could we somehow deal with it by sending a join event to the new room, and stick a reference to the user's membership event in the old room an the auth_events?

maybe, but I'm wary of tying too much auth in the new room to auth in the old (broken, possibly completely insecure) room.

uhoreg · 2018-10-17T16:07:41Z

proposals/1501-room-version-upgrades.md

+ * Displays, at the very bottom of the timeline of the old room: "This room
+   has been upgraded. Click here to follow the conversation to the new room".
+   The link is simply a permalink to the new room. When Bob opens it, he will
+   get joined to the new room.


Is it worth saying that if the new room is also tombstoned, then the client should (may?) automatically keep following the chain until it reaches a room that isn't dead?

yes, I guess that is something worth spelling out.

richvdh · 2018-10-17T19:15:54Z

Following up on #1502 (comment):

Alongside muting the room (demote and set default level for events to subzero), I wonder if it's a good idea to have capable servers reject events for the old room.

yes, this sounds sensible. We can always give that a go once the other bits land.

On reflection, I'm still a bit hesitant to recommend this:

we may still want admins to be able to insert power_levels events, in order to re-silence the room in the event of a state reset. And if we're allowing admins to do that sort of thing without letting people send messages, then it amounts to the same thing as we can do anyway with the power_levels.
I think there are still valid usecases for joining a tombstoned room - in particular, being able to see the history of a (non-peekable) room. So we may want to allow join/leave events anyway.

There are probably valid answers to both of those points, but I'm keen that we don't derail this MSC with something that we can equally well consider down the line, with better experience of how things work in practice. I just wanted to jot down a couple of thoughts here for future reference, but let's park this for now.

- the body of the tombstone is defined by the server. - the client can follow tombstones until it finds a live room

ara4n · 2018-10-17T20:09:43Z

proposals/1501-room-version-upgrades.md

+     we are not already a member, and then switches to a view on the new room.
+
+     The client should supply the name of the server which sent the tombstone
+     event as the `server_name` for the `/join` request.


what is the rationale here?

Added a note in 21a4594

ara4n · 2018-10-17T20:12:53Z

proposals/1501-room-version-upgrades.md

+
+   * I think they'll just show you two separate rooms (both with the same
+     name), and you won't be able to talk in one of them. It's not great but it
+     will probably do.


can we mitigate this by recommending (not mandating) that an m.room.message as well as an m.room.tombstone is sent into the old room (at least until we have fallback events for tombstones) so dumb clients can tell their users what's going on via a plain old IM?

I guess so. Are you expecting the message to be sent by the client or the server?

ara4n · 2018-10-17T20:21:36Z

this lgtm; might be nice to address the two minor q's i had.

mscbot · 2018-10-17T20:23:27Z

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @richvdh, I wasn't able to add the final-comment-period label, please do so.

mscbot · 2018-10-23T13:57:10Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

psst @richvdh, I wasn't able to add the finished-final-comment-period label, please do so.

turt2live · 2019-01-09T22:29:44Z

proposals/1501-room-version-upgrades.md

+       "state_key": "",
+       "room_id": "!QtykxKocfsgujksjgd:matrix.org",
+       "content": {
+           "version": "2",


Post-MSC note: This is actually supposed to be room_version as per MSC1425. Implementation of upgrades appears to use room_version as well.

proposal for room version upgrades

256ad01

richvdh added the proposal-pr label Aug 10, 2018

richvdh mentioned this pull request Aug 10, 2018

Room version upgrades #1501

Closed

s/settings/events

1607253

dbkr reviewed Aug 14, 2018

View reviewed changes

dbkr reviewed Aug 17, 2018

View reviewed changes

fix endpoint path

5455cc4

dbkr reviewed Aug 22, 2018

View reviewed changes

s/upgrade_room/upgrade

4750b29

richvdh commented Aug 28, 2018

View reviewed changes

KitsuneRal approved these changes Aug 29, 2018

View reviewed changes

anoadragon453 reviewed Oct 17, 2018

View reviewed changes

proposals/1501-room-version-upgrades.md Show resolved Hide resolved

turt2live reviewed Oct 17, 2018

View reviewed changes

uhoreg reviewed Oct 17, 2018

View reviewed changes

add version to example m.room.create event

b5e5a69

A couple of clarifications

14b98a0

- the body of the tombstone is defined by the server. - the client can follow tombstones until it finds a live room

ara4n reviewed Oct 17, 2018

View reviewed changes

richvdh added the final-comment-period label Oct 17, 2018

Explain rationale for setting server_name when joining upgraded room

21a4594

anoadragon453 added finished-final-comment-period and removed final-comment-period labels Oct 23, 2018

richvdh merged commit 42f7a21 into master Oct 23, 2018

richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Oct 23, 2018

richvdh mentioned this pull request Oct 25, 2018

Room version upgrade support matrix-org/synapse#4091

Merged

richvdh removed the spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec label Nov 5, 2018

turt2live reviewed Jan 9, 2019

View reviewed changes

ulope mentioned this pull request Feb 5, 2019

Maybe use Matrix room upgrade mechanism to get rid of duplicate rooms raiden-network/raiden#3399

Closed

afranke deleted the rav/room_upgrades branch September 22, 2021 10:30

turt2live mentioned this pull request Feb 7, 2019

Current room upgrade behavior does not work nicely with invite-only rooms matrix-org/matrix-spec#425

Open

turt2live mentioned this pull request Oct 17, 2022

Cannot redact messages sent before a room was upgraded matrix-org/matrix-spec#1291

Open

turt2live mentioned this pull request Jan 10, 2023

MSC3946: Dynamic room predecessor #3946

Open


		#### Have servers merge old and new rooms together

		Instead of expecting clients to interpret tombstone events, servers could merge

MSC 1501: Room version upgrades #1502

MSC 1501: Room version upgrades #1502

Conversation

richvdh commented Aug 10, 2018 • edited Loading

turt2live commented Aug 10, 2018

KitsuneRal commented Aug 12, 2018

richvdh commented Aug 13, 2018

richvdh commented Aug 13, 2018

richvdh commented Aug 13, 2018

turt2live commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitsuneRal commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitsuneRal commented Aug 25, 2018

KitsuneRal commented Aug 25, 2018

richvdh commented Aug 28, 2018

richvdh commented Aug 28, 2018

richvdh commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitsuneRal commented Aug 29, 2018

anoadragon453 commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented Oct 17, 2018

mscbot commented Oct 17, 2018

mscbot commented Oct 23, 2018

Choose a reason for hiding this comment

richvdh commented Aug 10, 2018 •

edited

Loading