bw6761 golang wrapper #2

ImmanuelSegol · 2023-09-26T15:18:47Z

Describe the changes

This PR...

Linked Issues

Resolves #

* refactor: remove boost

Update icicle crate name in examples and benches

Resolves ingonyama-zk#186

* refactor * lint * Typo and spacing --------- Co-authored-by: Jeremy Felder <jeremy.felder1@gmail.com>

* Add cmake tests for cpp primitives * Add cpp/cuda formatting * Add conditional steps based on files changed for faster required checks * Update runs-on for check files changed

DmytroTym · 2023-09-27T07:38:34Z

icicle/curves/bw6_761/curve_config.cuh

@@ -3,6 +3,9 @@
 #include "../../primitives/field.cuh"
 #include "../../primitives/projective.cuh"
 #if defined(G2_DEFINED)
+#include "../../primitives/extension_field.cuh"


I don't think you need lines 6-8. I assume they were added to solve a compilation error but the problem was in the other place (in projective.cu). G2 curve isn't over the extension field and we should be able to speed up compilation quite a bit by removing this unnecessary import.

DmytroTym · 2023-09-27T07:38:49Z

icicle/curves/bw6_761/curve_config.cuh

@@ -21,6 +24,7 @@ namespace BW6_761 {
  typedef Projective<point_field_t, scalar_t, b, gen_x, gen_y> projective_t;
  typedef Affine<point_field_t> affine_t;
 #if defined(G2_DEFINED)
+  typedef ExtensionField<PARAMS_BW6_761::fq_config> g2_point_field_t;


This should be removed as well

Done, i removed the wrong imports as well from 377

DmytroTym · 2023-09-27T07:43:44Z

icicle/utils/sharedmem.cuh

@@ -62,6 +62,12 @@
 #include "../curves/bls12_381/curve_config.cuh"
 #include "../curves/bn254/curve_config.cuh"

+__device__ void Error_UnsupportedType() 


@vhnatyk pls take a look, is this how Unresolved extern function '_Z21Error_UnsupportedTypev' should be resolved? Maybe #include "../curves/bw6_671/curve_config.cuh" can help?

while seems this may trick a linker - for compatibility at least with ntt templates - the file should be extended with like other curves

like it's 4years+ but seems there is no better workaround for templated dynamic shared memory other than Lei Mao blog's

I think this worked, im building it now

But you haven't included struct SharedMemory<BW6_671::scalar_t> {...} and struct SharedMemory<BW6_671::projective_t> {...}, is it ok? @vhnatyk could you pls take a look again?

while seems this may trick a linker - for compatibility at least with ntt templates - the file should be extended with like other curves

@ImmanuelSegol - this may build with that stub function, but ntt should not pass correctly if shared mem kernel is used

pls remove as it just disables linking error and hides the issue

__device__ void Error_UnsupportedType() { // This function does not need a body, as it's just to throw a linker error. }

and add the actual implementation like

template <> struct SharedMemory<BW6_671::scalar_t> { __device__ BW6_671::scalar_t* getPointer() { extern __shared__ BW6_671::scalar_t s_scalar_t_bw6_671[]; return s_scalar_t_bw6_671; } }; template <> struct SharedMemory<BW6_671::projective_t> { __device__ BW6_671::projective_t* getPointer() { extern __shared__ BW6_671::projective_t s_projective_t_bw6_671[]; return s_projective_t_bw6_671; } }; #endif //_SHAREDMEM_H_

DmytroTym · 2023-09-27T07:47:30Z

goicicle/curves/bw6761/g2.go

+}
+
+type G2PointAffine struct {
+	X, Y ExtentionField


That's not how G2 works for this curve unfortunately, the coordinates of points are from the same field as G1 points and not extension field elements

DmytroTym · 2023-09-27T07:52:26Z

goicicle/curves/bw6761/g2.go

+	return G2Point{
+		X: p.X,
+		Y: p.Y,
+		Z: ExtentionField{


In principle it's probably better to use functions like projective_from_affine from CUDA, it's better to have a single source of truth. Like in this case, because G2 is actually not over extension field, and the size of limbs is wrong too, this function might work incorrectly

Thanks, this is a left over function that I didnt update.

…#235) * refactor * refactor * revert * refactor: clang format * Update icicle/appUtils/msm/msm.cu

DmytroTym

Good job, I think you're almost done but I'd prefer that you delete newly auto-generated src/curves/bw6_761.rs and src/test_bw6_761.rs, they don't really work. Rust interface is not an immediate priority with this curve and we should introduce it as we refactor APIs on our feature branch IMO.

DmytroTym · 2023-10-08T06:55:11Z

icicle/utils/sharedmem.cuh

@@ -62,6 +62,12 @@
 #include "../curves/bls12_381/curve_config.cuh"
 #include "../curves/bn254/curve_config.cuh"

+__device__ void Error_UnsupportedType() 


But you haven't included struct SharedMemory<BW6_671::scalar_t> {...} and struct SharedMemory<BW6_671::projective_t> {...}, is it ok? @vhnatyk could you pls take a look again?

DmytroTym · 2023-10-08T07:18:27Z

icicle/curves/bw6_761/projective.cu

+         !((point1->x == BW6_761::g2_point_field_t::zero()) && (point1->y == BW6_761::g2_point_field_t::zero()) &&
+           (point1->z == BW6_761::g2_point_field_t::zero())) &&
+         !((point2->x == BW6_761::g2_point_field_t::zero()) && (point2->y == BW6_761::g2_point_field_t::zero()) &&
+           (point2->z == BW6_761::g2_point_field_t::zero()));


Trying to #include "projective.cu" leads to a compile error as the compiler doesn't know about g2_point_field_t (you deleted it from curve_config.cuh). I think you should revert to using point_field_t here.

Well not #include but rather add to build

vhnatyk · 2023-10-08T08:07:30Z

goicicle/curves/bw6761/ntt_test.go

+	NttBatch(&nttResult, false, count, 0)
+	assert.NotEqual(t, nttResult, scalars)
+
+	assert.Equal(t, nttResult, nttResult)


asserting the equality of the same objects?

vhnatyk · 2023-10-08T08:19:48Z

src/curves/mod.rs

@@ -1,4 +1,3 @@
 pub mod bls12_377;
 pub mod bls12_381;
 pub mod bn254;
-pub mod bw6_761;


why is it removed?

I asked to remove it during the previous review as I removed the (very much incorrectly working) autogenerated Rust wrappers for BW6-671

vhnatyk

just a few issues in the comments should be addressed, otherwise looks good 👍🏻

Hotfix to slowdown in go when more than one curve is imported.

Add concurrency groups at workflow level for CI. Remove dev CI since we no longer use dev branch. Resolves ingonyama-zk#180

ImmanuelSegol and others added 7 commits September 20, 2023 10:00

fix cuda test - remove boost (ingonyama-zk#197)

0886213

* refactor: remove boost

Fix CI - dont use deprecated package name (ingonyama-zk#216)

e368f00

Update icicle crate name in examples and benches

add mont_r and mont_r_inv and fix args (ingonyama-zk#187)

0a63978

Resolves ingonyama-zk#186

update docs (ingonyama-zk#222)

8612975

* refactor * lint * Typo and spacing --------- Co-authored-by: Jeremy Felder <jeremy.felder1@gmail.com>

[CI]: Adds C++/CUDA CI and conditional workflow runs (ingonyama-zk#223)

413e1d8

* Add cmake tests for cpp primitives * Add cpp/cuda formatting * Add conditional steps based on files changed for faster required checks * Update runs-on for check files changed

setup

efa777c

refactor: fix compile issues

03fac23

DmytroTym suggested changes Sep 27, 2023

View reviewed changes

Make dependency instructions more clear (ingonyama-zk#227)

9f67075

ImmanuelSegol mentioned this pull request Sep 27, 2023

[FEAT]: Create Golang bindings for f251 ingonyama-zk/icicle#228

Closed

jeremyfelder and others added 4 commits September 28, 2023 16:11

Fix: div by 0 when number of Elements is 1 (ingonyama-zk#230)

97f0079

update g2 size

e18dda3

fix memory error in single_stage_multi_reduction_kernel (ingonyama-zk…

9114ecb

…#235) * refactor * refactor * revert * refactor: clang format * Update icicle/appUtils/msm/msm.cu

refactor: fix curve

5413509

ImmanuelSegol requested review from vhnatyk and DmytroTym October 5, 2023 11:46

ImmanuelSegol added 2 commits October 5, 2023 13:31

refactor

0db70b1

refactor

01ff7c5

DmytroTym suggested changes Oct 8, 2023

View reviewed changes

vhnatyk reviewed Oct 8, 2023

View reviewed changes

ImmanuelSegol and others added 8 commits October 10, 2023 10:21

refactor

0f48ec6

Hotfix to go regression when 2 curves are imported (ingonyama-zk#245)

028bed1

Hotfix to slowdown in go when more than one curve is imported.

fix tests

cb441fc

Exclude target directory from format checks (ingonyama-zk#247)

1cf7b2e

[CI]: Add concurrency groups at workflow level (ingonyama-zk#238)

88c9c85

Add concurrency groups at workflow level for CI. Remove dev CI since we no longer use dev branch. Resolves ingonyama-zk#180

Fix readme badge links, fix CI cpp formatter (ingonyama-zk#249)

09d8c5d

Fixed issues preventing correctness for BW6-761

4630150

Merge pull request #1 from DmytroTym/bw6/gowrapper

48453a8

DmytroTym and others added 4 commits October 17, 2023 20:32

Merge remote-tracking branch 'ingo/main' into bw6/gowrapper

0e10dd2

Merge pull request #2 from DmytroTym/bw6/gowrapper

9784a26

REAMDE updated

fb13bdf

Merge pull request #3 from DmytroTym/bw6/gowrapper

d7559fe

liuxiaobleach approved these changes Oct 18, 2023

View reviewed changes

liuxiaobleach merged commit 89a5081 into celer-network:main Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bw6761 golang wrapper #2

bw6761 golang wrapper #2

ImmanuelSegol commented Sep 26, 2023

DmytroTym Sep 27, 2023 •

edited

Loading

ImmanuelSegol Oct 5, 2023

DmytroTym Sep 27, 2023

ImmanuelSegol Oct 5, 2023

DmytroTym Sep 27, 2023

vhnatyk Sep 27, 2023

vhnatyk Sep 27, 2023

ImmanuelSegol Oct 5, 2023

DmytroTym Oct 8, 2023 •

edited

Loading

vhnatyk Oct 8, 2023

vhnatyk Oct 8, 2023

DmytroTym Sep 27, 2023

ImmanuelSegol Oct 5, 2023

DmytroTym Sep 27, 2023

ImmanuelSegol Oct 5, 2023

ImmanuelSegol Oct 5, 2023

DmytroTym left a comment

DmytroTym Oct 8, 2023 •

edited

Loading

DmytroTym Oct 8, 2023

DmytroTym Oct 8, 2023

vhnatyk Oct 8, 2023

vhnatyk Oct 8, 2023

DmytroTym Oct 8, 2023

vhnatyk left a comment

bw6761 golang wrapper #2

bw6761 golang wrapper #2

Conversation

ImmanuelSegol commented Sep 26, 2023

Describe the changes

Linked Issues

DmytroTym Sep 27, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmytroTym Oct 8, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmytroTym left a comment

Choose a reason for hiding this comment

DmytroTym Oct 8, 2023 • 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

Choose a reason for hiding this comment

vhnatyk left a comment

Choose a reason for hiding this comment

DmytroTym Sep 27, 2023 •

edited

Loading

DmytroTym Oct 8, 2023 •

edited

Loading

DmytroTym Oct 8, 2023 •

edited

Loading