Skip to content
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

MSC3901: Deleting state #3901

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft

MSC3901: Deleting state #3901

wants to merge 34 commits into from

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Sep 28, 2022

@andybalaam andybalaam changed the title MSC3900: Deleting state MSC3901: Deleting state Sep 28, 2022
@andybalaam andybalaam marked this pull request as draft September 28, 2022 14:35
@turt2live turt2live added proposal A matrix spec change proposal s2s Server-to-Server API (federation) kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Sep 29, 2022
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Overall seems pretty thorough and an accurate assessment of the 2022 discussions.

proposals/3901-deleting-state.md Outdated Show resolved Hide resolved

We propose to update the definition of event redaction[^spec-redactions] to
specify that all redacted state events contain `m.obsolete: true` in their
content.
Copy link
Member

Choose a reason for hiding this comment

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

In practice, does this mean that content:{} == obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by ==? It does mean that all redacted events will have content:{} AND obsolete:true but I don't know whether there are other circumstances where content:{} is valid, especially in events defined outside the spec.

content.

Note: `membership: "ban"` events are not considered obsolete since this
information is needed in future to prevent bad actors from re-entering a room.
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly regarding power DAG P2P work, we treat "invite rejection" as a special important event, as it means you cannot see into an invite-only room. We probably want to keep invite rejections in this list along with bans.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note in f0eef7a, thanks. I don't know exactly what an invite rejection looks like so maybe you could suggest wording to make it more explicit?


If MSC3414 goes ahead, an obsolete encrypted state event should contain
`m.obsolete: true` in its unencrypted content, as a sibling of e.g. `algorithm`
and `ciphertext`.
Copy link
Member

Choose a reason for hiding this comment

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

in its encrypted content surely?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the part of the content that is not encrypted, so that servers can read it. Did I use ambiguous wording or am I missing something else?

proposals/3901-deleting-state.md Outdated Show resolved Hide resolved
When a client requests to upgrade a room using `POST /rooms/{roomId}/upgrade`,
this should be interpreted by the server as a request not only to create the
room, but also to invite all members of the old room to the new one, with the
same power level.
Copy link
Member

Choose a reason for hiding this comment

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

This should only apply if the upgraded room version == the existing room version. By allowing actual room version bumps we risk breaking entire servers who aren't on the latest version of synapse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we limit it to room "refactorings" (new version==old version)?

How will it break homeservers that don't support this? Won't they just see fairly normal-looking invitations, and then they themselves fail to do the part they are supposed to do? Isn't that "fine"?

proposals/3901-deleting-state.md Outdated Show resolved Hide resolved
proposals/3901-deleting-state.md Outdated Show resolved Hide resolved
proposals/3901-deleting-state.md Show resolved Hide resolved
include anything with a `state_key` that starts with the user's mxid plus
underscore.
* State whose contents include a top-level property `exclude_from_upgrade:
true`.
Copy link
Member

Choose a reason for hiding this comment

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

This feels very similar to obsolete events. Some prose explaining the difference and why we can't use one flag would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

One justification might be that someone creates custom events and relies on their sender property. But, since I could not think of a concrete reason I thought I'd try deleting this entirely and see what people think. d0e774e does that.

proposals/3901-deleting-state.md Outdated Show resolved Hide resolved
proposals/3901-deleting-state.md Show resolved Hide resolved
Comment on lines +606 to +607
Clients and external systems continue to use the existing room ID, and servers
use room ID + room version to identify the real actual room.
Copy link
Member

Choose a reason for hiding this comment

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

How does traversing history back into the old room work with this scheme? A lot of detail here is left out from the current proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this part is quite forward-looking and not fully-defined.

Comment on lines +412 to +414
MSC3325 also mentions as an alternative that the room membership of each user
could be set as `invited` without actually sending an invitation, to avoid
invite spam.
Copy link
Member

Choose a reason for hiding this comment

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

Another question is what should happen if more than one m.room.tombstone event is sent in a room. You would be able to invite spam a single user more than once.

I'm leaning towards extending the definition of the invite join_rule to mean "users joined to the predecessor room may join without an explicit invite". Essentially "restricted", but only for the predecessor room. That way you cut down on both bandwidth of sending invites over federation, as well as invite state events in the room.

The fact that a user has joined via this method should be known to other clients in the room, such that they can share historical message keys for the purposes of decrypting old room messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

This does sound good to me. Any objections from anyone? Does anyone want to write it up in the MSC?

@florianjacob
Copy link
Contributor

I just had a chat with @anoadragon453 after the very helpful Solving the Historical State Problem in Matrix
talk on the topic - the recording is already available and presents the ideas of this MSC in a more current fashion than what is written down.

The fundamental issue here with ideas going for actually deleting things might be that garbage collection, from a computer science theory perspective, fundamentally requires coordination among servers, even consensus, which Matrix cannot and does not want to provide in its current form. Everything is fine with hiding obsolete events from clients - that is just a projection of the data still present on the server, but as soon as you actually want to delete them, you will probably run into problems that require hard drawbacks like requiring that servers cannot be offline for an arbitrary length of time.

Room upgrades potentially circumvent the fundamental issue because just starting a new room does not actually garbage-collect the old state, strictly-speaking. However, having a monotonically-increasing integer as sub-version to the room identifier would also require coordination / consensus, or requires hope that concurrent room upgrades do not happen in practice and network partitions are short enough, or actually ignoring everything that was written in the concurrent room instance that "lost". One way to circumvent the issue with monotonically-increasing sub-versions would be to switch to a partial order of sub-versions, i.e., a DAG, which would then map concurrent room upgrades to branches in the DAG. However, one would want have something like a "current" room version - which could be done by having a third room upgrade that merges the branched room upgrades again.

Or, the room sub-version DAG could be topologically sorted, like with state resolution - and that is where we were thinking, what about “room upgrades as rooms”? Have a space as a meta-room that contains pointers to the sub-rooms, and a room upgrade is pointer in the space to a new room? Then the sub-version DAG would exactly be the regular space DAG, and topological sorting would be the regular room topological sorting. Also, it would keep room ids as-is, and servers do not need to follow all room ugprades to end up with the current room version. However, this idea also has problems, like keeping the state in the meta-room minimal, i.e., not requiring normal users to join there to be able to take part in the sub-rooms - potentially requires #2444. On the other hand, it would also massively profit from other improvements around spaces, like improved access control / power level inheritance from space to sub-rooms.

@andybalaam
Copy link
Member Author

andybalaam commented Jul 5, 2023

@florianjacob

I just had a chat with @anoadragon453 ...

Wow, really interesting stuff. I think this mostly makes me feel that the room sub-versions stuff is way harder than the rest, and should probably be separated out from it to allow the easier stuff to progress...

@andybalaam
Copy link
Member Author

OK, I think this MSC is in a decent state. There are various outstanding questions, but it seems to make a reasonable level of sense. The room sub-versions part is under-defined, but I think we can safely leave that until later.

So the problem now becomes how to progress this further, which means attempting implementations. I am going to figure out how to take this to Element leadership, but I'd appreciate input from people from other companies on whether they are able to put effort into this.

My guess on the Element side is that this will be seen as strategic long term, but not urgent enough to put time into right now.

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

For visibility, I had some thoughts that are relevant to state deletion and wrote them up recently on my blog: https://robin.town/blog/distributed-access-control

Specifically, I had the thought that if Matrix room state, as a data structure, used version vectors instead of tombstones as in https://inria.hal.science/hal-00738680, then it seems that implementations could forget deleted state immediately, without the need for coordinated room upgrades.

Comment on lines +185 to +188
We propose to update the definition of [membership
events](https://spec.matrix.org/v1.5/client-server-api/#mroommember) so that
every event with `membership: "leave"` must also have `m.obsolete: true` in its
content.
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, several folks have commented today in a TWIM discussion that forgetting whether a member has left vs. whether they've simply never joined, could be inconvenient for moderation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal s2s Server-to-Server API (federation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants