Refactor Field and RandomField:#142
Conversation
* Use ConstMontyParams and ConstMontyForm from crypto-bigint crate
* Bump crypto-bigint version
* Use crypto_bigint::Word for all platform-specific word sizes
* Add F{N} aliases for Field akin to U64, U128, ... for portability
* Remove BigInt and use crypto_bigint::Uint instead
* Remove all Config and ConfigReference and use ConstMontyParams instead
* Remove traits::{Uint, Int} and use crypto_bigint::{Uint, Int}
* Use From::from instead of FieldMap::map_to_field
Introduce traits for arithmetic operations:
* Value for Copy, Debug, ...
* Abelian for addition
* Monoid for multiplication
Refactor Polynomials:
* Unify MLE for Field and Integer into single implementation
Refactor CCS:
* Unify CCS implementation for Field and Integer into single implementation
Refactor Zip, Spartan, Sumcheck to use new Field and RandomField
|
Pending comments in the PR for clarity |
|
There are 3 tests in Spartan which are failing and I didn't fix them yet |
cupicmarko
left a comment
cupicmarko
left a comment
There was a problem hiding this comment.
There are some changes that introduce a lot of diffs, but are simple:
-
ZT: ZipTypesis replaced with direct use ofconst N: usize, const const L: usize, const K: usize, const M: usize, using a trait or struct to hold these consts would require switching to nightly -
RandomField<'cfg, const N: usize> is replaced by RandomField<ModP, LIMBS>
-
I: Integer is often replaced by const I: usize, I was not consistent with this, but it could be refactored in future
-
Witness and Witness_Z and similar CCS structs and traits are unified
-
MLEs are unified because without config they work for any type
There was a problem hiding this comment.
This entire module is removed, we are using ConstMontyForm instead of BigInt in RandomField
There was a problem hiding this comment.
This is completely replaced by crypto_bigint::ConstMontyForm
There was a problem hiding this comment.
Most of this moved to numeric.rs
There was a problem hiding this comment.
Since there is no Config anymore, we can use plain From trait
There was a problem hiding this comment.
Using crypto_bigint::Int instead
There was a problem hiding this comment.
Using crypto_bigint::Uint instead
There was a problem hiding this comment.
This is a "trick" used for P3 Merkle trees to avoid Orphan rule for crypto_bigint::Int and required trait Packable. As PackedInt is a complete new_type of Int, it has the same memory layout - size and alignment
There was a problem hiding this comment.
Unified into poly
There was a problem hiding this comment.
Unified into poly
There was a problem hiding this comment.
Not needed anymore
frozenspider
left a comment
frozenspider
left a comment
There was a problem hiding this comment.
Great work overall! Left some comments
| #![allow(clippy::eq_op)] | ||
|
|
||
| use ark_std::{ | ||
| use std::{ |
There was a problem hiding this comment.
Are we giving up on ark_std, or was this a changed by accident?
| + From<Uint<N>> | ||
| + From<Int<N>> | ||
| + for<'a> From<&'a Self> | ||
| + for<'a> AddAssign<&'a Self> |
There was a problem hiding this comment.
This already comes from Abelian
| pub fn absorb_random_field<F: Field<LIMBS>, const LIMBS: usize>(&mut self, v: &F) { | ||
| // self.absorb(&[0x3]); | ||
| // self.absorb(&ToBytes::to_be_bytes(MOD::PARAMS.modulus().as_ref())); | ||
| // self.absorb(&[0x5]); |
There was a problem hiding this comment.
Either dead code, or forgot to uncomment. I think it's the latter - we do need to save the modulus, otherwise I guess cheating prover can claim it to be different than the one he actually used
|
|
||
| *self = Initialized { config, value } | ||
| } | ||
| pub trait Monty<const LIMBS: usize>: ConstMontyParams<LIMBS> {} |
There was a problem hiding this comment.
Why do we need this wrapper trait? We don't seem to implement anything else on top of it?
| Initialized { value, .. } => value, | ||
| } | ||
| } | ||
| pub(crate) type Form<MOD, const LIMBS: usize> = ConstMontyForm<MOD, LIMBS>; |
There was a problem hiding this comment.
Nit: The name is a bit misleading. Maybe just use a direct import?
| impl<MOD: Monty<LIMBS>, const LIMBS: usize> From<isize> for RandomField<MOD, LIMBS> { | ||
| fn from(value: isize) -> Self { | ||
| // Use i64 for sign handling as specified | ||
| let magnitude = Uint::from((value as i64).abs_diff(0)); |
| Self(Form::new(&value)) | ||
| } | ||
| } | ||
| impl<MOD: Monty<LIMBS>, const LIMBS: usize /*, const N: usize*/> From<Int<LIMBS>> |
There was a problem hiding this comment.
(Here and below) commented dead code
| for _ in 0..num_products { | ||
| let num_multiplicands = rng.gen_range(num_multiplicands_range.0..num_multiplicands_range.1); | ||
| let num_multiplicands = | ||
| rand_range_usize(rng, num_multiplicands_range.0, num_multiplicands_range.1); |
There was a problem hiding this comment.
We're using rand crate anyway, so I think we should use rng.random_range(num_multiplicands_range.0..num_multiplicands_range.1); and get rid of rand_range_usize
| // // Method for efficient random prime sampling not yet implemented | ||
| // // Fixing the random prime q for now | ||
| // F::C::new(get_prime::<F>(transcript)) | ||
| // } |
| ccs: &CCS_F<F>, | ||
| ccs: &CcsF<F>, | ||
| ) -> Result<(), SpartanError<F>> { | ||
| let e = eq_eval(point_r, beta_s)?; |
There was a problem hiding this comment.
Why does this diverge from eq_eval now?
Instead of `N, L, K, M` usize constant bounds for `Int`s, both Zip and linear code now operates on 4 abstract rings: * `N => EvaluationRing` * `L` (previously: element width of the encoding matrix of a linear code) is no longer a generic, instead it's made into a property of linear code. `PrimeField` elements must be obtainable from it, but otherwise code is free to place any constraints on the rings that it needs to make it work. * `K => CodewordRing` * Added new ring `ChallengeRing`, used to generate challenges for random linear combination of codewords. Previously it was the same as evaluation ring. * `M => LinearCombinationRing` These ring aliases are implemented by `Int<_>` rings, so not much changes from a user perspective, except you have to specify `ChallengeRing` separately. ## Other major changes * Added a new trait `MulByScalar` to represent an ability to multiply a given ring element by `ChallengeRing` (and field elements with each other, as they are being combined too) * `LinearCode` no longer has generic `encode_wide`, instead 3 encode methods are now defined as follows: * `encode: EvaluationRing => CodewordRing` * `encode_wide: LinearCombinationRing => LinearCombinationRing` * `encode_f: PrimeField => PrimeField` * Tweaked `RaaCode` to have a configurable `check_for_overflows`. This is needed to keep encoding times on par with original Zinc encoding times - using `checked_add` adds ~20% overhead. * All to- and from-bytes encoding is now done through a new trait `Transcribable`. It can use whatever byte order as long as it allows serialization and deserialization cross-platform. (Actually uses little-endian order as being more efficient and more natural when applied to underlying `crypto-bigint` types) * This allows to make transcription reads/writes much more generalized, removing a bunch of type-specific methods * This also made `AsWords` trait unnecessary, removed it * Cross-platform deserialization wasn't actually tested yet, left a TODO ## Additional changes * `ConstRing` is now auto-implemented * `dense` multilinear polynomial module moved from `poly` to `poly::mle` to free up module name * `num_proximity_testing` no longer depends on `log2_q`. It was unused previously anyway, and if we need it in future, we'd need to determine what `log2_q` is for an arbitrary ring. ## Performance (single-threaded) * Encoding and MT construction is on par with original Zinc. * Commitment overall is ~35% faster. * Open and verify is on par with NethermindEth/zinc#142 (which is much faster than original Zinc)
2 participants
Introduce traits for arithmetic operations:
Refactor Polynomials:
Refactor CCS:
Refactor Zip, Spartan, Sumcheck to use new Field and RandomField