implement bls12-381 functions #1345

jayz22 · 2024-09-19T22:18:32Z

Corresponding env change: stellar/rs-soroban-env#1456

bump protocol_version locally add basic g1/g2_add test expose dst add mult-pairing-check update scalar to use U256 representation update env update env pick up xdr change bump sdk version implement bls12-381

Cargo.toml

soroban-sdk/src/crypto.rs

dmkozh · 2024-09-19T23:29:01Z

soroban-sdk/src/crypto.rs

+        let env = self.env();
+        let bin = internal::Env::bls12_381_g1_add(env, p0.to_object(), p1.to_object())
+            .unwrap_infallible();
+        G1Affine::from_bytes(bin.try_into_val(env).unwrap_optimized())


nit: Could try_into_val.unwrap be just into_val?

We should be able to do this with an unchecked op if we know for sure bin is always of the correct size. We do that in some areas of the SDK.

soroban-sdk/src/crypto.rs

soroban-sdk/src/tests/crypto_bls12_381.rs

dmkozh

LGTM for now, I think we can clean up the 'namespacing' and add more tests as a followup.

soroban-sdk/src/crypto.rs

leighmcculloch

Few comments inline.

soroban-sdk/src/crypto.rs

leighmcculloch · 2024-09-19T23:53:03Z

soroban-sdk/src/crypto.rs

+    env: Env,
+}
+
+/// # `G1Affine` is a point in the G1 group (subgroup defined over the base field


It's not common to make the first line of these doc comments a heading. Is there a reason we need it to be?

Suggested change

/// # `G1Affine` is a point in the G1 group (subgroup defined over the base field

/// `G1Affine` is a point in the G1 group (subgroup defined over the base field

leighmcculloch · 2024-09-19T23:53:32Z

soroban-sdk/src/crypto.rs

+        // #[derive(Clone)]
+        // #[repr(transparent)]
+        // pub struct $elem(BytesN<$size>);


leighmcculloch · 2024-09-19T23:55:08Z

soroban-sdk/src/crypto.rs

+#[repr(transparent)]
+pub struct Fp2(BytesN<96>);
+
+macro_rules! impl_bls_elements_bytes_repr {


This looks useful for types that wrap BytesN in general. Could we move this macro into the bytes file and name it something like impl_bytesn_repr?

leighmcculloch · 2024-09-20T00:16:53Z

soroban-sdk/src/crypto.rs

+    }
+
+    /// Adds two points `p0` and `p1` in G1.
+    pub fn g1_add(&self, p0: &G1Affine, p1: &G1Affine) -> G1Affine {


It looks like most of the functions on the Bls12_381 type orient around the bls types. For example, this one here g1_add could be a function on g1.

In general I quite like these style of APIs where modules contain the functionality and types are just data.

However it introduces some awkwardness when writing contracts.

If a developer writes code that uses the G1, or other, types, then they might be passing them around. If all the operations are on the env, they need to get the env either from the G1 or by passing it around too. In rust docs it also becomes harder to discover all the operations that can be done on a g1, g2, etc.

For some the operations, like add, we can impl the core lib's traits, e.g. Add.

fn do_things(a: G1Affine, b: G1Affine) /* -> ... */ { let c = a.env().crypto().bls12_381().g1_add(a, b); // ... }

vs

fn do_things(a: G1Affine, b: G1Affine) /* -> ... */ { let c = a.add(b); // ... }

leighmcculloch · 2024-09-20T00:28:43Z

soroban-sdk/src/crypto.rs

+                write!(f, "{}({:?})", $name, self.to_array())?;
+                Ok(())


Shouldn't need the branch return then okay.

Suggested change

write!(f, "{}({:?})", $name, self.to_array())?;

Ok(())

write!(f, "{}({:?})", $name, self.to_array())

soroban-sdk/src/crypto.rs

leighmcculloch · 2024-09-20T00:29:44Z

soroban-sdk/src/crypto.rs

+                .into();
+        match is_in_correct_subgroup {
+            true => res,
+            false => panic!("result G1 point not in the correct subgroup"),


We shouldn't ever use panic! in the SDK. It'll introduce text into the .wasm builds. In any case I think it'll go away when we switch to returning an Option.

Use the sdk_panic! macro, it's an internal macro that'll remove the text in the wasm build.

Looks like Jay addressed this

leighmcculloch · 2024-09-20T00:32:15Z

soroban-sdk/src/env.rs

+    #[inline(always)]
+    pub fn bls12_381(&self) -> Bls12_381 {
+        Bls12_381::new(self)
+    }


Given this is crypto, it should I think be accessible on the .crypto() fn instead of at the top-level.

Suggested change

#[inline(always)]

pub fn bls12_381(&self) -> Bls12_381 {

Bls12_381::new(self)

}

leighmcculloch · 2024-09-20T00:33:17Z

soroban-sdk/src/crypto.rs

+
+/// Bls12_381 provides access to curve and field arithmetics on the BLS12-381
+/// curve.
+pub struct Bls12_381 {


Can we put this type and all the subtypes in a module under crypto? There's quite a lot being added and the organisation would be helpful I think.

Are you saying we should create a crypto directory to organize the crypto related modules?

We don't need to refactor all the existing code, just yeah make a crypto directory and put the new bls code in a bls12_318.rs file.

sisuresh · 2024-09-20T22:07:01Z

deny.toml

@@ -244,6 +244,8 @@ deny = [
 # Certain crates/versions that will be skipped when doing duplicate detection.
 skip = [
    #{ name = "ansi_term", version = "=0.11.0" },
+    { name = "hashbrown", version = "=0.14.5" },


I think this change should be fine... but I'm highlighting this so it isn't missed.

jayz22 force-pushed the bls12-381 branch from 021fed7 to 6c8b8f0 Compare September 19, 2024 22:20

implement bls12-381 functions with tests

e7e70c0

bump protocol_version locally add basic g1/g2_add test expose dst add mult-pairing-check update scalar to use U256 representation update env update env pick up xdr change bump sdk version implement bls12-381

jayz22 force-pushed the bls12-381 branch from 6c8b8f0 to e7e70c0 Compare September 19, 2024 22:28

dmkozh reviewed Sep 19, 2024

View reviewed changes

jayz22 added 2 commits September 19, 2024 19:45

fix some deps

253c2c9

address comments

616e9ed

dmkozh approved these changes Sep 20, 2024

View reviewed changes

soroban-sdk/src/crypto.rs Outdated Show resolved Hide resolved

leighmcculloch reviewed Sep 20, 2024

View reviewed changes

Allow duplicates for arkworks deps

bbcce96

sisuresh reviewed Sep 20, 2024

View reviewed changes

sisuresh added 4 commits September 20, 2024 15:41

Address some comments

6add893

Fix doc comment

34437ad

Create separate bls12-381 module

4a55224

Merge branch 'main' into bls12-381

1f40b10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement bls12-381 functions #1345

implement bls12-381 functions #1345

jayz22 commented Sep 19, 2024

dmkozh Sep 19, 2024

jayz22 Sep 20, 2024

leighmcculloch Sep 20, 2024

dmkozh left a comment

leighmcculloch left a comment

leighmcculloch Sep 19, 2024

leighmcculloch Sep 19, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 19, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 20, 2024

leighmcculloch Sep 20, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 20, 2024

leighmcculloch Sep 20, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 20, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 20, 2024

sisuresh Sep 20, 2024

leighmcculloch Sep 20, 2024

sisuresh Sep 20, 2024

sisuresh Sep 20, 2024

	/// # `G1Affine` is a point in the G1 group (subgroup defined over the base field
	/// `G1Affine` is a point in the G1 group (subgroup defined over the base field

	write!(f, "{}({:?})", $name, self.to_array())?;
	Ok(())
	write!(f, "{}({:?})", $name, self.to_array())

implement bls12-381 functions #1345

Are you sure you want to change the base?

implement bls12-381 functions #1345

Conversation

jayz22 commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmkozh left a comment

Choose a reason for hiding this comment

leighmcculloch 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

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