Add trait to serialize field and curve objects directly into raw bytes without Montgomery reduction #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`secp256k1::{Fp,Fq}`
* I see no longer to use `ct_eq` direct implementation of `PartialEq`
* deriving `Hash` for all fields because it may be useful and doesn't
hurt
bytes * implement `SerdeObject` for all macro-derived prime fields (raw bytes means staying in Montgomery form) * implement `SerdeObject` for `Fq2` directly * implement `SerdeObject` via macro for curves `bn254` and `secp256k1` * add tests `test_serialization` for fields and curves
CPerezz
requested changes
Dec 22, 2022
There was a problem hiding this comment.
I love this!! is something I've always missed from the ZCash libs TBH. As big prover keys take an insane amount of time to be read.
I've placed some comments. But overall looks really solid.
Aside from the comments, I think we should consider:
- Add wrong tests to check correctness of the implementation. (Wrong bytes or wrong length of them should never end on a correct Struct formation unless we use
uncheck).
On a sidenote, I'll try to upstream this to the Halo2 dev group as if we could push this upstream and get it merged would be better (it would allow to keep trait interface compatibility). This will not condition the merge of this PR (if upstream is not accepted, we will merge it here most probably anyway).
kilic
reviewed
Dec 23, 2022
There was a problem hiding this comment.
- Not in this PR but we may consider removing
from_bytesandto_bytesimplementations which are only wrappers forfrom_reprandto_repr - How bad we want to add
read,writefunctions? Seems to me like they do the same withfrom_raw_bytes&to_raw_bytes - Some of added code here might be reused in
to_reprandfrom_repr
|
|
||
| /// Lexicographic comparison of Montgomery forms. | ||
| #[inline(always)] | ||
| fn is_less_than(x: &[u64; 4], y: &[u64; 4]) -> bool { |
There was a problem hiding this comment.
In PrimeField::from_repr implementation there might a bit more efficient method like subtracting from modulus and checking the borrow in the end. Would it be good idea to reuse it here for the same purpose?
There was a problem hiding this comment.
Great suggestion! I actually used is_less_than because I had some anecdotal impression the branching would make it on average faster. But I just added a criterion benchmark and the results speak for themselves:
Big less than methods/is_less_than/
time: [572.38 ps 573.28 ps 574.40 ps]
change: [-0.4200% -0.0114% +0.3645%] (p = 0.96 > 0.05)
No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
1 (1.00%) high mild
9 (9.00%) high severe
Benchmarking Big less than methods/check_underflow/: Warming up for 3.00 Benchmarking Big less than methods/check_underflow/: Collecting 100 samp Big less than methods/check_underflow/
time: [286.14 ps 286.48 ps 286.90 ps]
change: [-0.3090% +0.2068% +0.7208%] (p = 0.46 > 0.05)
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) high mild
10 (10.00%) high severe
This is very helpful to know.
* added a criterion benchmark comparing previous `is_less_than` vs the underflow check method. Underflow is indeed 2x faster!
jonathanpwang
added a commit
to axiom-crypto/halo2
that referenced
this pull request
Dec 23, 2022
kilic
approved these changes
Dec 23, 2022
* add test (only for `Fr`) to check `from_raw_bytes` actually does modulus check correctly.
|
@CPerezz I turned off Hash for fq2 and curves for now. I also added a basic test that |
|
Thanks! Can you add a tag for the current commit so we can update it in halo2_proofs? |
Waiting on #11 to be merged first |
jonathanpwang
added a commit
to axiom-crypto/halo2
that referenced
this pull request
Feb 10, 2023
* VerifyingKey Serialization: merge Nalin's PR zcash#661 that allows for vkey serialization/deserialization and fixes the previous selector optimization issue * fix: add `num_fixed_commitments` to vkey serialization so correct number of fixed columns, post selector compression, are read * fix: serialize/deserialize pkey directly to/from file * bn256: make `Fq2`, `Fq6`, `Fq12` public * fq12: fix comments to clarify computation of FQ12 coefficients and use of Montgomery form * update: remove serialization from evaluation as we now recalculate directly * feat: add trait `Hash` to `ff::Field` * feat: `region.assign_advice` now returns reference `&Assigned<F>` via `AssignedCell` * big change: in `prover::WitnessCollection` now use `Arc` to manage smart pointer to assigned advice value. * otherwise cannot pass a reference out of `assign_advice` because the lifetime of immutable borrow ends up forcing mutable borrow of constraint system to live too long * chore: restructure workspace to match halo2-ce monorepo * Squashed 'primitives/poseidon/' content from commit 5d29df01 git-subtree-dir: primitives/poseidon git-subtree-split: 5d29df01a95e3df6334080d28e983407f56b5da3 * primitives/poseidon: initial add from PSE repo * feat: add conversion functions to `ff:PrimeField` to go between `u64` limbs or `BigInt` * feat: add `is_zero_vartime` derived implementation for `Fr` * change `assign_advice` API to take `Value<Assigned<F>>` instead of `Value<F>` * Fix `secp256k1` compressed serialization * update: remove `region` from `Cell` since we only use one region * update: remove `?` from `WitnessCollection::assign_advice` due to performance issues * See comments for (more) unsafe code version that gets another 3-4% performance boost. * feat: add back `copy_advice` function for `AssignedCell<&Assigned<F>, F>` * feat: implement functions `row_offset` and `column` directly for `AssignedCell` (previously for `Cell`) * Add serde test for curves * feat: add `CurveAffineExt::into_coordinates` for raw unchecked affine coordinates of curve point * update: modify `assign_fixed` slightly for performance * feat: implement `ProvingKey` serialization without using external crates `serde` or `bincode` * examples: add `serialization` example to test `ProvingKey` read and write on "simple-example" * curves: switch to using rust nightly "bigint_helper_methods" for finite field implementations * further optimize finite field implementations by following gnark * also improve bigint conversion functions to limbs * feat: implement `From<$field>` for `[u64; 4]` so field elements can be converted to their little endian representation with `u64` limbs * feat: add `From<[u64;4]>` and `To<[u64;4]>` to field implementations * feat: add default implementation of `CurveAffineExt` for `CurveAffine` * feat: added `to/from_bytes` for `ProvingKey` and `VerifyingKey` * add `pack`/`unpack` helper functions between `&[bool]` and `u8` * made serialization example use smaller example of standard plonk for less code bloat * fix: get multi-phase constraint system to work with our version of `assign_advice` * exposed inner `Phase(u8)` to crate * feat: change `region.assign_advice` to allow any `Into<Assigned<F>>` but still always return `&Assigned<F>` * feat: change `layouter.assign_region` to take `FnOnce` closure instead of `FnMut` now that there is no "get shape" mode * feat: derive `Serialize`, `Deserialize` for common fields and curves * chore: derive `PartialEq, Hash` for `bn256::{Fq,Fr}` and `secp256k1::{Fp,Fq}` * I see no longer to use `ct_eq` direct implementation of `PartialEq` * deriving `Hash` for all fields because it may be useful and doesn't hurt * feat: add trait `SerdeObject` for serialization of objects into raw bytes * implement `SerdeObject` for all macro-derived prime fields (raw bytes means staying in Montgomery form) * implement `SerdeObject` for `Fq2` directly * implement `SerdeObject` via macro for curves `bn254` and `secp256k1` * add tests `test_serialization` for fields and curves * chore: remote direct `PartialEq` implementation in bn254/assembly * feat: read `VerifyingKey` and `ProvingKey` does not require `params` as long as we serialize `params.k()` * fix: add impl `SerdeObject` to assembly macro * feat: derive `Hash` for `Fq2, G1Affine, G2Affine` for future use * fix: change `assert` to `debug_assert` in `read_raw_unchecked` functions * feat: add `next_phase` and `get_challenge` functions to `Region` so that a circuit can move onto the next phase during a single call of `synthesize` * currently only supports `create_proof` on 1 circuit at a time; otherwise it is not compatible with the original API which does synthesize for all circuits in a single phase before moving onto the next * update: change `is_less_than` to use `borrowing_sub` because it is faster see privacy-scaling-explorations/halo2curves#10 (comment) for context * optimize: revert to old way of subtracting field elements without branching * see jonathanpwang/halo2curves@ea9af0d * feat: parallelize (cpu) shplonk prover * shplonk: improve `construct_intermediate_sets` using `BTreeSet` and `BTreeMap` more aggressively * shplonk: change `construct_intermediate_sets` to use `FxHashSet` instead of `BTreeSet` * shplonk: add `Send` and `Sync` to `Query` trait for more parallelization * chore: in derive `field`, default `mul` is `const` again * - Implements `PartialOrd` for `Value<F>` - Adds a `transpose` method to turn `Value<Result<_>>` into `Result<Value<_>>` - `Expression::identifier()` remove string memory reallocation * chore: switch to halo2curves 0.3.1 tag * 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 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 * feat: add enum `SerdeFormat` for user to select serialization/deserialization format of curve and field elements * fix: revert use of raw pointer in `MockProver` and switch to using `Arc` * feat: add docs --------- Co-authored-by: kilic <kiliconu@itu.edu.tr> Co-authored-by: NoCtrlZ <phantomofrotten@gmail.com> Co-authored-by: Brechtpd <Brechtp.Devos@gmail.com> Co-authored-by: David Nevado <david@davidnevado.xyz> Co-authored-by: han0110 <tinghan0110@gmail.com> Co-authored-by: Nalin Bhardwaj <nalinbhardwaj@nibnalin.me> Co-authored-by: Jonathan Wang <jonathanpwang@users.noreply.github.com> 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>
jonathanpwang
added a commit
to axiom-crypto/halo2curves
that referenced
this pull request
Apr 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Montgomery reduction dramatically slows down serialization speed for large halo2 proving keys.
Currently curves are serialized in compressed affine form. Deserializing involves a square root which is very expensive. For production purposes it is generally preferred to trade off more storage space for speed savings.