Refactor of morphism construction for more concise definition of statements #12
Conversation
src/fiat_shamir.rs
Outdated
| pub sigmap: P, | ||
| } | ||
|
|
||
| // QUESTION: Is the morphism supposed to be written to the transcript? I don't see that here. |
There was a problem hiding this comment.
I'm unsure why we would need to do it, we're only putting the domain_sep in the transcript (in the permanent transcript), and the commitments during a prove/verification (in the temporary clone). Do you see a case where it would be useful to do that?
There was a problem hiding this comment.
I do, ya. It decreases the risk of soundness issues when the relation may change (e.g. with versions of the software) or depend on context that is outside this system.
There was a problem hiding this comment.
To implement this: given our design for prove/verify step, where the transcript is cloned + fed commitments + destroyed, how do we put the morphism inside the transcript?
1: It is also cloned + fed commitments + destroyed => the cloned transcript for Morphism + the cloned transcript for prove/verify need to exist at the same time to work
2: It is permanently added to the transcript => no change to current workflow, but it means that successive proofs depend on former proofs for a later verifier (eg. need to add n morphisms to transcript to verify an independent n+1 morphism)
3: Other suggestion?
src/fiat_shamir.rs
Outdated
| witness: &P::Witness, | ||
| rng: &mut (impl RngCore + CryptoRng), | ||
| ) -> Result<Transcript<P>, ProofError> { | ||
| // QUESTION: Why is the self mutable? It's unclear whether the intention is to have a |
There was a problem hiding this comment.
It probably shouldn’t be, we followed @mmaker design to clone it at the beginning of every proof/verify to feed the Codec and throw away the clone afterwards
src/group_morphism.rs
Outdated
| pub linear_combination: Vec<LinearCombination>, | ||
| pub group_elements: Vec<G>, | ||
| pub num_scalars: usize, | ||
| /// QUESTION: Why is there a count of elements seperate from group_elements.len() |
There was a problem hiding this comment.
No good reason, we should just derive the length from group_elements directly with len()
src/group_morphism.rs
Outdated
| // QUESTION: This accesses the group_elements, but it does not attempt to tell | ||
| // whether they have been set. If I allocate a point var, and use it in the | ||
| // equations here, will the variables always be set. I think not, as a result, the | ||
| // solution here may not be valid. |
There was a problem hiding this comment.
We should check whether the fieds are non-empty before trying to evaluate them
There was a problem hiding this comment.
I've updated this to do so 👍
src/group_morphism.rs
Outdated
| pub fn allocate_elements(&mut self, n: usize) -> Vec<PointVar> { | ||
| let start = self.morphism.num_elements; | ||
| // NOTE: This uses identity as the default value for an allocated, but unassigned, | ||
| // variable. |
There was a problem hiding this comment.
Yes, I don’t know if there is a proper way of doing this other than this? Leave it empty maybe?
There was a problem hiding this comment.
Take a look at the GroupMap that exists now
nategraf
changed the title
The main goal of this PR is to refactor the morphism building API to make it more concise to build and prove relations. I also make some smaller changes to fix minor points in the rest of the repo, and add adjust the build configurations by adding a
rust-toolchain.tomlfile and turning onopt-level=1for tests.