feat: track gossiping per (chat, fingerprint) pair

This change simplifies
updating the gossip timestamps
when we receive a message
because we only need to know
the keys received in Autocrypt-Gossip
header and which chat the message is
assigned to.
We no longer need to iterate
over the member list.

This is a preparation
for PGP contacts
and member lists that contain
key fingerprints rather than
email addresses.

This change also removes encryption preference
from Autocrypt-Gossip header.
It SHOULD NOT be gossiped
according to the Autocrypt specification
and we ignore encryption preference anyway
since 1.157.0.

test_gossip_optimization is removed
because it relied on a per-chat gossip_timestamp.

Author: link2xt

python/tests/test_0_complex_or_slow.py

@@ -16,8 +16,6 @@ def test_group_many_members_add_leave_remove(self, acfactory, lp):
         lp.sec("ac1: send message to new group chat")
         msg1 = chat.send_text("hello")
         assert msg1.is_encrypted()
-        gossiped_timestamp = chat.get_summary()["gossiped_timestamp"]
-        assert gossiped_timestamp > 0
 
         assert chat.num_contacts() == 3 + 1
 
@@ -46,19 +44,13 @@ def test_group_many_members_add_leave_remove(self, acfactory, lp):
         assert to_remove.addr in sysmsg.text
         assert sysmsg.chat.num_contacts() == 3
 
-        # Receiving message about removed contact does not reset gossip
-        assert chat.get_summary()["gossiped_timestamp"] == gossiped_timestamp
-
         lp.sec("ac1: sending another message to the chat")
         chat.send_text("hello2")
         msg = ac2._evtracker.wait_next_incoming_message()
         assert msg.text == "hello2"
-        assert chat.get_summary()["gossiped_timestamp"] == gossiped_timestamp
 
         lp.sec("ac1: adding fifth member to the chat")
         chat.add_contact(ac5)
-        # Adding contact to chat resets gossiped_timestamp
-        assert chat.get_summary()["gossiped_timestamp"] >= gossiped_timestamp
 
         lp.sec("ac2: receiving system message about contact addition")
         sysmsg = ac2._evtracker.wait_next_incoming_message()

python/tests/test_1_online.py

@@ -798,12 +798,6 @@ def test_send_and_receive_will_encrypt_decrypt(acfactory, lp):
     msg3.mark_seen()
     assert not list(ac1.get_fresh_messages())
 
-    # Test that we do not gossip peer keys in 1-to-1 chat,
-    # as it makes no sense to gossip to peers their own keys.
-    # Gossip is only sent in encrypted messages,
-    # and we sent encrypted msg_back right above.
-    assert chat2b.get_summary()["gossiped_timestamp"] == 0
-
     lp.sec("create group chat with two members, one of which has no encrypt state")
     chat = ac1.create_group_chat("encryption test")
     chat.add_contact(ac2)
@@ -813,41 +807,6 @@ def test_send_and_receive_will_encrypt_decrypt(acfactory, lp):
     ac1._evtracker.get_matching("DC_EVENT_SMTP_MESSAGE_SENT")
 
 
-def test_gossip_optimization(acfactory, lp):
-    """Test that gossip timestamp is updated when someone else sends gossip,
-    so we don't have to send gossip ourselves.
-    """
-    ac1, ac2, ac3 = acfactory.get_online_accounts(3)
-
-    acfactory.introduce_each_other([ac1, ac2])
-    acfactory.introduce_each_other([ac2, ac3])
-
-    lp.sec("ac1 creates a group chat with ac2")
-    group_chat = ac1.create_group_chat("hello")
-    group_chat.add_contact(ac2)
-    msg = group_chat.send_text("hi")
-
-    # No Autocrypt gossip was sent yet.
-    gossiped_timestamp = msg.chat.get_summary()["gossiped_timestamp"]
-    assert gossiped_timestamp == 0
-
-    msg = ac2._evtracker.wait_next_incoming_message()
-    assert msg.is_encrypted()
-    assert msg.text == "hi"
-
-    lp.sec("ac2 adds ac3 to the group")
-    msg.chat.add_contact(ac3)
-
-    lp.sec("ac1 receives message from ac2 and updates gossip timestamp")
-    msg = ac1._evtracker.wait_next_incoming_message()
-    assert msg.is_encrypted()
-
-    # ac1 has updated the gossip timestamp even though no gossip was sent by ac1.
-    # ac1 does not need to send gossip because ac2 already did it.
-    gossiped_timestamp = msg.chat.get_summary()["gossiped_timestamp"]
-    assert gossiped_timestamp == int(msg.time_sent.timestamp())
-
-
 def test_send_first_message_as_long_unicode_with_cr(acfactory, lp):
     ac1, ac2 = acfactory.get_online_accounts(2)
 

src/chat.rs

@@ -1376,41 +1376,10 @@ impl ChatId {
     }
 
     pub(crate) async fn reset_gossiped_timestamp(self, context: &Context) -> Result<()> {
-        self.set_gossiped_timestamp(context, 0).await
-    }
-
-    /// Get timestamp of the last gossip sent in the chat.
-    /// Zero return value means that gossip was never sent.
-    pub async fn get_gossiped_timestamp(self, context: &Context) -> Result<i64> {
-        let timestamp: Option<i64> = context
-            .sql
-            .query_get_value("SELECT gossiped_timestamp FROM chats WHERE id=?;", (self,))
-            .await?;
-        Ok(timestamp.unwrap_or_default())
-    }
-
-    pub(crate) async fn set_gossiped_timestamp(
-        self,
-        context: &Context,
-        timestamp: i64,
-    ) -> Result<()> {
-        ensure!(
-            !self.is_special(),
-            "can not set gossiped timestamp for special chats"
-        );
-        info!(
-            context,
-            "Set gossiped_timestamp for chat {} to {}.", self, timestamp,
-        );
-
         context
             .sql
-            .execute(
-                "UPDATE chats SET gossiped_timestamp=? WHERE id=?;",
-                (timestamp, self),
-            )
+            .execute("DELETE FROM gossip_timestamp WHERE chat_id=?", (self,))
             .await?;
-
         Ok(())
     }
 
@@ -1917,7 +1886,6 @@ impl Chat {
             name: self.name.clone(),
             archived: self.visibility == ChatVisibility::Archived,
             param: self.param.to_string(),
-            gossiped_timestamp: self.id.get_gossiped_timestamp(context).await?,
             is_sending_locations: self.is_sending_locations,
             color: self.get_color(context).await?,
             profile_image: self
@@ -2460,9 +2428,6 @@ pub struct ChatInfo {
     /// This is the string-serialised version of `Params` currently.
     pub param: String,
 
-    /// Last time this client sent autocrypt gossip headers to this chat.
-    pub gossiped_timestamp: i64,
-
     /// Whether this chat is currently sending location-stream messages.
     pub is_sending_locations: bool,
 
@@ -3101,10 +3066,6 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) -
 
     let now = smeared_time(context);
 
-    if rendered_msg.is_gossiped {
-        msg.chat_id.set_gossiped_timestamp(context, now).await?;
-    }
-
     if rendered_msg.last_added_location_id.is_some() {
         if let Err(err) = location::set_kml_sent_timestamp(context, msg.chat_id, now).await {
             error!(context, "Failed to set kml sent_timestamp: {err:#}.");

src/mimefactory.rs

@@ -13,6 +13,7 @@ use mail_builder::headers::HeaderType;
 use mail_builder::mime::MimePart;
 use tokio::fs;
 
+use crate::aheader::{Aheader, EncryptPreference};
 use crate::blob::BlobObject;
 use crate::chat::{self, Chat};
 use crate::config::Config;
@@ -22,6 +23,7 @@ use crate::contact::{Contact, ContactId, Origin};
 use crate::context::Context;
 use crate::e2ee::EncryptHelper;
 use crate::ephemeral::Timer as EphemeralTimer;
+use crate::key::DcKey;
 use crate::location;
 use crate::message::{self, Message, MsgId, Viewtype};
 use crate::mimeparser::{is_hidden, SystemMessage};
@@ -131,7 +133,6 @@ pub struct RenderedEmail {
     pub message: String,
     // pub envelope: Envelope,
     pub is_encrypted: bool,
-    pub is_gossiped: bool,
     pub last_added_location_id: Option<u32>,
 
     /// A comma-separated string of sync-IDs that are used by the rendered email and must be deleted
@@ -433,41 +434,6 @@ impl MimeFactory {
         }
     }
 
-    async fn should_do_gossip(&self, context: &Context, multiple_recipients: bool) -> Result<bool> {
-        match &self.loaded {
-            Loaded::Message { chat, msg } => {
-                if chat.typ == Chattype::Broadcast {
-                    // Never send Autocrypt-Gossip in broadcast lists
-                    // as it discloses recipient email addresses.
-                    return Ok(false);
-                }
-
-                let cmd = msg.param.get_cmd();
-                if cmd == SystemMessage::MemberAddedToGroup
-                    || cmd == SystemMessage::SecurejoinMessage
-                {
-                    Ok(true)
-                } else if multiple_recipients {
-                    // beside key- and member-changes, force a periodic re-gossip.
-                    let gossiped_timestamp = chat.id.get_gossiped_timestamp(context).await?;
-                    let gossip_period = context.get_config_i64(Config::GossipPeriod).await?;
-                    // `gossip_period == 0` is a special case for testing,
-                    // enabling gossip in every message.
-                    // Otherwise "smeared timestamps" may result in the condition
-                    // to fail even if the clock is monotonic.
-                    if gossip_period == 0 || time() >= gossiped_timestamp + gossip_period {
-                        Ok(true)
-                    } else {
-                        Ok(false)
-                    }
-                } else {
-                    Ok(false)
-                }
-            }
-            Loaded::Mdn { .. } => Ok(false),
-        }
-    }
-
     fn should_attach_profile_data(msg: &Message) -> bool {
         msg.param.get_cmd() != SystemMessage::SecurejoinMessage || {
             let step = msg.param.get(Param::Arg).unwrap_or_default();
@@ -787,8 +753,6 @@ impl MimeFactory {
             }
         }
 
-        let mut is_gossiped = false;
-
         let peerstates = self.peerstates_for_recipients(context).await?;
         let is_encrypted = !self.should_force_plaintext()
             && (e2ee_guaranteed || encrypt_helper.should_encrypt(context, &peerstates).await?);
@@ -956,16 +920,78 @@ impl MimeFactory {
             // Add gossip headers in chats with multiple recipients
             let multiple_recipients =
                 peerstates.len() > 1 || context.get_config_bool(Config::BccSelf).await?;
-            if self.should_do_gossip(context, multiple_recipients).await? {
-                for peerstate in peerstates.iter().filter_map(|(state, _)| state.as_ref()) {
-                    if let Some(header) = peerstate.render_gossip_header(verified) {
-                        message = message.header(
-                            "Autocrypt-Gossip",
-                            mail_builder::headers::raw::Raw::new(header),
-                        );
-                        is_gossiped = true;
+
+            let gossip_period = context.get_config_i64(Config::GossipPeriod).await?;
+            let now = time();
+
+            match &self.loaded {
+                Loaded::Message { chat, msg } => {
+                    if chat.typ != Chattype::Broadcast {
+                        for peerstate in peerstates.iter().filter_map(|(state, _)| state.as_ref()) {
+                            let Some(key) = peerstate.peek_key(verified) else {
+                                continue;
+                            };
+
+                            let fingerprint = key.dc_fingerprint().hex();
+                            let cmd = msg.param.get_cmd();
+                            let should_do_gossip = cmd == SystemMessage::MemberAddedToGroup
+                                || cmd == SystemMessage::SecurejoinMessage
+                                || multiple_recipients && {
+                                    let gossiped_timestamp: Option<i64> = context
+                                        .sql
+                                        .query_get_value(
+                                            "SELECT timestamp
+                                         FROM gossip_timestamp
+                                         WHERE chat_id=? AND fingerprint=?",
+                                            (chat.id, &fingerprint),
+                                        )
+                                        .await?;
+
+                                    // `gossip_period == 0` is a special case for testing,
+                                    // enabling gossip in every message.
+                                    //
+                                    // If current time is in the past compared to
+                                    // `gossiped_timestamp`, we also gossip because
+                                    // either the `gossiped_timestamp` or clock is wrong.
+                                    gossip_period == 0
+                                        || gossiped_timestamp
+                                            .is_none_or(|ts| now >= ts + gossip_period || now < ts)
+                                };
+
+                            if !should_do_gossip {
+                                continue;
+                            }
+
+                            let header = Aheader::new(
+                                peerstate.addr.clone(),
+                                key.clone(),
+                                // Autocrypt 1.1.0 specification says that
+                                // `prefer-encrypt` attribute SHOULD NOT be included.
+                                EncryptPreference::NoPreference,
+                            )
+                            .to_string();
+
+                            message = message.header(
+                                "Autocrypt-Gossip",
+                                mail_builder::headers::raw::Raw::new(header),
+                            );
+
+                            context
+                                .sql
+                                .execute(
+                                    "INSERT INTO gossip_timestamp (chat_id, fingerprint, timestamp)
+                                     VALUES                       (?, ?, ?)
+                                     ON CONFLICT                  (chat_id, fingerprint)
+                                     DO UPDATE SET timestamp=excluded.timestamp",
+                                    (chat.id, &fingerprint, now),
+                                )
+                                .await?;
+                        }
                     }
                 }
+                Loaded::Mdn { .. } => {
+                    // Never gossip in MDNs.
+                }
             }
 
             // Set the appropriate Content-Type for the inner message.
@@ -1109,7 +1135,6 @@ impl MimeFactory {
             message,
             // envelope: Envelope::new,
             is_encrypted,
-            is_gossiped,
             last_added_location_id,
             sync_ids_to_delete: self.sync_ids_to_delete,
             rfc724_mid,

src/peerstate.rs

@@ -405,28 +405,6 @@ impl Peerstate {
         };
     }
 
-    /// Returns the contents of the `Autocrypt-Gossip` header for outgoing messages.
-    pub fn render_gossip_header(&self, verified: bool) -> Option<String> {
-        if let Some(key) = self.peek_key(verified) {
-            let header = Aheader::new(
-                self.addr.clone(),
-                key.clone(), // TODO: avoid cloning
-                // Autocrypt 1.1.0 specification says that
-                // `prefer-encrypt` attribute SHOULD NOT be included,
-                // but we include it anyway to propagate encryption
-                // preference to new members in group chats.
-                if self.last_seen_autocrypt > 0 {
-                    self.prefer_encrypt
-                } else {
-                    EncryptPreference::NoPreference
-                },
-            );
-            Some(header.to_string())
-        } else {
-            None
-        }
-    }
-
     /// Converts the peerstate into the contact public key.
     ///
     /// Similar to [`Self::peek_key`], but consumes the peerstate and returns owned key.