a16z / jolt

The simplest and most extensible zkVM. Fast and fully open source from a16z crypto and friends. ⚡
https://jolt.a16zcrypto.com
MIT License
587 stars 106 forks source link

feat(icicle): Add icicle msm support #322

Open PatStiles opened 2 months ago

PatStiles commented 2 months ago

This PR adds support for Ingonyama's icicle library.

To integrate Icicle with minimal changes the following was done:

2-5 of the e2e tests which fail intermittently. I thought it was weird that the test commit_prove_verify for hyrax passed but the e2e tests failed. I investigated on on my own by comparing the results of the different msm variations done to speed up small msm operations and saw they produce different results. As it stands the results of icicle msm match the results of msm_bigint which is probably why some (hyrax prove, verify) but not all of the tests are passing.

I assume they different msm results between variations is expected behavior. Let me know if you have suggestions on how to address the failing tests.

sragss commented 2 months ago

Great work.

For the traits, let's rebase this on top of #309 after #318 is merged in. We should be able to make Icicle a new backend for Hyrax exclusively without touching other traits.

PatStiles commented 2 months ago

Currently some of the e2e tests are failing due to failed Hyrax Proof verification. Initially investigating this it seems the msm results of icicle and jolt_core::msm::VariableBase::msm differ. After comparing the results of the msm variants with icicle_msm, it appears the results of icicle_msm and msm_bigint match but differ from the results of msm_u64 and msm_binary.

On Sam Rags advice I compared the results icicle_msm() and msm() for random distribtutions of different scalars as following:

The results of these tests match my earlier attempts at debugging (the result of icicle matches msm_bigint and is different for the other msm_vairants). However across 10 runs the results of icicle_msm() and msm_bigint() were different 2 times. My suspicion is that is why the e2e tests are inconsistently failing.

Pinging the icicle team for more help: @jeremyfelder @LeonHibnik @DmytroTym for visibility

sragss commented 2 months ago

It's quite possible this is a bug in the custom Jolt MSMs. We use the same function in prove and verify so I don't think it would cause a verification error on the opening proofs.

PatStiles commented 2 months ago

I've isolated a failing test case (below) where msm_bigint() is different from icicle_msm(). This is for msm_length = 3 on bn254 G1. I compared the output of msm_bigint, icicle_msm to arkworks_msm as a baseline. Icicle_msm differs from both msm_bigint and arkworks_msm which match.

scalars: [BigInt([9016117505638758543, 352751388875653018, 14946620785396285244, 211688466542070544]), BigInt([9016117505638758543, 352751388875653018, 14946620785396285244, 211688466542070544]), BigInt([9016117505638758543, 352751388875653018, 14946620785396285244, 211688466542070544])]

bases: [(7688067217989994385175370005327028282099909322677106416431281707406319639423, 7687918639911294339882576580611551419932980906448618049918745820988988940544), (7688067217989994385175370005327028282099909322677106416431281707406319639423, 7687918639911294339882576580611551419932980906448618049918745820988988940544), (7688067217989994385175370005327028282099909322677106416431281707406319639423, 7687918639911294339882576580611551419932980906448618049918745820988988940544)]

Icicle_msm: G1Projective { 8762571254902059470736832095824632961938556720240244842612617999683902078046, 10092157708131004888930512565411407719316059820784025059444099681630462320054, 11168217204278712267950517896802450080070912830806623791345428292444699192104}

msm_bigint/arkworks_msm: G1Projective{ 8556297242478855084502684745245972997742728399108653613517190125786236661471, 8050224918617737447598769520885148623820108452568297893643817103436495388203, 11360129327871628975120284343765061813104420509947328992138115752583312160988 }

sragss commented 2 months ago

For reference we had some conversion logic between ark_ff::PrimeField and ff::PrimeField back when we were using Spartan2 externally: #188