supranational / supra_seal

Apache License 2.0
29 stars 21 forks source link

C2: Verification in bellperson fails #9

Closed vmx closed 1 year ago

vmx commented 1 year ago

I currently try to add basic sanity tests to bellperson to make sure SupraSeal works. Somehow the proofs return wrong results. It's probably a user error from my side, but I don't know what I'm doing wrong. Here are the steps to reproduce:

git clone https://github.com/filecoin-project/bellperson --branch debug-supraseal-verification
cd bellperson
RUST_LOG=trace cargo test --lib supraseal --features cuda-supraseal -- --nocapture 

The error is something like

[2023-06-14T13:46:15Z INFO  bellperson::groth16::prover::supraseal] Bellperson 0.25.0 with SupraSeal is being used!
[2023-06-14T13:46:15Z INFO  bellperson::groth16::prover::supraseal] synthesis time: 141.139µs
[2023-06-14T13:46:15Z INFO  bellperson::groth16::prover::supraseal] starting proof timer
[2023-06-14T13:46:15Z INFO  bellperson::groth16::prover::supraseal] prover time: 23.884762ms
vmx: verifier: actual, expected:
Gt(Fp12 { c0: Fp6 { c0: Fp2 { c0: Fp(0x0cfe3fdf598e319677b0886544792663bc690b0c94b6884fb95d250aeef46811852ae9060cade1fa01981ca3e99b2f12), c1: Fp(0x06a8a49a8c9fc902ea52ce72c7b8b4164800e388bff2dde183ad303039c0c17173467ec9bad2e17c13ad65ed63f105e3) }, c1: Fp2 { c0: Fp(0x0fd9e879db9b6469d8c2068932aefc59314a8184f35e40f9a7bba06701fc4f459753f9bdaf4eece601f124b1ec420a4b), c1: Fp(0x0549c160badb4cede86b65dcf7965ea5cd6ddee15d3e3f9e236da2aa0abee18e9ce9700c6d2f3503fdcf766c94ec2fd8) }, c2: Fp2 { c0: Fp(0x11bcc2d3a9eb51d9cd34546bc817425a75892792c479ec67d68f00db8c77503fc1238ffa123afc164fa453025369378b), c1: Fp(0x14690dcee46368d4fef6bc8a662ad230c0b9ff125a4042a6c268d2bca59e933c977d00cb7131f7bfc8628d3d7b1bde16) } }, c1: Fp6 { c0: Fp2 { c0: Fp(0x11a7bfa8ab38793dab010b613b86ff122a21e8b55919d64b59a23c6bdc121c8b907516fd2eb4946bd66d332a5c49e1a4), c1: Fp(0x1218cd3115a4220c73b717e9bc29f492c018735b2fa6b86ca145fe5e4898f5c8b9137c434e910f737f113b5dad271f5d) }, c1: Fp2 { c0: Fp(0x0be47d343e9848c639fd704009a8d2652cad85772178e1703032b5944fab5daf72e99fe477af24c762848d4b7def53a3), c1: Fp(0x07cc632c005383e1f2d535a1c040b68766ddb0c37c40b9bda002b1cc473bd073c5054ea7b31b11f118f6b33187c9aca1) }, c2: Fp2 { c0: Fp(0x1700ae3e8c3042e372118bb9c0f6348d96d91538384784cebf28886ed1913db75b3add343c841d64fcd1939114cd2abc), c1: Fp(0x1800e73e8f8aab04cefd6ecd27ce169e983102ff95ca06bb4c430ef4d2ecddd4584de8497425a66f91c0b8a4b192a545) } } })
Gt(Fp12 { c0: Fp6 { c0: Fp2 { c0: Fp(0x0178003eae7c88a4ad5f13c854310b283506ada155e5a53643cc552caff14b426550b202c4e4c0cd57df72b38c2b5cef), c1: Fp(0x09d10f6c60254d3188250a19dbc68cde82cab5e8ff3f849fed84792548f94cb444e03e75ca576ec70ea440368885fa14) }, c1: Fp2 { c0: Fp(0x0b4253b22208ab7825f4781dd283ee02d0786f0800e2bce30c13b60f6c9170e0120c18797239755b1ece10203ed46b91), c1: Fp(0x16f3ef2df75e25d76bc393cf4f58e623a9deb49502b6d56017aaca1ef3af9b66fc0907d2c717e48fe358b3441f91938c) }, c2: Fp2 { c0: Fp(0x01a0a4e89ba1ca1fa7d81a91391de6c2be4178c54b413000741a4f5912175cf3876e42c521d04b435fb62b1dc3a07280), c1: Fp(0x12c7655bcf58e9c176e0cd7fd78e8d3431dba4360d008cf85571d55a9cb1bd95a98facd177e7c644702e56c28d6c8b29) } }, c1: Fp6 { c0: Fp2 { c0: Fp(0x0f3603d7e45ea3e8874e65348508b3c51ce17d1fa8ec551606f14faeb922a507d3c25a272d543d761927b0730b471c16), c1: Fp(0x14c23ce6c943fd7aee9f1e62dd5923a7dd48cf0500d17b6bea5f5195b304d2afac7d2b0e0871434d81491533b2479b6a) }, c1: Fp2 { c0: Fp(0x154859b44a49b3b89f3ec018b504bcc8f54b1da8945e518dd801f0533ce1c27eed5a53f6381fa4d6acec131aaf7e97ef), c1: Fp(0x02c0f976a3752e18db1a89e4f8ec732934ec0853ff56d0a2b7ef468515e6a5789b67e0e467c3c5a432603a202e569e8a) }, c2: Fp2 { c0: Fp(0x0cd4c92c74deeea37ffd73da4f0cdb8d0f752a78ff94eca381315cc11853572eacf8ff301234e9290eaa932ef55cfe6d), c1: Fp(0x0283eea35195b0effdf845dee0ff56bd5188a0150002ad13e81cc1d76c32e47d625ee60f585a05ce4e4bd5334a585d6d) } } })
thread 'groth16::proof::test_with_bls12_381::supraseal' panicked at 'assertion failed: verify_proof(&pvk, &proof, &[c]).unwrap()', src/groth16/proof.rs:410:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test groth16::proof::test_with_bls12_381::supraseal ... FAILED

The source for the test is at https://github.com/filecoin-project/bellperson/blob/debug-supraseal-verification/src/groth16/proof.rs#L339-L411. I tried to make it minimal and deterministic as possible.

It passes when using the non-suprasal code path. It can be run via:

RUST_LOG=trace cargo test --lib supraseal -- --nocapture

Any help would be appreciated.

sandsentinel commented 1 year ago

The latest commit should solve this. There needs to be a slight change to bellperson, where b_input_density_total has to be provided after this argument. You can refer to the updated .patch for an example.

dot-asm commented 1 year ago

Just in case, there is an opportunity to omit some redundant casts and mutability. Below harmonizes your code with the abovementioned commit and does the cast ajustments.

index 0f5d412..ec664d1 100644
--- a/src/groth16/prover/supraseal.rs
+++ b/src/groth16/prover/supraseal.rs
@@ -74,9 +74,8 @@ where
     let mut input_assignments_ref = Vec::with_capacity(num_circuits);
     let mut aux_assignments_ref = Vec::with_capacity(num_circuits);
     for i in 0..num_circuits {
-        input_assignments_ref
-            .push(input_assignments_no_repr[i].as_ptr() as *const _ as *const E::Fr);
-        aux_assignments_ref.push(aux_assignments_no_repr[i].as_ptr() as *const _ as *const E::Fr);
+        input_assignments_ref.push(input_assignments_no_repr[i].as_ptr());
+        aux_assignments_ref.push(aux_assignments_no_repr[i].as_ptr());
     }

     let mut a_ref = Vec::with_capacity(num_circuits);
@@ -105,13 +104,14 @@ where
         b_ref.as_slice(),
         c_ref.as_slice(),
         provers[0].a.len(),
-        input_assignments_ref.as_mut_slice(),
-        aux_assignments_ref.as_mut_slice(),
+        input_assignments_ref.as_slice(),
+        aux_assignments_ref.as_slice(),
         input_assignment_len,
         aux_assignment_len,
         provers[0].a_aux_density.bv.as_raw_slice(),
         provers[0].b_aux_density.bv.as_raw_slice(),
         a_aux_density_total,
+        b_input_density_total,
         b_aux_density_total,
         num_circuits,
         r_s.as_slice(),
vmx commented 1 year ago

Thanks a lot for the fix, it works.

Though, if I run it without a GPU being available, then the proof returns a wrong result. I don't know if there's a CPU fallback implemented or not. So in case there is no CPU fallback, I think it should produce an error on proving time, rather then returning a wrong result.

If I should create a separate issue for that, please let me know.

vmx commented 1 year ago

I've pushed new changes to the debug-supraseal-verification branch. So the instructions above still apply (I suggest really making a fresh checking, I had issues things not updating correctly even with running cargo update).

If you want to test it without GPU you can run it as:

CUDA_VISIBLE_DEVICES='' RUST_LOG=trace cargo test --lib supraseal --features cuda-supraseal -- --nocapture

to see the failure.

dot-asm commented 1 year ago

if I run it without a GPU being available, then the proof returns a wrong result.

It's not really a surprise, as there is no CPU fallback and error handling needs more work. We'll get there:-) Thanks for nudging!

vmx commented 1 year ago

I'm seeing a failure (invalid proofs) on CI and locally. I force pushed to debug-supraseal-verification again.

Many tests work, but the minroot one fails. This is the command to run it:

cargo test --test minroot minroot_test --features cuda-supraseal

It's not really a minimal example, it's just the test we have. The code for the failing test is at https://github.com/filecoin-project/bellperson/blob/4c7c3af45e2ddbbf43347d8c78b2571653338e59/tests/minroot.rs#L172-L219. So if that's too big to debug/find the issue let me know and I'll try to minimize it. My hope would be that even in it's current state it's good enough.

The code for reading the parameters doesn't seem to be the issue, the tests at mimc use the same code, but they work as expected.

sandsentinel commented 1 year ago

I pushed another commit which should fix the 2 minroot tests. We require 1 more parameter from bellperson, so bellperson will again need to be modified slightly.

vmx commented 1 year ago

Thanks for the really quick fix, I've verified it locally. Now that all bellperson tests pass, I'm closing this issue. For not having an error when no GPU is available, I've opened a separate issue at https://github.com/supranational/supra_seal/issues/10.