Closed danielmasny closed 5 months ago
Attention: Patch coverage is 98.83721%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 91.40%. Comparing base (
7bb1325
) to head (f7e309a
). Report is 9 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
...ol/ipa_prf/validation_protocol/proof_generation.rs | 99.06% | 3 Missing :warning: |
.../src/protocol/ipa_prf/malicious_security/prover.rs | 95.45% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Comment from Andy:
It would be helpful to have a summary of what is changing and why in the PR description.
If I understand right,
λ
is renamed toR
, and theR
type parameter is used directly in places where the parameter N=2λ+1 was used before. How does this simplify the code?The
ProofGenerator
type seems to be replaced withUVStore
. Part of what I understand (based on in-person discussions, as opposed to studying the PR) to be going on here is that the first pass of the proof will work with an iterator over u and v values rather than storing them all in an array, since that is (1) possible for the first pass, and (2) a fully expanded copy of u and v for the first pass would be very large -- a significant multiple larger than the raw multiplication intermediates, at least until we implement packing. Writing the proof generation routines in terms of iterators enables them to work either from an array or iterator of u/v values. Since the nameUVStore
implies a data storage type, I think it would be better not to implement complex prover functionality in methods on theUVStore
type. Similarly, if we're getting rid of theZeroKnowledgeProof
type in favor of usingGenericArray
directly, then we can't implement complex prover functionality as methods onGenericArray
. Possibly the prover functionality could be implemented as free functions in theprover
module. I see how the methods are serving as constructors forUVStore
andZeroKnowledgeProof
, but I'm not sure there's a compelling reason for them to be that way. I feel like the logic that structures and terminates the recursion should also be implemented here, rather than left to the caller, but maybe there's a good reason an abstraction around that doesn't work.It would be useful to have some additional comments on these routines explaining what they do and how they are to be used. Some amount of the detail is in the paper and when it is, references to the paper are fine, but I don't think it's possible to match the notation in the paper precisely enough that no additional explanation is necessary. It would also be nice if the implementation had enough self-contained documentation that it is possible to use the proof system without referring to the paper. (This concern is not new with the changes in this PR -- I feel the same about the original.)
Assuming we can align on the changes, is there a reason not to go ahead and update the tests to reflect the revisions (instead of leaving some routines as deprecated but used by tests)?
PR that implements the protocol for generating and distributing the dzkp proofs.
It does not include verification and only includes some simple tests. I will add extensive tests once the verification is also implemented. All previous tests that check the mathematical components still pass.
I changed some of the structs in prover.rs to work more smoothly with iterators. Unfortunately, the PR became larger than I initially anticipated.