check_verified_properties(): Don't ignore fails of Peerstate::set_verified()

- Return Result from set_verified() so that it can't be missed.
- Pass Fingerprint to set_verified() by value to avoid cloning it inside. This optimises out an
  extra clone() if we already have a value that can be moved at the caller side. However, this may
  add an extra clone() if set_verified() fails, but let's not optimise the fail scenario.

src/peerstate.rs

@@ -17,7 +17,7 @@ use crate::message::Message;
 use crate::mimeparser::SystemMessage;
 use crate::sql::Sql;
 use crate::stock_str;
-use anyhow::{Context as _, Result};
+use anyhow::{Context as _, Error, Result};
 use num_traits::FromPrimitive;
 
 #[derive(Debug)]
@@ -369,43 +369,48 @@ impl Peerstate {
     /// verifier:
     ///   The address which verifies the given contact
     ///   If we are verifying the contact, use that contacts address
-    /// Returns whether the value of the key has changed
     pub fn set_verified(
         &mut self,
         which_key: PeerstateKeyType,
-        fingerprint: &Fingerprint,
+        fingerprint: Fingerprint,
         verified: PeerstateVerifiedStatus,
         verifier: String,
-    ) -> bool {
+    ) -> Result<()> {
         if verified == PeerstateVerifiedStatus::BidirectVerified {
             match which_key {
                 PeerstateKeyType::PublicKey => {
                     if self.public_key_fingerprint.is_some()
-                        && self.public_key_fingerprint.as_ref().unwrap() == fingerprint
+                        && self.public_key_fingerprint.as_ref().unwrap() == &fingerprint
                     {
                         self.verified_key = self.public_key.clone();
-                        self.verified_key_fingerprint = self.public_key_fingerprint.clone();
+                        self.verified_key_fingerprint = Some(fingerprint);
                         self.verifier = Some(verifier);
-                        true
+                        Ok(())
                     } else {
-                        false
+                        Err(Error::msg(format!(
+                            "{} is not peer's public key fingerprint",
+                            fingerprint,
+                        )))
                     }
                 }
                 PeerstateKeyType::GossipKey => {
                     if self.gossip_key_fingerprint.is_some()
-                        && self.gossip_key_fingerprint.as_ref().unwrap() == fingerprint
+                        && self.gossip_key_fingerprint.as_ref().unwrap() == &fingerprint
                     {
                         self.verified_key = self.gossip_key.clone();
-                        self.verified_key_fingerprint = self.gossip_key_fingerprint.clone();
+                        self.verified_key_fingerprint = Some(fingerprint);
                         self.verifier = Some(verifier);
-                        true
+                        Ok(())
                     } else {
-                        false
+                        Err(Error::msg(format!(
+                            "{} is not peer's gossip key fingerprint",
+                            fingerprint,
+                        )))
                     }
                 }
             }
         } else {
-            false
+            Err(Error::msg("BidirectVerified required"))
         }
     }
 

src/receive_imf.rs

@@ -2178,10 +2178,10 @@ async fn check_verified_properties(
                     if let Some(fp) = fp {
                         peerstate.set_verified(
                             PeerstateKeyType::GossipKey,
-                            &fp,
+                            fp,
                             PeerstateVerifiedStatus::BidirectVerified,
                             contact.get_addr().to_owned(),
-                        );
+                        )?;
                         peerstate.save_to_db(&context.sql).await?;
                         is_verified = true;
                     }

src/securejoin.rs

@@ -415,7 +415,7 @@ pub(crate) async fn handle_securejoin_handshake(
                 .await?
                 .get_addr()
                 .to_owned();
-            if mark_peer_as_verified(context, &fingerprint, contact_addr)
+            if mark_peer_as_verified(context, fingerprint.clone(), contact_addr)
                 .await
                 .is_err()
             {
@@ -613,28 +613,23 @@ pub(crate) async fn observe_securejoin_on_other_device(
                         return Ok(HandshakeMessage::Ignore);
                     }
                 };
-                if peerstate.set_verified(
+                if let Err(err) = peerstate.set_verified(
                     PeerstateKeyType::GossipKey,
-                    &fingerprint,
+                    fingerprint,
                     PeerstateVerifiedStatus::BidirectVerified,
                     addr,
                 ) {
-                    peerstate.prefer_encrypt = EncryptPreference::Mutual;
-                    peerstate.save_to_db(&context.sql).await.unwrap_or_default();
-                } else {
                     could_not_establish_secure_connection(
                         context,
                         contact_id,
                         info_chat_id(context, contact_id).await?,
-                        &format!(
+                        &format!("Could not mark peer as verified at step {}: {}", step, err),
-                            "Could not mark peer as verified for fingerprint {} at step {}",
-                            fingerprint.hex(),
-                            step,
-                        ),
                     )
                     .await?;
                     return Ok(HandshakeMessage::Ignore);
                 }
+                peerstate.prefer_encrypt = EncryptPreference::Mutual;
+                peerstate.save_to_db(&context.sql).await.unwrap_or_default();
             } else if let Some(fingerprint) =
                 mime_message.get_header(HeaderDef::SecureJoinFingerprint)
             {
@@ -643,7 +638,7 @@ pub(crate) async fn observe_securejoin_on_other_device(
                 let fingerprint = fingerprint.parse()?;
                 if mark_peer_as_verified(
                     context,
-                    &fingerprint,
+                    fingerprint,
                     Contact::load_from_db(context, contact_id)
                         .await?
                         .get_addr()
@@ -715,25 +710,25 @@ async fn could_not_establish_secure_connection(
 
 async fn mark_peer_as_verified(
     context: &Context,
-    fingerprint: &Fingerprint,
+    fingerprint: Fingerprint,
     verifier: String,
 ) -> Result<(), Error> {
-    if let Some(ref mut peerstate) = Peerstate::from_fingerprint(context, fingerprint).await? {
+    if let Some(ref mut peerstate) = Peerstate::from_fingerprint(context, &fingerprint).await? {
-        if peerstate.set_verified(
+        if let Err(err) = peerstate.set_verified(
             PeerstateKeyType::PublicKey,
             fingerprint,
             PeerstateVerifiedStatus::BidirectVerified,
             verifier,
         ) {
-            peerstate.prefer_encrypt = EncryptPreference::Mutual;
+            error!(context, "Could not mark peer as verified: {}", err);
-            peerstate.save_to_db(&context.sql).await.unwrap_or_default();
+            return Err(err);
-            return Ok(());
         }
+        peerstate.prefer_encrypt = EncryptPreference::Mutual;
+        peerstate.save_to_db(&context.sql).await.unwrap_or_default();
+        Ok(())
+    } else {
+        bail!("no peerstate in db for fingerprint {}", fingerprint.hex());
     }
-    bail!(
-        "could not mark peer as verified for fingerprint {}",
-        fingerprint.hex()
-    );
 }
 
 /* ******************************************************************************

src/securejoin/bobstate.rs

@@ -368,7 +368,7 @@ impl BobState {
         }
         mark_peer_as_verified(
             context,
-            self.invite.fingerprint(),
+            self.invite.fingerprint().clone(),
             mime_message.from.addr.to_string(),
         )
         .await?;