From e3d21a3d87614980f840bd50ed3cb387d06381c4 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 4 Jan 2022 08:18:44 +1100 Subject: [PATCH 1/5] Clean up test imports with key module The import statements can be simplified by using an import wildcard (`super::*`). While we are at it put them in std, external crate, this crate order. --- src/key.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/key.rs b/src/key.rs index f2152b6..c6974e3 100644 --- a/src/key.rs +++ b/src/key.rs @@ -964,16 +964,16 @@ impl<'de> ::serde::Deserialize<'de> for XOnlyPublicKey { #[cfg(test)] mod test { - use Secp256k1; - use {from_hex, to_hex}; - use super::super::Error::{InvalidPublicKey, InvalidSecretKey}; - use super::{PublicKey, SecretKey}; - use super::super::constants; + use super::*; + + use std::iter; + use std::str::FromStr; use rand::{Error, ErrorKind, RngCore, thread_rng}; use rand_core::impls; - use std::iter; - use std::str::FromStr; + + use {to_hex, constants}; + use Error::{InvalidPublicKey, InvalidSecretKey}; #[cfg(target_arch = "wasm32")] use wasm_bindgen_test::wasm_bindgen_test as test; From edafb88f8c6b9f8f21b253621cd5ecbf1e2bffd1 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 4 Jan 2022 08:23:05 +1100 Subject: [PATCH 2/5] Move key unit tests to key module There are currently two unit tests in the `schnorr` module that are testing keys from the `key` module. This is possible because the tests are only testing the public interface, none the less they are better placed in the `key` module. --- src/key.rs | 34 ++++++++++++++++++++++++++++++++++ src/schnorr.rs | 33 --------------------------------- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/key.rs b/src/key.rs index c6974e3..57a4280 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1476,4 +1476,38 @@ mod test { assert_tokens(&pk.readable(), &[Token::String(PK_STR)]); } + + #[test] + fn test_tweak_add_assign_then_tweak_add_check() { + let s = Secp256k1::new(); + + for _ in 0..10 { + let mut tweak = [0u8; 32]; + thread_rng().fill_bytes(&mut tweak); + let (mut kp, mut pk) = s.generate_schnorrsig_keypair(&mut thread_rng()); + let orig_pk = pk; + kp.tweak_add_assign(&s, &tweak).expect("Tweak error"); + let parity = pk.tweak_add_assign(&s, &tweak).expect("Tweak error"); + assert_eq!(XOnlyPublicKey::from_keypair(&kp), pk); + assert!(orig_pk.tweak_add_check(&s, &pk, parity, tweak)); + } + } + + #[test] + fn test_from_key_pubkey() { + let kpk1 = PublicKey::from_str( + "02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443", + ) + .unwrap(); + let kpk2 = PublicKey::from_str( + "0384526253c27c7aef56c7b71a5cd25bebb66dddda437826defc5b2568bde81f07", + ) + .unwrap(); + + let pk1 = XOnlyPublicKey::from(kpk1); + let pk2 = XOnlyPublicKey::from(kpk2); + + assert_eq!(pk1.serialize()[..], kpk1.serialize()[1..]); + assert_eq!(pk2.serialize()[..], kpk2.serialize()[1..]); + } } diff --git a/src/schnorr.rs b/src/schnorr.rs index 4516f2d..fc1a628 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -565,37 +565,4 @@ mod tests { assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]); assert_tokens(&pk.readable(), &[Token::String(PK_STR)]); } - #[test] - fn test_addition() { - let s = Secp256k1::new(); - - for _ in 0..10 { - let mut tweak = [0u8; 32]; - thread_rng().fill_bytes(&mut tweak); - let (mut kp, mut pk) = s.generate_schnorrsig_keypair(&mut thread_rng()); - let orig_pk = pk; - kp.tweak_add_assign(&s, &tweak).expect("Tweak error"); - let parity = pk.tweak_add_assign(&s, &tweak).expect("Tweak error"); - assert_eq!(XOnlyPublicKey::from_keypair(&kp), pk); - assert!(orig_pk.tweak_add_check(&s, &pk, parity, tweak)); - } - } - - #[test] - fn test_from_key_pubkey() { - let kpk1 = ::key::PublicKey::from_str( - "02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443", - ) - .unwrap(); - let kpk2 = ::key::PublicKey::from_str( - "0384526253c27c7aef56c7b71a5cd25bebb66dddda437826defc5b2568bde81f07", - ) - .unwrap(); - - let pk1 = XOnlyPublicKey::from(kpk1); - let pk2 = XOnlyPublicKey::from(kpk2); - - assert_eq!(pk1.serialize()[..], kpk1.serialize()[1..]); - assert_eq!(pk2.serialize()[..], kpk2.serialize()[1..]); - } } From 1b768b2749f2b3508f44ad875e99968a7a1eb268 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 4 Jan 2022 08:36:46 +1100 Subject: [PATCH 3/5] Make tweak_add_assign return statements uniform We have two `tweak_add_assign` methods (one for keypair and one for x-only pubkey). Both check the return value from a FFI function call. We can make both sites uniform to _slightly_ reduce cognitive load when reading the code. Use C style code to make it obvious to readers that this is basically C code. --- src/key.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/key.rs b/src/key.rs index 57a4280..04a1522 100644 --- a/src/key.rs +++ b/src/key.rs @@ -636,12 +636,11 @@ impl KeyPair { &mut self.0, tweak.as_c_ptr(), ); - - if err == 1 { - Ok(()) - } else { - Err(Error::InvalidTweak) + if err != 1 { + return Err(Error::InvalidTweak); } + + Ok(()) } } } @@ -846,7 +845,6 @@ impl XOnlyPublicKey { self.as_c_ptr(), tweak.as_c_ptr(), ); - if err != 1 { return Err(Error::InvalidTweak); } From fbc64c77252e86100a657fa3d381b36c22eeb48e Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 4 Jan 2022 08:49:08 +1100 Subject: [PATCH 4/5] Add opaque parity type Two functions in the FFI secp code return and accept a parity int. Currently we are manually converting this to a bool. Doing so forces readers of the code to think what the bool means even though understanding this bool is not needed since in is just passed back down to the FFI code. We can abstract this away by using an opaque type to hold the original int and not converting it to a boolean value. Add 'Return' and 'Error' sections to `tweak_add_assign` while fixing the docs to describe the new opaque parity type. --- src/key.rs | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/key.rs b/src/key.rs index 04a1522..791b63b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -824,15 +824,18 @@ impl XOnlyPublicKey { /// Tweak an x-only PublicKey by adding the generator multiplied with the given tweak to it. /// - /// Returns a boolean representing the parity of the tweaked key, which can be provided to + /// # Return + /// An opaque type representing the parity of the tweaked key, this should be provided to /// `tweak_add_check` which can be used to verify a tweak more efficiently than regenerating - /// it and checking equality. Will return an error if the resulting key would be invalid or - /// if the tweak was not a 32-byte length slice. + /// it and checking equality. + /// + /// # Error + /// If the resulting key would be invalid or if the tweak was not a 32-byte length slice. pub fn tweak_add_assign( &mut self, secp: &Secp256k1, tweak: &[u8], - ) -> Result { + ) -> Result { if tweak.len() != 32 { return Err(Error::InvalidTweak); } @@ -856,12 +859,11 @@ impl XOnlyPublicKey { &mut parity, &pubkey, ); - if err == 0 { - Err(Error::InvalidPublicKey) - } else { - Ok(parity != 0) + return Err(Error::InvalidPublicKey); } + + Ok(parity.into()) } } @@ -878,7 +880,7 @@ impl XOnlyPublicKey { &self, secp: &Secp256k1, tweaked_key: &Self, - tweaked_parity: bool, + tweaked_parity: Parity, tweak: [u8; 32], ) -> bool { let tweaked_ser = tweaked_key.serialize(); @@ -886,7 +888,7 @@ impl XOnlyPublicKey { let err = ffi::secp256k1_xonly_pubkey_tweak_add_check( secp.ctx, tweaked_ser.as_c_ptr(), - if tweaked_parity { 1 } else { 0 }, + tweaked_parity.into(), &self.0, tweak.as_c_ptr(), ); @@ -896,6 +898,21 @@ impl XOnlyPublicKey { } } +/// Opaque type used to hold the parity passed between FFI function calls. +pub struct Parity(i32); + +impl From for Parity { + fn from(parity: i32) -> Parity { + Parity(parity) + } +} + +impl From for i32 { + fn from(parity: Parity) -> i32 { + parity.0 + } +} + impl CPtr for XOnlyPublicKey { type Target = ffi::XOnlyPublicKey; fn as_c_ptr(&self) -> *const Self::Target { From ede114fb1a82206824dc678c3ea1a19df718f41d Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 4 Jan 2022 08:52:56 +1100 Subject: [PATCH 5/5] Improve docs on tweak_add_check method It is not immediately apparent what 'err == 1' means, one must determine that the FFI function call returns 1 for success. We can help readers of the code by adding a 'Return' section to the method documentation. Add trailing full stop to method docs initial line also. --- src/key.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/key.rs b/src/key.rs index 791b63b..8415ac7 100644 --- a/src/key.rs +++ b/src/key.rs @@ -867,7 +867,7 @@ impl XOnlyPublicKey { } } - /// Verify that a tweak produced by `tweak_add_assign` was computed correctly + /// Verify that a tweak produced by `tweak_add_assign` was computed correctly. /// /// Should be called on the original untweaked key. Takes the tweaked key and /// output parity from `tweak_add_assign` as input. @@ -876,6 +876,9 @@ impl XOnlyPublicKey { /// and checking equality. However, in future this API will support batch /// verification, which is significantly faster, so it is wise to design /// protocols with this in mind. + /// + /// # Return + /// True if tweak and check is successful, false otherwise. pub fn tweak_add_check( &self, secp: &Secp256k1,