Merge pull request #2743 from deltachat/improve-gossip

Optimize Autocrypt gossip

python/src/deltachat/chat.py

@@ -3,7 +3,7 @@
 import mimetypes
 import calendar
 import json
-from datetime import datetime
+from datetime import datetime, timezone
 import os
 from .cutil import as_dc_charpointer, from_dc_charpointer, iter_array
 from .capi import lib, ffi
@@ -512,8 +512,9 @@ class Chat(object):
                 latitude=lib.dc_array_get_latitude(dc_array, i),
                 longitude=lib.dc_array_get_longitude(dc_array, i),
                 accuracy=lib.dc_array_get_accuracy(dc_array, i),
-                timestamp=datetime.utcfromtimestamp(
+                timestamp=datetime.fromtimestamp(
-                    lib.dc_array_get_timestamp(dc_array, i)
+                    lib.dc_array_get_timestamp(dc_array, i),
+                    timezone.utc
                 ),
                 marker=from_dc_charpointer(lib.dc_array_get_marker(dc_array, i)),
             )

python/src/deltachat/cutil.py

@@ -1,6 +1,6 @@
 from .capi import lib
 from .capi import ffi
-from datetime import datetime
+from datetime import datetime, timezone
 
 
 def as_dc_charpointer(obj):
@@ -44,4 +44,4 @@ class DCLot:
         ts = lib.dc_lot_get_timestamp(self._dc_lot)
         if ts == 0:
             return None
-        return datetime.utcfromtimestamp(ts)
+        return datetime.fromtimestamp(ts, timezone.utc)

python/src/deltachat/message.py

@@ -6,7 +6,7 @@ from . import props
 from .cutil import from_dc_charpointer, as_dc_charpointer
 from .capi import lib, ffi
 from . import const
-from datetime import datetime
+from datetime import datetime, timezone
 
 
 class Message(object):
@@ -170,7 +170,7 @@ class Message(object):
         :returns: naive datetime.datetime() object.
         """
         ts = lib.dc_msg_get_timestamp(self._dc_msg)
-        return datetime.utcfromtimestamp(ts)
+        return datetime.fromtimestamp(ts, timezone.utc)
 
     @props.with_doc
     def time_received(self):
@@ -180,7 +180,7 @@ class Message(object):
         """
         ts = lib.dc_msg_get_received_timestamp(self._dc_msg)
         if ts:
-            return datetime.utcfromtimestamp(ts)
+            return datetime.fromtimestamp(ts, timezone.utc)
 
     @props.with_doc
     def ephemeral_timer(self):
@@ -200,7 +200,7 @@ class Message(object):
         """
         ts = lib.dc_msg_get_ephemeral_timestamp(self._dc_msg)
         if ts:
-            return datetime.utcfromtimestamp(ts)
+            return datetime.fromtimestamp(ts, timezone.utc)
 
     @property
     def quoted_text(self):

python/tests/test_account.py

@@ -10,7 +10,7 @@ from deltachat.tracker import ImexTracker
 from deltachat.hookspec import account_hookimpl
 from deltachat.capi import ffi, lib
 from deltachat.cutil import iter_array
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
 
 
 @pytest.mark.parametrize("msgtext,res", [
@@ -447,7 +447,7 @@ class TestOfflineChat:
         contact1.create_chat().send_text("hello")
 
     def test_chat_message_distinctions(self, ac1, chat1):
-        past1s = datetime.utcnow() - timedelta(seconds=1)
+        past1s = datetime.now(timezone.utc) - timedelta(seconds=1)
         msg = chat1.send_text("msg1")
         ts = msg.time_sent
         assert msg.time_received is None
@@ -1221,6 +1221,40 @@ class TestOnlineAccount:
         assert not msg.is_encrypted()
         ac1._evtracker.get_matching("DC_EVENT_SMTP_MESSAGE_SENT")
 
+    def test_gossip_optimization(self, 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_many_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_gossip_encryption_preference(self, acfactory, lp):
         """Test that encryption preference of group members is gossiped to new members.
         This is a Delta Chat extension to Autocrypt 1.1.0, which Autocrypt-Gossip headers
@@ -2163,7 +2197,7 @@ class TestOnlineAccount:
                 break  # DC is done with reading messages
 
     def test_send_receive_locations(self, acfactory, lp):
-        now = datetime.utcnow()
+        now = datetime.now(timezone.utc)
         ac1, ac2 = acfactory.get_two_online_accounts()
 
         lp.sec("ac1: create chat with ac2")

src/chat.rs

@@ -397,7 +397,7 @@ impl ChatId {
         context.emit_event(EventType::ChatModified(self));
 
         // make sure, the receivers will get all keys
-        reset_gossiped_timestamp(context, self).await?;
+        self.reset_gossiped_timestamp(context).await?;
 
         Ok(())
     }
@@ -857,6 +857,48 @@ impl ChatId {
     pub fn to_u32(self) -> u32 {
         self.0
     }
+
+    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=?;",
+                paramsv![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=?;",
+                paramsv![timestamp, self],
+            )
+            .await?;
+
+        Ok(())
+    }
 }
 
 impl std::fmt::Display for ChatId {
@@ -1057,10 +1099,6 @@ impl Chat {
         Ok(None)
     }
 
-    pub async fn get_gossiped_timestamp(&self, context: &Context) -> Result<i64> {
-        get_gossiped_timestamp(context, self.id).await
-    }
-
     pub async fn get_color(&self, context: &Context) -> Result<u32> {
         let mut color = 0;
 
@@ -1093,7 +1131,7 @@ impl Chat {
             name: self.name.clone(),
             archived: self.visibility == ChatVisibility::Archived,
             param: self.param.to_string(),
-            gossiped_timestamp: self.get_gossiped_timestamp(context).await?,
+            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
@@ -2371,7 +2409,7 @@ pub(crate) async fn add_contact_to_chat_ex(
     let contact = Contact::get_by_id(context, contact_id).await?;
     let mut msg = Message::default();
 
-    reset_gossiped_timestamp(context, chat_id).await?;
+    chat_id.reset_gossiped_timestamp(context).await?;
 
     /*this also makes sure, not contacts are added to special or normal chats*/
     let mut chat = Chat::load_from_db(context, chat_id).await?;
@@ -2453,45 +2491,6 @@ pub(crate) async fn add_contact_to_chat_ex(
     Ok(true)
 }
 
-pub(crate) async fn reset_gossiped_timestamp(context: &Context, chat_id: ChatId) -> Result<()> {
-    set_gossiped_timestamp(context, chat_id, 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(context: &Context, chat_id: ChatId) -> Result<i64> {
-    let timestamp: Option<i64> = context
-        .sql
-        .query_get_value(
-            "SELECT gossiped_timestamp FROM chats WHERE id=?;",
-            paramsv![chat_id],
-        )
-        .await?;
-    Ok(timestamp.unwrap_or_default())
-}
-
-pub(crate) async fn set_gossiped_timestamp(
-    context: &Context,
-    chat_id: ChatId,
-    timestamp: i64,
-) -> Result<()> {
-    ensure!(!chat_id.is_special(), "can not add member to special chats");
-    info!(
-        context,
-        "set gossiped_timestamp for chat #{} to {}.", chat_id, timestamp,
-    );
-
-    context
-        .sql
-        .execute(
-            "UPDATE chats SET gossiped_timestamp=? WHERE id=?;",
-            paramsv![timestamp, chat_id],
-        )
-        .await?;
-
-    Ok(())
-}
-
 pub(crate) async fn shall_attach_selfavatar(context: &Context, chat_id: ChatId) -> Result<bool> {
     // versions before 12/2019 already allowed to set selfavatar, however, it was never sent to others.
     // to avoid sending out previously set selfavatars unexpectedly we added this additional check.

src/dc_receive_imf.rs

@@ -212,6 +212,26 @@ pub(crate) async fn dc_receive_imf_inner(
     .await
     .map_err(|err| err.context("add_parts error"))?;
 
+    // Update gossiped timestamp for the chat if someone else or our other device sent
+    // Autocrypt-Gossip for all recipients in the chat to avoid sending Autocrypt-Gossip ourselves
+    // and waste traffic.
+    if !chat_id.is_special()
+        && mime_parser
+            .recipients
+            .iter()
+            .all(|recipient| mime_parser.gossiped_addr.contains(&recipient.addr))
+    {
+        info!(
+            context,
+            "Received message contains Autocrypt-Gossip for all members, updating timestamp."
+        );
+        if chat_id.get_gossiped_timestamp(context).await? < sent_timestamp {
+            chat_id
+                .set_gossiped_timestamp(context, sent_timestamp)
+                .await?;
+        }
+    }
+
     let insert_msg_id = if let Some((_chat_id, msg_id)) = created_db_entries.last() {
         *msg_id
     } else {
@@ -2044,7 +2064,7 @@ async fn check_verified_properties(
         let peerstate = Peerstate::from_addr(context, &to_addr).await?;
 
         // mark gossiped keys (if any) as verified
-        if mimeparser.gossipped_addr.contains(&to_addr) {
+        if mimeparser.gossiped_addr.contains(&to_addr) {
             if let Some(mut peerstate) = peerstate {
                 // if we're here, we know the gossip key is verified:
                 // - use the gossip-key as verified-key if there is no verified-key

src/job.rs

@@ -991,7 +991,7 @@ pub async fn send_msg_job(context: &Context, msg_id: MsgId) -> Result<Option<Job
     }
 
     if rendered_msg.is_gossiped {
-        chat::set_gossiped_timestamp(context, msg.chat_id, time()).await?;
+        msg.chat_id.set_gossiped_timestamp(context, time()).await?;
     }
 
     if 0 != rendered_msg.last_added_location_id {

src/mimefactory.rs

@@ -361,7 +361,7 @@ impl<'a> MimeFactory<'a> {
         match &self.loaded {
             Loaded::Message { chat } => {
                 // beside key- and member-changes, force re-gossip every 48 hours
-                let gossiped_timestamp = chat.get_gossiped_timestamp(context).await?;
+                let gossiped_timestamp = chat.id.get_gossiped_timestamp(context).await?;
                 if time() > gossiped_timestamp + (2 * 24 * 60 * 60) {
                     Ok(true)
                 } else {

src/mimeparser.rs

@@ -57,7 +57,9 @@ pub struct MimeMessage {
     /// this set is empty.
     pub signatures: HashSet<Fingerprint>,
 
-    pub gossipped_addr: HashSet<String>,
+    /// The set of mail recipient addresses for which gossip headers were applied, regardless of
+    /// whether they modified any peerstates.
+    pub gossiped_addr: HashSet<String>,
     pub is_forwarded: bool,
     pub is_system_message: SystemMessage,
     pub location_kml: Option<location::Kml>,
@@ -198,7 +200,7 @@ impl MimeMessage {
 
         // Memory location for a possible decrypted message.
         let mut mail_raw = Vec::new();
-        let mut gossipped_addr = Default::default();
+        let mut gossiped_addr = Default::default();
 
         let (mail, signatures, warn_empty_signature) =
             match e2ee::try_decrypt(context, &mail, message_time).await {
@@ -221,7 +223,7 @@ impl MimeMessage {
                         if !signatures.is_empty() {
                             let gossip_headers =
                                 decrypted_mail.headers.get_all_values("Autocrypt-Gossip");
-                            gossipped_addr = update_gossip_peerstates(
+                            gossiped_addr = update_gossip_peerstates(
                                 context,
                                 message_time,
                                 &mail,
@@ -279,7 +281,7 @@ impl MimeMessage {
 
             // only non-empty if it was a valid autocrypt message
             signatures,
-            gossipped_addr,
+            gossiped_addr,
             is_forwarded: false,
             mdn_reports: Vec::new(),
             is_system_message: SystemMessage::Unknown,
@@ -1380,6 +1382,9 @@ impl MimeMessage {
     }
 }
 
+/// Parses `Autocrypt-Gossip` headers from the email and applies them to peerstates.
+///
+/// Returns the set of mail recipient addresses for which valid gossip headers were found.
 async fn update_gossip_peerstates(
     context: &Context,
     message_time: i64,
@@ -1387,42 +1392,46 @@ async fn update_gossip_peerstates(
     gossip_headers: Vec<String>,
 ) -> Result<HashSet<String>> {
     // XXX split the parsing from the modification part
-    let mut gossipped_addr: HashSet<String> = Default::default();
+    let mut gossiped_addr: HashSet<String> = Default::default();
 
     for value in &gossip_headers {
-        let gossip_header = value.parse::<Aheader>();
+        let header = match value.parse::<Aheader>() {
-
+            Ok(header) => header,
-        if let Ok(ref header) = gossip_header {
+            Err(err) => {
-            if get_recipients(&mail.headers)
+                warn!(context, "Failed parsing Autocrypt-Gossip header: {}", err);
-                .iter()
+                continue;
-                .any(|info| info.addr == header.addr.to_lowercase())
-            {
-                let mut peerstate = Peerstate::from_addr(context, &header.addr).await?;
-                if let Some(ref mut peerstate) = peerstate {
-                    peerstate.apply_gossip(header, message_time);
-                    peerstate.save_to_db(&context.sql, false).await?;
-                } else {
-                    let p = Peerstate::from_gossip(header, message_time);
-                    p.save_to_db(&context.sql, true).await?;
-                    peerstate = Some(p);
-                }
-                if let Some(peerstate) = peerstate {
-                    peerstate
-                        .handle_fingerprint_change(context, message_time)
-                        .await?;
-                }
-
-                gossipped_addr.insert(header.addr.clone());
-            } else {
-                warn!(
-                    context,
-                    "Ignoring gossipped \"{}\" as the address is not in To/Cc list.", &header.addr,
-                );
             }
+        };
+
+        if !get_recipients(&mail.headers)
+            .iter()
+            .any(|info| info.addr == header.addr.to_lowercase())
+        {
+            warn!(
+                context,
+                "Ignoring gossiped \"{}\" as the address is not in To/Cc list.", &header.addr,
+            );
+            continue;
         }
+
+        let peerstate;
+        if let Some(mut p) = Peerstate::from_addr(context, &header.addr).await? {
+            p.apply_gossip(&header, message_time);
+            p.save_to_db(&context.sql, false).await?;
+            peerstate = p;
+        } else {
+            let p = Peerstate::from_gossip(&header, message_time);
+            p.save_to_db(&context.sql, true).await?;
+            peerstate = p;
+        };
+        peerstate
+            .handle_fingerprint_change(context, message_time)
+            .await?;
+
+        gossiped_addr.insert(header.addr.clone());
     }
 
-    Ok(gossipped_addr)
+    Ok(gossiped_addr)
 }
 
 #[derive(Debug)]