refactor: eccvm transcript builder #9026
Merged
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
iakovenkos
commented
Oct 4, 2024
| VMState state{ | ||
| .pc = total_number_of_muls, | ||
| .count = 0, | ||
| .accumulator = CycleGroup::affine_point_at_infinity, | ||
| .msm_accumulator = offset_generator(), | ||
| .is_accumulator_empty = true, | ||
| }; | ||
|
|
||
| VMState updated_state; |
There was a problem hiding this comment.
tried to remove it, but seems tricky
iakovenkos
commented
Oct 4, 2024
| accumulator_trace[i] = state.accumulator; | ||
| msm_accumulator_trace[i] = msm_transition ? updated_state.msm_accumulator : Element::infinity(); | ||
| intermediate_accumulator_trace[i] = | ||
| msm_transition ? (updated_state.msm_accumulator - offset_generator()) : Element::infinity(); | ||
| if (entry.mul && next_not_msm && !row.accumulator_empty) { |
There was a problem hiding this comment.
state is re-written by the updates state, so this check is unnecessary
iakovenkos
commented
Oct 4, 2024
| auto rhsy = accumulator_trace[i].is_point_at_infinity() ? (0) : accumulator_trace[i].y; | ||
| inverse_trace_x[i] = lhsx - rhsx; | ||
| inverse_trace_y[i] = lhsy - rhsy; | ||
| } else if (add) { |
There was a problem hiding this comment.
here add = row.q_add, but it coincides with entry.add, so the logic could be simplified a little
iakovenkos
commented
Oct 4, 2024
| for (size_t i = 0; i < accumulator_trace.size(); ++i) { | ||
| TranscriptRow& row = transcript_state[i + 1]; |
There was a problem hiding this comment.
this is not necessary, but makes the operations with the current transcipt row way clearer
iakovenkos
commented
Oct 4, 2024
| @@ -10,8 +10,13 @@ class ECCVMTranscriptBuilder { | |||
| using FF = grumpkin::fr; | |||
| using Element = typename CycleGroup::element; | |||
| using AffineElement = typename CycleGroup::affine_element; | |||
| using VMOperation = typename bb::eccvm::VMOperation<CycleGroup>; | |||
| using Accumulator = typename std::vector<Element>; | |||
|
|
|||
| struct TranscriptRow { | |||
There was a problem hiding this comment.
seemed logical to group the fields in the order of being populated
iakovenkos
commented
Oct 4, 2024
| @@ -212,7 +212,7 @@ void ECCVMTranscriptRelationImpl<FF>::accumulate(ContainerOverSubrelations& accu | |||
| * If q_mul = 1 OR q_add = 1 OR q_eq = 1, require (transcript_Px, transcript_Py) is valid ecc point | |||
| * q_mul/q_add/q_eq mutually exclusive, can represent as sum of 3 | |||
| */ | |||
| const auto validate_on_curve = q_mul + q_add + q_mul + q_eq; | |||
| const auto validate_on_curve = q_add + q_mul + q_eq; | |||
There was a problem hiding this comment.
must have been a typo
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Rumata888
reviewed
Oct 7, 2024
Rumata888
approved these changes
Oct 7, 2024
| } | ||
|
|
||
| private: | ||
| static void populate_transcript_row(TranscriptRow& row, |
There was a problem hiding this comment.
All of these functions need descriptions
| } | ||
| } | ||
| static void batch_invert(std::vector<FF>& inverse_trace_x, |
There was a problem hiding this comment.
I think these calls can stay in the original function
| TranscriptRow& final_row = transcript_state.back(); | ||
| final_row.pc = updated_state.pc; | ||
| final_row.accumulator_x = | ||
| (updated_state.accumulator.is_point_at_infinity()) ? 0 : AffineElement(updated_state.accumulator).x; | ||
| updated_state.accumulator.is_point_at_infinity() ? 0 : AffineElement(updated_state.accumulator).x; |
There was a problem hiding this comment.
Should probably add an explanation for (0,0)
There was a problem hiding this comment.
Currently, I don't have a good explanation, because that's exactly where the point at infinity breaks Goblin
| add_lambda_numerator[i], | ||
| add_lambda_denominator[i]); | ||
| } else { | ||
| row.transcript_add_x_equal = 0; |
There was a problem hiding this comment.
the slopes are only used when there's an msm transition or an addition
iakovenkos
commented
Oct 7, 2024
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.
improved the structure of the main method compute_rows
cleaned up logic without modifying it
improved code sharing
added docs (not very detailed but should be helpful)
added the point at infinity test to ECCVMCircuitBuilderTests. this test passing means that we are not handling point at infinity correctly