feat: new group membership update algorithm #4185

Septias · 2023-03-20T13:52:22Z

Replacement for #3807.

This PR implements the idea sketched in #3782 by acting along these guidlines:

If self is no in chat, reject the membership update.
If a membership update message has an outdated timestamp, reject the update.
If the sender is a mua rebuild the contact list, but only add new members.
If we don't know the referenced message, rebuild the contact list because we missed messages which could have been add/remove.

By always discarding outdated membership messages there might occur situations in which two people simultaneously change members and the updates come in out of order, leading to the discard of one of the messages. It is to note though, that this is only a problem when there was no synchronisation between the two senders because otherwise the membership-list will be rebuild because of 4.

This problem also occures in the current master' implementation because it usese the same logic as 2 and is hard to mitigate because there is no obvious way to decide whether to accept an old update or not. There is maybe a way by storing a timestamp per group member which would allow us to reason better about the membership of individual users instead of membership changes on a group-level, but this can be part of future work.

Tests

This PR also introduces some new tests:

Bob will catchup to groupstate when readded. (test_sync_member_list_on_rejoin)
Alice rebuilds the contact list if she misses a message. (test_rebuild_contact_list_on_missing_message)
Alice doesn't rebuild contact list because of single addition/deletion. (test_dont_recreate_contacts_on_add_remove)
Bob doesn't readd himself to a group because he receives a normal message. (test_dont_readd_with_normal_msg)
A delayed receival of an addition or removal will not change group state.(test_recreate_contact_list_on_missing_message)
A mua user can't remove someone (test_mua_cant_remove)
A mua user can add someone (test_mua_can_add)

hpk42 · 2023-03-20T22:28:24Z

is this still implementing #3782 ? -- please try to add a bit more context to PRs. Was expecting to read a description of the new algorithm that is in the PR title somewhere .... :)

src/receive_imf.rs

iequidoo · 2023-04-04T16:41:07Z

src/receive_imf.rs

+                    true
+                } else {
+                    match mime_parser.get_header(HeaderDef::InReplyTo) {
+                        // If we don't know the referenced message, we missed some messages which could be add/delete


And also we can miss messages about an addition of ourselves. So, this code should have a preference over "self" checks done above. And also a comment would be good on why these "self" checks are needed

When self is added, the recreate_membership_list flag is set and apply_groupchanges aswell. This will lead to a new better_msg in the header-handling code, so a stockstring should be created.

But what if we did't receive a ChatGroupMemberAdded message about "self"? Afaiu, this PR is fixing problems when messages can be missed because of delivery problems. I don't get why "self" is a special case in ragards to handling of InReplyTo which references a missed message

I moved it outwards where it belongs

Anyway, consider the following scenario:

We left the group

Then we were re-added, but missed the addition message

We receive a message with InReplyTo referencing a message we don't have (the message adding us to the group). But above there's the check:

if !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? && !self_added {

So, we're not in the group and don't handle this scenario. Is it correct? Why is it special for SELF?

I think that current problems with unintentional re-additions come from the current approach that u described:

* If the message has a Chat-Group-Member-Removed header, then we completely recreate the group member list (i.e. use the member list of the incoming message). * If the message has a Chat-Group-Member-Added, then we recreate the group member list, but don't remove anyone (i.e. add recipients of the incoming messages as members).

It's not difficult to see how each of the both arms above can lead to an unintentional re-addition.

@Hocuri, i still don't get why we can't just use the algo you suggested initially in #3782 (comment). For me it looks more simple and straightforward. Also it was discussed in the "DC Core Rust" chat, just search for "InReplyTo" (starting from Apr 17)

IIRC at some point, there were lots of complaints that people couldn't leave a group because they were constantly re-added.

The problem was that some users in a large group kept reinstalling DC and restored an old backup with old member list. Then they immediately left some groups, sending a Chat-Group-Member-Removed header, which caused recreation of the member list and readdition of self to the group for you and other members. Other members also tried to leave the group, sending lots of "... left the group" messages to some broken group. It was difficult to teach everyone not to leave the group, but to delete it or block it and forget about it.

Reading the thread here, tbh, I don't remember anymore what the problems discussed here were. But I think it's fine, IIRC we discussed pretty minor edge cases, time will tell if there are problems we should fix (and whether the new algo is an improvement at all). @iequidoo I hope you don't feel like we missed your concerns?

The problem is that the current algo is not resillent to missed messages about addition of SELF. If it happens, the only way to fix the group is to remove the member and re-add them again. The algo you suggested in #3782 (comment) fixes this inconsistency when we receive a message with orphaned InReplyTo. Also it looks simpler. And as for the problem with messages from a Delta Chat that restored an old backup, it will ignore them as well because they will reference a known message in InReplyTo.

I think it's not very important right now, but i'd prefer to try your "truly consistent" algo and if it doesn't bring new problems, why not use it?

src/receive_imf.rs

Hocuri

I reviewed the new logic, directly pushed some changes with explanation (please have a look if they seem good), and have one (blocking) comment.

So, only the tests need to be reviewed still. Edit: The tests also look good; in many places, they could use some newer APIs from test_utils.rs in order to be a lot shorter / more concise, but it would also be OK-ish to merge as-is.

src/receive_imf.rs

Hocuri

🎉🎉🎉!

Before merging, let's

wait for @link2xt to review / have a glance on it
make sure that nobody is going to make a release right afterwards

Septias · 2023-06-02T11:54:40Z

Ty for rebasing @link2xt

New algorithm improves group consistency in cases of missing messages, restored old backups and replies from classic MUAs. Co-authored-by: Hocuri <hocuri@gmx.de> Co-authored-by: link2xt <link2xt@testrun.org>

link2xt · 2023-06-07T00:23:33Z

I have added some logs, removed unnecessary .cloned(), rebased and squashed the PR, then merged once the tests passed.

link2xt · 2023-06-07T00:24:41Z

make sure that nobody is going to make a release right afterwards

The changes are incremental and only add tests without modifying existing tests, so I think it is ok to get this into 1.117.0 and turn 1.117 into a stable branch.

Septias · 2023-06-07T08:35:10Z

Finally done! Thanks everybody for the great effort!

link2xt · 2023-07-05T20:05:52Z

src/receive_imf.rs

+    };
+
+    if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) {
+        removed_id = Contact::lookup_id_by_addr(context, removed_addr, Origin::Unknown).await?;


This may fail to load the contact if the contact is blocked. In this case all the code below is skipped and better_msg is not used.

Found this while looking for the reason of #4528

iequidoo reviewed Mar 22, 2023

View reviewed changes

src/receive_imf.rs Show resolved Hide resolved

src/receive_imf.rs Outdated Show resolved Hide resolved

Septias force-pushed the sk/better_group_consistency branch from d23186d to 3a75706 Compare April 4, 2023 10:04

iequidoo reviewed Apr 4, 2023

View reviewed changes

Septias force-pushed the sk/better_group_consistency branch from 91db9c7 to a279f96 Compare April 7, 2023 12:52

Septias requested review from Hocuri and link2xt April 7, 2023 14:32

iequidoo mentioned this pull request Apr 13, 2023

Don't let blocking be bypassed using groups #4316

Merged

link2xt force-pushed the master branch from cfacebb to 514074d Compare April 22, 2023 16:43

Hocuri force-pushed the sk/better_group_consistency branch 3 times, most recently from a7da289 to 5284f25 Compare May 18, 2023 20:14

Hocuri reviewed May 18, 2023

View reviewed changes

src/receive_imf.rs Outdated Show resolved Hide resolved

src/receive_imf.rs Show resolved Hide resolved

Hocuri changed the title ~~New algorithm for group membership consistency~~ feat: New algorithm for group membership consistency May 18, 2023

Hocuri approved these changes May 22, 2023

View reviewed changes

link2xt force-pushed the master branch from a1cc433 to 5b435d1 Compare June 1, 2023 12:31

link2xt force-pushed the sk/better_group_consistency branch 4 times, most recently from f41ffde to 69ef783 Compare June 6, 2023 23:40

link2xt changed the title ~~feat: New algorithm for group membership consistency~~ feat: new group membership update algorithm Jun 6, 2023

feat: new group membership update algorithm

6eb8abe

New algorithm improves group consistency in cases of missing messages, restored old backups and replies from classic MUAs. Co-authored-by: Hocuri <hocuri@gmx.de> Co-authored-by: link2xt <link2xt@testrun.org>

link2xt force-pushed the sk/better_group_consistency branch from 69ef783 to 6eb8abe Compare June 6, 2023 23:50

link2xt merged commit 6eb8abe into master Jun 7, 2023
35 checks passed

link2xt deleted the sk/better_group_consistency branch June 7, 2023 00:22

Hocuri mentioned this pull request Jun 8, 2023

Recreate the group member list if we don't know the parent message #3782

Closed

link2xt mentioned this pull request Jul 5, 2023

Message about leaving the group is displayed as is #4528

Closed

link2xt reviewed Jul 5, 2023

View reviewed changes

This was referenced Jul 23, 2023

apply_group_changes updates MemberListTimestamp even when there are no changes #4570

Closed

Self-adding contact to group during blocking SMTP relay leads to false report that member has been added #2352

Closed

link2xt mentioned this pull request Aug 24, 2023

Add self to the group when a MUA reply is received #4624

Closed

iequidoo mentioned this pull request Aug 28, 2023

fix: Allow membership changes by a MUA if we're not in the group (#4624) #4638

Merged

link2xt mentioned this pull request Nov 6, 2023

Users are implicitly removed without any visible message #4934

Closed

link2xt mentioned this pull request Nov 18, 2023

fix: recognize Chat-Group-Member-Added of self case-insensitively #5031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new group membership update algorithm #4185

feat: new group membership update algorithm #4185

Septias commented Mar 20, 2023 •

edited by link2xt

Loading

hpk42 commented Mar 20, 2023

iequidoo Apr 4, 2023

Septias Apr 5, 2023

iequidoo Apr 6, 2023

Septias Apr 7, 2023

iequidoo Apr 7, 2023

iequidoo Apr 10, 2023

iequidoo Jun 3, 2023 •

edited

Loading

link2xt Jun 7, 2023

Hocuri Jun 8, 2023

iequidoo Jul 17, 2023

Hocuri left a comment •

edited

Loading

Hocuri left a comment

Septias commented Jun 2, 2023

link2xt commented Jun 7, 2023

link2xt commented Jun 7, 2023 •

edited

Loading

Septias commented Jun 7, 2023

link2xt Jul 5, 2023

feat: new group membership update algorithm #4185

feat: new group membership update algorithm #4185

Conversation

Septias commented Mar 20, 2023 • edited by link2xt Loading

Tests

hpk42 commented Mar 20, 2023

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

Choose a reason for hiding this comment

iequidoo Jun 3, 2023 • edited Loading

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

Hocuri left a comment • edited Loading

Choose a reason for hiding this comment

Hocuri left a comment

Choose a reason for hiding this comment

Septias commented Jun 2, 2023

link2xt commented Jun 7, 2023

link2xt commented Jun 7, 2023 • edited Loading

Septias commented Jun 7, 2023

Choose a reason for hiding this comment

Septias commented Mar 20, 2023 •

edited by link2xt

Loading

iequidoo Jun 3, 2023 •

edited

Loading

Hocuri left a comment •

edited

Loading

link2xt commented Jun 7, 2023 •

edited

Loading