-
Notifications
You must be signed in to change notification settings - Fork 598
chore: op wires index from 0 #11986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: op wires index from 0 #11986
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,16 +20,15 @@ template <typename FF_> class EccOpQueueRelationImpl { | |
|
|
||
| template <typename AllEntities> inline static bool skip([[maybe_unused]] const AllEntities& in) | ||
| { | ||
| // The prover can skip execution of this relation altogether since an honest input will lead to a zero | ||
| // contribution at every row, even when the selector lagrange_ecc_op is on | ||
| return true; | ||
| // The prover can skip execution of this relation if the ecc op selector is identically zero | ||
| return in.lagrange_ecc_op.is_zero(); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Expression for the generalized permutation sort gate. | ||
| * @details The relation is defined as C(in(X)...) = | ||
| * \alpha_{base} * | ||
| * ( \Sum_{i=0}^3 \alpha^i * (w_i - w_{op,i}) * \chi_{ecc_op} + | ||
| * ( \Sum_{i=0}^3 \alpha^i * (w_i_shift - w_{op,i}) * \chi_{ecc_op} + | ||
| * \Sum_{i=0}^3 \alpha^{i+4} w_{op,i} * \bar{\chi}_{ecc_op} ) | ||
| * | ||
| * where w_{op,i} are the ecc op gate wires, \chi_{ecc_op} is the indicator for the portion of the domain | ||
|
|
@@ -38,6 +37,8 @@ template <typename FF_> class EccOpQueueRelationImpl { | |
| * The first four sub-relations check that the values in the conventional wires are identical to the values in the | ||
| * ecc op wires over the portion of the execution trace representing ECC op queue gates. The next four check | ||
| * that the op wire polynomials are identically zero everywhere else. | ||
| * @note This relation utilizes the shifted wires so that the ecc op wires can store the data begining at index 0, | ||
| * unlike the wires which contain an initial zero row to facilitate the left-shift-by-1 needed by other relations. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't it right-shift-by-1?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it depends how your reference but the things we label
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so |
||
| * | ||
| * @param evals transformed to `evals + C(in(X)...)*scaling_factor` | ||
| * @param in an std::array containing the fully extended Univariate edges. | ||
|
|
@@ -57,10 +58,10 @@ template <typename FF_> class EccOpQueueRelationImpl { | |
| // 3). To do a degree-1 multiplication in the coefficient basis requires 3 Fp muls and 4 Fp adds (karatsuba | ||
| // multiplication). But a multiplication of a degree-3 Univariate only requires 3 Fp muls. | ||
| // We still cast to CoefficientAccumulator so that the degree is extended to degree-3 from degree-1 | ||
| auto w_1 = Accumulator(CoefficientAccumulator(in.w_l)); | ||
| auto w_2 = Accumulator(CoefficientAccumulator(in.w_r)); | ||
| auto w_3 = Accumulator(CoefficientAccumulator(in.w_o)); | ||
| auto w_4 = Accumulator(CoefficientAccumulator(in.w_4)); | ||
| auto w_1_shift = Accumulator(CoefficientAccumulator(in.w_l_shift)); | ||
| auto w_2_shift = Accumulator(CoefficientAccumulator(in.w_r_shift)); | ||
| auto w_3_shift = Accumulator(CoefficientAccumulator(in.w_o_shift)); | ||
| auto w_4_shift = Accumulator(CoefficientAccumulator(in.w_4_shift)); | ||
| auto op_wire_1 = Accumulator(CoefficientAccumulator(in.ecc_op_wire_1)); | ||
| auto op_wire_2 = Accumulator(CoefficientAccumulator(in.ecc_op_wire_2)); | ||
| auto op_wire_3 = Accumulator(CoefficientAccumulator(in.ecc_op_wire_3)); | ||
|
|
@@ -72,22 +73,22 @@ template <typename FF_> class EccOpQueueRelationImpl { | |
| auto complement_ecc_op_by_scaling = -lagrange_by_scaling + scaling_factor; | ||
|
|
||
| // Contribution (1) | ||
| auto tmp = op_wire_1 - w_1; | ||
| auto tmp = op_wire_1 - w_1_shift; | ||
| tmp *= lagrange_by_scaling; | ||
| std::get<0>(accumulators) += tmp; | ||
|
|
||
| // Contribution (2) | ||
| tmp = op_wire_2 - w_2; | ||
| tmp = op_wire_2 - w_2_shift; | ||
| tmp *= lagrange_by_scaling; | ||
| std::get<1>(accumulators) += tmp; | ||
|
|
||
| // Contribution (3) | ||
| tmp = op_wire_3 - w_3; | ||
| tmp = op_wire_3 - w_3_shift; | ||
| tmp *= lagrange_by_scaling; | ||
| std::get<2>(accumulators) += tmp; | ||
|
|
||
| // Contribution (4) | ||
| tmp = op_wire_4 - w_4; | ||
| tmp = op_wire_4 - w_4_shift; | ||
| tmp *= lagrange_by_scaling; | ||
| std::get<3>(accumulators) += tmp; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have thought at some point that this is bizzare but didn't get the chance to investigate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think this was just faulty/rushed thinking that got reinforced by an erroneously passing test