Conversation
| pub(crate) type MainAffine<E> = <E as CurveCycle>::E1; | ||
| pub(crate) type HelpAffine<E> = <E as CurveCycle>::E2; | ||
|
|
||
| pub(crate) type MainField<E> = <<E as CurveCycle>::E2 as AffineCurve>::BaseField; |
There was a problem hiding this comment.
| pub(crate) type MainField<E> = <<E as CurveCycle>::E2 as AffineCurve>::BaseField; | |
| pub(crate) type MainField<E> = <<E as CurveCycle>::E1 as AffineCurve>::ScalarField; |
There was a problem hiding this comment.
For some reason, applying this change causes the library to not compile. An example of an error is: the trait ark_ff::PrimeField is not implemented for <<E as CurveCycle>::E2 as AffineCurve>::BaseField. It isn't clear to me why it isn't detecting it as a PrimeField. Maybe it is just substituting the BaseField in without doing anything else due to https://github.com/arkworks-rs/algebra/blob/dae430780082b8ebd3e092f08759c8e274d47e56/ec/src/lib.rs#L336-L338?
| let cs = ConstraintSystem::<HelpField<E>>::new_ref(); | ||
| let _hash = FpVar::new_input(cs.clone(), || Ok(HelpField::<E>::zero())).unwrap(); | ||
| return cs.num_instance_variables(); |
There was a problem hiding this comment.
If we know we're only ever allocating one variable, we can hard code the result of cs.num_instance_variables(). If that can change, we should instead invoke the appropriate code (eg: SpongeOutout::new_input(cs.clone(), || Ok(dummy_output))).
There was a problem hiding this comment.
I agree that we should invoke the appropriate code rather than directly using the HelpField.
However, wouldn't it be a poor choice to break the abstraction of the ConstraintSystem by hard coding the expected number of instance variables? If I am remembering correctly, one implementation detail of ConstraintSystem is that it adds a one when initializing the instance variables. However, such detail isn't very relevant to this library (we only care that we are using the outputs of CS) and if that implementation detail ever changes (would it?), then it may introduce some difficult to find bugs.
Considering this point, would we still want to hard code the number of instance variables?
| let main_circuit_input_len = MainCircuit::<E, PC, P>::public_input_size(); | ||
| let (main_input_instances, main_old_accumulator_instances) = if base_case { | ||
| ( | ||
| vec![InputInstance::zero(main_circuit_input_len, MAKE_ZK); P::PRIOR_MSG_LEN], |
There was a problem hiding this comment.
Maybe we should make this also placeholder, instead of zero?
There was a problem hiding this comment.
I have opened a PR in accumulation to apply this change (arkworks-rs/accumulation#34).
| * Allocation | ||
| */ | ||
|
|
||
| let claimed_input_hash_var = FpVar::new_input(ns!(cs, "alloc_claimed_input_hash"), || { |
There was a problem hiding this comment.
We can avoid the _var suffix; we aren't using claimed_input_hash anywhere afterwards, right? Similarly for the other cases.
There was a problem hiding this comment.
claimed_input_hash_var and other variables have their native counterparts, so I think having _var is necessary.
pcd/src/r1cs_nark_pcd/help_circuit.rs
Line 178 in 30ae1ef
pcd/src/r1cs_nark_pcd/help_circuit.rs
Lines 133 to 141 in 30ae1ef
| Ok(claimed_input_hash) | ||
| })?; | ||
|
|
||
| let base_case_var = Boolean::new_witness(ns!(cs, "alloc_base_case_bit"), || Ok(base_case))?; |
There was a problem hiding this comment.
This should be allocated as a input, otherwise the prover can always set it to true, forcing the accumulation part to be ignored.
EDIT: unless this is checked via the hash, which it isn't right now.
There was a problem hiding this comment.
If we allocate base_case_var as an input, the prover in the next recursive step would need to know whether the prior step was a base case for the accumulation right? Currently, I don't think the PCD library gives the prover this information. Would we need to modify the API to include such detail or is there any other way of passing such information around?
| type MainCurveVar: CurveVar<MainProjective<E>, HelpField<E>> + AbsorbableGadget<HelpField<E>>; | ||
|
|
||
| /// The curve var for the help affine. | ||
| type HelpCurveVar: CurveVar<HelpProjective<E>, MainField<E>> + AbsorbableGadget<MainField<E>>; |
There was a problem hiding this comment.
Maybe you can extract this into a CurveCycleGadget trait, similar to CurveCycle?
There was a problem hiding this comment.
Would putting this relation into a gadget/var be appropriate? The constraint fields are different. Gadgets and vars usually specify a single constraint field right?
|
Hi all, what is the status of this PR? I'm trying to play with some IVC stuff and I'd like to see the feasibility of this scheme for our idea. Currently this branch doesn't compile. The errors are coming from the |
Hello! I am not currently developing for this repository. This branch depended on an older version of |
3 participants
Dummy PR