Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ICICLE integration to use v3 ICICLE #1318

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

jeremyfelder
Copy link
Contributor

@jeremyfelder jeremyfelder commented Nov 18, 2024

Description

This PR updates the ICICLE integration to use v3 of ICICLE.

Fixes #1166

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How has this been tested?

package main

import (
	"fmt"
	"time"

	"github.com/consensys/gnark-crypto/ecc"
	"github.com/consensys/gnark/backend"
	"github.com/consensys/gnark/backend/groth16"
	"github.com/consensys/gnark/frontend"
	"github.com/consensys/gnark/frontend/cs/r1cs"
)

// CubicCircuit defines a simple circuit
// x**3 + x + 5 == y
type CubicCircuit struct {
	// struct tags on a variable is optional
	// default uses variable name and secret visibility.
	X frontend.Variable `gnark:"x"`
	Y frontend.Variable `gnark:",public"`
}

// Define declares the circuit constraints
// x**3 + x + 5 == y
func (circuit *CubicCircuit) Define(api frontend.API) error {
	x3 := api.Mul(circuit.X, circuit.X, circuit.X)
	for i := 0; i < 1000000; i++ {
		api.AssertIsEqual(circuit.Y, api.Add(x3, circuit.X, 5))
	}
	return nil
}

func main() {
	// compiles our circuit into a R1CS
	var circuit CubicCircuit
	ccs, _ := frontend.Compile(ecc.BN254.ScalarField(), r1cs.NewBuilder, &circuit)

	// groth16 zkSNARK: Setup
	pk, vk, _ := groth16.Setup(ccs)

	// witness definition
	assignment := CubicCircuit{X: 3, Y: 35}
	witness, _ := frontend.NewWitness(&assignment, ecc.BN254.ScalarField())
	publicWitness, _ := witness.Public()
	// groth16: Prove & Verify
	icicleTimeStart := time.Now()
	proof, err := groth16.Prove(ccs, pk, witness, backend.WithIcicleAcceleration())
	fmt.Println("Icicle proof time:", time.Since(icicleTimeStart))

	if err != nil {
		fmt.Println(err)
	}

	if err = groth16.Verify(proof, vk, publicWitness); err != nil {
		panic("Failed verification")
	}
	
	gnarkTimeStart := time.Now()
	proofNoIcicle, err := groth16.Prove(ccs, pk, witness)
	fmt.Println("Gnark CPU proof time:", time.Since(gnarkTimeStart))
	
	if err != nil {
		fmt.Println(err)
	}

	if err = groth16.Verify(proofNoIcicle, vk, publicWitness); err != nil {
		panic("Failed verification")
	}
}

How has this been benchmarked?

  • Using the above example circuit on a i9-13900K, 64GB RAM + NVIDIA RTX 4080
    image

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@jeremyfelder jeremyfelder marked this pull request as ready for review November 18, 2024 13:01
@p4u
Copy link

p4u commented Nov 20, 2024

hey @jeremyfelder, can you try with bls12377 ?

Because I managed to make it work on bn254, but the same code on bls12377, icicle acceleration is just ignored.

I'm compiling the Go icicle wrapper with ./build -curves=all so it should be available (acually I see the compilation traces on the bls12377).

@jeremyfelder
Copy link
Contributor Author

hey @jeremyfelder, can you try with bls12377 ?

Because I managed to make it work on bn254, but the same code on bls12377, icicle acceleration is just ignored.

I'm compiling the Go icicle wrapper with ./build -curves=all so it should be available (acually I see the compilation traces on the bls12377).

Great to hear that you got bn254 working 🔥!
Right now the gnark<>ICICLE integration only includes bn254. It should be relatively simple to add all the curves/fields that ICICLE supports though I didn't want to bloat this PR with something that should be designed better to stay updated as @ivokub and I have discussed in the past.

@ivokub
Copy link
Collaborator

ivokub commented Nov 20, 2024

hey @jeremyfelder, can you try with bls12377 ?
Because I managed to make it work on bn254, but the same code on bls12377, icicle acceleration is just ignored.
I'm compiling the Go icicle wrapper with ./build -curves=all so it should be available (acually I see the compilation traces on the bls12377).

Great to hear that you got bn254 working 🔥! Right now the gnark<>ICICLE integration only includes bn254. It should be relatively simple to add all the curves/fields that ICICLE supports though I didn't want to bloat this PR with something that should be designed better to stay updated as @ivokub and I have discussed in the past.

Yep - I'll try to review and merge this PR promptly and then figure out a bit better to switch between accelerated and non-accelerated implementations. Or see if the current approach scales to different curves nicely.

@ivokub
Copy link
Collaborator

ivokub commented Dec 12, 2024

Hi @jeremyfelder - could you please merge the branch https://github.com/ivokub/gnark/tree/feat/groth16-icicle-v3.1.0? I have fixed some issues with copying the commitment inputs and reduced copies. Otherwise I'm done with the review and would proceed with merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Icicle integration is out of date
3 participants