test: add group consistency bug test

[link2xt]

[link2xt]

The problem with the logic introduced in https://github.com/deltachat/deltachat-core-rust/pull/5376 is that it assumes that if timestamp_sent of the message is newer than the last membership change in the chat, then the user has already received all membership changes and was aware of them when sending. In fact user may have sent the message while being offline and will only receive membership changes later when going offline, or not at all if they are not delivered.

I think instead of timestamp_sent we should look at the timestamp_sent of the message referenced by In-Reply-To. If it is older than the timestamp of the most recent member list change, then user has not received the message with the change yet and we should not allow them to change member list.

The logic is completely contained here, could be reordered to first try to find the parent and then use its timestamp instead of timestamp_sent of just received message: https://github.com/deltachat/deltachat-core-rust/blob/46922d4d9d500f18790d1414399dc9b9850367e0/src/receive_imf.rs#L2104-L2145

[iequidoo]

I think instead of timestamp_sent we should look at the timestamp_sent of the message referenced by In-Reply-To. If it is older than the timestamp of the most recent member list change, then user has not received the message with the change yet and we should not allow them to change member list.

The problem is that the referenced message might be also sent being offline, or by another user not having recent messages in the chat.

EDIT: The solution from https://github.com/deltachat/deltachat-core-rust/pull/5376#discussion_r1554535366 should fix the observed bug as well, but it requires adding a new column msgs.timestamp_members, that's why i still haven't tried to implement it.

[link2xt]

The problem is that the referenced message might be also sent being offline, or by another user not having recent messages in the chat.

But at least, assuming everyone's clock is in sync and message delivery is instant once user gets online, if you receive a message referencing parent message that has sent timestamp x, you can tell user has downloaded all messages that were delivered up to time x. If membership changed last time at time y < x, then user was up to date, otherwise you cannot trust them with membership changes.

EDIT: If this logic is sufficient to fix this particular test case without breaking other test cases we already have, then I'd rather implement it instead of adding new columns, and close the issue (well, PR) until we hit another problem in real chats.

[iequidoo]

But at least, assuming everyone's clock is in sync and message delivery is instant once user gets online, if you receive a message referencing parent message that has sent timestamp x, you can tell user has downloaded all messages that were delivered up to time x.

It's not clear whether the user downloaded all messages if the user's message reference only their previous messages because currently we add to References OutPending and even OutFailed messages. I.e. if the user sends multiple messages from offline, we should look at the first message to tell that. But it would be a recursive algorithm which is better to avoid even if that requires adding new db columns.

[iequidoo]

    let sync_member_list = member_list_ts                                                                                                                                                                            
        .filter(|t| *t <= mime_parser.timestamp_sent)
        .is_some();

This is the code because of which the new test fails (Fiona is "synced back"). We can change it to look not at mime_parser.timestamp_sent, but at the parent message timestamp_sent and also check that the parent message is from another member. But that also may not work properly because the parent message may be from another member also being offline for a long time. I'd say it's unclear what "offline" means, e.g. a temporary network split occurs, but finally all members receive each other's messages, how to merge group member lists then?

[link2xt]

If looking at the parent timestamp fixes the test without breaking other tests then it's good enough to close this PR/issue until we get another problem in a real group chat. I would also not add any conditions like "is it another member message", if the message referenced is OutPending or OutFailed then it does not exist on the receiver device (OutPending actually may because we may have sent it but got disconnected before getting SMTP server response) anyway and we cannot look at its timestamp.

[iequidoo]

I just want to say that looking at the parent message timestamp apparently doesn't fix the problem because Bob in this test can send two messages while being offline and the second message would have as a parent the first one (if Bob is offline, his first message would be OutPending, but anyway it would be referenced by his second message).

[link2xt]

Assigning this to myself so nobody tries to fix it in ad-hoc way.

Plan for fixing group consistency is at https://github.com/chatmail/specs/tree/main/group-membership, I'll open an issue describing the algorithm in detail (with header names, how to handle non-DC and legacy DC messages etc.) at some point.

EDIT: the issue for the new algorithm is at https://github.com/deltachat/deltachat-core-rust/issues/6401

[link2xt]

I moved the test into #6404 and plan to fix it there.