Add bindings to the ElligatorSwift implementation #627

Davidson-Souza · 2023-07-13T15:06:50Z

~~Marking as draft as this is an unreleased feature from libsecp~~

From upstream:
This implements encoding of curve points using the ElligatorSwift algorithm, using 4 new API calls:

secp256k1_ellswift_encode, which converts a public key to a 64-byte pseudorandom encoding.
secp256k1_ellswift_decode, the reverse operation to convert back to normal public keys.
secp256k1_ellswift_create, which can be seen as a combination of secp256k1_ec_pubkey_create + secp256k1_ellswift_encode, but is somewhat safer.
secp256k1_ellswift_xdh, which implements x-only Diffie-Hellman directly on top of 64-byte encoded public keys, and more efficiently than decoding + invoking normal ECDH.

This algorithm allows mapping any pair of field elements (u, t) to a (valid) x coordinate in the curve. This allows representing a field element as a 64-bytes bit string that is indistinguishable from random. You can build a pair of (u, t) from any group element as well.
We also have an integrated x-only ECDH that can be used to establish a shared secret between two parties. All algorithms are compatible with BIP324 and are tested against the BIP's test cases.

I have a few questions about the rust side of the implementation:
Should it be always on, or leave it behind a cargo feature? In libsecp this module is default on, but you can disable it.
I'm not exposing the low-level functions, instead you can use high-level types to interact with ellswift. Is it reasonable to also expose a safe version of the functions above?

elichai · 2023-07-19T19:01:54Z

src/ellswift.rs

+        aux_rand: Option<[u8; 32]>,
+    ) -> ElligatorSwift {
+        let mut es_out = [0u8; constants::ELLSWIFT_ENCODING_SIZE];
+        let aux_rand = aux_rand.map(|rand| rand.as_c_ptr()).unwrap_or(std::ptr::null());


aux_rand here is a dangling pointer, you should either make it an Option<&[u8; 32]> or do:

let aux_rand_ptr = aux_rand.as_ref().map(|rand| rand.as_c_ptr()).unwrap_or(std::ptr::null());

Hey @elichai I can't work out your comment, doesn't the array coerce to a slice and as_ptr is called on it the same as it would be in your code?

Let's look at this function expanded a bit:

let aux_rand: Option<[u8; 32]>; // This is on the function's stack as a function argument let map_function: FnOnce([u8; 32]) -> *const u8 = { // This is a new stack for this function. let aux_rand: [u8; 32]; // this is on the closure's stack as a function argument. let ptr = <[u8]>::as_c_ptr(deref_to_slice(&aux_rand)); // this creates a slice pointing at `aux_rand` in the closure's stack, it then passes it to `as_c_ptr` which then returns a pointer to that slice. ptr // returns a pointer to the `aux_rand` that is on the closure's stack, and will get invalidated immediately }; let aux_rand = Option::map(aux_rand, map_function); // Copies the original `aux_rand` into the `Option::map` stack (which will then pass it into `map_function`)

elichai · 2023-07-19T19:06:06Z

src/ellswift.rs

+        secret_key: SecretKey,
+        party: ElligatorSwiftParty,
+        data: Option<Vec<u8>>,
+        hasher: EllswiftECDHHashFn,


Feel free to look here for a rustier API: af8fa21

Davidson-Souza · 2023-07-24T19:06:49Z

Pushed a680bc0 addressing @elichai's comments.

secp256k1-sys/src/lib.rs

Adidev-KGP · 2023-08-13T19:56:31Z

secp256k1-sys/src/lib.rs

+        ctx: *const Context,
+        ell64: *mut c_uchar,
+        seckey32: *const c_uchar,
+        aux_rand32: *const c_uchar,


The rustsecp256k1_v0_8_1_ellswift_create has aux_rand32 as an optional parameter.

SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int rustsecp256k1_v0_8_1_ellswift_create( const rustsecp256k1_v0_8_1_context *ctx, unsigned char *ell64, const unsigned char *seckey32, const unsigned char *auxrnd32 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

since SECP256K1_ARG_NONNULL(4) is absent we know that auxrnd32 is an optional parameter.
The rust binding aux_rand32: *const c_uchar doesn't let aux_rand32 to be an optional parameter. So is it possible to achieve this state in Rust?

One way it can be done in an FFI safe way is by defining a repr(C) attribute :

#[repr(C)] pub enum COptionPtr<T> { Some(T), None, }

outside the extern "C" block.

Basically, COptionPtr is an enum that represents an Option type in a way that is FFI-safe. It has two variants: Some which wraps the pointer, and None. The enum itself has a clear representation in both Rust and C but Option<> doesn't have so we cannot use it directly for a C function we want to bind in Rust.

Now using aux_rand32: COptionPtr<*const c_uchar> definition here aux_rand32 can be set as an optional parameter.

Additionally, we will need to have the following changes to ellswift.rs:

include COptionPtr here

replace this line with

let aux_rand = aux_rand.map(|rand| COptionPtr::Some(rand.as_c_ptr())).unwrap_or(COptionPtr::Some(std::ptr::null()));

Pass a COptionPtr type COptionPtr::Some(rand32.as_ptr()) to secp256k1_ellswift_create function on this line

Tried the above way to set aux_rand32 as an optional parameter. The code compiles but when running against test vectors signal: 11, SIGSEGV: invalid memory reference comes.

I see what you mean. I'm using the same approach as other parts of the lib where optional data can be passed in, like inrustsecp256k1_v0_8_1_ecdh. But I'll try something with the suggested COptionPtr.

secp256k1-sys/src/lib.rs

Adidev-KGP · 2023-08-14T17:15:40Z

secp256k1-sys/src/lib.rs

+        seckey32: *const c_uchar,
+        party: c_int,
+        hashfp: EllswiftECDHHashFn,
+        data: *const u8,


data is optional in rustsecp256k1_v0_8_1_ellswift_xdh. How do we do it in rust bindings? (Similar to previous review comment)

Davidson-Souza · 2023-08-22T16:59:39Z

Resolved most of @Adidev-KGP's comments. Only the COptionPtr is open because it needs more tinkering.
Furthermore, I think it's ok to remove the draft status, even though libsecp didn't release it officially. Bitcoin Core, for example, already uses this feature. Any thoughts?

apoelstra · 2023-09-19T00:16:46Z

@Davidson-Souza would you be willing to split out the 0.4.0 update into its own PR?

Alternately, could you undraft this?

sanket1729 · 2023-09-20T18:42:05Z

Having a separate PR for 0.4.0 will help us make quicker progress here.

Davidson-Souza · 2023-09-20T20:51:58Z

I definitely can do that! Do you want the clean-up commits from #645 or just this update?

tcharding · 2023-09-21T03:16:06Z

I'd say we need everything from #645 but with 0.4.0 instead of 0.3.2 (incl. version bump and ci fix).

tcharding · 2023-09-25T23:22:15Z

We want this in before release still though, right?

apoelstra · 2023-09-26T13:14:02Z

@tcharding yeah, I think so. Though if we can do it in a strictly additive way (I think we can) we can do a point release with it so we probably don't need to hold up the release.

tcharding · 2023-09-26T20:01:00Z

Cool, I went over all the changes in the last patch quickly and all lines are green.

80b2a8d Update vendored libsecp to v0.4.0 (Davidson Souza) d2285c9 ci: Remove MIPS* from CI (Davidson Souza) 0d58f50 ci: generalize grp in "illegal callback" test (Andrew Poelstra) acf9ac1 delete `test_manual_create_destroy` test (Andrew Poelstra) 04ce508 lib: fix bad unit test (Andrew Poelstra) e4cca90 gitignore: remove things that shouldn't be there (Andrew Poelstra) Pull request description: Replaces #645 and #652. Precedes #627. I'm basically using #652 but resolving the linking problems, My local CI is erring on windows cross-test, but I can compile without issue with `cargo build --target x86_64-pc-windows-gnu`. Some MIPS jobs failed before even installing cross, I think those aren't really related to this PR. Any ideas on what can be happening? ACKs for top commit: apoelstra: ACK 80b2a8d Tree-SHA512: 62c2e04348110e3995111fa666f10dcc403b963770d047361f9209cf45b45db8744a7eb6d9ee3278d18007412dab5131ac3e1dd3e3d704963c6a6f232d57199a

apoelstra · 2023-10-01T15:31:15Z

src/ellswift.rs

+    /// Decode an ElligatorSwift object from a 64-byte array.
+    pub fn from_array(ser: [u8; 64]) -> ElligatorSwift {
+        ElligatorSwift(ffi::ElligatorSwift::from_array(ser))
+    }


In 6cde4c3:

I guess we want a corresponding to_array.

apoelstra · 2023-10-01T15:32:21Z

src/ellswift.rs

+        let mut ser = [0u8; 64];
+        from_hex(hex, &mut ser).unwrap();
+        ElligatorSwift::from_array(ser)
+    }


In 6cde4c3:

We want a FromStr and Display impl here which use from_hex. (I'm ambivalent as to whether from_hex itself should be exposed. I'm inclined to say no, and just use from_str.)

Also this function definitely needs to return a result and not panic when given bad hex :).

apoelstra · 2023-10-01T15:35:01Z

src/ellswift.rs

+    type Target = [u8; 32];
+
+    fn deref(&self) -> &Self::Target { &self.0 }
+}


In 6cde4c3:

I think we should drop this Deref impl. As a rule we avoid using Deref to make "transparent newtypes" like this. In this library the only existing use of Deref is on SerializedSignature which is explicitly a variable-length byte array type and is supposed to behave like an owned slice in every way.

A shared secret, on the other hand, is its own thing (though I appreciate it has no invariants) and should require some explicit action to let you get the bytes out of it. If anything, because these bytes are supposed to be secret so we don't want them to leak accidentally.

apoelstra · 2023-10-01T15:36:25Z

src/ellswift.rs

+            ElligatorSwiftParty::B => 1,
+        }
+    }
+}


In 6cde4c3:

I'm a bit suspicious of giving these explicit values and having a i32 conversion. What is the purpose of this? It does not appear to be used in the API.

I guess these are for use with the FFI functions. In this case I'd make an explicit method to_ffi_int and not bother assigning values or implementing a From impl.

Davidson-Souza · 2023-10-02T17:14:28Z

Marking as ready for review. I've addressed @apoelstra's comments, rebased with #653 and fixed all CI's complaints.

tcharding

I do not work at this level very often, if my review includes anything braindead feel free to just say so :)

tcharding · 2023-10-03T04:40:13Z

secp256k1-sys/src/lib.rs

@@ -82,6 +82,16 @@ pub type SchnorrNonceFn = Option<unsafe extern "C" fn(
    data: *mut c_void,
 ) -> c_int>;

+pub type EllswiftECDHHashFn = Option<


nit: Acronyms should be lowercase eg, EllswiftEcdhHashFn

Could have a rustdoc comment (although I note we have other public things that do not).

We should follow the Rust convention everywhere. If there are existing violations we should fix those.

Warning: Incoming clean up PRs to secp256k1-sys :)

tcharding · 2023-10-03T04:43:09Z

secp256k1-sys/src/lib.rs

+        ell_b64: *const c_uchar,
+        data: *mut c_void,
+    ) -> c_int,
+>;


This code block is formatted differently to the surrounding code, can we have it the same please? (Has to be done manually, we skip formatting for secp256k1-sys in rustfmt.toml.)

tcharding · 2023-10-03T04:47:58Z

secp256k1-sys/src/lib.rs

@@ -517,11 +527,44 @@ impl core::hash::Hash for Keypair {
    }
 }

+pub struct XOnlySharedSecret(pub [u8; 32]);


This type is not used in secp256k1 what is it for?

tcharding · 2023-10-03T04:53:12Z

secp256k1-sys/src/lib.rs

+}
+
+impl_array_newtype!(XOnlySharedSecret, u8, 32);
+impl_raw_debug!(XOnlySharedSecret);


Do we want to enable debugging output to contain the secret like this? I don't know the answer, the question is to flag it.

I think it's fine to output the raw bytes in secp256k1-sys. (Right now I don't think secp256k1-sys has any secret key types; we use pointers to uchars for secret keys.)

In the corresponding secp256k1 type I think we should re-hash before Debug output, similar to what we do for SecretKey.

tcharding · 2023-10-03T04:54:07Z

secp256k1-sys/src/lib.rs

+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ElligatorSwift([u8; 64]);


A few comments/questions on this type:

Would be nice to have a comment on this type.

Should have #[repr(C)] because it is passed across the ffi boundry, right?

tcharding · 2023-10-03T05:13:39Z

src/ellswift.rs

+            secp256k1_ellswift_xdh(
+                ffi::secp256k1_context_no_precomp,
+                shared_secret.as_mut_c_ptr(),
+                ellswift_a.0.as_c_ptr(),


Perhaps we should implement ffi::CPtr on ElligatorSwift so we don't have to use .0?

tcharding · 2023-10-03T05:33:44Z

src/ellswift.rs

+use secp256k1_sys::types::{c_int, c_uchar, c_void};
+
+use crate::ffi::{
+    self, secp256k1_ellswift_create, secp256k1_ellswift_decode, secp256k1_ellswift_encode, CPtr,


Perhaps don't import these and use ffi:: on all callsites so its explicit we are going across the ffi boundry? Above on line 42 as well.

tcharding · 2023-10-03T05:43:06Z

secp256k1-sys/src/lib.rs

@@ -600,6 +643,38 @@ extern "C" {
                                 output_pubkey: *mut PublicKey,
                                 keypair: *const Keypair)
                                 -> c_int;
+    // Elligator Swift
+    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_9_0_ellswift_encode")]
+    pub fn secp256k1_ellswift_encode(


All the code here and below uses different whitespace and formatting to the surrounding code, uniform would be better IMO.

tcharding · 2023-10-03T05:45:19Z

src/ellswift.rs

+    ) -> ElligatorSwiftSharedSecret {
+        let mut shared_secret = [0u8; 32];
+        unsafe {
+            secp256k1_ellswift_xdh(


Is there a reason that the return value of this function call is not checked?

We should debug_assert it. It can fail if one of the ElligatorSwift objects is invalid (prevented at the type level) or if hashfp fails (which it can't because hash_callback never returns 0).

Added a debug_assert_eq in all calls of C code.

tcharding · 2023-10-03T05:50:26Z

src/ellswift.rs

+                &mut hash_function as *mut F as *mut c_void,
+            );
+        }
+        ElligatorSwiftSharedSecret(shared_secret)


What is the state of shared_secret if the ffi function call fails?

Probably "undefined" but in a safe way. But I think this is irrelevant since the FFI function can't fail (see above).

Davidson-Souza · 2023-10-03T20:29:37Z

Forced-pushed 505b079. I think I've addressed all @tcharding comments.

tcharding

A bunch more suggestions, I hope I'm not dragging this out too much for you. Thanks for sticking with it.

tcharding · 2023-10-03T21:59:19Z

secp256k1-sys/src/lib.rs

+    ell_b64: *const c_uchar,
+    data: *mut c_void,
+) -> c_int>;
+


Legend, thanks for implementing the review suggestions!

tcharding · 2023-10-03T22:03:45Z

src/key.rs

@@ -450,6 +451,12 @@ impl PublicKey {
            PublicKey(pk)
        }
    }
+    /// Creates a new public key from a [`ElligatorSwift`] and the global [`SECP256K1`] context.


Suggested change

/// Creates a new public key from a [`ElligatorSwift`] and the global [`SECP256K1`] context.

/// Creates a new public key from an [`ElligatorSwift`].

ElligatorSwift::ellswift_decode uses ffi::secp256k1_context_no_precomp .

That's actually a good observation. Should it use secp256k1_context_no_precomp? I think it make sense because the ES encode/decode only uses field operations, and AFAIK, the precomp helps with group operations. But I might be wrong on that, any thought @apoelstra?

Yep, if no_precomp works (and it definitely will if this is really just field ops, no ecmults) then we should use it.

src/key.rs

src/ellswift.rs

tcharding · 2023-10-03T22:49:40Z

src/ellswift.rs

+            ElligatorSwiftParty::A => 0,
+            ElligatorSwiftParty::B => 1,


We are still assigning values here, did you mean to remove those?

I think @apoelstra's comment was about using something externally visible like From. We need, somehow, to get a 0/1 to give the C code, the alternative is to remove this at all and ask the user to explicitly pass 0/1. I think this is more ergonomic and less error-prone (users can't pass an invalid value, like 2).

These values are hard to obtain (I think you need to cast to get them). I think you should drop them and instead have a dedicated method to extract them.

I think you should drop them and instead have a dedicated method to extract them.

Do you mean an API call? Currently, I have this

impl ElligatorSwiftParty { fn to_ffi_int(self) -> c_int { match self { ElligatorSwiftParty::A => 0, ElligatorSwiftParty::B => 1, } } }

and the generated code is pretty straightforward.

Yep, exactly. If we have this function then we don't need to assign values to the enum variants.

Right. That's what I'm doing on 9b9090b

pub enum ElligatorSwiftParty { A, B, } impl ElligatorSwiftParty { fn to_ffi_int(self) -> c_int { match self { ElligatorSwiftParty::A => 0, ElligatorSwiftParty::B => 1, } } }

No enum variant value, but the user exposed API doesn't expose those internal values.

Oh I may have caused the confusion, I saw the to_ffi_int function but I thought there were explicit values on the enum still. This thread is not showing as "outdated" so I'm guessing I was looking in the wrong place. My bad.

tcharding · 2023-10-03T22:51:03Z

src/ellswift.rs

+    }
+}
+
+impl FromStr for ElligatorSwift {


The same question about creating an arbitrary ElligatorSwift apply here, no response needed, just flagging it.

Davidson-Souza · 2023-10-04T17:23:29Z

Updating with the following diff

diff --git a/src/ellswift.rs b/src/ellswift.rs
diff --git a/src/ellswift.rs b/src/ellswift.rs
index 001c4428..0db32957 100644
--- a/src/ellswift.rs
+++ b/src/ellswift.rs
@@ -28,7 +28,7 @@
 //!
 //! If the Y coordinate is relevant, it is given the same parity as t.
 //!
-//! Changes w.r.t. the paper:
+//! Changes w.r.t. the the paper:
 //! - The u=0, t=0, and u^3+t^2+7=0 conditions result in decoding to the point
 //!   at infinity in the paper. Here they are remapped to finite points.
 //! - The paper uses an additional encoding bit for the parity of y. Here the
@@ -42,7 +42,7 @@ use core::str::FromStr;
 use ffi::CPtr;
 use secp256k1_sys::types::{c_int, c_uchar, c_void};
 
-use crate::{constants, ffi, from_hex, Error, PublicKey, Secp256k1, SecretKey, Verification};
+use crate::{constants, ffi, from_hex, Context, Error, PublicKey, Secp256k1, SecretKey};
 
 unsafe extern "C" fn hash_callback<F>(
     output: *mut c_uchar,
@@ -72,7 +72,7 @@ where
 }
 
 /// `ElligatorSwift` is an encoding of a uniformly chosen point on the curve
-/// as a 64-byte array that is indistinguishable from a uniformly random array.
+/// as a 64-byte array that is indistinguishable from a uniformly random.
 /// This object holds two field elements u and t, which are the inputs to
 /// the `ElligatorSwift` encoding function.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -80,26 +80,26 @@ pub struct ElligatorSwift(ffi::ElligatorSwift);
 
 impl ElligatorSwift {
     /// Create a new `ElligatorSwift` object from a 64-byte array.
-    pub fn new(secret_key: SecretKey, rand: [u8; 32]) -> ElligatorSwift {
+    pub fn ellswift_create(priv32: SecretKey, rand32: [u8; 32]) -> ElligatorSwift {
         let mut ell_out = [0u8; constants::ELLSWIFT_ENCODING_SIZE];
         unsafe {
             let ret = ffi::secp256k1_ellswift_create(
                 ffi::secp256k1_context_no_precomp,
                 ell_out.as_mut_c_ptr(),
-                secret_key.as_c_ptr(),
-                rand.as_ptr(),
+                priv32.as_c_ptr(),
+                rand32.as_ptr(),
             );
             debug_assert_eq!(ret, 1);
         }
         ElligatorSwift(ffi::ElligatorSwift::from_array(ell_out))
     }
 
-    /// Creates an `ElligatorSwift` object from a 64-byte array.
-    pub fn from_array(ellswift: [u8; 64]) -> ElligatorSwift {
-        ElligatorSwift(ffi::ElligatorSwift::from_array(ellswift))
+    /// Decode an `ElligatorSwift` object from a 64-byte array.
+    pub fn from_array(ser: [u8; 64]) -> ElligatorSwift {
+        ElligatorSwift(ffi::ElligatorSwift::from_array(ser))
     }
 
-    /// Returns the 64-byte array representation of this `ElligatorSwift` object.
+    /// Encode an `ElligatorSwift` object as a 64-byte array.
     pub fn to_array(&self) -> [u8; 64] { self.0.to_array() }
 
     /// Creates the Elligator Swift encoding from a secret key, using some aux_rand if defined.
@@ -114,7 +114,7 @@ impl ElligatorSwift {
     ///     let es = ElligatorSwift::from_seckey(&secp, sk, None);
     /// # }
     /// ```
-    pub fn from_seckey<C: Verification>(
+    pub fn from_seckey<C: Context>(
         secp: &Secp256k1<C>,
         sk: SecretKey,
         aux_rand: Option<[u8; 32]>,
@@ -146,7 +146,7 @@ impl ElligatorSwift {
     /// # }
     ///
     /// ```
-    pub fn from_pubkey(pk: PublicKey) -> ElligatorSwift { Self::encode(pk) }
+    pub fn from_pubkey(pk: PublicKey) -> ElligatorSwift { Self::ellswift_encode(pk) }
 
     /// Computes a shared secret only known by Alice and Bob. This is obtained by computing
     /// the x-only Elliptic Curve Diffie-Hellman (ECDH) shared secret between Alice and Bob.
@@ -231,7 +231,7 @@ impl ElligatorSwift {
     }
 
     /// Encodes a public key into an `ElligatorSwift` encoding
-    fn encode(pk: PublicKey) -> ElligatorSwift {
+    fn ellswift_encode(pk: PublicKey) -> ElligatorSwift {
         let mut ell_out = [0u8; constants::ELLSWIFT_ENCODING_SIZE];
         unsafe {
             let ret = ffi::secp256k1_ellswift_encode(
@@ -245,18 +245,19 @@ impl ElligatorSwift {
         ElligatorSwift(ffi::ElligatorSwift::from_array(ell_out))
     }
 
-    /// Decodes an `ElligatorSwift` encoding into a [`PublicKey`].
-    pub(crate) fn decode(ell: ElligatorSwift) -> PublicKey {
+    /// Decodes an `ElligatorSwift` encoding into a [ffi::PublicKey]. This function is a low
+    /// level function, and will be used in internal functions.
+    pub(crate) fn ellswift_decode(ell: ElligatorSwift) -> ffi::PublicKey {
         unsafe {
             let mut pk = ffi::PublicKey::new();
 
             let ret = ffi::secp256k1_ellswift_decode(
                 ffi::secp256k1_context_no_precomp,
                 pk.as_mut_c_ptr(),
-                ell.as_c_ptr(),
+                ell.0.as_c_ptr(),
             );
             debug_assert_eq!(ret, 1);
-            PublicKey::from(pk)
+            pk
         }
     }
 }
@@ -274,9 +275,9 @@ pub struct ElligatorSwiftSharedSecret([u8; 32]);
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum ElligatorSwiftParty {
     /// We are the initiator of the ECDH
-    A,
+    A = 0,
     /// We are the responder of the ECDH
-    B,
+    B = 1,
 }
 
 impl ElligatorSwiftParty {
diff --git a/src/key.rs b/src/key.rs
index 3b288523..fbc5ece6 100644
--- a/src/key.rs
+++ b/src/key.rs
@@ -451,9 +451,12 @@ impl PublicKey {
             PublicKey(pk)
         }
     }
-    /// Creates a new public key from an [`ElligatorSwift`].
+    /// Creates a new public key from a [`ElligatorSwift`] and the global [`SECP256K1`] context.
     #[inline]
-    pub fn from_ellswift(ellswift: ElligatorSwift) -> PublicKey { ElligatorSwift::decode(ellswift) }
+    pub fn from_ellswift(ellswift: ElligatorSwift) -> PublicKey {
+        let pk = ElligatorSwift::ellswift_decode(ellswift);
+        PublicKey(pk)
+    }

tcharding · 2023-10-04T23:10:33Z

Thanks for the consideration by posting the diff, reviewers can get that by doing git range-diff 505b0797...@ after checking out the branch. I got the commit ID from the tip of the old version that I still had checked out from last review. Andrew caches it sometimes by posting a message to himself "reviewed upto xxxxxxx". (Sharing because I only learned about range-diff when I started hacking in rust-bitcoin :)

tcharding · 2023-10-04T23:12:17Z

And I had a chuckle at myself because confusingly the red/green diff colouring is inverted. I nearly posted "this is a regression" :)

tcharding

ACK 9b9090b

tcharding · 2023-10-04T23:14:49Z

Thanks man, appreciate your effort. I had fun reviewing this PR.

Davidson-Souza · 2023-10-05T14:42:16Z

And I had a chuckle at myself because confusingly the red/green diff colouring is inverted. I nearly posted "this is a regression" :)

Oh, I messed up the order of the arguments, didn't I? Doesn't matter how often I use git diff, I always get it wrong.

tcharding · 2023-10-06T01:44:21Z

And I had a chuckle at myself because confusingly the red/green diff colouring is inverted. I nearly posted "this is a regression" :)

Oh, I messed up the order of the arguments, didn't I? Doesn't matter how often I use git diff, I always get it wrong.

Yep, me too. One thing that helps me, if I'm diffing a file that I made the changes to, then the args are ordered in time order, so the "old" original file is on the left and the "new" edited file is on the right.

apoelstra · 2023-10-07T14:17:18Z

src/ellswift.rs

+    // Copy the output from a [ElligatorSwiftSharedSecret] into the output pointer
+    ptr::copy_nonoverlapping(secret.0.as_ptr(), output, secret.0.len());
+
+    secret.0.len() as c_int


In 9b9090b:

I'm a bit confused by this return value. The docs say that an XDH hash function should return 1 on success, 0 on failure. It looks like we have no way to signal failure, which I think is fine, but it seems we are unconditionally returning secret.0.len which will be 64, not 1.

apoelstra · 2023-10-07T14:19:50Z

src/ellswift.rs

+    ) -> ElligatorSwift {
+        let mut es_out = [0u8; constants::ELLSWIFT_ENCODING_SIZE];
+        let aux_rand_ptr =
+            aux_rand.as_ref().map(|rand| rand.as_c_ptr()).unwrap_or(core::ptr::null());


In 9b9090b:

Could you actually just implement CPtr for Option<T> where T: CPtr? Then you could avoid doing this complicated unwrap thing each time.

apoelstra · 2023-10-07T14:23:59Z

9b9090b looks great other than those two nits!

Create bindings for all methods and static types in ellswift.h in secp256k1-sys and their respective safe-rust types. All methods are extensively commented and tested using BIP324's test vectors

tcharding

ACK 39febcb

apoelstra

ACK 39febcb

elichai reviewed Jul 19, 2023

View reviewed changes

Davidson-Souza force-pushed the ellswift-mod branch from 605baf9 to a680bc0 Compare July 24, 2023 19:05

Adidev-KGP reviewed Aug 12, 2023

View reviewed changes

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

Adidev-KGP reviewed Aug 14, 2023

View reviewed changes

Davidson-Souza force-pushed the ellswift-mod branch 3 times, most recently from b9862d3 to f92852a Compare August 22, 2023 16:48

Davidson-Souza force-pushed the ellswift-mod branch from f92852a to fadc4cf Compare August 22, 2023 17:03

Davidson-Souza mentioned this pull request Sep 6, 2023

update vendored libsecp to 0.3.2 #645

Closed

tcharding mentioned this pull request Sep 18, 2023

Update vendored libsecp to 0.4.0 #652

Closed

Davidson-Souza mentioned this pull request Sep 21, 2023

Update vendored lib secp256k1 to v0.4.0 #653

Merged

tcharding mentioned this pull request Sep 25, 2023

Tracking PR for release: secp256k1 v0.28.0 and secp256k-sys 0.10.0 #650

Merged

Davidson-Souza force-pushed the ellswift-mod branch from fadc4cf to 6cde4c3 Compare September 30, 2023 19:25

apoelstra reviewed Oct 1, 2023

View reviewed changes

Davidson-Souza force-pushed the ellswift-mod branch 2 times, most recently from 4a67f0b to d70d402 Compare October 1, 2023 17:41

Davidson-Souza force-pushed the ellswift-mod branch from d70d402 to 14db029 Compare October 2, 2023 16:58

Davidson-Souza marked this pull request as ready for review October 2, 2023 17:08

Davidson-Souza force-pushed the ellswift-mod branch from 14db029 to 9bad710 Compare October 2, 2023 17:11

tcharding reviewed Oct 3, 2023

View reviewed changes

Davidson-Souza force-pushed the ellswift-mod branch from 9bad710 to 505b079 Compare October 3, 2023 19:58

tcharding reviewed Oct 3, 2023

View reviewed changes

Davidson-Souza force-pushed the ellswift-mod branch from 505b079 to 9b9090b Compare October 4, 2023 17:23

tcharding approved these changes Oct 4, 2023

View reviewed changes

apoelstra reviewed Oct 7, 2023

View reviewed changes

Create rust-bidings

39febcb

Create bindings for all methods and static types in ellswift.h in secp256k1-sys and their respective safe-rust types. All methods are extensively commented and tested using BIP324's test vectors

Davidson-Souza force-pushed the ellswift-mod branch from 9b9090b to 39febcb Compare October 9, 2023 17:58

tcharding approved these changes Oct 9, 2023

View reviewed changes

apoelstra approved these changes Oct 9, 2023

View reviewed changes

apoelstra merged commit 3aada83 into rust-bitcoin:master Oct 9, 2023
20 checks passed

		#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
		pub struct ElligatorSwift([u8; 64]);

	/// Creates a new public key from a [`ElligatorSwift`] and the global [`SECP256K1`] context.
	/// Creates a new public key from an [`ElligatorSwift`].

Add bindings to the ElligatorSwift implementation #627

Add bindings to the ElligatorSwift implementation #627

Conversation

Davidson-Souza commented Jul 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidson-Souza commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidson-Souza commented Aug 22, 2023

apoelstra commented Sep 19, 2023

sanket1729 commented Sep 20, 2023

Davidson-Souza commented Sep 20, 2023

tcharding commented Sep 21, 2023

tcharding commented Sep 25, 2023

apoelstra commented Sep 26, 2023

tcharding commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidson-Souza commented Oct 2, 2023

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidson-Souza commented Oct 3, 2023

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Oct 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidson-Souza commented Oct 4, 2023

tcharding commented Oct 4, 2023

tcharding commented Oct 4, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Oct 4, 2023

Davidson-Souza commented Oct 5, 2023

tcharding commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Oct 7, 2023

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Davidson-Souza commented Jul 13, 2023 •

edited

elichai Jul 24, 2023 •

edited

tcharding Oct 3, 2023 •

edited

tcharding Oct 3, 2023 •

edited

tcharding Oct 3, 2023 •

edited

tcharding Oct 6, 2023 •

edited