Serializaiton regression tests for proof-systems and parts of kimchi #2598

volhovm · 2024-09-24T13:46:52Z

This PR adds some serialization regression tests, in particular for SRS, PolyComm, OpeningProof, ProverProof.

I also had to make rng an explicit parameter in kimchi's prover.

Reference values are valid w.r.t. 1494cf973d40fb276465929eb7db1952c5de7bdc (arkworks 0.3.0).

dannywillems · 2024-09-24T14:21:00Z

kimchi/src/prover.rs

@@ -165,13 +169,15 @@ where
    pub fn create_recursive<


That seems to introduce a change also in the whole Mina repository. We might want to be more careful regarding this. Can we avoid this in this PR?

We can't really, since then the prover is non-deterministic and it serializes to a different value every time.

dannywillems

I don't think we should introduce the rng parameter in the prover in this PR.

volhovm · 2024-09-24T16:35:38Z

@dannywillems Do you want it in a different PR or...? I think it's a fairly small cosmetic change, and it's necessary to be able to run the prover in a reproducible way, so why not?

Edit: you're right that it introduces a change in the mina repo, but only in like ... two places in bindings. So it's still quite lightweight.

dannywillems · 2024-09-24T20:10:57Z

@dannywillems Do you want it in a different PR or...? I think it's a fairly small cosmetic change, and it's necessary to be able to run the prover in a reproducible way, so why not?

Edit: you're right that it introduces a change in the mina repo, but only in like ... two places in bindings. So it's still quite lightweight.

Ok, giving a second thought...
Can you make the change in Mina accordingly so we can merge all of them? It will imply changing the o1js-bindings also. Let's try to keep the old convention that the branches in o1js-bindings, mina and proof-systems for develop should be all aligned all the time.

volhovm · 2024-09-24T21:20:47Z

Sounds good, will do tomorrow!

… kimchi

volhovm · 2024-09-25T10:16:04Z

Done for mina in MinaProtocol/mina#16139. WIll see regarding o1js.

volhovm · 2024-09-25T10:46:24Z

And here's o1js and o1js-bindings:

kimchi/src/tests/and.rs

poly-commitment/src/tests/commitment.rs

utils/Cargo.toml

dannywillems · 2024-09-25T15:20:20Z

kimchi/src/tests/and.rs

+fn test_serialization_regression() {
+    // Generated with commit 1494cf973d40fb276465929eb7db1952c5de7bdc
+    let buf_expected = vec![
+        149, 148, 159, 145, 145, 196, 33, 36, 165, 245, 213, 186, 207, 201, 96, 141, 145, 71, 154,


Nit: could be nice to load the value from a file.

dannywillems · 2024-09-25T15:20:42Z

poly-commitment/src/tests/serialization.rs

+    let data_expected = SRS::<Vesta>::create_trusted_setup(td1, 1 << 3);
+    // Generated with commit 1494cf973d40fb276465929eb7db1952c5de7bdc
+    let buf_expected: Vec<u8> = vec![
+        146, 152, 196, 33, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,


Nit: could be nice to load the value from a file

dannywillems · 2024-09-25T15:20:52Z

poly-commitment/src/tests/serialization.rs

+    let data_expected = SRS::<Pallas>::create_trusted_setup(td2, 1 << 3);
+    // Generated with commit 1494cf973d40fb276465929eb7db1952c5de7bdc
+    let buf_expected: Vec<u8> = vec![
+        146, 152, 196, 33, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,


Nit: could be nice to load the value from a file

dannywillems · 2024-09-25T15:21:03Z

poly-commitment/src/tests/serialization.rs

+    let data_expected = chunked_commitment;
+    // Generated with commit 1494cf973d40fb276465929eb7db1952c5de7bdc
+    let buf_expected: Vec<u8> = vec![
+        145, 150, 196, 33, 36, 158, 70, 161, 147, 233, 138, 19, 54, 52, 87, 58, 158, 154, 255, 197,


Nit: could be nice to load the value from a file

dannywillems · 2024-09-25T15:21:12Z

poly-commitment/src/tests/serialization.rs

+
+    // Generated with commit 1494cf973d40fb276465929eb7db1952c5de7bdc
+    let buf_expected: Vec<u8> = vec![
+        149, 151, 146, 196, 33, 166, 183, 76, 209, 121, 62, 56, 194, 134, 133, 238, 52, 190, 163,


Nit: could be nice to load the value from a file

I'd rather keep it here tbh. I know it's big and ugly, but file management is more annoying imo, you rely on extra kernel calls, it's more files, and if we change serialization in a certain way git would probably just think the whole binary file was replaced, even if it's just a bit. The last case we had before actually when I was replacing the SRS serialization, you could clearly see that the new serialization is just one bit off, which helps with debugging.

dannywillems · 2024-09-25T18:10:19Z

Feel free to merge when the PR in o1js/o1js-bindings are good to be merged too.

volhovm force-pushed the volhovm/arkworks042-regression-tests branch from c2c8040 to d48799c Compare September 24, 2024 13:56

volhovm requested a review from shimkiv September 24, 2024 13:57

volhovm assigned dannywillems and unassigned dannywillems Sep 24, 2024

volhovm requested a review from dannywillems September 24, 2024 13:57

volhovm force-pushed the volhovm/arkworks042-regression-tests branch 2 times, most recently from 90ba6d9 to a1f5545 Compare September 24, 2024 14:00

dannywillems reviewed Sep 24, 2024

View reviewed changes

dannywillems requested changes Sep 24, 2024

View reviewed changes

Write serialization regression tests for poly-commitment and parts of…

eb5357a

… kimchi

volhovm force-pushed the volhovm/arkworks042-regression-tests branch from a1f5545 to eb5357a Compare September 25, 2024 09:52

volhovm mentioned this pull request Sep 25, 2024

Update proof-systems: external prover randomness & regression tests MinaProtocol/mina#16139

Merged

This was referenced Sep 25, 2024

Update mina to contain explicit prover randomness (compatible) o1-labs/o1js#1835

Merged

Regenerate bindings w.r.t. proof-systems update (external prover rng) o1-labs/o1js-bindings#301

Merged