Small refactoring of challenge-based protocols #1567
+27
−44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
georgwiese
commented
Jul 12, 2024
| @@ -67,11 +64,6 @@ let lookup: expr, expr[], Fp2<expr>, Fp2<expr>, Constr, expr -> Constr[] = |is_f | |||
|
|
|||
| let (lhs_selector, lhs, rhs_selector, rhs) = unpack_lookup_constraint(lookup_constraint); | |||
|
|
|||
| let _ = assert(len(lhs) == len(rhs), || "LHS and RHS should have equal length"); | |||
There was a problem hiding this comment.
This is guaranteed by the type of Constr::Lookup
There was a problem hiding this comment.
But you can work around that by just using the enum manually without the in operator.
There was a problem hiding this comment.
The enum now stores an array of pairs so @georgwiese is right?
georgwiese
commented
Jul 12, 2024
Comment on lines
-71
to
-73
| let _ = if !is_extension(acc) { | ||
| assert(!needs_extension(), || "The Goldilocks field is too small and needs to move to the extension field. Pass two accumulators instead!") | ||
| } else { }; |
There was a problem hiding this comment.
Done inside fp2_from_array now.
georgwiese
commented
Jul 12, 2024
Comment on lines
-84
to
-89
| let next_acc = if is_extension(acc) { | ||
| next_ext(acc_ext) | ||
| } else { | ||
| // The second component is 0, but the next operator is not defined on it... | ||
| from_base(acc[0]') | ||
| }; |
There was a problem hiding this comment.
Seems like this has been fixed in the meantime :)
chriseth
reviewed
Jul 14, 2024
| @@ -111,7 +116,16 @@ let is_extension = |arr| match len(arr) { | |||
| }; | |||
|
|
|||
| /// Constructs an extension field element `a0 + a1 * X` from either `[a0, a1]` or `[a0]` (setting `a1`to zero in that case) | |||
| let fp2_from_array = |arr| if is_extension(arr) { Fp2::Fp2(arr[0], arr[1]) } else { from_base(arr[0]) }; | |||
| let fp2_from_array = |arr| { | |||
| let _ = if !is_extension(arr) { | |||
There was a problem hiding this comment.
I think you don't need let _ = any more here.
chriseth
reviewed
Jul 14, 2024
Comment on lines
+122
to
+123
| } else { }; | ||
| if is_extension(arr) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some small refactorings of
lookup.asmandpermutation.asm.