[WIP] Merge our own rust-secp256k1-zkp into rust-bitcoin/rust-secp256k1-zkp #1

quentinlesceller · 2021-04-08T18:56:28Z

As titled. Work in progress.

GeneFerneau · 2021-04-09T22:25:53Z

src/ecdh.rs

-
-    #[test]
-    fn ecdh_with_hash_callback() {
-        let s = Secp256k1::signing_only();
-        let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
-        let expect_result: [u8; 64] = [123; 64];
-        let mut x_out = [0u8; 32];
-        let mut y_out = [0u8; 32];
-        let result = SharedSecret::new_with_hash(&pk1, &sk1, |x, y| {
-            x_out = x;
-            y_out = y;
-            expect_result.into()
-        });
-        assert_eq!(&expect_result[..], &result[..]);
-        assert_ne!(x_out, [0u8; 32]);
-        assert_ne!(y_out, [0u8; 32]);
-    }
-
-    #[test]
-    fn test_c_callback() {
-        let x = [5u8; 32];
-        let y = [7u8; 32];
-        let mut output = [0u8; 64];
-        let res = unsafe { super::c_callback(output.as_mut_ptr(), x.as_ptr(), y.as_ptr(), ::ptr::null_mut()) };
-        assert_eq!(res, 1);
-        let mut new_x = [0u8; 32];
-        let mut new_y = [0u8; 32];
-        new_x.copy_from_slice(&output[..32]);
-        new_y.copy_from_slice(&output[32..]);
-        assert_eq!(x, new_x);
-        assert_eq!(y, new_y);
-    }


Why are these tests removed?

I've reintroduced both tests along with the unsafe c_callback function. However for some reason x_out is NOT null thus the ecdh_with_hash_callback test fails.

Do we have any idea why the test is failing here?

GeneFerneau · 2021-04-09T22:27:47Z

src/key.rs

    #[inline]
    /// Negates one secret key.
    pub fn negate_assign(
        &mut self
    ) {
        unsafe {
-            let res = ffi::secp256k1_ec_seckey_negate(
+            let res = ffi::secp256k1_ec_privkey_negate(


These APIs are deprecated upstream. It would be a good idea to update the FFI library to use the latest seckey version.

Couldn't agree more. For now, I think it's safe to use the deprecated one until someone add the newer function into our secp256k1-zkp.

GeneFerneau · 2021-04-09T22:31:02Z

secp256k1-sys/src/lib.rs

@@ -258,239 +300,128 @@ impl hash::Hash for KeyPair {
 }

 extern "C" {
-    /// Default ECDH hash function
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_ecdh_hash_function_default")]


Why is symbol renaming removed?

GeneFerneau · 2021-04-09T22:32:46Z

secp256k1-sys/src/lib.rs

-    // Extra keys
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_keypair_create")]
-    pub fn secp256k1_keypair_create(
-        cx: *const Context,
-        keypair: *mut KeyPair,
-        seckey: *const c_uchar,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_xonly_pubkey_parse")]
-    pub fn secp256k1_xonly_pubkey_parse(
-        cx: *const Context,
-        pubkey: *mut XOnlyPublicKey,
-        input32: *const c_uchar,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_xonly_pubkey_serialize")]
-    pub fn secp256k1_xonly_pubkey_serialize(
-        cx: *const Context,
-        output32: *mut c_uchar,
-        pubkey: *const XOnlyPublicKey,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_xonly_pubkey_from_pubkey")]
-    pub fn secp256k1_xonly_pubkey_from_pubkey(
-        cx: *const Context,
-        xonly_pubkey: *mut XOnlyPublicKey,
-        pk_parity: *mut c_int,
-        pubkey: *const PublicKey,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_xonly_pubkey_tweak_add")]
-    pub fn secp256k1_xonly_pubkey_tweak_add(
-        cx: *const Context,
-        output_pubkey: *mut PublicKey,
-        internal_pubkey: *const XOnlyPublicKey,
-        tweak32: *const c_uchar,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_keypair_xonly_pub")]
-    pub fn secp256k1_keypair_xonly_pub(
-        cx: *const Context,
-        pubkey: *mut XOnlyPublicKey,
-        pk_parity: *mut c_int,
-        keypair: *const KeyPair
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_keypair_xonly_tweak_add")]
-    pub fn secp256k1_keypair_xonly_tweak_add(
-        cx: *const Context,
-        keypair: *mut KeyPair,
-        tweak32: *const c_uchar,
-    ) -> c_int;
-
-    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_4_0_xonly_pubkey_tweak_add_check")]
-    pub fn secp256k1_xonly_pubkey_tweak_add_check(
-        cx: *const Context,
-        tweaked_pubkey32: *const c_uchar,
-        tweaked_pubkey_parity: c_int,
-        internal_pubkey: *const XOnlyPublicKey,
-        tweak32: *const c_uchar,
-    ) -> c_int;


Are these unused in Grin?

All of these functions does not yet exist in https://github.com/mimblewimble/secp256k1-zkp

Can we leave these in and stub them out (for now) in the lib? This would allow us to remain closer to upstream here.

GeneFerneau · 2021-04-09T22:35:38Z

secp256k1-sys/src/lib.rs

@@ -530,7 +749,7 @@ extern "C" {
 //  In:      flags: which parts of the context to initialize.
 #[no_mangle]
 #[cfg(all(feature = "std", not(rust_secp_no_symbol_renaming)))]
-pub unsafe extern "C" fn rustsecp256k1_v0_4_0_context_create(flags: c_uint) -> *mut Context {


nit: Since upstream uses symbol renaming to prevent collisions, maybe keeping this convention is a good idea? Pretty minor, though.

It is annoying when doing development that has functionally the same library for most of it, but preventing symbol collision could improve security.

GeneFerneau · 2021-04-09T22:36:55Z

secp256k1-sys/src/lib.rs

-
-    /// Verifies that sig is msg32||pk[32..]
-    pub unsafe fn secp256k1_schnorrsig_verify(
-        cx: *const Context,
-        sig64: *const c_uchar,
-        msg32: *const c_uchar,
-        pubkey: *const XOnlyPublicKey,
-    ) -> c_int {
-        // Check context is built for verification
-        let mut new_pk = PublicKey::new();
-        let _ = secp256k1_xonly_pubkey_tweak_add(cx, &mut new_pk, pubkey, msg32);
-        // Actually verify
-        let sig_sl = slice::from_raw_parts(sig64 as *const u8, 64);
-        let msg_sl = slice::from_raw_parts(msg32 as *const u8, 32);
-        if &sig_sl[..32] == msg_sl && sig_sl[32..] == (*pubkey).0[..32] {
-            1
-        } else {
-            0
-        }
-    }
-
-    /// Sets sig to msg32||pk[..32]
-    pub unsafe fn secp256k1_schnorrsig_sign(
-        cx: *const Context,
-        sig64: *mut c_uchar,
-        msg32: *const c_uchar,
-        keypair: *const KeyPair,
-        noncefp: SchnorrNonceFn,
-        noncedata: *const c_void
-    ) -> c_int {
-        // Check context is built for signing
-        let mut new_kp = KeyPair::new();
-        if secp256k1_keypair_create(cx, &mut new_kp, (*keypair).0.as_ptr()) != 1 {
-            return 0;
-        }
-        assert_eq!(new_kp, *keypair);
-        // Sign
-        let sig_sl = slice::from_raw_parts_mut(sig64 as *mut u8, 64);
-        let msg_sl = slice::from_raw_parts(msg32 as *const u8, 32);
-        sig_sl[..32].copy_from_slice(msg_sl);
-        sig_sl[32..].copy_from_slice(&new_kp.0[32..64]);
-        1
-    }


Why were these removed from the fuzzing mocks?

GeneFerneau · 2021-04-09T22:38:22Z

src/constants.rs

+/// The maximum size of a message embedded in a range proof
+#[cfg(not(feature = "bullet-proof-sizing"))]
+pub const PROOF_MSG_SIZE: usize = 2048;
+#[cfg(feature = "bullet-proof-sizing")]
+pub const PROOF_MSG_SIZE: usize = 2048;


Why the conditional compilation with the same values?

This is directly taken from the base repo https://github.com/mimblewimble/rust-secp256k1-zkp. I agree that it does not make sense for me too. I'd think about removing the not case.

I'm guessing this is a holdover from an old change somewhere. Presumably they used to have different values.
Should be no problem removing this conditional logic.

GeneFerneau · 2021-04-09T22:49:37Z

Did an initial review with some questions, and a couple minor suggestions. Skipped reviewing aggsig.rs and pedersen.rs for now, per discussion on keybase.

Will review those files when this is closer to being final.

GeneFerneau · 2021-04-09T22:53:18Z

secp256k1-sys/depend/scratch_impl.h.patch

@@ -1,26 +0,0 @@
-13,37d12


Instead of making secp256k1-zkp a submodule, it would be easier to track changes against upstream with those files inline.

What are your thoughts on including secp256k1-zkp in a future version of this PR?

Yeah I'm wondering that too. I'd be closer to upstream too.

antiochp

Couple of comments.

antiochp · 2021-04-15T18:05:26Z

src/constants.rs

+/// The maximum size of a message embedded in a range proof
+#[cfg(not(feature = "bullet-proof-sizing"))]
+pub const PROOF_MSG_SIZE: usize = 2048;
+#[cfg(feature = "bullet-proof-sizing")]
+pub const PROOF_MSG_SIZE: usize = 2048;


I'm guessing this is a holdover from an old change somewhere. Presumably they used to have different values.
Should be no problem removing this conditional logic.

antiochp · 2021-04-15T18:07:08Z

src/schnorrsig.rs

@@ -1,784 +0,0 @@
-//! # schnorrsig


Is this removed because we do not use this?
Does it make it harder to keep this fork sync'd with upstream if we remove this? Is it worth considering leaving it in for ease of keeping the repo up to date?

antiochp · 2021-04-15T18:09:01Z

secp256k1-sys/depend/secp256k1-HEAD-revision.txt

@@ -1,2 +0,0 @@
-# This file was automatically created by ./vendor-libsecp.sh


Probably a dumb question but we are doing this to use the mimblewimble specific libs here?
And it replaces all the removed vendored code below?

antiochp · 2021-04-15T18:10:41Z

src/macros.rs

+            .map($mapfn)
+            .collect::<Vec<_>>();
+    }
+}


Nit: newline at end of file.

antiochp · 2021-04-15T18:11:39Z

src/lib.rs

@@ -144,7 +145,8 @@ mod context;
 pub mod constants;
 pub mod ecdh;
 pub mod key;
-pub mod schnorrsig;


See comment below. Is it worth leaving schnorrsig in place and simply not exposing it here?

quentinlesceller added 8 commits April 8, 2021 14:54

Delete secp256k1

036ca62

Add our own secp256k1-zkp

0edd026

Change build.rs based on mimblewimble/rust-secp256k1-zkp#71

42126d8

Remove renaming

aa930a8

Remove inexistant functions

4920bb2

Remove schnorrsig from build.rs

843b4de

Fix ecdh.rs with old API

1e712a1

Add aggsig.rs from mw repo

706e104

quentinlesceller force-pushed the grin branch from 7f2951e to 706e104 Compare April 8, 2021 19:43

quentinlesceller added 13 commits April 8, 2021 15:56

Add missing aggsig and rangeproofs bindings

3dcebac

Add missing macro

865c354

Add missing scratch space functions

ddad19f

Add missing types

480af90

Adapt aggsig

c4eed03

Add pedersen.rs

559d9fe

Add missing constants

e1582e2

Add commitment context

aef1f11

Rename commitment to commit

f9a5eb2

Adapt pedersen.rs

fafca0a

Formatting, cleanup and rename of function

36b7f9f

Add missing shcnorrsig in build.rs

824bec4

Fix tests

7fe5316

GeneFerneau reviewed Apr 9, 2021

View reviewed changes

Reintroduce c_callback and ecdh_with_hash_callback test (fails)

70f25c4

antiochp reviewed Apr 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Merge our own rust-secp256k1-zkp into rust-bitcoin/rust-secp256k1-zkp #1

[WIP] Merge our own rust-secp256k1-zkp into rust-bitcoin/rust-secp256k1-zkp #1

quentinlesceller commented Apr 8, 2021

GeneFerneau Apr 9, 2021

quentinlesceller Apr 14, 2021

antiochp Apr 15, 2021

GeneFerneau Apr 9, 2021

quentinlesceller Apr 14, 2021

GeneFerneau Apr 9, 2021

GeneFerneau Apr 9, 2021

quentinlesceller Apr 14, 2021

antiochp Apr 15, 2021

GeneFerneau Apr 9, 2021 •

edited

Loading

GeneFerneau Apr 9, 2021

GeneFerneau Apr 9, 2021

quentinlesceller Apr 14, 2021

antiochp Apr 15, 2021

GeneFerneau commented Apr 9, 2021

GeneFerneau Apr 9, 2021

quentinlesceller Apr 14, 2021

antiochp left a comment

antiochp Apr 15, 2021

antiochp Apr 15, 2021

antiochp Apr 15, 2021

antiochp Apr 15, 2021

antiochp Apr 15, 2021

		@@ -1,2 +0,0 @@
		# This file was automatically created by ./vendor-libsecp.sh

[WIP] Merge our own rust-secp256k1-zkp into rust-bitcoin/rust-secp256k1-zkp #1

Are you sure you want to change the base?

[WIP] Merge our own rust-secp256k1-zkp into rust-bitcoin/rust-secp256k1-zkp #1

Conversation

quentinlesceller commented Apr 8, 2021

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

GeneFerneau Apr 9, 2021 • edited Loading

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

GeneFerneau commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antiochp 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

GeneFerneau Apr 9, 2021 •

edited

Loading