Merge branch 'link2xt/peerstate-to-save-simplify'

CHANGELOG.md

@@ -11,6 +11,7 @@
 - Make sure malformed messsages will never block receiving further messages anymore #3771
 - strip leading/trailing whitespace from "Chat-Group-Name{,-Changed}:" headers content #3650
 - Assume all Thunderbird users prefer encryption #3774
+- refactor peerstate handling to ensure no duplicate peerstates #3776
 
 
 ## 1.102.0

src/decrypt.rs

@@ -306,7 +306,7 @@ pub(crate) async fn get_autocrypt_peerstate(
             if addr_cmp(&peerstate.addr, from) {
                 if allow_change {
                     peerstate.apply_header(header, message_time);
-                    peerstate.save_to_db(&context.sql, false).await?;
+                    peerstate.save_to_db(&context.sql).await?;
                 } else {
                     info!(
                         context,
@@ -322,7 +322,7 @@ pub(crate) async fn get_autocrypt_peerstate(
             // to the database.
         } else {
             let p = Peerstate::from_header(header, message_time);
-            p.save_to_db(&context.sql, true).await?;
+            p.save_to_db(&context.sql).await?;
             peerstate = Some(p);
         }
     } else {

src/e2ee.rs

@@ -147,7 +147,6 @@ mod tests {
     use crate::chat;
     use crate::message::{Message, Viewtype};
     use crate::param::Param;
-    use crate::peerstate::ToSave;
     use crate::test_utils::{bob_keypair, TestContext};
 
     use super::*;
@@ -297,7 +296,6 @@ Sent with my Delta Chat Messenger: https://delta.chat";
             gossip_key_fingerprint: Some(pub_key.fingerprint()),
             verified_key: Some(pub_key.clone()),
             verified_key_fingerprint: Some(pub_key.fingerprint()),
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
         vec![(Some(peerstate), addr)]

src/mimeparser.rs

@@ -310,7 +310,7 @@ impl MimeMessage {
                         // && decryption_info.dkim_results.allow_keychange
                         {
                             peerstate.degrade_encryption(message_time);
-                            peerstate.save_to_db(&context.sql, false).await?;
+                            peerstate.save_to_db(&context.sql).await?;
                         }
                     }
                     (Ok(mail), HashSet::new(), false)
@@ -1586,11 +1586,11 @@ async fn update_gossip_peerstates(
         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?;
+            p.save_to_db(&context.sql).await?;
             peerstate = p;
         } else {
             let p = Peerstate::from_gossip(&header, message_time);
-            p.save_to_db(&context.sql, true).await?;
+            p.save_to_db(&context.sql).await?;
             peerstate = p;
         };
         peerstate

src/peerstate.rs

@@ -46,7 +46,6 @@ pub struct Peerstate {
     pub gossip_key_fingerprint: Option<Fingerprint>,
     pub verified_key: Option<SignedPublicKey>,
     pub verified_key_fingerprint: Option<Fingerprint>,
-    pub to_save: Option<ToSave>,
     pub fingerprint_changed: bool,
 }
 
@@ -63,7 +62,6 @@ impl PartialEq for Peerstate {
             && self.gossip_key_fingerprint == other.gossip_key_fingerprint
             && self.verified_key == other.verified_key
             && self.verified_key_fingerprint == other.verified_key_fingerprint
-            && self.to_save == other.to_save
             && self.fingerprint_changed == other.fingerprint_changed
     }
 }
@@ -84,19 +82,11 @@ impl fmt::Debug for Peerstate {
             .field("gossip_key_fingerprint", &self.gossip_key_fingerprint)
             .field("verified_key", &self.verified_key)
             .field("verified_key_fingerprint", &self.verified_key_fingerprint)
-            .field("to_save", &self.to_save)
             .field("fingerprint_changed", &self.fingerprint_changed)
             .finish()
     }
 }
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, FromPrimitive, ToPrimitive)]
-#[repr(u8)]
-pub enum ToSave {
-    Timestamps = 0x01,
-    All = 0x02,
-}
-
 impl Peerstate {
     pub fn from_header(header: &Aheader, message_time: i64) -> Self {
         Peerstate {
@@ -111,7 +101,6 @@ impl Peerstate {
             gossip_timestamp: 0,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         }
     }
@@ -137,7 +126,6 @@ impl Peerstate {
             gossip_timestamp: message_time,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         }
     }
@@ -229,7 +217,6 @@ impl Peerstate {
                         .map(|s| s.parse::<Fingerprint>())
                         .transpose()
                         .unwrap_or_default(),
-                    to_save: None,
                     fingerprint_changed: false,
                 };
 
@@ -248,14 +235,10 @@ impl Peerstate {
             let old_public_fingerprint = self.public_key_fingerprint.take();
             self.public_key_fingerprint = Some(public_key.fingerprint());
 
-            if old_public_fingerprint.is_none()
+            if old_public_fingerprint.is_some()
-                || self.public_key_fingerprint.is_none()
+                && old_public_fingerprint != self.public_key_fingerprint
-                || old_public_fingerprint != self.public_key_fingerprint
             {
-                self.to_save = Some(ToSave::All);
+                self.fingerprint_changed = true;
-                if old_public_fingerprint.is_some() {
-                    self.fingerprint_changed = true;
-                }
             }
         }
 
@@ -267,8 +250,6 @@ impl Peerstate {
                 || self.gossip_key_fingerprint.is_none()
                 || old_gossip_fingerprint != self.gossip_key_fingerprint
             {
-                self.to_save = Some(ToSave::All);
-
                 // Warn about gossip key change only if there is no public key obtained from
                 // Autocrypt header, which overrides gossip key.
                 if old_gossip_fingerprint.is_some() && self.public_key_fingerprint.is_none() {
@@ -281,7 +262,6 @@ impl Peerstate {
     pub fn degrade_encryption(&mut self, message_time: i64) {
         self.prefer_encrypt = EncryptPreference::Reset;
         self.last_seen = message_time;
-        self.to_save = Some(ToSave::All);
     }
 
     pub fn apply_header(&mut self, header: &Aheader, message_time: i64) {
@@ -292,19 +272,16 @@ impl Peerstate {
         if message_time > self.last_seen {
             self.last_seen = message_time;
             self.last_seen_autocrypt = message_time;
-            self.to_save = Some(ToSave::Timestamps);
             if (header.prefer_encrypt == EncryptPreference::Mutual
                 || header.prefer_encrypt == EncryptPreference::NoPreference)
                 && header.prefer_encrypt != self.prefer_encrypt
             {
                 self.prefer_encrypt = header.prefer_encrypt;
-                self.to_save = Some(ToSave::All)
             }
 
             if self.public_key.as_ref() != Some(&header.public_key) {
                 self.public_key = Some(header.public_key.clone());
                 self.recalc_fingerprint();
-                self.to_save = Some(ToSave::All);
             }
         }
     }
@@ -316,11 +293,9 @@ impl Peerstate {
 
         if message_time > self.gossip_timestamp {
             self.gossip_timestamp = message_time;
-            self.to_save = Some(ToSave::Timestamps);
             if self.gossip_key.as_ref() != Some(&gossip_header.public_key) {
                 self.gossip_key = Some(gossip_header.public_key.clone());
                 self.recalc_fingerprint();
-                self.to_save = Some(ToSave::All)
             }
 
             // This is non-standard.
@@ -339,7 +314,6 @@ impl Peerstate {
                 && gossip_header.prefer_encrypt == EncryptPreference::Mutual
             {
                 self.prefer_encrypt = EncryptPreference::Mutual;
-                self.to_save = Some(ToSave::All);
             }
         };
     }
@@ -395,7 +369,6 @@ impl Peerstate {
                     if self.public_key_fingerprint.is_some()
                         && self.public_key_fingerprint.as_ref().unwrap() == fingerprint
                     {
-                        self.to_save = Some(ToSave::All);
                         self.verified_key = self.public_key.clone();
                         self.verified_key_fingerprint = self.public_key_fingerprint.clone();
                         true
@@ -407,7 +380,6 @@ impl Peerstate {
                     if self.gossip_key_fingerprint.is_some()
                         && self.gossip_key_fingerprint.as_ref().unwrap() == fingerprint
                     {
-                        self.to_save = Some(ToSave::All);
                         self.verified_key = self.gossip_key.clone();
                         self.verified_key_fingerprint = self.gossip_key_fingerprint.clone();
                         true
@@ -421,66 +393,48 @@ impl Peerstate {
         }
     }
 
-    pub async fn save_to_db(&self, sql: &Sql, create: bool) -> Result<()> {
+    pub async fn save_to_db(&self, sql: &Sql) -> Result<()> {
-        if self.to_save == Some(ToSave::All) || create {
+        sql.execute(
-            sql.execute(
+            "INSERT INTO acpeerstates (
-                if create {
+                last_seen,
-                    "INSERT INTO acpeerstates ( \
+                last_seen_autocrypt,
-                         last_seen, \
+                prefer_encrypted,
-                         last_seen_autocrypt, \
+                public_key,
-                         prefer_encrypted, \
+                gossip_timestamp,
-                         public_key, \
+                gossip_key,
-                         gossip_timestamp, \
+                public_key_fingerprint,
-                         gossip_key, \
+                gossip_key_fingerprint,
-                         public_key_fingerprint, \
+                verified_key,
-                         gossip_key_fingerprint, \
+                verified_key_fingerprint,
-                         verified_key, \
+                addr)
-                         verified_key_fingerprint, \
+                VALUES (?,?,?,?,?,?,?,?,?,?,?)
-                         addr \
+                ON CONFLICT (addr)
-                ) VALUES(?,?,?,?,?,?,?,?,?,?,?)"
+                DO UPDATE SET
-                } else {
+                  last_seen = excluded.last_seen,
-                    "UPDATE acpeerstates \
+                  last_seen_autocrypt = excluded.last_seen_autocrypt,
-                 SET last_seen=?, \
+                  prefer_encrypted = excluded.prefer_encrypted,
-                 last_seen_autocrypt=?, \
+                  public_key = excluded.public_key,
-                 prefer_encrypted=?, \
+                  gossip_timestamp = excluded.gossip_timestamp,
-                 public_key=?, \
+                  gossip_key = excluded.gossip_key,
-                 gossip_timestamp=?, \
+                  public_key_fingerprint = excluded.public_key_fingerprint,
-                 gossip_key=?, \
+                  gossip_key_fingerprint = excluded.gossip_key_fingerprint,
-                 public_key_fingerprint=?, \
+                  verified_key = excluded.verified_key,
-                 gossip_key_fingerprint=?, \
+                  verified_key_fingerprint = excluded.verified_key_fingerprint",
-                 verified_key=?, \
+            paramsv![
-                 verified_key_fingerprint=? \
+                self.last_seen,
-                 WHERE addr=?"
+                self.last_seen_autocrypt,
-                },
+                self.prefer_encrypt as i64,
-                paramsv![
+                self.public_key.as_ref().map(|k| k.to_bytes()),
-                    self.last_seen,
+                self.gossip_timestamp,
-                    self.last_seen_autocrypt,
+                self.gossip_key.as_ref().map(|k| k.to_bytes()),
-                    self.prefer_encrypt as i64,
+                self.public_key_fingerprint.as_ref().map(|fp| fp.hex()),
-                    self.public_key.as_ref().map(|k| k.to_bytes()),
+                self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()),
-                    self.gossip_timestamp,
+                self.verified_key.as_ref().map(|k| k.to_bytes()),
-                    self.gossip_key.as_ref().map(|k| k.to_bytes()),
+                self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()),
-                    self.public_key_fingerprint.as_ref().map(|fp| fp.hex()),
+                self.addr,
-                    self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()),
+            ],
-                    self.verified_key.as_ref().map(|k| k.to_bytes()),
+        )
-                    self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()),
+        .await?;
-                    self.addr,
-                ],
-            )
-            .await?;
-        } else if self.to_save == Some(ToSave::Timestamps) {
-            sql.execute(
-                "UPDATE acpeerstates SET last_seen=?, last_seen_autocrypt=?, gossip_timestamp=? \
-                 WHERE addr=?;",
-                paramsv![
-                    self.last_seen,
-                    self.last_seen_autocrypt,
-                    self.gossip_timestamp,
-                    self.addr
-                ],
-            )
-            .await?;
-        }
-
         Ok(())
     }
 
@@ -651,14 +605,8 @@ pub async fn maybe_do_aeap_transition(
                 "Internal error: Tried to do an AEAP transition without an autocrypt header??",
             )?;
             peerstate.apply_header(header, info.message_time);
-            peerstate.to_save = Some(ToSave::All);
 
-            // We don't know whether a peerstate with this address already existed, or a
+            peerstate.save_to_db(&context.sql).await?;
-            // new one should be created, so just try both create=false and create=true,
-            // and if this fails, create=true, one will succeed (this is a very cold path,
-            // so performance doesn't really matter).
-            peerstate.save_to_db(&context.sql, true).await?;
-            peerstate.save_to_db(&context.sql, false).await?;
         }
     }
 
@@ -710,7 +658,7 @@ mod tests {
 
         let pub_key = alice_keypair().public;
 
-        let mut peerstate = Peerstate {
+        let peerstate = Peerstate {
             addr: addr.into(),
             last_seen: 10,
             last_seen_autocrypt: 11,
@@ -722,12 +670,11 @@ mod tests {
             gossip_key_fingerprint: Some(pub_key.fingerprint()),
             verified_key: Some(pub_key.clone()),
             verified_key_fingerprint: Some(pub_key.fingerprint()),
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
 
         assert!(
-            peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(),
+            peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(),
             "failed to save to db"
         );
 
@@ -736,8 +683,6 @@ mod tests {
             .expect("failed to load peerstate from db")
             .expect("no peerstate found in the database");
 
-        // clear to_save, as that is not persissted
-        peerstate.to_save = None;
         assert_eq!(peerstate, peerstate_new);
         let peerstate_new2 = Peerstate::from_fingerprint(&ctx.ctx, &pub_key.fingerprint())
             .await
@@ -764,16 +709,15 @@ mod tests {
             gossip_key_fingerprint: None,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
 
         assert!(
-            peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(),
+            peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(),
             "failed to save"
         );
         assert!(
-            peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(),
+            peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(),
             "double-call with create failed"
         );
     }
@@ -785,7 +729,7 @@ mod tests {
 
         let pub_key = alice_keypair().public;
 
-        let mut peerstate = Peerstate {
+        let peerstate = Peerstate {
             addr: addr.into(),
             last_seen: 10,
             last_seen_autocrypt: 11,
@@ -797,12 +741,11 @@ mod tests {
             gossip_key_fingerprint: None,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
 
         assert!(
-            peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(),
+            peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(),
             "failed to save"
         );
 
@@ -810,8 +753,6 @@ mod tests {
             .await
             .expect("failed to load peerstate from db");
 
-        // clear to_save, as that is not persissted
-        peerstate.to_save = None;
         assert_eq!(Some(peerstate), peerstate_new);
     }
 
@@ -862,7 +803,6 @@ mod tests {
             gossip_key_fingerprint: None,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: None,
             fingerprint_changed: false,
         };
         assert_eq!(peerstate.prefer_encrypt, EncryptPreference::NoPreference);

src/qr.rs

@@ -641,7 +641,6 @@ mod tests {
     use crate::aheader::EncryptPreference;
     use crate::chat::{create_group_chat, ProtectionStatus};
     use crate::key::DcKey;
-    use crate::peerstate::ToSave;
     use crate::securejoin::get_securejoin_qr;
     use crate::test_utils::{alice_keypair, TestContext};
     use anyhow::Result;
@@ -894,11 +893,10 @@ mod tests {
             gossip_key_fingerprint: None,
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
         assert!(
-            peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(),
+            peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(),
             "failed to save peerstate"
         );
 

src/receive_imf.rs

@@ -2129,7 +2129,7 @@ async fn check_verified_properties(
                             &fp,
                             PeerstateVerifiedStatus::BidirectVerified,
                         );
-                        peerstate.save_to_db(&context.sql, false).await?;
+                        peerstate.save_to_db(&context.sql).await?;
                         is_verified = true;
                     }
                 }

src/securejoin.rs

@@ -18,7 +18,7 @@ use crate::key::{DcKey, Fingerprint, SignedPublicKey};
 use crate::message::{Message, Viewtype};
 use crate::mimeparser::{MimeMessage, SystemMessage};
 use crate::param::Param;
-use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus, ToSave};
+use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus};
 use crate::qr::check_qr;
 use crate::stock_str;
 use crate::token;
@@ -640,11 +640,7 @@ async fn mark_peer_as_verified(context: &Context, fingerprint: &Fingerprint) ->
             PeerstateVerifiedStatus::BidirectVerified,
         ) {
             peerstate.prefer_encrypt = EncryptPreference::Mutual;
-            peerstate.to_save = Some(ToSave::All);
+            peerstate.save_to_db(&context.sql).await.unwrap_or_default();
-            peerstate
-                .save_to_db(&context.sql, false)
-                .await
-                .unwrap_or_default();
             return Ok(());
         }
     }
@@ -932,10 +928,9 @@ mod tests {
             gossip_key_fingerprint: Some(alice_pubkey.fingerprint()),
             verified_key: None,
             verified_key_fingerprint: None,
-            to_save: Some(ToSave::All),
             fingerprint_changed: false,
         };
-        peerstate.save_to_db(&bob.ctx.sql, true).await?;
+        peerstate.save_to_db(&bob.ctx.sql).await?;
 
         // Step 1: Generate QR-code, ChatId(0) indicates setup-contact
         let qr = get_securejoin_qr(&alice.ctx, None).await?;

src/sql.rs

@@ -280,7 +280,7 @@ impl Sql {
             for addr in &addrs {
                 if let Some(ref mut peerstate) = Peerstate::from_addr(context, addr).await? {
                     peerstate.recalc_fingerprint();
-                    peerstate.save_to_db(self, false).await?;
+                    peerstate.save_to_db(self).await?;
                 }
             }
         }

src/sql/migrations.rs

@@ -616,6 +616,39 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid);
         )
         .await?;
     }
+    if dbversion < 94 {
+        sql.execute_migration(
+            // Create new `acpeerstates` table, same as before but with unique constraint.
+            //
+            // This allows to use `UPSERT` to update existing or insert a new peerstate
+            // depending on whether one exists already.
+            "CREATE TABLE new_acpeerstates (
+             id INTEGER PRIMARY KEY,
+             addr TEXT DEFAULT '' COLLATE NOCASE,
+             last_seen INTEGER DEFAULT 0,
+             last_seen_autocrypt INTEGER DEFAULT 0,
+             public_key,
+             prefer_encrypted INTEGER DEFAULT 0,
+             gossip_timestamp INTEGER DEFAULT 0,
+             gossip_key,
+             public_key_fingerprint TEXT DEFAULT '',
+             gossip_key_fingerprint TEXT DEFAULT '',
+             verified_key,
+             verified_key_fingerprint TEXT DEFAULT '',
+             UNIQUE (addr) -- Only one peerstate per address
+             );
+            INSERT OR IGNORE INTO new_acpeerstates SELECT * FROM acpeerstates;
+            DROP TABLE acpeerstates;
+            ALTER TABLE new_acpeerstates RENAME TO acpeerstates;
+            CREATE INDEX acpeerstates_index1 ON acpeerstates (addr);
+            CREATE INDEX acpeerstates_index3 ON acpeerstates (public_key_fingerprint);
+            CREATE INDEX acpeerstates_index4 ON acpeerstates (gossip_key_fingerprint);
+            CREATE INDEX acpeerstates_index5 ON acpeerstates (verified_key_fingerprint);
+            ",
+            94,
+        )
+        .await?;
+    }
 
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)

src/tests/aeap.rs

@@ -341,9 +341,8 @@ async fn mark_as_verified(this: &TestContext, other: &TestContext) {
 
     peerstate.verified_key = peerstate.public_key.clone();
     peerstate.verified_key_fingerprint = peerstate.public_key_fingerprint.clone();
-    peerstate.to_save = Some(peerstate::ToSave::All);
 
-    peerstate.save_to_db(&this.sql, false).await.unwrap();
+    peerstate.save_to_db(&this.sql).await.unwrap();
 }
 
 async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Option<Message> {