chore!: cycle group #6#17174
Conversation
| // this way we are able to fuse mutliple ecc add / ecc double operations and reduce total gate count. | ||
| // (ecc add/ecc double gates normally cost 2 Ultra gates. However if we chain add->add, add->double, | ||
| // double->add, double->double, they only cost one) | ||
| std::vector<cycle_group> points_to_add; |
There was a problem hiding this comment.
This is another case where we can simplify due to the fact that we now organize the circuit into gate-type blocks. This logic was previously separated with the only purpose of ensuring that the ECC addition gates were consecutive - but this now happens by automatically
| // Hence the following `create_ecc_dbl_gate` will throw an ASSERTION error | ||
| if (this->is_point_at_infinity().is_constant() && this->is_point_at_infinity().get_value()) { | ||
| // If this is a constant point at infinity, return early | ||
| if (this->is_constant_point_at_infinity()) { |
There was a problem hiding this comment.
This is the only logic change here, i.e. I've moved the handling of constant point at infinity prior to computation of modified_y. Aside from clarifying the logic, it saves one gate in that case that is_constant_point_at_infinity is true.
| // Compute the doubled point coordinates (either from hint or by native calculation) | ||
| bb::fr x3; | ||
| bb::fr y3; | ||
| if (hint.has_value()) { |
There was a problem hiding this comment.
The remaining changes here are simply to make the logic easier to follow. Rather than having nested conditionals and early returns we just handle two distinct phases separately: (1) get the native result of the doubling (possibly from hint), (2) construct the witnesses + constraint (unless constant).
| } | ||
| cycle_group result; | ||
|
|
||
| // Compute the result coordinates (either from hint or by native calculation) |
There was a problem hiding this comment.
Just reordering for clarity here, similar to what I did in dbl()
| std::array<MultiTableId, 2> table_id = table::get_lookup_table_ids_for_point(point); | ||
| plookup_table_ids.push_back(table_id[0]); | ||
| plookup_table_ids.push_back(table_id[1]); | ||
| plookup_base_points.push_back(point); |
There was a problem hiding this comment.
base points were unused here since everything just comes from the lookup tables
| builder.finalize_circuit(/*ensure_nonzero=*/false); | ||
| } | ||
| uint32_t actual_gates = static_cast<uint32_t>(builder.get_num_finalized_gates()); | ||
| // Ultra builder always creates 1 gate for the zero constant (this->zero_idx = put_constant_variable(FF::zero())) |
There was a problem hiding this comment.
This is what accounts for the -1 gate update to the tests below. The reason I'm doing this is to avoid annoying conditional logic in the tests to handle U;ltra vs Mega since Mega adds 4 total constant by default (instead of 1 for Ultra).
barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp
Show resolved
Hide resolved
| return _is_infinity.is_constant() && _is_infinity.get_value(); | ||
| } | ||
| #ifdef FUZZING | ||
| void set_point_at_infinity(const bool_t& is_infinity); |
There was a problem hiding this comment.
I've marked this overly complicated method as fuzzing only since its not used elsewhere. It seems odd to have the fuzzer use methods that aren't used/accessible from production code but this can be sorted out with pending fuzzer updates
barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| // If the subtraction amounts to a doubling then the result is the doubling result, else the subtraction result. | ||
| const bool_t double_predicate = (x_coordinates_match && !y_coordinates_match); |
There was a problem hiding this comment.
I wonder if we should have a stricter check for double_predicate here, something like:
const bool_t y_coordinates_cancel = (y + other.y).is_zero();
const bool_t double_predicate = (x_coordinates_match && y_coordinates_cancel);There was a problem hiding this comment.
isn't this equivalent to what we have now? If the x-coords agree and the y coords don't then it must be that
There was a problem hiding this comment.
I agree with your implication when the inputs are always points on the curve. But if there are no prior checks on points being on curve, we could have a case when a malicious prover inputs
There was a problem hiding this comment.
Its a very good point. I'm going to mark this with an AUDITTODO then take a hard look at our on_curve checks in this class. My preference would be to make it impossible to construct a cycle_group element without the appropriate validate_on_curve check
barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp
Show resolved
Hide resolved
| } else { | ||
| lambda = | ||
| field_t::from_witness(this->get_context(other), (-y2.get_value() - y1.get_value()) / x_diff.get_value()); | ||
| // Note: branch saves one gate vs just using divide_no_zero_check because we avoid computing y2 - y1 in circuit |
There was a problem hiding this comment.
super nit: edit the comment to:
// Note: branch saves one gate vs just using divide_no_zero_check because we avoid computing (-y2 - y1) in circuitThere was a problem hiding this comment.
thanks. I reaaallly wanted to just get rid of this optimization since this could all be replace with the one liner const field_t lambda = (-y2 - y1).divide_no_zero_check(x_diff); but I resisted the urge for some reason
suyash67
left a comment
suyash67
left a comment
There was a problem hiding this comment.
Amazing work with the cleanup! Code looks much more cleaner and easy to understand.
BEGIN_COMMIT_OVERRIDE fix: secp256k1 ecdsa mul - fix handling of point at infinity (#16679) fix: secp256k1 ecdsa mul handling of stagger point additions (#16685) chore: biggroup audit of lookup and rom tables (#16895) fix(bb): oversight that disabled batch commits (#17278) docs(bb): add initial cli reference (#17244) chore!: cycle group #6 (#17174) fix: mac bb publish (#17276) END_COMMIT_OVERRIDE
Comments and minor updates to the methods of cycle_group. Changes include: - New standalone tests for `dbl()`, `unconditional_add()`/`subtract()`, `operator+`/`-` (with gate count pinning to prevent unintended circuit changes) - New helper methods `constant_infinity()` and `is_constant_point_at_infinity()` for improved clarity - Remove member `_is_constant` in favor of method `is_constant()` which simply checks that `x.is_constant() && y.is_constant() && _is_infinity.is_constant()` - Reorganize `dbl()` and `_unconditional_add_or_subtract()` to remove hard to follow nested conditional logic - Simplify logic in `_variable_base_batch_mul_internal()` that was splitting up lookup gates and ECC add gates to ensure consecutive adds - this is no longer necessary due to our circuit block structure. (Note: this is what causes the VK change. No change in number of gates, just ordering)
Comments and minor updates to the methods of cycle_group. Changes include: - New standalone tests for `dbl()`, `unconditional_add()`/`subtract()`, `operator+`/`-` (with gate count pinning to prevent unintended circuit changes) - New helper methods `constant_infinity()` and `is_constant_point_at_infinity()` for improved clarity - Remove member `_is_constant` in favor of method `is_constant()` which simply checks that `x.is_constant() && y.is_constant() && _is_infinity.is_constant()` - Reorganize `dbl()` and `_unconditional_add_or_subtract()` to remove hard to follow nested conditional logic - Simplify logic in `_variable_base_batch_mul_internal()` that was splitting up lookup gates and ECC add gates to ensure consecutive adds - this is no longer necessary due to our circuit block structure. (Note: this is what causes the VK change. No change in number of gates, just ordering)
2 participants
Comments and minor updates to the methods of cycle_group. Changes include:
dbl(),unconditional_add()/subtract(),operator+/-(with gate count pinning to prevent unintended circuit changes)constant_infinity()andis_constant_point_at_infinity()for improved clarity_is_constantin favor of methodis_constant()which simply checks thatx.is_constant() && y.is_constant() && _is_infinity.is_constant()dbl()and_unconditional_add_or_subtract()to remove hard to follow nested conditional logic_variable_base_batch_mul_internal()that was splitting up lookup gates and ECC add gates to ensure consecutive adds - this is no longer necessary due to our circuit block structure. (Note: this is what causes the VK change. No change in number of gates, just ordering)