[step1] implementing serialization of types via Serde #160

mimoo · 2021-10-04T17:37:35Z

We currently use serialization of our types by hand: https://github.com/MinaProtocol/mina/blob/develop/src/lib/marlin_plonk_bindings/stubs/src/index_serialization.rs

This is error-prone, and hard to maintain. As types and structs change, we need to make sure to change the serialization logic. The Rust-way is to use Serde, which offers a set of macros that we can use to implement serialization and deserialization traits for all of our types.

One of the problem we have is that arkworks, for some yet-unknown reason, doesn't implement Serde::{Serialize, Deserialize} on its types, but instead its own set of ark-serialize traits. This means that we need to tell serde how to use these arkworks-specific traits to serialize and deserialize.

No biggy, the serde_with crate comes to the rescue. We just need to annotate fields like this now:

#[serde_as]
#[derive(Debug, Clone, Copy, Serialize, Deserialize)]
pub struct EvaluationDomains<F: FftField> {
    #[serde_as(as = "o1_utils::serialization::SerdeAs")]
    pub d1: Domain<F>, // size n

The nice part is that it works with containers too:

    #[serde_as(as = "[o1_utils::serialization::SerdeAs; PERMUTS]")]
    pub shift: [Fr<G>; PERMUTS],

mimoo · 2021-10-04T17:52:18Z

utils/src/dense_polynomial.rs

@@ -1,5 +1,12 @@
+//! This adds a few utility functions for the [DensePolynomial] arkworks type.


squeezing in this commit, it's a no-op

mimoo · 2021-10-04T17:52:45Z

circuits/plonk-15-wires/src/gate.rs

@@ -13,7 +13,7 @@ use serde_with::serde_as;
 use std::io::{Error, ErrorKind, Read, Result as IoResult, Write};


squeezing this commit as well, just adding some ocaml types that I need

mimoo · 2021-10-04T18:05:42Z

ocaml-gen/derive/src/lib.rs

@@ -139,7 +139,7 @@ pub fn derive_ocaml_enum(item: TokenStream) -> TokenStream {



squeezing this one as well. This is to get better errors when using ocaml-gen

mimoo · 2021-10-04T18:06:10Z

circuits/plonk-15-wires/src/nolookup/constraints.rs

    pub endo: F,

    /// random oracle argument parameters
+    #[serde(skip)]


I created an issue about skipping fields: #161

mrmr1993

Approved with nits. Can you add comments for the `TODO-someday's?

mrmr1993 · 2021-10-11T17:40:06Z

utils/src/serialization.rs

+
+    /// You can use this to serialize an arkworks type with serde and the "serialize_with" attribute.
+    /// See https://serde.rs/field-attrs.html
+    pub fn serialize<S>(val: impl CanonicalSerialize, serializer: S) -> Result<S::Ok, S::Error>


Efficiency nit: using a generic <T: CanonicalSerialize>(val: T, ...) gets compile-time specialised instead of using a vtable

I think these two are the same :o

the only difference is when the impl is on the return value, IIUC

Indeed, it does seem to just be syntactic sugar. I'd confused it with dyn 🤦‍♂️ Sorry for the noise!

mrmr1993 · 2021-10-11T17:51:18Z

circuits/plonk-15-wires/src/gate.rs

+        T2: From<T1>,
+    {
+        (
+            a[0].clone().into(),


TODO-someday: It would be better to use ocaml::Value::alloc_tuple and then store_field for each value, to avoid cloning

actually, I think we can do this:

let (a0, a1, a2) = (a[0], a[1], a[2])

which might move the value

created issue #178

mrmr1993 · 2021-10-11T17:52:02Z

circuits/plonk-15-wires/src/gate.rs

+        T2: From<T1>,
+    {
+        [
+            a.0.into(),


TODO-someday: it would be better to use ocaml::Value::field to avoid copying in rust here.

mrmr1993 · 2021-10-11T17:55:15Z

dlog/plonk-15-wires/tests/generic.rs

@@ -61,7 +60,15 @@ fn test_generic_gate() {
    // add a zero gate, just because
    let wires = Wire::new(abs_row);
    gates.push(CircuitGate::<Fp>::zero(abs_row, wires));
-    abs_row += 1;
+    //abs_row += 1;


left there in case someone wants to continue the test with more gates :o

…de_with

…ved to serde)

because Into doesn't automatically derive From, whereas From does derive Into magically.

…rifierIndex

…ialization

…to capitalized

mimoo · 2021-10-11T18:58:11Z

rebase

mimoo requested review from imeckler, jspada and mrmr1993 October 4, 2021 17:38

mimoo mentioned this pull request Oct 4, 2021

[crypto] kimchi MinaProtocol/mina#9218

Closed

9 tasks

mimoo commented Oct 4, 2021

View reviewed changes

mimoo mentioned this pull request Oct 4, 2021

[serialization] safer serialization #161

Closed

mimoo commented Oct 4, 2021

View reviewed changes

Base automatically changed from mimoo/15-wire-plonk-fixes-3 to master October 7, 2021 17:38

mimoo changed the title ~~implementing serialization of types via Serde~~ [step1] implementing serialization of types via Serde Oct 11, 2021

mrmr1993 approved these changes Oct 11, 2021

View reviewed changes

mimoo mentioned this pull request Oct 11, 2021

[ocaml] refactor tuple <-> array conversion #178

Closed

mimoo added 17 commits October 11, 2021 11:57

[serialization] implementing serialization of types via serde and ser…

bd74b00

…de_with

[serialization] implement serialization on several types

1deb692

[o1-utils] cleaning up + doc

f90299d

[ocaml] implementing ocaml wrappers for circuit gate and wires

57dde0b

[ocaml-gen] better errors

cb63fc0

[SRS] move SRSValue and SRSSpec to dlog/plonk (it's plonkv1 specific)

02cb1c4

[SRS] remove manual serialization/deserialization functions (as we mo…

7f4cb53

…ved to serde)

[SRS] implement serialization for SRS

4b5d8df

[15-wires] remove SRSSpec and SRSValue and use Rc<SRS> instead

ff15939

[ocaml] implement From instead of Into

d0e7339

because Into doesn't automatically derive From, whereas From does derive Into magically.

[15-wires][serialization] make serde work for PolyComm and VerifierIndex

3c168c7

[15-wires][serialization] implement serialization to/from file for Ve…

d03c89c

…rifierIndex

[15-wires][serialization] add test for VerifierIndex and PolyComm ser…

4dfdd3a

…ialization

[ocaml] changes to make ocaml bindings work

2507be4

[ocaml-gen] write to output file instead of stdout (+ indentation)

0d0cf72

[ocaml-gen] enforce documentation through macro

ca1d205

[ocaml-gen] add README

f6b144e

mimoo added 5 commits October 11, 2021 11:57

[ocaml-gen] use README for rustdoc

bcbfeea

[ocaml-gen] make custom types re-declarable

0c7baaa

[ocaml-gen] camelcase is not a requirement for ocaml modules, change …

6a3418b

…to capitalized

[ocaml-gen] introduce decl_type_alias! macro

7b643b2

[ocaml-gen] add 'nonrec' to every type declaration

aff4fd3

mimoo force-pushed the mimoo/serialization3 branch from c2f59a4 to aff4fd3 Compare October 11, 2021 18:58

mimoo merged commit 34e0c85 into master Oct 11, 2021

mimoo deleted the mimoo/serialization3 branch October 11, 2021 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[step1] implementing serialization of types via Serde #160

[step1] implementing serialization of types via Serde #160

mimoo commented Oct 4, 2021 •

edited

Loading

mimoo Oct 4, 2021

mimoo Oct 4, 2021

mimoo Oct 4, 2021

mimoo Oct 4, 2021

mrmr1993 left a comment

mrmr1993 Oct 11, 2021

mimoo Oct 11, 2021

mimoo Oct 11, 2021

mrmr1993 Oct 11, 2021 •

edited

Loading

mimoo Oct 11, 2021

mrmr1993 Oct 11, 2021

mimoo Oct 11, 2021

mimoo Oct 11, 2021

mrmr1993 Oct 11, 2021

mrmr1993 Oct 11, 2021

mimoo Oct 11, 2021

mimoo commented Oct 11, 2021

		@@ -1,5 +1,12 @@
		//! This adds a few utility functions for the [DensePolynomial] arkworks type.

		@@ -13,7 +13,7 @@ use serde_with::serde_as;
		use std::io::{Error, ErrorKind, Read, Result as IoResult, Write};

		@@ -139,7 +139,7 @@ pub fn derive_ocaml_enum(item: TokenStream) -> TokenStream {

[step1] implementing serialization of types via Serde #160

[step1] implementing serialization of types via Serde #160

Conversation

mimoo commented Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmr1993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmr1993 Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo commented Oct 11, 2021

mimoo commented Oct 4, 2021 •

edited

Loading

mrmr1993 Oct 11, 2021 •

edited

Loading