-
Notifications
You must be signed in to change notification settings - Fork 414
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 <[email protected]>
Co-authored-by: Johannes Marbach <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
|
@mscbot resolve Unclear if addendum is normative for spec process purposes |
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.
Have split the comments into threads (#4354 (comment))
proposals/4354-sticky-events.md
Outdated
|
|
||
| To implement these properties, servers MUST: | ||
|
|
||
| * Attempt to send their own[^origin] sticky events to all joined servers, whilst respecting per-server backoff times. |
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.
Moving from #4354 (comment)
The lack of atomicity in
/sendmeans clients may flicker RTC member state (update to old values, then immediately to newer values). This happens today too with state events, but less often.
In Synapse this will be especially slow as when we process each sticky event we go and fetch the previous 10 events and then query the state (assuming a large enough gap). This doesn't happen for state, as we'll get the last event and calculate the state for that chunk and atomically persist it. State flickering can happen if the server receives a chunk of events that contain a bunch of state changes, though empirically this is fairly rare.
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 doesn't happen for state, as we'll get the last event and calculate the state for that chunk and atomically persist it.
I don't follow this. If I send 50 PDUs all in the same room, a nice linear chain with no forks, we:
- treat all 50 PDUs as live (so will send them down /sync)
- calculate the state before each PDU (only the earliest incurring a state query hit)
- process each PDU atomically, but not the batch of 50.
So you will see flickering?
I think flickering of ephemeral per-user state is inevitable if we wish to hide the key we're modifying in the map from the server. It's definitely a security / UX tradeoff to make, though we've increasingly leant on the side of security for quite some time now. What would the implications be for flicking live-location shares or flickering RTC members? The former likely means the location is updated gradually as the server/client catch up. I think RTC members are reasonably static (they don't change mid-call), so flickering RTC members could make it appear that older members are joined to the call who then leave the call a few milliseconds later? Is this a problem for the call state machine? cc @toger5
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.
Obviously if someone sends 50 sticky events in short succession then that will cause "flickering" as things come down live, but that is reflecting the reality that that state is flickering. That's totally fine.
However, if those 50 events happened over the course of an hour and you see them flickering of state changes then that is a different thing. We have previously made efforts to avoid much flickering on clients.
I think flickering of ephemeral per-user state is inevitable if we wish to hide the key we're modifying in the map from the server
Doesn't some of the encrypted state proposals allow encrypting the state key as well? Or potentially you could have a pointer to previous sticky events that get superseded and these are pulled in automatically (and if the server pulls them in then it knows not to treat them as "live")?
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.
Obviously if someone sends 50 sticky events in short succession then that will cause "flickering" as things come down live, but that is reflecting the reality that that state is flickering. That's totally fine. However, if those 50 events happened over the course of an hour and you see them flickering of state changes then that is a different thing.
Okay, but we have no notion of time in a decentralised system, so how are you in practice determining what is "over the course of an hour" - by the sender timestamp or receiver timestamp? Assuming receiver timestamp because it isn't gameable then by that measure the fact that we sent a bunch of new sticky events over federation on catchup falls into the category of:
that is reflecting the reality that that state is flickering. That's totally fine.
The real concern then is the fact that these events will flicker down to clients due to the fact that we deliver sticky events down /sync in a strict chronological-by-receiver-timestamp order. This then begins to overlap with @MadLittleMods's concern of the lack of a pagination endpoint.
Two birds can be killed with one stone by adding such a pagination endpoint to ensure that we actually deliver the newest stuff first in the common case. The net observed behaviour then for clients will be:
- if they are actively syncing when the remote server sends catch up traffic, then it flickers. This is fine.
- if they are NOT actively syncing when the remote server sends catch up traffic, then they will get the most recent events first, reducing the chances of flickering occuring. This assumes the server sends catch up sticky events in chronological order, which we already mandate:
Servers MUST send old sticky events in the order they were created on the server (stream ordering / based on origin_server_ts). This ensures that sticky events appear in roughly the right place in the timeline as servers use the arrival ordering to determine an event's position in the timeline.
This should resolve your concern here?
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.
Okay, but we have no notion of time in a decentralised system, so how are you in practice determining what is "over the course of an hour" - by the sender timestamp or receiver timestamp? Assuming receiver timestamp because it isn't gameable then by that measure the fact that we sent a bunch of new sticky events over federation on catchup falls into the category of:
I'm not suggesting we do things based on time, I'm suggesting that we use the same behaviour as for state? I.e. when synchronising you sync send the current/latest state, rather than sending each delta in order. That doesn't require time or anything, and is perfectly possible evidenced by the fact we have it today.
proposals/4354-sticky-events.md
Outdated
|
|
||
| To implement these properties, servers MUST: | ||
|
|
||
| * Attempt to send their own[^origin] sticky events to all joined servers, whilst respecting per-server backoff times. |
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.
Moving from #4354 (comment)
how does MatrixRTC handle push notifications for incoming calls? (tangential to this MSC but whatever)
The question is: do we want to use sticky events for MatrixRTC notifications, and if so will that make the flickering problem much more noticeable/problematic?
Naively to me it feels odd to not use sticky events for call notifications, e.g. I'd have thought you would want to be notified for all calls in a DM. If you don't use sticky events you could end up in the situation where you see the call in the UI but not be notified about it.
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 don't know. @toger5 will have more context on the tradeoffs and what goes wrong if you do / do not.
The primary concern from the original comment thread was:
Say you have two servers S1 and S2 which share a room, with S2 down. A call is started between multiple people on S1, they chat a bit and then end the call. S2 then comes back online and receives the sticky events for all the call participants in order, and S2 process them in order (including backfilling and fetching /state etc). On syncing clients I think this would result in the call sticky events trickling down, and e.g. triggering the client to ring and then stop ringing when the call-end is finished processing? This would also presumably trigger push notifications as well?
This scenario is somewhat diverged from reality as there is no "call-end" event iirc, but the general idea is valid:
- Your server is offline.
- A call is placed and ended in a room you're in.
- Your server comes back online and gets all these events.
- Does it cause the client to ring then stop ringing?
My understanding is that "ringing" is just an @ mention, but I lack the context to know how the client knows when to stop "ringing", hence cc @toger5
proposals/4354-sticky-events.md
Outdated
|
|
||
| To implement these properties, servers MUST: | ||
|
|
||
| * Attempt to send their own[^origin] sticky events to all joined servers, whilst respecting per-server backoff times. |
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.
Moving from #4354 (comment)
we will accumulate more forward extremities when catching up as we are now including sticky events in the initial batch of events when catching up. This is a concern, but having to deal with lots of forward extremities isn't a new concern.
One potential security concern here is that it makes it easier for users on one server to generate lots of extremities on another server, which can lead to performance issues in very large rooms. This does only work when the connection between the two servers is down (e.g. the remote server is down).
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 makes it easier for users on one server to generate lots of extremities on another server
This is true today via message events surely? Like, I can trivially make lots of events and trickle every Nth message to cause forward extremity accumulation?
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.
You can't as a user on the server, but yes the server can.
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.
The impact here is it's a DoS risk right?
A user can't unilaterally do that unless they can manipulate the connection between two servers, which feels a bit of a stretch. We lack any kind of threat model here to know if this is something we should care about 🤷🏼 but at least in my view if you can control the connection then you're likely a server admin of one of the servers, in which case this opens up no additional weakness. If you're just a user, then this attack can't be reliably done - if you can DoS one of the servers to take them offline then uh.. well, that's just an easier way of doing this, since the attack is also a DoS :S in other words I just don't see how this is concretely affecting anything which isn't affected already?
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 <[email protected]> Co-authored-by: Timo <[email protected]>
Rendered
SCT Stuff:
FCP tickyboxes
MSC checklist