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.

Sign up for GitHub

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

Jump to bottom

Wrong proof generated by latest iciclegnark #7

Open
hussein-aitlahcen opened this issue Sep 23, 2023 · 7 comments
Open

Wrong proof generated by latest iciclegnark #7

hussein-aitlahcen opened this issue Sep 23, 2023 · 7 comments
Assignees
@ImmanuelSegol
Labels
type:bug Something isn't working

Comments

@hussein-aitlahcen
Copy link

hussein-aitlahcen commented Sep 23, 2023 •
edited
Loading

Hi,

I tried using the latest gnark fork you guys have along the latest icicle and the result, when switching from CPU to GPU proving via the gpu tag when building our binary is:

groth16.Verify on bn254 is failing for the GPU (gpu tag) generated proof: pairing doesn't match
groth16.Verify on bn254 is succeeding for the non GPU generated proof: {"level":"debug","curve":"bn254","backend":"groth16","took":0.826976,"time":"2023-09-23T13:18:30+02:00","message":"verifier done"}

For the same exact circuit, with the same exact inputs.

Maybe I shouldn't be using latest commits?

GPU:
image
@ImmanuelSegol
Copy link
Contributor

@hussein-aitlahcen Thanks for opening the issue.

I will investigate.

@ImmanuelSegol ImmanuelSegol self-assigned this Sep 23, 2023
@ImmanuelSegol
Copy link
Contributor

@hussein-aitlahcen Im sorry I didn't ask you, what circuit are you using?

@hussein-aitlahcen
Copy link
Author

@hussein-aitlahcen Im sorry I didn't ask you, what circuit are you using?

It's a circuit that is not open source yet... Perhaps I can discuss with the team to add you in the repo if you want to replay the issue.

@hussein-aitlahcen
Copy link
Author

hussein-aitlahcen commented Sep 25, 2023 •
edited
Loading

@ImmanuelSegol
I tried to investigate and found that when computing krs, we don't wait for ar and bs1 to be computed, in the original gnark code computeKRS is waiting for ar and bs1 to be computed, was it supposed to be done this way? In any case I forked and fixed the issue by reordering to try but it didn't solve my problem.

I also added some logs after msm compuation of ar, bs1, bs and krs and have a discrepancy in the output:

i.e.

                // prove.go
		if _, err := bs1.MultiExp(pk.G1.B, wireValuesB, ecc.MultiExpConfig{NbTasks: n / 2}); err != nil {
			chBs1Done <- err
			close(chBs1Done)
			return
		}
		fmt.Println("bs1: ", bs1)

...
                // prove_gpu.go
 		bs1, _, _ = iciclegnark.MsmOnDevice(wireValuesBDevice.P, pk.G1Device.B, wireValuesBDevice.Size, true)
		fmt.Println("bs1: ", bs1)
...
gpu

bs: {{[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]}}
ar:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
bs1:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
krs:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}

ar, bs1, krs are = 1 in mont???

non gpu

Bs:  {{[4636322392879697240 2384471692377456582 4556942363646472051 813635155070355686] [14626696712275888559 11080686283781174876 14467499977088519560 284481842883952647]} {[637172356924027360 16850950472009441986 11891712156479364010 2192150005802278260] [5751625923871861710 17730865525814601000 12803040783397251468 1691390020363999973]} {[7920156426300269433 5149651697609590255 9906526044991732446 3314290308708089264] [12279958031118287131 17661061699100988233 991386651093401781 333610601336327577]}}
bs1:  {[15403469495131510827 1175663692734111092 15688015184333491738 703810843658371249] [7964450159908730923 446238346755840542 3405801049435662475 2171517101667273338] [3575578639912884561 11501828061521654759 6382638011729568991 645800270268746503]}
ar:  {[5836583453540608800 11989822530383293069 2958706776637200154 2993799242673407613] [12235819207099940230 17748403818490228955 12165302879220095557 2885306535149423449] [389139827242955288 16545878758785633611 4447694843524427253 1808335461326242003]}
krs:  {[3213847375034341348 15167574436140310312 12611616496990658474 1685307844261831545] [14282052022244955669 14890175442922758155 9668945922477019892 937689974697692563] [3646646787137154949 9148282517108681545 4704083787257830644 835441358773014708]}

I'm compiling latest icicle using nix and gcc 11 + cuda>=12 with:

          cuda = pkgs-unstable.cudaPackages_12.cudatoolkit.overrideAttrs (old: old // { meta = null; });
          cudart = pkgs-unstable.cudaPackages_12.cuda_cudart.overrideAttrs (old: old // { meta = null; });
          pkgs.gcc11Stdenv.mkDerivation {
            pname = "icicle";
            version = "0.0.1";
            src = pkgs.fetchFromGitHub {
              owner = "ingonyama-zk";
              repo = "icicle";
              rev = "04e5ff5d1af4";
              hash = "sha256-flqfyD/r614gJPN+w/I+PksJ5gnbltLMXdMq7Vh7ziY=";
            };
            buildPhase = ''
              ${cuda}/bin/nvcc -DG2_DEFINED -Xcompiler -fPIC -std=c++17 -shared -L${cudart}/lib \
                  icicle/curves/bn254/lde.cu \
                  icicle/curves/bn254/msm.cu \
                  icicle/curves/bn254/projective.cu \
                  icicle/curves/bn254/ve_mod_mult.cu \
                  -o libbn254.so
            '';
            installPhase = ''
              mkdir -p $out/lib
              mv libbn254.so $out/lib
            '';
            enableParallelBuilding = true;
            doCheck = true;
          };

EDIT:

Looks like we ignore the commit return code also

iciclegnark/curves/bn254/gnark.go

Line 43 in 7c37237

icicle.Commit(out_d, scalars_d, points_d, count, 10)

@ImmanuelSegol
Copy link
Contributor

@ImmanuelSegol I tried to investigate and found that when computing krs, we don't wait for ar and bs1 to be computed, in the original gnark code computeKRS is waiting for ar and bs1 to be computed, was it supposed to be done this way? In any case I forked and fixed the issue by reordering to try but it didn't solve my problem.

I also added some logs after msm compuation of ar, bs1, bs and krs and have a discrepancy in the output:

i.e.

                // prove.go
		if _, err := bs1.MultiExp(pk.G1.B, wireValuesB, ecc.MultiExpConfig{NbTasks: n / 2}); err != nil {
			chBs1Done <- err
			close(chBs1Done)
			return
		}
		fmt.Println("bs1: ", bs1)

...
                // prove_gpu.go
 		bs1, _, _ = iciclegnark.MsmOnDevice(wireValuesBDevice.P, pk.G1Device.B, wireValuesBDevice.Size, true)
		fmt.Println("bs1: ", bs1)
...
gpu

bs: {{[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]}}
ar:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
bs1:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
krs:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}

ar, bs1, krs are = 1 in mont???

non gpu

Bs:  {{[4636322392879697240 2384471692377456582 4556942363646472051 813635155070355686] [14626696712275888559 11080686283781174876 14467499977088519560 284481842883952647]} {[637172356924027360 16850950472009441986 11891712156479364010 2192150005802278260] [5751625923871861710 17730865525814601000 12803040783397251468 1691390020363999973]} {[7920156426300269433 5149651697609590255 9906526044991732446 3314290308708089264] [12279958031118287131 17661061699100988233 991386651093401781 333610601336327577]}}
bs1:  {[15403469495131510827 1175663692734111092 15688015184333491738 703810843658371249] [7964450159908730923 446238346755840542 3405801049435662475 2171517101667273338] [3575578639912884561 11501828061521654759 6382638011729568991 645800270268746503]}
ar:  {[5836583453540608800 11989822530383293069 2958706776637200154 2993799242673407613] [12235819207099940230 17748403818490228955 12165302879220095557 2885306535149423449] [389139827242955288 16545878758785633611 4447694843524427253 1808335461326242003]}
krs:  {[3213847375034341348 15167574436140310312 12611616496990658474 1685307844261831545] [14282052022244955669 14890175442922758155 9668945922477019892 937689974697692563] [3646646787137154949 9148282517108681545 4704083787257830644 835441358773014708]}

I'm compiling latest icicle using nix and gcc 11 + cuda>=12 with:

          cuda = pkgs-unstable.cudaPackages_12.cudatoolkit.overrideAttrs (old: old // { meta = null; });
          cudart = pkgs-unstable.cudaPackages_12.cuda_cudart.overrideAttrs (old: old // { meta = null; });
          pkgs.gcc11Stdenv.mkDerivation {
            pname = "icicle";
            version = "0.0.1";
            src = pkgs.fetchFromGitHub {
              owner = "ingonyama-zk";
              repo = "icicle";
              rev = "04e5ff5d1af4";
              hash = "sha256-flqfyD/r614gJPN+w/I+PksJ5gnbltLMXdMq7Vh7ziY=";
            };
            buildPhase = ''
              ${cuda}/bin/nvcc -DG2_DEFINED -Xcompiler -fPIC -std=c++17 -shared -L${cudart}/lib \
                  icicle/curves/bn254/lde.cu \
                  icicle/curves/bn254/msm.cu \
                  icicle/curves/bn254/projective.cu \
                  icicle/curves/bn254/ve_mod_mult.cu \
                  -o libbn254.so
            '';
            installPhase = ''
              mkdir -p $out/lib
              mv libbn254.so $out/lib
            '';
            enableParallelBuilding = true;
            doCheck = true;
          };

EDIT:

Looks like we ignore the commit return code also

iciclegnark/curves/bn254/gnark.go

Line 43 in 7c37237

icicle.Commit(out_d, scalars_d, points_d, count, 10)

@hussein-aitlahcen Is there any way you can perhaps send me or the scalars and points that are inputed into the prover or the circuits so I can run them locally?

Because if you try to run some other circuit such as here https://github.com/ingonyama-zk/gnark/tree/clean-full-gpu/examples then it will pass.

So im suspecting this is related to some sort of anomaly.

@hussein-aitlahcen
Copy link
Author

hussein-aitlahcen commented Sep 26, 2023 •
edited
Loading

@ImmanuelSegol I tried to investigate and found that when computing krs, we don't wait for ar and bs1 to be computed, in the original gnark code computeKRS is waiting for ar and bs1 to be computed, was it supposed to be done this way? In any case I forked and fixed the issue by reordering to try but it didn't solve my problem.
I also added some logs after msm compuation of ar, bs1, bs and krs and have a discrepancy in the output:
i.e.

                // prove.go
		if _, err := bs1.MultiExp(pk.G1.B, wireValuesB, ecc.MultiExpConfig{NbTasks: n / 2}); err != nil {
			chBs1Done <- err
			close(chBs1Done)
			return
		}
		fmt.Println("bs1: ", bs1)

...
                // prove_gpu.go
 		bs1, _, _ = iciclegnark.MsmOnDevice(wireValuesBDevice.P, pk.G1Device.B, wireValuesBDevice.Size, true)
		fmt.Println("bs1: ", bs1)
...
gpu

bs: {{[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]} {[0 0 0 0] [0 0 0 0]}}
ar:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
bs1:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}
krs:  {[15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [15230403791020821917 754611498739239741 7381016538464732716 1011752739694698287] [0 0 0 0]}

ar, bs1, krs are = 1 in mont???

non gpu

Bs:  {{[4636322392879697240 2384471692377456582 4556942363646472051 813635155070355686] [14626696712275888559 11080686283781174876 14467499977088519560 284481842883952647]} {[637172356924027360 16850950472009441986 11891712156479364010 2192150005802278260] [5751625923871861710 17730865525814601000 12803040783397251468 1691390020363999973]} {[7920156426300269433 5149651697609590255 9906526044991732446 3314290308708089264] [12279958031118287131 17661061699100988233 991386651093401781 333610601336327577]}}
bs1:  {[15403469495131510827 1175663692734111092 15688015184333491738 703810843658371249] [7964450159908730923 446238346755840542 3405801049435662475 2171517101667273338] [3575578639912884561 11501828061521654759 6382638011729568991 645800270268746503]}
ar:  {[5836583453540608800 11989822530383293069 2958706776637200154 2993799242673407613] [12235819207099940230 17748403818490228955 12165302879220095557 2885306535149423449] [389139827242955288 16545878758785633611 4447694843524427253 1808335461326242003]}
krs:  {[3213847375034341348 15167574436140310312 12611616496990658474 1685307844261831545] [14282052022244955669 14890175442922758155 9668945922477019892 937689974697692563] [3646646787137154949 9148282517108681545 4704083787257830644 835441358773014708]}

I'm compiling latest icicle using nix and gcc 11 + cuda>=12 with:

          cuda = pkgs-unstable.cudaPackages_12.cudatoolkit.overrideAttrs (old: old // { meta = null; });
          cudart = pkgs-unstable.cudaPackages_12.cuda_cudart.overrideAttrs (old: old // { meta = null; });
          pkgs.gcc11Stdenv.mkDerivation {
            pname = "icicle";
            version = "0.0.1";
            src = pkgs.fetchFromGitHub {
              owner = "ingonyama-zk";
              repo = "icicle";
              rev = "04e5ff5d1af4";
              hash = "sha256-flqfyD/r614gJPN+w/I+PksJ5gnbltLMXdMq7Vh7ziY=";
            };
            buildPhase = ''
              ${cuda}/bin/nvcc -DG2_DEFINED -Xcompiler -fPIC -std=c++17 -shared -L${cudart}/lib \
                  icicle/curves/bn254/lde.cu \
                  icicle/curves/bn254/msm.cu \
                  icicle/curves/bn254/projective.cu \
                  icicle/curves/bn254/ve_mod_mult.cu \
                  -o libbn254.so
            '';
            installPhase = ''
              mkdir -p $out/lib
              mv libbn254.so $out/lib
            '';
            enableParallelBuilding = true;
            doCheck = true;
          };

EDIT:
Looks like we ignore the commit return code also

iciclegnark/curves/bn254/gnark.go

Line 43 in 7c37237

icicle.Commit(out_d, scalars_d, points_d, count, 10)

@hussein-aitlahcen Is there any way you can perhaps send me or the scalars and points that are inputed into the prover or the circuits so I can run them locally?

Because if you try to run some other circuit such as here https://github.com/ingonyama-zk/gnark/tree/clean-full-gpu/examples then it will pass.

So im suspecting this is related to some sort of anomaly.

Discussed with the team we can add you to the repo, do you have matrix handle?

@ImmanuelSegol ImmanuelSegol added the type:bug Something isn't working label Sep 26, 2023
@jeremyfelder
Copy link
Collaborator

@hussein-aitlahcen Thanks for finding the KRS not waiting on AR and BS1 bug! 💪🏻 🚀.

Just so you know, the setup phase moves all of the points and some additional data thats needed to the GPU immediately. I see that the GPU you are using has 6141MiB of mem; depending on the circuit size its possible that there isn't enough memory so when one of the functions tries to use an invalid pointer the GPU errors and can't recover.

You should be able to see if this is the case by running nvidia-smi with -lms and other options to print memory usage or if the total prover time is extremely low (less than 500ms)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ImmanuelSegol ImmanuelSegol

Labels
type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hussein-aitlahcen @ImmanuelSegol @jeremyfelder