Skip to content
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.

Sign up for GitHub

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!: change ec_add to unsafe implementation (but much better perf) #8374

Merged
merged 9 commits into from
Sep 24, 2024
Merged

chore!: change ec_add to unsafe implementation (but much better perf) #8374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af52e3d
change ec_add to unsafe implementation (but much better perf)
guipublic Sep 4, 2024
7602e38
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 4, 2024
5ee3307
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 5, 2024
a7a54e1
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 5, 2024
48edb7d
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 19, 2024
2fbb666
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 20, 2024
ccfcaac
Merge branch 'master' into gd/unsafe_ec_add
TomAFrench Sep 23, 2024
43f6a23
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 24, 2024
096b2ee
Merge branch 'master' into gd/unsafe_ec_add
guipublic Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.cpp
View file
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this comment it looks like the ACIR representation of an ecc point doesn't allow for a point at infinity?

What happens if I have two constant points, A and B where A = -B, and I compute A+B in noir? Is the compiler clever enough to return a point at infinity, or will the compiler cause the current code path (i.e. acir_format::create_ec_add_constraint) to be executed?

If it's the latter then I think it's better if we throw an error for the case where infinite.is_constant() is triggered, because the alternative is weird undefined behavior. If we're planning to fix the ACIR serialization in the near term we can skip, but if we forget about this issue it will cause us problems in the future because, if I understand correctly, the current implementation produces either undefined behavior or underconstrained code (i.e. the case where we add a constant point with its inverse)

if (infinite.is_constant()) {
builder.fix_witness(input.result_infinite, infinite.get_value());
} else {
Expand Down
11 changes: 9 additions & 2 deletions barretenberg/cpp/src/barretenberg/dsl/acir_format/witness_constant.cpp
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pedantic point: if infinite is defined as a bool_ct then we don't need to assume input_infinite is boolean - the bool_ct constructor will explicitly 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
8 changes: 8 additions & 0 deletions barretenberg/cpp/src/barretenberg/dsl/acir_format/witness_constant.hpp
View file
Open in desktop
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
Loading