Channels not using CRDT-like group synchronization may result in losing members in multi-device setups

[link2xt]

If you have two devices that handle QR invites into the channel, and Bob1 subscribes via device 1 and then Bob2 subscribes via device 2 at the same time, device 1 and device 2 will each send a sync message with only Bob1 or Bob2:

core/src/securejoin.rs

Lines 655 to 659 in 5f84be7

	if chat.typ == Chattype::OutBroadcast {
	// We don't use the membership consistency algorithm for broadcast channels,
	// so, sync the memberlist when adding a contact
	chat.sync_contacts(context).await.log_err(context).ok();
	} else {

These SetPgpContacts messages do not have per-member timestamps, so they are not merged, but overwrite the whole chatlist:

core/src/chat.rs

Lines 5107 to 5110 in 5f84be7

	/// Set chat contacts by their fingerprints.
	///
	/// The list is a list of pairs of fingerprint and address.
	SetPgpContacts(Vec<(String, String)>),

Slightly related to just opened issue #7952

[iequidoo]

See set_contacts_by_fingerprints():

        .transaction(move |transaction| {
            // For broadcast channels, we only add members,                                                                                                                                                          
            // because we don't use the membership consistency algorithm,                                                                                                                                            
            // and are using sync messages as a basic way to ensure consistency between devices.                                                                                                                     
            // For groups, we also remove members,                                                                                                                                                                   
            // because the sync message is used in order to sync unpromoted groups.                                                                                                                                  
            if chat.typ != Chattype::OutBroadcast {
                transaction.execute("DELETE FROM chats_contacts WHERE chat_id=?", (id,))?;
            }

So the problem isn't losing members, but readding a member which just left, if another member is being added at the same time.

[link2xt]

It looks like sync_contacts is only used in broadcast lists since it was introduced in 4d1a9c2
Back then contacts were added manually by the broadcast list owner and it was somewhat ok to sync like this because owner added contacts slowly and usually from the same device.

Now channels reused sync_contacts here in securejoin, and securejoin messages are processed by a random device that is the first to pick the message, or maybe both:

core/src/securejoin.rs

Lines 659 to 661 in bdc9e7c

	// We don't use the membership consistency algorithm for broadcast channels,
	// so, sync the memberlist when adding a contact
	chat.sync_contacts(context).await.log_err(context).ok();

I am also not sure if sync_contacts is used for unencrypted chats. If it is not used, this whole branch and SetContacts can be removed:

core/src/chat.rs

Lines 2043 to 2057 in bdc9e7c

	let addrs = context
	.sql
	.query_map_vec(
	"SELECT c.addr \
	FROM contacts c INNER JOIN chats_contacts cc \
	ON c.id=cc.contact_id \
	WHERE cc.chat_id=? AND cc.add_timestamp >= cc.remove_timestamp",
	(self.id,),
	|row| {
	let addr: String = row.get(0)?;
	Ok(addr)
	},
	)
	.await?;
	self.sync(context, SyncAction::SetContacts(addrs)).await?;

SetPgpContacts sync message should also be deprecated. It has the problem of not having display names and this results in displaying email addresses instead: #8011

So what should roughly be done is:

1. Remove SetContacts
2. Stop sending SetPgpContacts but process them for now for compatibility.
3. Create a new message for syncing channel memberlist with addresses, fingerprints, display names and timestamps of addition and removal (for past members) and use it instead.

There is a separate issue that second device does not have the key of the user and as a result does not have the relay list, so multi-relay is not working. Also if user removes the relay channel owner sends to, user will stop receiving messages completely. Some users send read receipts and this is how channel owner gets the keys eventually, but nothing we can do if user does not ever send anything back. Let's say this is out of scope for the issue.

[iequidoo]

It looks like sync_contacts is only used in broadcast lists since it was introduced in 4d1a9c2

It's used for unpromoted groups also. Anyway, it sends SetPgpContacts for them.

Create a new message for syncing channel memberlist with addresses, fingerprints, display names and timestamps of addition and removal (for past members) and use it instead.

Display names are a derivative of authnames and user-set names, it seems that we need to only sync authnames because if it's a new contact, it doesn't have a user-set name yet. For syncing user-set names there's chat::SyncAction::Rename.

As for past members, we don't maintain them for broadcasts, see 29c57ad.

[link2xt]

Display names are a derivative of authnames and user-set names, it seems that we need to only sync authnames because if it's a new contact, it doesn't have a user-set name yet. For syncing user-set names there's chat::SyncAction::Rename.

I agree, what needs synchronizing is the same as what goes to the To header for groups, that's authname.

As for past members, we don't maintain them for broadcasts, see 29c57ad.

We probably need to revert this, otherwise users will not be able to leave the channel reliably.

[link2xt]

It's used for unpromoted groups also. Anyway, it sends SetPgpContacts for them.

Is it actually useful? The other device does not know about unpromoted group, so the message is ignored, right?

[iequidoo]

Is it actually useful? The other device does not know about unpromoted group, so the message is ignored, right?

It works somehow, i can create an encrypted group on one device and e.g. add members on another, and they are synced. IIRC SyncAction::CreateGroupEncrypted is used to create unpromoted groups on the 2nd device.
...
I find it a bit strange UX-wise because we don't sync message drafts, but "group drafts" (unpromoted groups) are synced. Just a note, i'm not suggesting to change this.

[link2xt]

So SetContacts and SetPgpContacts are left for unpromoted groups, no need to change anything about them.

But channels need a new sync message with a list of members with address, fingerprints, authname and addition timestamp, and a list of past member fingerprints and removal timestamps. All the same things that are normally sent in groups via headers, but not sent in channels as we don't want to tell members about each other. We will also need to store past members for channels and revert 29c57ad.

[iequidoo]

We will also need to store past members for channels and revert 29c57ad.

Not sure this is the right thing to do, there's a comment:

    // Alice must not remember old members,
    // because we would like to remember the minimum information possible

Considering that Alice's db may be stolen, broadcasts are safer for joining than groups. CC @Hocuri
...
Maybe to avoid storing contacts who left a group/broadcast in the db for a long time we should create a separate table past_members (h, remove_timestamp) where h is a hash of concatenated group id and the contact fingerprint? So if a db is stolen, at least for contacts who aren't in the db anymore it's impossible to identify them.