Repository navigation
Make BitZ generic over semirings, rings and fields - #140
frozenspider wants to merge 18 commits into
Conversation
# Conflicts: # crates/tests/examples/dump_bitz.rs # crates/tests/tests/host.rs # crates/tests/tests/prove.rs # crates/verifier/src/verify.rs # tooling/cli/src/end_to_end.rs # tooling/cli/tests/circuits.rs # tooling/cli/tests/end_to_end.rs
| type S = num_bigint::BigUint; | ||
| type R = num_bigint::BigInt; |
There was a problem hiding this comment.
I don't mind single letter types, but here I'm missing the mnemonic for S and R.
There was a problem hiding this comment.
It stands for (S)emiring and (R)ing
| params: &BitZParams<Q>, | ||
| row_weights: Vec<Fq<Q>>, | ||
| column_weights: Vec<Fq<Q>>, | ||
| target: Fq<Q>, |
There was a problem hiding this comment.
Both Q and Fq<Q> got replaced by F. Since Q in BitZParams is a phantom data it might not matter that much. But do investigate, these things tend to break later on.
There was a problem hiding this comment.
This is by design, as the whole BitZParams was reworked to be generic over any field, not just Fq over some Q.
| } | ||
|
|
||
| impl<const Q: u128> BitZParams<Q> { | ||
| impl<F: BitzClaimField> BitZParams<F> { |
There was a problem hiding this comment.
Continuating of previous point. u128 -> BitzClaimField, while somewhere else Fq<Q> -> BitzClaimField.
| pub fn fold_bound(&self) -> F::Integer { | ||
| let rows = u64::try_from(self.shape.rows()).expect("Too many rows"); | ||
| let rows = F::Integer::from(rows); | ||
| let max_f = F::max_value().lift(); | ||
| rows.checked_mul(&max_f).expect("Multiplication overflow") |
There was a problem hiding this comment.
This many expects in arithmetic code feels off. Later on the similar calculation is done without expects.
There was a problem hiding this comment.
These are just some guardrails - they should probably become Result<_, _> at some point, to avoid DoS in real systems.
How do you want to treat them now?
| pub struct VirtualStatement<'a, F, M: VirtualMap> { | ||
| params: VirtualParams<F>, | ||
| map: &'a M, | ||
| claim: &'a LinearClaim<Fq<Q>>, | ||
| claim: &'a LinearClaim<F>, |
There was a problem hiding this comment.
Same as before, Q -> F and Fq<Q> -> F
| Box::new((0..n).map(|_| F128::from(rand::random::<u128>())).collect()); | ||
| let packed: &'static _ = packed.leak(); | ||
| let params: BitZParams<Q114> = | ||
| let params: BitZParams<Fq<Q114>> = |
There was a problem hiding this comment.
Continuation of earlier point about BitZParams. Now Fq shows up in BitZParams while it wasn't there before.
There was a problem hiding this comment.
Yep, as BitZParams is now generic over a field, Fq<Q114> is the concrete field type used here.
|
|
||
| for fold in &folds { | ||
| transcript.prover_message(&fold.to_le_bytes()); | ||
| transcript.prover_message(&fold.to_le_bytes().as_ref()); |
There was a problem hiding this comment.
Would Deref be better than AsRef in this case?
There was a problem hiding this comment.
I don't quite follow, how do you want to deref it if we specifically need a reference?
To give some context, I was following the existing convention (partially enforced by spongefish) - the type bound T: Encoding<[u8]> + NargSerialize + ?Sized and argument type &T, so we need to provide a reference so something that is NargSerialize. Given that [u8] is not NargSerialize, we pass in &&[u8].
| let modulus = F::modulus(); | ||
| let fold = (shape.rows() as u128) * (modulus - 1); | ||
| let params = BitZParams::<F>::new(shape, smallest_generator()).unwrap(); |
There was a problem hiding this comment.
Ties back to earlier point of too much exception handling. This piece of code performs the same calculation but doesn't need expect.
There was a problem hiding this comment.
This is test code, so we don't care about expects, etc.
A follow-up to #135, another step toward #136 (hopefully second-to-last).
Summary of changes:
BitzClaimField.FqtoF: BitzClaimField.u128andBigUint->BitzSemiringin a few places.ConstFieldrequirement toField.BitzRingtoBitzConstraintRingto avoid confusion withBitzClaimField::Integer.BitzConstraintRingandBitzClaimFieldin the form ofProjectConstraint.BigUint,BigIntandFq, replacing them with type aliases (S,R,F).toolingcrate in order to get access toProjectBigIntToFq.A follow-up PR will replace
Fqwith dynamic field.