Replace _assign with _tweak

The key methods `add_assign`, `add_expr_assign`, and `mul_assign` are
cumbersome to use because a local variable that uses these methods
changes meaning but keeps the same identifier. It would be more useful
if we had methods that consumed `self` and returned a new key.

Observe also that these to methods are for adding/multiplying a key by a
tweak, rename the methods appropriately.

Add methods `add_tweak` and `mul_tweak` to the `SecretKey` and
`PublicKey` type. Deprecate `add_assign`, `add_expr_assign`, and
`mul_assign`.

Author: tcharding

src/key.rs

@@ -227,55 +227,75 @@ impl SecretKey {
         }
     }
 
-    #[inline]
     /// Adds one secret key to another, modulo the curve order.
     ///
     /// # Errors
     ///
     /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
     /// length slice.
-    pub fn add_assign(
-        &mut self,
-        other: &[u8],
-    ) -> Result<(), Error> {
-        if other.len() != 32 {
+    #[inline]
+    #[deprecated(since = "0.23.0", note = "Use add_tweak instead")]
+    pub fn add_assign(&mut self, other: &[u8]) -> Result<(), Error> {
+        *self = self.add_tweak(other)?;
+        Ok(())
+    }
+
+    /// Tweaks a [`SecretKey`] by adding `tweak` modulo the curve order.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
+    /// length slice.
+    #[inline]
+    pub fn add_tweak(mut self, tweak: &[u8]) -> Result<SecretKey, Error> {
+        if tweak.len() != 32 {
             return Err(Error::InvalidTweak);
         }
         unsafe {
             if ffi::secp256k1_ec_seckey_tweak_add(
                 ffi::secp256k1_context_no_precomp,
                 self.as_mut_c_ptr(),
-                other.as_c_ptr(),
+                tweak.as_c_ptr(),
             ) != 1
             {
                 Err(Error::InvalidTweak)
             } else {
-                Ok(())
+                Ok(self)
             }
         }
     }
 
-    #[inline]
     /// Multiplies one secret key by another, modulo the curve order. Will
     /// return an error if the resulting key would be invalid or if
     /// the tweak was not a 32-byte length slice.
-    pub fn mul_assign(
-        &mut self,
-        other: &[u8],
-    ) -> Result<(), Error> {
-        if other.len() != 32 {
+    #[inline]
+    #[deprecated(since = "0.23.0", note = "Use mul_tweak instead")]
+    pub fn mul_assign(&mut self, other: &[u8]) -> Result<(), Error> {
+        *self = self.mul_tweak(other)?;
+        Ok(())
+    }
+
+    /// Tweaks a [`SecretKey`] by multiplying by `tweak` modulo the curve order.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
+    /// length slice.
+    #[inline]
+    pub fn mul_tweak(mut self, tweak: &[u8]) -> Result<SecretKey, Error> {
+        if tweak.len() != 32 {
             return Err(Error::InvalidTweak);
         }
         unsafe {
             if ffi::secp256k1_ec_seckey_tweak_mul(
                 ffi::secp256k1_context_no_precomp,
                 self.as_mut_c_ptr(),
-                other.as_c_ptr(),
+                tweak.as_c_ptr(),
             ) != 1
             {
                 Err(Error::InvalidTweak)
             } else {
-                Ok(())
+                Ok(self)
             }
         }
     }
@@ -497,48 +517,82 @@ impl PublicKey {
         }
     }
 
-    #[inline]
     /// Adds the `other` public key to `self` in place.
     ///
     /// # Errors
     ///
     /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
     /// length slice.
+    #[inline]
+    #[deprecated(since = "0.23.0", note = "Use add_tweak instead")]
     pub fn add_exp_assign<C: Verification>(
         &mut self,
         secp: &Secp256k1<C>,
         other: &[u8]
     ) -> Result<(), Error> {
-        if other.len() != 32 {
+        *self = self.add_tweak(secp, other)?;
+        Ok(())
+    }
+
+    /// Tweaks a [`PublicKey`] by adding `tweak` modulo the curve order.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
+    /// length slice.
+    #[inline]
+    pub fn add_tweak<C: Verification>(
+        mut self,
+        secp: &Secp256k1<C>,
+        tweak: &[u8]
+    ) -> Result<PublicKey, Error> {
+        if tweak.len() != 32 {
             return Err(Error::InvalidTweak);
         }
         unsafe {
-            if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0, other.as_c_ptr()) == 1 {
-                Ok(())
+            if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0, tweak.as_c_ptr()) == 1 {
+                Ok(self)
             } else {
                 Err(Error::InvalidTweak)
             }
         }
     }
 
-    #[inline]
     /// Muliplies the public key in place by the scalar `other`.
     ///
     /// # Errors
     ///
     /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
     /// length slice.
+    #[deprecated(since = "0.23.0", note = "Use mul_tweak instead")]
+    #[inline]
     pub fn mul_assign<C: Verification>(
         &mut self,
         secp: &Secp256k1<C>,
         other: &[u8],
     ) -> Result<(), Error> {
+        *self = self.mul_tweak(secp, other)?;
+        Ok(())
+    }
+
+    /// Tweaks a [`PublicKey`] by multiplying by `tweak` modulo the curve order.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
+    /// length slice.
+    #[inline]
+    pub fn mul_tweak<C: Verification>(
+        mut self,
+        secp: &Secp256k1<C>,
+        other: &[u8],
+    ) -> Result<PublicKey, Error> {
         if other.len() != 32 {
             return Err(Error::InvalidTweak);
         }
         unsafe {
             if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx, &mut self.0, other.as_c_ptr()) == 1 {
-                Ok(())
+                Ok(self)
             } else {
                 Err(Error::InvalidTweak)
             }
@@ -1501,6 +1555,8 @@ pub mod serde_keypair {
 #[cfg(test)]
 #[allow(unused_imports)]
 mod test {
+    use super::*;
+
     use core::str::FromStr;
 
     #[cfg(any(feature = "alloc", feature = "std"))]
@@ -1765,43 +1821,68 @@ mod test {
 
     #[test]
     #[cfg(any(feature = "alloc", feature = "std"))]
-    fn test_addition() {
+    fn tweak_add_arbitrary_data() {
         let s = Secp256k1::new();
 
-        let (mut sk1, mut pk1) = s.generate_keypair(&mut thread_rng());
-        let (mut sk2, mut pk2) = s.generate_keypair(&mut thread_rng());
+        let (sk, pk) = s.generate_keypair(&mut thread_rng());
+        assert_eq!(PublicKey::from_secret_key(&s, &sk), pk); // Sanity check.
 
-        assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1);
-        assert!(sk1.add_assign(&sk2[..]).is_ok());
-        assert!(pk1.add_exp_assign(&s, &sk2[..]).is_ok());
-        assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1);
+        // TODO: This would be better tested with a _lot_ of different tweaks.
+        let tweak = random_32_bytes(&mut thread_rng());
 
-        assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2);
-        assert!(sk2.add_assign(&sk1[..]).is_ok());
-        assert!(pk2.add_exp_assign(&s, &sk1[..]).is_ok());
-        assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2);
+        let tweaked_sk = sk.add_tweak(&tweak[..]).unwrap();
+        assert_ne!(sk, tweaked_sk); // Make sure we did something.
+        let tweaked_pk = pk.add_tweak(&s, &tweak[..]).unwrap();
+        assert_ne!(pk, tweaked_pk);
+
+        assert_eq!(PublicKey::from_secret_key(&s, &tweaked_sk), tweaked_pk);
     }
 
     #[test]
     #[cfg(any(feature = "alloc", feature = "std"))]
-    fn test_multiplication() {
+    fn tweak_add_zero() {
+        let s = Secp256k1::new();
+
+        let (sk, pk) = s.generate_keypair(&mut thread_rng());
+
+        let tweak = &[0u8; 32];
+
+        let tweaked_sk = sk.add_tweak(tweak).unwrap();
+        assert_eq!(sk, tweaked_sk); // Tweak by zero does nothing.
+        let tweaked_pk = pk.add_tweak(&s, tweak).unwrap();
+        assert_eq!(pk, tweaked_pk);
+    }
+
+     #[test]
+    #[cfg(any(feature = "alloc", feature = "std"))]
+    fn tweak_mul_arbitrary_data() {
         let s = Secp256k1::new();
 
-        let (mut sk1, mut pk1) = s.generate_keypair(&mut thread_rng());
-        let (mut sk2, mut pk2) = s.generate_keypair(&mut thread_rng());
+        let (sk, pk) = s.generate_keypair(&mut thread_rng());
+        assert_eq!(PublicKey::from_secret_key(&s, &sk), pk); // Sanity check.
 
-        assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1);
-        assert!(sk1.mul_assign(&sk2[..]).is_ok());
-        assert!(pk1.mul_assign(&s, &sk2[..]).is_ok());
-        assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1);
+        // TODO: This would be better tested with a _lot_ of different tweaks.
+        let tweak = random_32_bytes(&mut thread_rng());
 
-        assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2);
-        assert!(sk2.mul_assign(&sk1[..]).is_ok());
-        assert!(pk2.mul_assign(&s, &sk1[..]).is_ok());
-        assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2);
+        let tweaked_sk = sk.mul_tweak(&tweak[..]).unwrap();
+        assert_ne!(sk, tweaked_sk); // Make sure we did something.
+        let tweaked_pk = pk.mul_tweak(&s, &tweak[..]).unwrap();
+        assert_ne!(pk, tweaked_pk);
+
+        assert_eq!(PublicKey::from_secret_key(&s, &tweaked_sk), tweaked_pk);
     }
 
     #[test]
+    #[cfg(any(feature = "alloc", feature = "std"))]
+    fn tweak_mul_zero() {
+        let s = Secp256k1::new();
+        let (sk, _) = s.generate_keypair(&mut thread_rng());
+
+        let tweak = &[0u8; 32];
+        assert!(sk.mul_tweak(tweak).is_err())
+    }
+
+   #[test]
     #[cfg(any(feature = "alloc", feature = "std"))]
     fn test_negation() {
         let s = Secp256k1::new();
@@ -1904,7 +1985,7 @@ mod test {
     fn create_pubkey_combine() {
         let s = Secp256k1::new();
 
-        let (mut sk1, pk1) = s.generate_keypair(&mut thread_rng());
+        let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
         let (sk2, pk2) = s.generate_keypair(&mut thread_rng());
 
         let sum1 = pk1.combine(&pk2);
@@ -1913,8 +1994,8 @@ mod test {
         assert!(sum2.is_ok());
         assert_eq!(sum1, sum2);
 
-        assert!(sk1.add_assign(&sk2.as_ref()[..]).is_ok());
-        let sksum = PublicKey::from_secret_key(&s, &sk1);
+        let tweaked = sk1.add_tweak(&sk2.as_ref()[..]).unwrap();
+        let sksum = PublicKey::from_secret_key(&s, &tweaked);
         assert_eq!(Ok(sksum), sum1);
     }
 

src/lib.rs

@@ -337,7 +337,7 @@ pub enum Error {
     InvalidSharedSecret,
     /// Bad recovery id.
     InvalidRecoveryId,
-    /// Invalid tweak for `add_*_assign` or `mul_*_assign`.
+    /// Tried to add/multiply by an invalid tweak.
     InvalidTweak,
     /// Didn't pass enough memory to context creation with preallocated memory.
     NotEnoughMemory,