bug: gnark-crypto implementation of GPU IcicleCommit does not match Commit

[winston-h-zhang]

Description

The provided IcicleCommit function does not seem to have the same output as the original Commit function. For example, adding the following test in ingonyama/gnark-crypto, in ecc/bn254/kzg/kzg_test.go:

func TestVerifyIcicleCommit(t *testing.T) {
	// create a polynomial
	f := randomPolynomial(60)
	var kzgIcicle iciclecore.DeviceSlice
	(iciclecore.HostSlice[bn254.G1Affine])(testSrs.Pk.G1).CopyToDevice(&kzgIcicle, true)

	// commit the polynomial
	digest, err := Commit(f, testSrs.Pk)
	if err != nil {
		t.Fatal(err)
	}
	icicle_digest := IcicleCommit(f, kzgIcicle)
	if digest != icicle_digest {
		t.Fatal("commits not equal")
	}
}

And it fails with:

winston@winston-gpu-l4-x1:~/gnark-crypto$ go test -timeout 30s -run ^TestVerifyIcicleCommit$ github.com/consensys/gnark-crypto/ecc/bn254/kzg
--- FAIL: TestVerifyIcicleCommit (0.24s)
    kzg_test.go:248: commits not equal
FAIL
FAIL    github.com/consensys/gnark-crypto/ecc/bn254/kzg 0.319s
FAIL

Expected Behavior

Test should pass.

Actual Behavior

Test fails.

Possible Fix

Steps to Reproduce

1. Copy the given code to ingonyama/gnark-crypto, in ecc/bn254/kzg/kzg_test.go
2. Run the TestVerifyIcicleCommit test.

Context

We are trying to integrate the Icicle GPU backend for gnark, but ran into this issue.

Your Environment

• gnark-crypto version used: HEAD@integrate-icicle
• go version (e.g. 1.20.6): go1.22.3 linux/amd64
• Operating System and version: Ubuntu 22.04.4 LTS

[krakhit]

I was able to reproduce the problem in the version I am working with
github.com/ingonyama-zk/icicle/v2 v2.0.0-20240624112314-26dbd224b43f

in icicle commit() there is a flag cfg.ArePointsMontgomeryForm which should be true. Since gnark uses Montgomery form for everything

when I added the flag explicitly inicicle commit() the flag cfg.ArePointsMontgomeryForm = true it passes the test.

@LeonHibnik , @jeremyfelder these commits iciclev2 used in both gnark and gnarl-crypto look very old, perhaps one should bump them to the latest version, also gnark and gnark crypto of ours is so many commits behind the original.

[jeremyfelder]

@winston-h-zhang Thanks for opening the issue!

I assume you were using this commit?

Our changes in gnark-crypto there were for a specific integration we did with plonk over bn254 where we convert the points from Montgomery during the setup in gnark here. We did this because the same points are reused throughout multiple MSMs and converting them on each MSM adds additional latency.

The ideal would be to add a cfg iciclemsm.MSMConfig parameter to the IcicleCommit signature in gnark-crypto allowing the caller to define how the MSM should interpret the data.

I hope this helps. If you have any other questions let us know, we are always happy to help!

cc @krakhit @LeonHibnik