fix: Add tolerance to MemberListTimestamp (#5366) #5376
Conversation
Let's add a 1-minute tolerance to `Params::MemberListTimestamp`. This adds to the group membership consistency algo the following properties: - If remote group membership changes were made by two members in parallel, both of them are applied, no matter in which order the messages are received. - If we remove a member locally, only explicit remote member additions/removals made in parallel are allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to reply earlier than receiving our removal of a member, we added it back which doesn't look good.
Otherwise golden_test_chat() fails when run around midnight.
iequidoo
force-pushed
the
iequidoo/MemberListTimestamp-tolerance
branch
from
297029c
to
a2b28f1
Compare
| .update_timestamp( | ||
| context, | ||
| Param::MemberListTimestamp, | ||
| now.saturating_add(constants::TIMESTAMP_SENT_TOLERANCE), |
There was a problem hiding this comment.
This assumes that the sender is online? Otherwise if the sender removes someone locally from the group, sets timestamp to now + 60 seconds, but only gets online an hour later, nobody else even have a chance to receive the message about member removal.
Or the other way round, you are online, remove someone from the group and set timestamp to now + 60 seconds. 5 minutes later someone who is offline sends a message without receiving your message about the change, it will appear as new according to Date but it was constructed for an outdated member list.
There was a problem hiding this comment.
I don't think looking at the Date of incoming message can solve this. What we are interested in is not whether the incoming message is newer than previous change (by 60 seconds or any other margin), but if the sender of the message have seen this previous change when the message was constructed.
Maybe instead of adding 60-second margin it is sufficient to look at the date of the parent message instead of the message itself when we consider whether to apply the change?
There was a problem hiding this comment.
This assumes that the sender is online? Otherwise if the sender removes someone locally from the group, sets timestamp to now + 60 seconds, but only gets online an hour later, nobody else even have a chance to receive the message about member removal.
I think this is ok. If there are newer messages in the group, the sender will "sync back" the removed member and (optionally) retry to remove it. The removal was done based on the outdated state anyway.
Or the other way round, you are online, remove someone from the group and set timestamp to now + 60 seconds. 5 minutes later someone who is offline sends a message without receiving your message about the change, it will appear as new according to
Datebut it was constructed for an outdated member list.
This is a problem, yes, but maybe my message was lost at all, so i can't know whether the message i received was constructed for an "outdated" member list. Maybe i'm the only one who has the "updated" member list.
Maybe instead of adding 60-second margin it is sufficient to look at the date of the parent message instead of the message itself when we consider whether to apply the change?
The parent message could be sent offline too.
I think adding the proposed tolerance has sense as it improves some scenarios in the tests, but to improve the consistency algo further, i'd suggest to add a new msgs.timestamp_members column:
- If we send/receive a message,
timestamp_membersis inherited from the parent message. - If the message also changes group members,
timestamp_members = max(timestamp_members + 1, timestamp_sent). - If
timestamp_members>=MemberListTimestampof the chat, updateMemberListTimestampand the member list from the message.
But even then adding some tolerance makes sense to allow unordered parallel changes.
See commit messages.
Fix #5366