Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form #111

jonathanpwang · 2022-12-21T19:41:02Z

This resolves the discussion in #103 (comment)

With feature "serde-raw", field elements are serialized into [u8; 32] in Montgomery form a * R (mod p) without reduction. Elements are deserialized to [u64; 4] and then checked to see that they are less than the modulus < p.
With feature "raw-unchecked", same as "serde-raw" but the less than modulus check is skipped. Also when possible I favor unwrapping instead of try-branch / try-collect since this feature is optimized for speed.
With no feature selected it goes back to the previous serialization where Montgomery reduction is done, and curves are stored in affine compressed form.

Should be merged after privacy-scaling-explorations/halo2curves#10 but can be reviewed concurrently.

Changes are overall isolated to the helper traits in helpers.rs but there is some cosmetic annoyance in that to implement traits like Params or Prover I had to add extra where clauses. I didn't see a way around this, and in any case the helper traits are automatically implemented so it should have no effect on external users of the API.

long as we serialize `params.k()`

serialize/deserialize KZG params, verifying key, and proving key directly into raw bytes in internal memory format. So field elements are stored in Montgomery form `a * R (mod p)` and curve points are stored without compression.

CPerezz

Independently of the changes requested on privacy-scaling-explorations/halo2curves#10 (review) which may enforce changes here too, I have some concerns regarding the PR:

Why are we forcing the usage of features here? This would mean that I cannot serialize on different manners if I want to. As the read/write will be done only on one way prohibiting the other one.
IMO, this should be something that can co-exist. So that you have methods avaliable to serialize and you choose whatever you want.

Let's see first what @kilic and @han0110 think. But for me this change should be done no matter what.

jonathanpwang · 2022-12-22T14:46:29Z

Ah I think I incorrectly inferred from our previous discussion that the optionality should be toggled with features. But it is an easy fix: I can either just have separate functions read, read_raw, read_raw_unchecked or have read accept an Enum with the 3 options.

CPerezz · 2022-12-23T10:56:40Z

Ah I think I incorrectly inferred from our previous discussion that the optionality should be toggled with features. But it is an easy fix: I can either just have separate functions read, read_raw, read_raw_unchecked or have read accept an Enum with the 3 options.

I probably miss-explained myself 😆. Not strongly oppinionated on the Enum/multiple functions. I'd say the closest to the standard way of doing it in Rust, the better!

CPerezz · 2023-01-04T12:50:18Z

halo2curves new tag was released with the serialization proposal by @jonathanpwang. See: https://github.com/privacy-scaling-explorations/halo2curves/releases/tag/0.3.1

We should work towards merging this too.

jonathanpwang · 2023-01-04T15:20:13Z

OK I will work on it.

han0110

All changes look good to me, also don't have strong opinion about read with enum or multiple functions.

halo2_proofs/src/poly/kzg/commitment.rs

CPerezz · 2023-01-13T08:37:27Z

@jonathanpwang do you plan to update this and merge with the features or are you going to impl multiple functions??

jonathanpwang · 2023-01-13T16:32:18Z

Sorry for the delay. I plan to remove the features and add some enum option. I'll do it this weekend.

CPerezz · 2023-01-14T11:41:07Z

Hey @jonathanpwang no rush at all mate! Was mainly to know so that we might or not include this in the next release tag. Nothing else! Take your time!

jonathanpwang · 2023-01-14T18:37:49Z

Would be great to include in the next tag, so I will rush :)

serialization/deserialization format of curve and field elements

jonathanpwang · 2023-01-16T02:06:10Z

@CPerezz I pushed an update removing the features and using an enum SerdeFormat to specify serialization strategy. Please take a look.

han0110

All LGTM now! Nice work!

CPerezz

LGTM!!! Thanks @jonathanpwang

@lispc

* - Implements `PartialOrd` for `Value<F>` - Adds a `transpose` method to turn `Value<Result<_>>` into `Result<Value<_>>` - `Expression::identifier()` remove string memory reallocation * Fix MockProver `assert_verify` panic errors (privacy-scaling-explorations#118) * fix: Support dynamic lookups in `MockProver::assert_verify` Since lookups can only be `Fixed` in Halo2-upstream, we need to add custom suport for the error rendering of dynamic lookups which doesn't come by default when we rebase to upstream. This means that now we have to print not only `AdviceQuery` results to render the `Expression` that is being looked up. But also support `Instance`, `Advice`, `Challenge` or any other expression types that are avaliable. This addresses the rendering issue, renaming also the `table_columns` variable for `lookup_columns` as the columns do not have the type `TableColumn` by default as opposite to what happens upstream. * fix: Don't error and emit empty String for Empty queries * feat: Add `assert_sarisfied_par` fn to `MockProver` * fix: Address clippy errors * chore: Address review comments * chore: Fix clippy lints Resolves: privacy-scaling-explorations#116 * Remove partial ordering for value * Remove transpose * Parallelize SHPLONK multi-open prover (privacy-scaling-explorations#114) * feat: parallelize (cpu) shplonk prover * shplonk: improve `construct_intermediate_sets` using `BTreeSet` and `BTreeMap` more aggressively * shplonk: add `Send` and `Sync` to `Query` trait for more parallelization * fix: ensure the order of the collection of rotation sets is independent of the values of the opening points Co-authored-by: Jonathan Wang <jonathanpwang@users.noreply.github.com> * fix: FailureLocation::find empty-region handling (privacy-scaling-explorations#121) After working on fixing privacy-scaling-explorations/zkevm-circuits#1024, a bug was found in the verification fn of the MockProver which implies that while finding a FailureLocation, if a Region doesn't contain any rows. This is fixed by introducing a 2-line solution suggested by @lispc. Resolves: privacy-scaling-explorations#117 * Feature: Expose Fixed columns & Assembly permutation structs in MockProver instance (privacy-scaling-explorations#123) * feat: Expose fixed columns in MockProver * change: Make `Assembly` object public & add getters * chore: Address leftover TODOs * Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form (privacy-scaling-explorations#111) * feat: read `VerifyingKey` and `ProvingKey` does not require `params` as long as we serialize `params.k()` * feat: add features "serde-raw" and "raw-unchecked" to serialize/deserialize KZG params, verifying key, and proving key directly into raw bytes in internal memory format. So field elements are stored in Montgomery form `a * R (mod p)` and curve points are stored without compression. * chore: switch to halo2curves 0.3.1 tag * feat: add enum `SerdeFormat` for user to select serialization/deserialization format of curve and field elements Co-authored-by: Jonathan Wang <jonathanpwang@users.noreply.github.com> * Add support for Column annotations for MockProver debugging (privacy-scaling-explorations#109) * feat: Add `name_column` to `Layouter` & `RegionLayouter` This adds the trait-associated function `name_column` in order to enable the possibility of the Layouter to store annotations aobut the colums. This function does nothing for all the trait implementors (V1, SimpleFloor, Assembly....) except for the `MockProver`. Which is responsible of storing a map that links within a `Region` index, the `column::Metadata` to the annotation `String`. * feta: Update metadata/dbg structs to hold Col->Ann mapping * feat: Update emitter module to print Column annotations * feat: Add lookup column annotations This adds the fn `annotate_lookup_column` for `ConstraintSystem` which allows to carry annotations for the lookup columns declared for a circuit within a CS. * feat: Add Lookup TableColumn annotations This allows to annotate lookup `TableColumn`s and print it's annotation within the `assert_satisfied` fn. This has required to change the `ConstraintSystem::lookup_annotations` to have keys as `metadata::Column` rather than `usize` as otherwise it's impossible within the `emitter` scope to distinguish between regular advice columns (local to the Region) and fixed columns which come from `TableColumn`s. * fix: Customly derive PartialEq for metadata::Region This allows to ignore the annotation map of the metadata::Region so that is easier to match against `VerifyFailure` errors in tests. * fix: Update ConstraintNotSatisfied testcase * fix: Update Debug & Display for VerifyFailure It was necessary to improve the `prover.verify` output also. To do so, this required auxiliary types which are obfuscated to any other part of the lib but that are necessary in order to be able to inject the Column names inside of the `Column` section itself. This also required to re-implement manually `Debug` and `Display` for this enum. This closes zcash#705 * fix: Address clippy & warnings * fix: Add final comments & polish * fix: Resolve cherry-pick merge conflics & errors * chore: Change DebugColumn visibility * chore: Allow to fetch annotations from metadata * chore: Fix clippy lints * chore: Remove comments from code for testing * feat: Add support for Advice and Instance anns in lookups * feat: Allow `V1` layouter to annotate columns too * fix: Support `Constant` & `Selector` for lookup exprs * chore: Address review comments * chore: Propagete write! result in `VerifyFailure::Display` * chore: Address clippy lints * chore: Move Codecov, wasm-build, Bitrot & doc-tests to push (privacy-scaling-explorations#125) * chore: Move Codecov, wasm-build, Bitrot & doc-tests to push This should cut down significantly the CI times on every push done to a branch for a PR. Resolves: privacy-scaling-explorations#124 * chore: Add back `push` on CI checks * fix: Allow to compare `Assembly` structs (privacy-scaling-explorations#126) This was missing in privacy-scaling-explorations#123 so this PR fixes it. * Add keccak256 hasher for transcript (#2) * Add keccak256 hasher for transcript * Fix keccak256 common point prefix * Remove unnecessary hasher_* variables * fix: transcript instantiation in poseidon benchmark loop (privacy-scaling-explorations#128) * Improve performance of vk & pk keygen and of default `parallelize` chunking size (privacy-scaling-explorations#127) * Squashed commit of the following: commit 17e3c4e Author: Mickey <hedgefund996@gmail.com> Date: Fri Jul 15 11:10:32 2022 +0800 speed up generate vk pk with multi-thread * fix * Improve performance of vk & pk keygen and of default `parallelize` chunking size. Reduces proving time on large circuits consistently >=3%. Builts upon [speed up generate vk pk with multi-thread](privacy-scaling-explorations#88) Fixes: privacy-scaling-explorations#83 * fix: Force `VerifyFailure` to own the annotations map (privacy-scaling-explorations#131) * fix: Force `VerifyFailure` to own the annotations map Since otherwise we can't move the `VerifyFailure` vec's confortably, and also, we're required to have a lot of lifetime annotations, it was decided to force the `VerifyFailure` to own the Annotation maps. This shouldn't be too harmful as it only triggers when testing. Resolves: privacy-scaling-explorations#130 * chore: Address clippy lints * feat: call synthesize in `MockProver` multiple times to behave same as real prover * feat: check advice assignment consistency between different phases * fix: Support annotations for CellNotAssigned in verify_par (privacy-scaling-explorations#138) * feat: Add `assert_satisfied_at_rows_par` variant (privacy-scaling-explorations#139) Resolves: privacy-scaling-explorations#133 * Expose mod `permutation` and re-export `permutation::keygen::Assembly` (privacy-scaling-explorations#149) * feat: expose mod ule `permutation` and re-export `permutation::keygen::Assembly` * feat: derive `lone` for `permutation::keygen::Assembly` * feat: bump MSRV for `inferno` * feat(MockProver): replace errors by asserts In MockProver, replace all code that returns an error by an assert that panics instead of returning the error. This change aims to make it easier to debug circuit code bugs by getting backtraces. * MockProver test utililities (privacy-scaling-explorations#153) * test/unwrap_value: escape Value safety in the dev module * test/mock-prover-values: MockProver exposes the generated columns to tests * test/mock-prover-values: doc * mockprover-util: remove unwrap_value --------- Co-authored-by: Aurélien Nicolas <info@nau.re> * feat: Parallel random blinder poly impl (privacy-scaling-explorations#152) * feat: Parallelize `commit` blinder poly generator method Solves the concerns raised in privacy-scaling-explorations#151 related to the performance of the random poly generator inside of `commit`. Resolves: privacy-scaling-explorations#151 * chore: add `from_evals` for Polynomial * chore: add benches for commit_zk serial vs par * fix: Correct thread_seeds iter size * fix: Clippy * chore: apply review suggestions * fix: Inconsisten num of Scalars generated parallely This fix from @ed255 fixes an error on the code proposal which was rounding the num of Scalars to be generated and so, was producing failures. Co-authored-by: Edu <eduardsanou@posteo.net> * remove: legacy comments & code --------- Co-authored-by: Edu <eduardsanou@posteo.net> * change: Migrate workspace to pasta_curves-0.5 (privacy-scaling-explorations#157) * change: Migrate workspace to pasta_curves-0.5 This ports the majority of the workspace to the `pasta_curves-0.5.0` leaving some tricky edge-cases that we need to handle carefully. Resolves: privacy-scaling-explorations#132 * fix: Complete latest trait bounds to compile halo2proofs * change: Migrate examples & benches to pasta 0.5 * change: Migrate halo2_gadgets to pasta-0.5 * change: Update gadgets outdated code with latest upstream * fix: Sha3 gadget circuit * fix: doc tests * chore: Update merged main * fix: Apply review suggestions * fix: pin `halo2curves` version to `0.3.2` * Extend Circuit trait to take parameters in config (privacy-scaling-explorations#168) * Extend Circuit trait to take parameters in config The Circuit trait is extended with the following: ``` pub trait Circuit<F: Field> { /// [...] type Params: Default; fn params(&self) -> Self::Params { Self::Params::default() } fn configure_with_params(meta: &mut ConstraintSystem<F>, params: &Self::Params) -> Self::Config { Self::configure(meta) } fn configure(meta: &mut ConstraintSystem<F>) -> Self::Config; } ``` This allows runtime parametrization of the circuit configuration. The extension to the Circuit trait has been designed to minimize the breaking change: existing circuits only need to define the associated `type Params`. Unfortunately "Associated type defaults" are unstable in Rust, otherwise this would be a non-breaking change. See rust-lang/rust#29661 * Implement circuit params under feature flag * Don't overwrite configure method * Fix doc test * Allow halo2 constraint names to have non static names (privacy-scaling-explorations#156) * static ref to String type in Gates, Constraints, VirtualCell, Argument * 'lookup'.to_string() * return &str for gate name and constriant_name, also run fmt * Update halo2_gadgets/Cargo.toml Co-authored-by: Han <tinghan0110@gmail.com> * upgrade rust-toochain --------- Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> Co-authored-by: Han <tinghan0110@gmail.com> * Improve halo2 query calls (privacy-scaling-explorations#154) * return expression from cell * add example * selector * recurse Expression to fill in index * minimized changes from the original * backword compatible meta.query_X & challange.expr() * cargo fmt * fixed lookup to pass all tests * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * Update comments Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * update Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> * add primitives.rs back * remove example2 * backward compatible meta.query_X & Column.cur(), next(), prev(), at(usize) * impl Debug & make side effects only when query.index.is_none() * change impl Debug for Expression instead & revert test in plonk_api * upgrade rust-toolchain * Update halo2_proofs/src/plonk/circuit.rs Co-authored-by: Han <tinghan0110@gmail.com> * Update halo2_proofs/src/plonk/circuit.rs Co-authored-by: Han <tinghan0110@gmail.com> * ran clippy * Update halo2_proofs/src/plonk/circuit.rs Co-authored-by: Han <tinghan0110@gmail.com> --------- Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> Co-authored-by: Han <tinghan0110@gmail.com> * fix: compute `num_chunks` more precisely (privacy-scaling-explorations#172) * Implement Clone trait for Hash, Absorbing, and Sponge structs (privacy-scaling-explorations#171) * Revert double-assignment mock prover check Revert the check introduced in privacy-scaling-explorations#129 to detect double assignments with different values, because it breaks some tests in the zkevm project. There's a legitimate use case of double assignment with different values, which is overwriting cells in order to perform negative tests (tests with bad witness that should not pass the constraints). Also in the EVM Circuit from the zkevm project we "abuse" the assignment of cells as a cache: sometimes we assign some cells with a guess value, and later on we reassign with the correct value. I believe this check is interesting to have, so we could think of ways to add it back as an optional feature. * fix: Fix serialization for VerifyingKey (privacy-scaling-explorations#178) Now the value returned when the number of selectors is a multiple of 8 is correct. Resolves: privacy-scaling-explorations#175 * Add more getters to expose internal fields * add a constructor (privacy-scaling-explorations#164) * add a constructor * add more comment * fix as review * remove clone * remove * no need to use new variable * change comment * fix clippy * rename to from_parts * remove n declaration * feat: send sync region (privacy-scaling-explorations#180) * feat: send / sync region * Update layout.rs * update * lol * debug * Update keygen.rs * Update keygen.rs * Update keygen.rs * Update keygen.rs * thread-safe-region feature flag * cleanup * patch dev-graph * patch non-determinism in mapping creation * reduce mem usage for vk and pk * mock proving examples * swap for hashmap for insertion speed * reduce update overhead * replace BTree with Vec * add benchmarks * make the benchmarks massive * patch clippy * simplify lifetimes * patch benches * Update halo2_proofs/src/plonk/permutation/keygen.rs Co-authored-by: Han <tinghan0110@gmail.com> * Update halo2_proofs/examples/vector-mul.rs Co-authored-by: Han <tinghan0110@gmail.com> * rm benches * order once * patch lints --------- Co-authored-by: Han <tinghan0110@gmail.com> * Fix `parallelize` workload imbalance (privacy-scaling-explorations#186) * fix parallelize workload imbalance * remove the need of unsafe * implement native shuffle argument and api * fix: remove nonsense comment * strictly check shuffle rows * address doc typos * move compression into product commitment * typo * add shuffle errors for `verify_at_rows_par` * dedup expression evaluation * cargo fmt * fix fields in sanity-checks feature * Updates halo2_curves dependency to released package (privacy-scaling-explorations#190) THe package release ressets the version from those inherited by the legacy halo2curves repo's fork history. The upstream diff is: https://github.com/privacy-scaling-explorations/halo2curves/compare/9f5c50810bbefe779ee5cf1d852b2fe85dc35d5e..9a7f726fa74c8765bc7cdab11519cf285d169ecf * chore: remove monorepo Go back to having halo2curves and poseidon in separate repos. * chore: fix clippy and tests * fix: remove thread-safe-regions feature `WitnessCollection` in `create_proof` isn't thread-safe. We removed `Region`s from `SimpleLayouter` anyways. * fix: rustfmt * fix: dev-graph * chore: update lint CI name * chore: fix clippy * chore: autoexample = false turn off examples that use layouter * chore(CI): separate job for examples * chore: remove prefetch from asm, not used * chore: fix asm feature --------- Co-authored-by: adria0 <nowhere@> Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com> Co-authored-by: Jonathan Wang <jonathanpwang@users.noreply.github.com> Co-authored-by: kilic <kiliconu@itu.edu.tr> Co-authored-by: dante <45801863+alexander-camuto@users.noreply.github.com> Co-authored-by: pinkiebell <40266861+pinkiebell@users.noreply.github.com> Co-authored-by: han0110 <tinghan0110@gmail.com> Co-authored-by: Eduard S <eduardsanou@posteo.net> Co-authored-by: naure <naure@users.noreply.github.com> Co-authored-by: Aurélien Nicolas <info@nau.re> Co-authored-by: CeciliaZ030 <45245961+CeciliaZ030@users.noreply.github.com> Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com> Co-authored-by: Enrico Bottazzi <85900164+enricobottazzi@users.noreply.github.com> Co-authored-by: Ethan-000 <s2026080@ed.ac.uk> Co-authored-by: Mamy Ratsimbazafy <mamy_github@numforge.co> Co-authored-by: kilic <onurkilic1004@gmail.com> Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>

jonathanpwang added 2 commits December 21, 2022 12:16

feat: read VerifyingKey and ProvingKey does not require params as

aa09e10

long as we serialize `params.k()`

feat: add features "serde-raw" and "raw-unchecked" to

db67f11

serialize/deserialize KZG params, verifying key, and proving key directly into raw bytes in internal memory format. So field elements are stored in Montgomery form `a * R (mod p)` and curve points are stored without compression.

CPerezz self-requested a review December 22, 2022 09:55

CPerezz requested changes Dec 22, 2022

View reviewed changes

CPerezz requested review from kilic and han0110 January 4, 2023 12:50

chore: switch to halo2curves 0.3.1 tag

7a444d3

han0110 reviewed Jan 6, 2023

View reviewed changes

halo2_proofs/src/poly/kzg/commitment.rs Outdated Show resolved Hide resolved

jonathanpwang added 2 commits January 15, 2023 17:45

Merge remote-tracking branch 'upstream/main' into feat/serde-raw

fe81922

feat: add enum SerdeFormat for user to select

607c3a4

serialization/deserialization format of curve and field elements

han0110 approved these changes Jan 16, 2023

View reviewed changes

CPerezz approved these changes Jan 16, 2023

View reviewed changes

CPerezz merged commit 4ad07d4 into privacy-scaling-explorations:main Jan 16, 2023

alexander-camuto mentioned this pull request Jan 31, 2023

Implement SerdeObject for pasta curves privacy-scaling-explorations/halo2curves#23

Closed

guorong009 mentioned this pull request Oct 11, 2024

Serialization privacy-scaling-explorations/halo2curves#176

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form #111

Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form #111

jonathanpwang commented Dec 21, 2022

CPerezz left a comment

jonathanpwang commented Dec 22, 2022

CPerezz commented Dec 23, 2022

CPerezz commented Jan 4, 2023

jonathanpwang commented Jan 4, 2023

han0110 left a comment

CPerezz commented Jan 13, 2023

jonathanpwang commented Jan 13, 2023

CPerezz commented Jan 14, 2023

jonathanpwang commented Jan 14, 2023

jonathanpwang commented Jan 16, 2023

han0110 left a comment

CPerezz left a comment

Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form #111

Feature to serialize/deserialize KZG params, verifying key, and proving key into uncompressed Montgomery form #111

Conversation

jonathanpwang commented Dec 21, 2022

CPerezz left a comment

Choose a reason for hiding this comment

jonathanpwang commented Dec 22, 2022

CPerezz commented Dec 23, 2022

CPerezz commented Jan 4, 2023

jonathanpwang commented Jan 4, 2023

han0110 left a comment

Choose a reason for hiding this comment

CPerezz commented Jan 13, 2023

jonathanpwang commented Jan 13, 2023

CPerezz commented Jan 14, 2023

jonathanpwang commented Jan 14, 2023

jonathanpwang commented Jan 16, 2023

han0110 left a comment

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment