-
Notifications
You must be signed in to change notification settings - Fork 423
MSC4354: Sticky Events #4354
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
base: main
Are you sure you want to change the base?
MSC4354: Sticky Events #4354
Conversation
It wasn't particulalry useful for clients, and doesn't help equivocation much.
Co-authored-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Co-authored-by: Johannes Marbach <n0-0ne+github@mailbox.org>
richvdh
left a comment
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.
Nothing major, but I do have some minor questions, and a bunch of suggestions for making this easier to read (and hence likelier to land)
| thus leak metadata. As a result, the key now falls within the encrypted `content` payload, and clients are expected to | ||
| implement the map-like semantics should they wish to. | ||
| [^ttl]: Earlier designs had servers inject a new `unsigned.ttl_ms` field into the PDU to say how many milliseconds were left. | ||
| This was problematic because it would have to be modified every time the server attempted delivery of the event to another server. |
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.
This was problematic because it would have to be modified every time the server attempted delivery of the event to another server.
Doesn't the spec require that today with the age field?
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.
Yeah but not over federation. I mostly added this because Erik seemed to think this was a downside in his earlier proposal:
Also having a short expiry makes retries over federation annoying (as they are for events with
age), since you need to mutate the contents before retrying a request
Do you want me to add anything to this?
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: Timo <16718859+toger5@users.noreply.github.com>
MSC: matrix-org/matrix-spec-proposals#4354 This is for policyserv so we can block sticky events in our public rooms (they aren't needed there)
|
This MSC appears to still be in flux and is failing to acquire checkboxes. Because I'm the one that initially asked for FCP, I'm cancelling that request at the moment. Another SCT member can put it forward again at any time. @mscbot fcp cancel |
proposals/4354-sticky-events.md
Outdated
| Over Simplified Sliding Sync, Sticky Events have their own extension `sticky_events`, which has the following response shape: | ||
|
|
||
| ```js | ||
| { | ||
| "rooms": { | ||
| "!726s6s6q:example.com": { |
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.
Clarify which rooms show up here. All rooms or only rooms relevant to the lists/room_subscriptions
| - B) Persist the sticky events but wait a while before delivering them to clients. | ||
|
|
||
| Option A means servers don't need to store sticky events in their database, protecting disk usage at the cost of more bandwidth. | ||
| To implement this, servers MUST return a non-2xx status code from `/send` such that the sending server |
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.
suspicious: rather than saying what code you should use, it says what code you shouldn't. If we want to suggest this approach I think we should provide stronger guidance.
But the /send endpoint already has per-event statuses, so this feels a bit off as an approach. Separately, a non-2xx response will likely just cause the sending server to retry and/or mark you as down.
For this reason it's hard to recommend this approach and I'm wondering if we should recommend against it, except as a fallback when you're desperate / dealing with a highly-likely malicious case?
2183282 to
68581b7
Compare
68581b7 to
331484d
Compare
| Sticky messages MAY be sent in the timeline section of the `/sync` response, regardless of whether | ||
| or not they exceed the timeline limit[^ordering]. If a sticky event is in the timeline, it MAY be |
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.
This (allowing servers to exceed the limit of the existing timeline section) seems like a breaking change and I'd be uncomfortable supporting it.
| Sticky messages MAY be sent in the timeline section of the `/sync` response, regardless of whether | ||
| or not they exceed the timeline limit[^ordering]. If a sticky event is in the timeline, it MAY be | ||
| omitted from the `sticky.events` section. This ensures we minimise duplication in the `/sync` response JSON. | ||
| This proposal recommends always putting sticky events into the `sticky.events` section _except_ if |
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.
I'd like to suggest we make this MUST rather than recommendation.
I don't see any benefit in allowing divergence and it could result in clients mishandling events on some server implementations.
| These messages may be combined with [MSC4140: Delayed Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4140) | ||
| to provide heartbeat semantics (e.g required for MatrixRTC). Note that the sticky duration in this proposal | ||
| is distinct from that of delayed events. The purpose of the sticky duration in this proposal is to ensure sticky events are cleaned up, | ||
| whereas the purpose of delayed events is to affect the send time (and thus start time for stickiness) of an event. |
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.
Sticky events is an awesome and fun name, I would even say elegant if it wasnt for the following issue. It appears to be named after an implication of what it does, but I think the metaphor isn't at all obvious. This is particularly true wrt self-destruct semantics (= inverted heartbeat) of delayed events as the apparent need for this clarifying section emphasizes.
In conversation with peers, people who have not read this MSC but only somehow heard about it are usually assuming some kind of self-destruction related to the timer. Perhaps the event "sticks around" that long. It's quite possible this is also burdened by mixup with that actual requirement as delayed events and matrixrtc as a whole remain a developing topic, and so often both being mentioned in one sentence.
My interpretation after reading is that they are sticky in a sense of sticking to the top, eager high priority sharing, similar to state events, and eventually unsticking to fall back to regular priority.
I realize I'm a bit late to complain about the name which has already been used proudly in public a bunch. We could sti consider finding a name that more intuitively fits the purpose. Priority Events, Important Events?
BillCarsonFr
left a comment
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.
I have a question regarding mixed content sticky chains (clear sticky replacing encrypted sticky)
| As such, this proposal relies somewhat on NTP to ensure clocks over federation are roughly in sync. | ||
| As a consequence of this, the sticky duration SHOULD NOT be set to below 5 minutes.[^ttl] | ||
|
|
||
| ### Encryption |
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.
What is the strategy regarding Mixed Content sticky chain? Like a clear event replacing an encrypted one?
Can we disable mixed content? Only an encrypted event can replace an encrypted sticked event.
Or at least having a way to discard such a sticky event?
If not it would be like allowing clear edits of encrypted messages without showing a big red warning.
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.
FWIW there is prior art / similar rules for edits validity of replacement events
Maybe we could add similar rules? to be consistent.
Things like
- The replacement and original events must have the same type
- If the original event was encrypted the replacement should be too.
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.
I guess this is referring to the 'Addendum: Implementing an ephemeral map'? I wonder if the formalisation of that addendum would be better-suited to another MSC?
|
|
||
| When an event loses its stickiness, these properties disappear with the stickiness. Servers SHOULD NOT | ||
| eagerly synchronise such events anymore, nor send them down `/sync`, nor re-evaluate their soft-failure status. | ||
| Note: policy servers and other similar antispam techniques still apply to these events. |
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.
It should probably be described what this concretely means. (coming from the PR review)
From the Synapse PR that was developed alongside the MSC, I'm taking this to mean that we don't treat the events as sticky at all if our policy server is not happy with them, so we don't propagate them to other servers or clients.
Rendered
SCT Stuff:
FCP tickyboxes
MSC checklist