Skip to content

Commit

Permalink
chore!: change ec_add to unsafe implementation (but much better perf) (
Browse files Browse the repository at this point in the history
…#8374)

Use `unconditional_add` for ec_add ACIR opcode.
This improves a lot the performance of the opcode, but also makes it
unsafe.
~~I keep the PR as draft until aztec packages are sync with Noir PR
noir-lang/noir#5858 which adds the checks in the
stdlib function.~~
The unsafe version can then be used when we know the inputs are valid
(for instance if they come from a previous add).
n.b.: the real performance boost will happen when we will be able to use
the unsafe version.

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
guipublic and TomAFrench authored Sep 24, 2024
1 parent d5fd155 commit aabd2d8
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@

namespace acir_format {

// This functions assumes that:
// 1. none of the points are infinity
// 2. the points are on the curve
// 3a. the points have not the same abssissa, OR
// 3b. the points are identical (same witness index or same value)
template <typename Builder>
void create_ec_add_constraint(Builder& builder, const EcAdd& input, bool has_valid_witness_assignments)
{
Expand All @@ -19,7 +24,32 @@ void create_ec_add_constraint(Builder& builder, const EcAdd& input, bool has_val
input.input2_x, input.input2_y, input.input2_infinite, has_valid_witness_assignments, builder);

// Addition
cycle_group_ct result = input1_point + input2_point;
// Check if operands are the same
bool x_match = false;
if (!input1_point.x.is_constant() && !input2_point.x.is_constant()) {
x_match = (input1_point.x.get_witness_index() == input2_point.x.get_witness_index());
} else {
if (input1_point.x.is_constant() && input2_point.x.is_constant()) {
x_match = (input1_point.x.get_value() == input2_point.x.get_value());
}
}
bool y_match = false;
if (!input1_point.y.is_constant() && !input2_point.y.is_constant()) {
y_match = (input1_point.y.get_witness_index() == input2_point.y.get_witness_index());
} else {
if (input1_point.y.is_constant() && input2_point.y.is_constant()) {
y_match = (input1_point.y.get_value() == input2_point.y.get_value());
}
}

cycle_group_ct result;
// If operands are the same, we double.
if (x_match && y_match) {
result = input1_point.dbl();
} else {
result = input1_point.unconditional_add(input2_point);
}

cycle_group_ct standard_result = result.get_standard_form();
auto x_normalized = standard_result.x.normalize();
auto y_normalized = standard_result.y.normalize();
Expand All @@ -35,6 +65,8 @@ void create_ec_add_constraint(Builder& builder, const EcAdd& input, bool has_val
} else {
builder.assert_equal(y_normalized.witness_index, input.result_y);
}
// TODO: remove the infinite result, because the function should always return a non-zero point.
// But this requires an ACIR serialisation change and it will be done in a subsequent PR.
if (infinite.is_constant()) {
builder.fix_witness(input.result_infinite, infinite.get_value());
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,17 @@ bb::stdlib::cycle_group<Builder> to_grumpkin_point(const WitnessOrConstant<FF>&
bool has_valid_witness_assignments,
Builder& builder)
{
using bool_ct = bb::stdlib::bool_t<Builder>;
auto point_x = to_field_ct(input_x, builder);
auto point_y = to_field_ct(input_y, builder);
auto infinite = bool_ct(to_field_ct(input_infinite, builder));
// We assume input_infinite is boolean, so we do not add a boolean gate
bool_t infinite(&builder);
if (!input_infinite.is_constant) {
infinite.witness_index = input_infinite.index;
infinite.witness_bool = get_value(input_infinite, builder) == FF::one();
} else {
infinite.witness_index = IS_CONSTANT;
infinite.witness_bool = input_infinite.value == FF::one();
}

// When we do not have the witness assignments, we set is_infinite value to true if it is not constant
// else default values would give a point which is not on the curve and this will fail verification
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,12 @@ bb::stdlib::cycle_group<Builder> to_grumpkin_point(const WitnessOrConstant<FF>&
bool has_valid_witness_assignments,
Builder& builder);

template <typename Builder, typename FF> FF get_value(const WitnessOrConstant<FF>& input, Builder& builder)
{
if (input.is_constant) {
return input.value;
}
return builder.variables[input.index];
}

} // namespace acir_format

0 comments on commit aabd2d8

Please sign in to comment.