fix(wallet): slightly improve error output for failed decryption (#4972)

* fix(wallet): slightly improve error output for failed decryption

* remove unused function

* reduce secret cloning

base_layer/core/src/proto/transaction.proto

@@ -93,7 +93,7 @@ message OutputFeatures {
     // Version
     uint32 version = 1;
     // Flags are the feature flags that differentiate between outputs, eg Coinbase all of which has different rules
-    uint32 flags = 2;
+    uint32 output_type = 2;
     // The maturity of the specific UTXO. This is the min lock height at which an UTXO can be spend. Coinbase UTXO
     // require a min maturity of the Coinbase_lock_height, this should be checked on receiving new blocks.
     uint64 maturity = 3;

base_layer/core/src/proto/transaction.rs

@@ -309,16 +309,16 @@ impl TryFrom<proto::types::OutputFeatures> for OutputFeatures {
             .map(SideChainFeature::try_from)
             .transpose()?;
 
-        let flags = features
-            .flags
+        let output_type = features
+            .output_type
             .try_into()
             .map_err(|_| "Invalid output type: overflowed")?;
 
         Ok(OutputFeatures::new(
             OutputFeaturesVersion::try_from(
                 u8::try_from(features.version).map_err(|_| "Invalid version: overflowed u8")?,
             )?,
-            OutputType::from_byte(flags).ok_or_else(|| "Invalid or unrecognised output type".to_string())?,
+            OutputType::from_byte(output_type).ok_or_else(|| "Invalid or unrecognised output type".to_string())?,
             features.maturity,
             features.metadata,
             sidechain_feature,
@@ -329,7 +329,7 @@ impl TryFrom<proto::types::OutputFeatures> for OutputFeatures {
 impl From<OutputFeatures> for proto::types::OutputFeatures {
     fn from(features: OutputFeatures) -> Self {
         Self {
-            flags: u32::from(features.output_type.as_byte()),
+            output_type: u32::from(features.output_type.as_byte()),
             maturity: features.maturity,
             metadata: features.metadata,
             version: features.version as u32,

base_layer/core/src/transactions/transaction_components/side_chain/sidechain_feature.rs

@@ -45,13 +45,4 @@ impl SideChainFeature {
             _ => None,
         }
     }
-
-    pub fn as_byte(&self) -> u8 {
-        #[allow(clippy::enum_glob_use)]
-        use SideChainFeature::*;
-        match self {
-            ValidatorNodeRegistration(_) => 0x01,
-            TemplateRegistration(_) => 0x02,
-        }
-    }
 }

base_layer/wallet/src/key_manager_service/storage/sqlite_db/key_manager_state.rs

@@ -170,7 +170,7 @@ impl Encryptable<XChaCha20Poly1305> for KeyManagerStateSql {
 
     fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
         self.primary_key_index =
-            decrypt_bytes_integral_nonce(cipher, self.domain("primary_key_index"), self.primary_key_index.clone())?;
+            decrypt_bytes_integral_nonce(cipher, self.domain("primary_key_index"), &self.primary_key_index)?;
 
         Ok(())
     }

base_layer/wallet/src/output_manager_service/service.rs

@@ -591,7 +591,7 @@ where
                             Err(OutputManagerProtocolError { id, error }) => {
                                 warn!(
                                     target: LOG_TARGET,
-                                    "Error completing UTXO Validation Protocol (Id: {}): {:?}", id, error
+                                    "Error completing UTXO Validation Protocol (Id: {}): {}", id, error
                                 );
                                 let event_payload = match error {
                                     OutputManagerError::InconsistentBaseNodeDataError(_) |

base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs

@@ -1332,7 +1332,7 @@ impl Encryptable<XChaCha20Poly1305> for KnownOneSidedPaymentScriptSql {
     }
 
     fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
-        self.private_key = decrypt_bytes_integral_nonce(cipher, self.domain("private_key"), self.private_key.clone())?;
+        self.private_key = decrypt_bytes_integral_nonce(cipher, self.domain("private_key"), &self.private_key)?;
         Ok(())
     }
 }

base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs

@@ -135,9 +135,7 @@ impl NewOutputSql {
 impl Encryptable<XChaCha20Poly1305> for NewOutputSql {
     fn domain(&self, field_name: &'static str) -> Vec<u8> {
         // WARNING: using `OUTPUT` for both NewOutputSql and OutputSql due to later transition without re-encryption
-        [Self::OUTPUT, self.script.as_slice(), field_name.as_bytes()]
-            .concat()
-            .to_vec()
+        [Self::OUTPUT, self.script.as_slice(), field_name.as_bytes()].concat()
     }
 
     fn encrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
@@ -157,14 +155,10 @@ impl Encryptable<XChaCha20Poly1305> for NewOutputSql {
     }
 
     fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
-        self.spending_key =
-            decrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), self.spending_key.clone())?;
+        self.spending_key = decrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), &self.spending_key)?;
 
-        self.script_private_key = decrypt_bytes_integral_nonce(
-            cipher,
-            self.domain("script_private_key"),
-            self.script_private_key.clone(),
-        )?;
+        self.script_private_key =
+            decrypt_bytes_integral_nonce(cipher, self.domain("script_private_key"), &self.script_private_key)?;
 
         Ok(())
     }

base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs

@@ -804,14 +804,10 @@ impl Encryptable<XChaCha20Poly1305> for OutputSql {
     }
 
     fn decrypt(&mut self, cipher: &XChaCha20Poly1305) -> Result<(), String> {
-        self.spending_key =
-            decrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), self.spending_key.clone())?;
+        self.spending_key = decrypt_bytes_integral_nonce(cipher, self.domain("spending_key"), &self.spending_key)?;
 
-        self.script_private_key = decrypt_bytes_integral_nonce(
-            cipher,
-            self.domain("script_private_key"),
-            self.script_private_key.clone(),
-        )?;
+        self.script_private_key =
+            decrypt_bytes_integral_nonce(cipher, self.domain("script_private_key"), &self.script_private_key)?;
 
         Ok(())
     }

base_layer/wallet/src/storage/sqlite_db/wallet.rs

@@ -117,7 +117,7 @@ impl WalletSqliteDatabase {
                         decrypt_bytes_integral_nonce(
                             cipher,
                             b"wallet_setting_master_seed".to_vec(),
-                            from_hex(seed_str.as_str())?,
+                            &from_hex(seed_str.as_str())?,
                         )
                         .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?,
                     );
@@ -205,7 +205,7 @@ impl WalletSqliteDatabase {
                     // we must zeroize decrypted_key_bytes, as this contains sensitive data,
                     // including private key informations
                     let decrypted_key_bytes = Hidden::hide(
-                        decrypt_bytes_integral_nonce(cipher, b"wallet_setting_tor_id".to_vec(), from_hex(&key_str)?)
+                        decrypt_bytes_integral_nonce(cipher, b"wallet_setting_tor_id".to_vec(), &from_hex(&key_str)?)
                             .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?,
                     );
 
@@ -537,7 +537,7 @@ impl WalletBackend for WalletSqliteDatabase {
             decrypt_bytes_integral_nonce(
                 &cipher,
                 b"wallet_setting_master_seed".to_vec(),
-                from_hex(master_seed_str.as_str())?,
+                &from_hex(master_seed_str.as_str())?,
             )
             .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?,
         );
@@ -563,7 +563,7 @@ impl WalletBackend for WalletSqliteDatabase {
             // decrypted_key_bytes contains sensitive information regarding private key, we thus
             // make sure the data is appropriately zeroized when we leave the current scope
             let decrypted_key_bytes = Hidden::hide(
-                decrypt_bytes_integral_nonce(&cipher, b"wallet_setting_tor_id".to_vec(), from_hex(v.as_str())?)
+                decrypt_bytes_integral_nonce(&cipher, b"wallet_setting_tor_id".to_vec(), &from_hex(v.as_str())?)
                     .map_err(|e| WalletStorageError::AeadError(format!("Decryption Error:{}", e)))?,
             );
 
@@ -722,11 +722,11 @@ fn check_db_encryption_status(
                     // decrypted key contains sensitive data, we make sure we appropriately zeroize
                     // the corresponding data buffer, when leaving the current scope
                     let decrypted_key = Hidden::hide(
-                        decrypt_bytes_integral_nonce(&cipher_inner, b"wallet_setting_master_seed".to_vec(), sk_bytes)
+                        decrypt_bytes_integral_nonce(&cipher_inner, b"wallet_setting_master_seed".to_vec(), &sk_bytes)
                             .map_err(|e| {
-                            error!(target: LOG_TARGET, "Incorrect passphrase ({})", e);
-                            WalletStorageError::InvalidPassphrase
-                        })?,
+                                error!(target: LOG_TARGET, "Incorrect passphrase ({})", e);
+                                WalletStorageError::InvalidPassphrase
+                            })?,
                     );
 
                     let _cipher_seed =
@@ -867,7 +867,7 @@ impl Encryptable<XChaCha20Poly1305> for ClientKeyValueSql {
         let mut decrypted_value = decrypt_bytes_integral_nonce(
             cipher,
             self.domain("value"),
-            from_hex(self.value.as_str()).map_err(|e| e.to_string())?,
+            &from_hex(self.value.as_str()).map_err(|e| e.to_string())?,
         )?;
 
         self.value = from_utf8(decrypted_value.as_slice())

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs

@@ -1550,7 +1550,7 @@ impl Encryptable<XChaCha20Poly1305> for InboundTransactionSql {
         let mut decrypted_protocol = decrypt_bytes_integral_nonce(
             cipher,
             self.domain("receiver_protocol"),
-            from_hex(self.receiver_protocol.as_str()).map_err(|e| e.to_string())?,
+            &from_hex(self.receiver_protocol.as_str()).map_err(|e| e.to_string())?,
         )?;
 
         self.receiver_protocol = from_utf8(decrypted_protocol.as_slice())
@@ -1804,7 +1804,7 @@ impl Encryptable<XChaCha20Poly1305> for OutboundTransactionSql {
         let mut decrypted_protocol = decrypt_bytes_integral_nonce(
             cipher,
             self.domain("sender_protocol"),
-            from_hex(self.sender_protocol.as_str()).map_err(|e| e.to_string())?,
+            &from_hex(self.sender_protocol.as_str()).map_err(|e| e.to_string())?,
         )?;
 
         self.sender_protocol = from_utf8(decrypted_protocol.as_slice())
@@ -2213,7 +2213,7 @@ impl Encryptable<XChaCha20Poly1305> for CompletedTransactionSql {
         let mut decrypted_protocol = decrypt_bytes_integral_nonce(
             cipher,
             self.domain("transaction_protocol"),
-            from_hex(self.transaction_protocol.as_str()).map_err(|e| e.to_string())?,
+            &from_hex(self.transaction_protocol.as_str()).map_err(|e| e.to_string())?,
         )?;
 
         self.transaction_protocol = from_utf8(decrypted_protocol.as_slice())

base_layer/wallet/src/util/encryption.rs

@@ -23,7 +23,7 @@
 use std::mem::size_of;
 
 use chacha20poly1305::{
-    aead::{Aead, Error as AeadError, Payload},
+    aead::{Aead, Payload},
     Tag,
     XChaCha20Poly1305,
     XNonce,
@@ -51,11 +51,11 @@ pub trait Encryptable<C> {
 pub fn decrypt_bytes_integral_nonce(
     cipher: &XChaCha20Poly1305,
     domain: Vec<u8>,
-    ciphertext: Vec<u8>,
+    ciphertext: &[u8],
 ) -> Result<Vec<u8>, String> {
     // We need at least a nonce and tag, or there's no point in attempting decryption
     if ciphertext.len() < size_of::<XNonce>() + size_of::<Tag>() {
-        return Err(AeadError.to_string());
+        return Err("Ciphertext is too short".to_string());
     }
 
     // Extract the nonce
@@ -68,7 +68,9 @@ pub fn decrypt_bytes_integral_nonce(
     };
 
     // Attempt authentication and decryption
-    let plaintext = cipher.decrypt(nonce_ga, payload).map_err(|e| e.to_string())?;
+    let plaintext = cipher
+        .decrypt(nonce_ga, payload)
+        .map_err(|e| format!("Decryption failed: {}", e))?;
 
     Ok(plaintext)
 }
@@ -91,7 +93,9 @@ pub fn encrypt_bytes_integral_nonce(
     };
 
     // Attempt authenticated encryption
-    let mut ciphertext = cipher.encrypt(nonce_ga, payload).map_err(|e| e.to_string())?;
+    let mut ciphertext = cipher
+        .encrypt(nonce_ga, payload)
+        .map_err(|e| format!("Failed to encrypt: {}", e))?;
 
     // Concatenate the nonce and ciphertext (which already include the tag)
     let mut ciphertext_integral_nonce = nonce.to_vec();
@@ -130,19 +134,20 @@ mod test {
         );
 
         // Valid decryption must succeed and yield correct plaintext
-        let decrypted_text =
-            decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), ciphertext.clone()).unwrap();
+        let decrypted_text = decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), &ciphertext).unwrap();
         assert_eq!(decrypted_text, plaintext);
 
         // Must fail on an incorrect domain
-        assert!(decrypt_bytes_integral_nonce(&cipher, b"wrong_domain".to_vec(), ciphertext.clone()).is_err());
+        assert!(decrypt_bytes_integral_nonce(&cipher, b"wrong_domain".to_vec(), &ciphertext).is_err());
 
         // Must fail with an evil nonce
         let ciphertext_with_evil_nonce = ciphertext
             .clone()
             .splice(0..size_of::<XNonce>(), [0u8; size_of::<XNonce>()])
-            .collect();
-        assert!(decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), ciphertext_with_evil_nonce).is_err());
+            .collect::<Vec<_>>();
+        assert!(
+            decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), &ciphertext_with_evil_nonce).is_err()
+        );
 
         // Must fail with malleated ciphertext
         let ciphertext_with_evil_ciphertext = ciphertext
@@ -151,9 +156,10 @@ mod test {
                 size_of::<XNonce>()..(ciphertext.len() - size_of::<Tag>()),
                 vec![0u8; plaintext.len()],
             )
-            .collect();
+            .collect::<Vec<_>>();
         assert!(
-            decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), ciphertext_with_evil_ciphertext).is_err()
+            decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), &ciphertext_with_evil_ciphertext)
+                .is_err()
         );
 
         // Must fail with malleated authentication tag
@@ -166,14 +172,14 @@ mod test {
                 >(
                 )
             ])
-            .collect();
-        assert!(decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), ciphertext_with_evil_tag).is_err());
+            .collect::<Vec<_>>();
+        assert!(decrypt_bytes_integral_nonce(&cipher, b"correct_domain".to_vec(), &ciphertext_with_evil_tag).is_err());
 
         // Must fail if truncated too short (if shorter than a nonce and tag, decryption is not even attempted)
         assert!(decrypt_bytes_integral_nonce(
             &cipher,
             b"correct_domain".to_vec(),
-            ciphertext[0..(size_of::<XNonce>() + size_of::<Tag>() - 1)].to_vec()
+            &ciphertext[0..(size_of::<XNonce>() + size_of::<Tag>() - 1)]
         )
         .is_err());
     }