From 4b9d189e9d72081e5c8a877e2f23adf01dafb710 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 30 Aug 2024 11:15:35 +0000 Subject: [PATCH 1/4] improve ec_add --- .../barretenberg/stdlib/primitives/field/field.cpp | 12 ++++-------- .../stdlib/primitives/group/cycle_group.cpp | 11 +++++++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp index 5d84e42450fd..01ee036daabf 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp @@ -694,18 +694,14 @@ template bool_t field_t::operator==(const f bb::fr fd = fa - fb; bool is_equal = (fa == fb); bb::fr fc = is_equal ? bb::fr::one() : fd.invert(); - - bool_t result(witness_t(ctx, is_equal)); - field_t r(result); + bool_t result(ctx, is_equal); field_t x(witness_t(ctx, fc)); - const field_t& a = *this; const field_t& b = other; const field_t diff = a - b; - - const field_t t1 = r.madd(-x + 1, x); - const field_t t2 = diff.madd(t1, r - 1); - t2.assert_equal(0); + // these constraints ensures that result is a boolean + field_t::evaluate_polynomial_identity(diff, x, result, field_t(-1)); + field_t::evaluate_polynomial_identity(diff, result, field_t(0), field_t(0)); return result; } diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp index 993d67b31ea0..ba99728003ef 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp @@ -447,7 +447,15 @@ template cycle_group cycle_group::operator+ // if x_coordinates match, lambda triggers a divide by zero error. // Adding in `x_coordinates_match` ensures that lambda will always be well-formed auto x_diff = x2.add_two(-x1, x_coordinates_match); - auto lambda = (y2 - y1) / x_diff; + // Computes lambda = (y2-y1)/x_diff, using the fact that x_diff is never 0 + field_t lambda; + if ((!y2.is_constant() || !y1.is_constant()) && !x_diff.is_constant()) { + lambda = field_t::from_witness(context, (y2.get_value() - y1.get_value()) / x_diff.get_value()); + field_t::evaluate_polynomial_identity(x_diff, lambda, -y2, y1); + } else { + lambda = (y2 - y1).divide_no_zero_check(x_diff); + } + auto x3 = lambda.madd(lambda, -(x2 + x1)); auto y3 = lambda.madd(x1 - x3, -y1); cycle_group add_result(x3, y3, x_coordinates_match); @@ -473,7 +481,6 @@ template cycle_group cycle_group::operator+ // is result point at infinity? // yes = infinity_predicate && !lhs_infinity && !rhs_infinity // yes = lhs_infinity && rhs_infinity - // n.b. can likely optimize this bool_t result_is_infinity = infinity_predicate && (!lhs_infinity && !rhs_infinity); result_is_infinity = result_is_infinity || (lhs_infinity && rhs_infinity); result.set_point_at_infinity(result_is_infinity); From 398717f7ee762733e25ffc2fa84a6a88b3560d31 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 30 Aug 2024 14:36:54 +0000 Subject: [PATCH 2/4] properly construct the bool_t and fix test case --- .../barretenberg/stdlib/primitives/field/field.cpp | 10 +++++++--- .../stdlib/primitives/field/field.test.cpp | 12 ++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp index 01ee036daabf..26f08fa0862c 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp @@ -695,13 +695,17 @@ template bool_t field_t::operator==(const f bool is_equal = (fa == fb); bb::fr fc = is_equal ? bb::fr::one() : fd.invert(); bool_t result(ctx, is_equal); + auto result_witness = witness_t(ctx, is_equal); + result.witness_index = result_witness.witness_index; + result.witness_bool = is_equal; + field_t x(witness_t(ctx, fc)); const field_t& a = *this; const field_t& b = other; const field_t diff = a - b; - // these constraints ensures that result is a boolean - field_t::evaluate_polynomial_identity(diff, x, result, field_t(-1)); - field_t::evaluate_polynomial_identity(diff, result, field_t(0), field_t(0)); + // these constraints ensure that result is a boolean + field_t::evaluate_polynomial_identity(diff, x, result, -field_t(bb::fr::one())); + field_t::evaluate_polynomial_identity(diff, result, field_t(bb::fr::zero()), field_t(bb::fr::zero())); return result; } diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.test.cpp index 76ecb764289e..e8aec8a47458 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.test.cpp @@ -325,9 +325,9 @@ template class stdlib_field : public testing::Test { // This logic requires on madd in field, which creates a big mul gate. // This gate is implemented in standard by create 2 actual gates, while in ultra there are 2 if constexpr (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 6UL); + EXPECT_EQ(gates_after - gates_before, 5UL); } else if (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 4UL); + EXPECT_EQ(gates_after - gates_before, 3UL); } bool result = CircuitChecker::check(builder); @@ -353,9 +353,9 @@ template class stdlib_field : public testing::Test { // This logic requires on madd in field, which creates a big mul gate. // This gate is implemented in standard by create 2 actual gates, while in ultra there are 2 if constexpr (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 6UL); + EXPECT_EQ(gates_after - gates_before, 5UL); } else if (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 4UL); + EXPECT_EQ(gates_after - gates_before, 3UL); } bool result = CircuitChecker::check(builder); @@ -382,9 +382,9 @@ template class stdlib_field : public testing::Test { // This logic requires on madd in field, which creates a big mul gate. // This gate is implemented in standard by create 2 actual gates, while in ultra there are 2 if constexpr (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 11UL); + EXPECT_EQ(gates_after - gates_before, 9UL); } else if (std::same_as) { - EXPECT_EQ(gates_after - gates_before, 7UL); + EXPECT_EQ(gates_after - gates_before, 5UL); } bool result = CircuitChecker::check(builder); From 13061bece61b3165f75d555905248ce65a33e727 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 30 Aug 2024 15:10:02 +0000 Subject: [PATCH 3/4] fix another test case --- .../src/barretenberg/examples/join_split/join_split.test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/barretenberg/cpp/src/barretenberg/examples/join_split/join_split.test.cpp b/barretenberg/cpp/src/barretenberg/examples/join_split/join_split.test.cpp index 7d7626a58ce6..9ea4a5d8e022 100644 --- a/barretenberg/cpp/src/barretenberg/examples/join_split/join_split.test.cpp +++ b/barretenberg/cpp/src/barretenberg/examples/join_split/join_split.test.cpp @@ -701,7 +701,7 @@ TEST_F(join_split_tests, test_0_input_notes_and_detect_circuit_change) // The below part detects any changes in the join-split circuit constexpr size_t DYADIC_CIRCUIT_SIZE = 1 << 16; - constexpr uint256_t CIRCUIT_HASH("0x9170317e02f4131b84f6b4efdd3ac23e5f392d815df37750c8f05a94c64797b2"); + constexpr uint256_t CIRCUIT_HASH("0x2b30566e4d921ea9b0c76802d86ea5b8381ffa78ef143af1b0d0e3045862cb6b"); const uint256_t circuit_hash = circuit.hash_circuit(); // circuit is finalized now From edcb0d4ec0938c98bb54e0c21804b29b1ad1de34 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 3 Sep 2024 09:30:31 +0000 Subject: [PATCH 4/4] Code review --- .../barretenberg/stdlib/primitives/group/cycle_group.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp index ba99728003ef..593fdfa6aa87 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp @@ -449,11 +449,11 @@ template cycle_group cycle_group::operator+ auto x_diff = x2.add_two(-x1, x_coordinates_match); // Computes lambda = (y2-y1)/x_diff, using the fact that x_diff is never 0 field_t lambda; - if ((!y2.is_constant() || !y1.is_constant()) && !x_diff.is_constant()) { + if ((y1.is_constant() && y2.is_constant()) || x_diff.is_constant()) { + lambda = (y2 - y1).divide_no_zero_check(x_diff); + } else { lambda = field_t::from_witness(context, (y2.get_value() - y1.get_value()) / x_diff.get_value()); field_t::evaluate_polynomial_identity(x_diff, lambda, -y2, y1); - } else { - lambda = (y2 - y1).divide_no_zero_check(x_diff); } auto x3 = lambda.madd(lambda, -(x2 + x1));