fix ecdsa pai issue, add conditional_select fn and tests.

Co-authored-by: federicobarbacovi <[email protected]>

Author: suyash67

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

@@ -218,6 +218,25 @@ template <class Builder_, class Fq, class Fr, class NativeGroup> class element {
         return result;
     }
 
+    element conditional_select(const element& other, const bool_ct& predicate) const
+    {
+        // If predicate is constant, we can select out of circuit
+        if (predicate.is_constant()) {
+            return predicate.get_value() ? other : *this;
+        }
+
+        // Get the builder context
+        Builder* ctx = get_context(other) ? get_context(other) : predicate.get_context();
+        BB_ASSERT_NEQ(ctx, nullptr, "biggroup::conditional_select must have a context");
+
+        element result(*this);
+        result.x = result.x.conditional_select(other.x, predicate);
+        result.y = result.y.conditional_select(other.y, predicate);
+        result._is_infinity =
+            bool_ct::conditional_assign(predicate, other.is_point_at_infinity(), result.is_point_at_infinity());
+        return result;
+    }
+
     element normalize() const
     {
         element result(*this);

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp

@@ -343,6 +343,57 @@ template <typename TestType> class stdlib_biggroup : public testing::Test {
         EXPECT_CIRCUIT_CORRECTNESS(builder);
     }
 
+    static void test_conditional_negate()
+    {
+        Builder builder;
+        size_t num_repetitions = 10;
+        for (size_t i = 0; i < num_repetitions; ++i) {
+            affine_element input_a(element::random_element());
+            bool negate = (engine.get_random_uint32() % 2) == 1;
+            bool_ct negate_ct = bool_ct(witness_ct(&builder, negate ? 1 : 0));
+            element_ct a = element_ct::from_witness(&builder, input_a);
+            a.set_origin_tag(submitted_value_origin_tag);
+
+            element_ct c = a.conditional_negate(negate_ct);
+
+            // Check the resulting tag is preserved
+            EXPECT_EQ(c.get_origin_tag(), submitted_value_origin_tag);
+
+            affine_element c_expected = negate ? affine_element(-element(input_a)) : input_a;
+            EXPECT_EQ(c.get_value(), c_expected);
+        }
+
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
+
+    static void test_conditional_select()
+    {
+        Builder builder;
+        size_t num_repetitions = 10;
+        for (size_t i = 0; i < num_repetitions; ++i) {
+            affine_element input_a(element::random_element());
+            affine_element input_b(element::random_element());
+            bool select_a = (engine.get_random_uint32() % 2) == 1;
+            bool_ct select_a_ct = bool_ct(witness_ct(&builder, select_a ? 1 : 0));
+            element_ct a = element_ct::from_witness(&builder, input_a);
+            element_ct b = element_ct::from_witness(&builder, input_b);
+
+            // Set different tags in a and b
+            a.set_origin_tag(submitted_value_origin_tag);
+            b.set_origin_tag(challenge_origin_tag);
+
+            element_ct c = a.conditional_select(b, select_a_ct);
+
+            // Check that the resulting tag is the union of inputs' tags
+            EXPECT_EQ(c.get_origin_tag(), first_two_merged_tag);
+
+            affine_element c_expected = select_a ? input_b : input_a;
+            EXPECT_EQ(c.get_value(), c_expected);
+        }
+
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
+
     static void test_montgomery_ladder()
     {
         Builder builder;
@@ -1676,6 +1727,14 @@ TYPED_TEST(stdlib_biggroup, dbl)
 {
     TestFixture::test_dbl();
 }
+TYPED_TEST(stdlib_biggroup, conditional_negate)
+{
+    TestFixture::test_conditional_negate();
+}
+TYPED_TEST(stdlib_biggroup, conditional_select)
+{
+    TestFixture::test_conditional_select();
+}
 TYPED_TEST(stdlib_biggroup, montgomery_ladder)
 {
     if constexpr (HasGoblinBuilder<TypeParam>) {

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp

@@ -235,6 +235,16 @@ template <class Builder_, class Fq, class Fr, class NativeGroup> class goblin_el
         return result;
     }
 
+    goblin_element conditional_select(const goblin_element& other, const bool_ct& predicate) const
+    {
+        goblin_element result(*this);
+        result.x = Fq::conditional_assign(predicate, other.x, result.x);
+        result.y = Fq::conditional_assign(predicate, other.y, result.y);
+        result._is_infinity =
+            bool_ct::conditional_assign(predicate, other.is_point_at_infinity(), result.is_point_at_infinity());
+        return result;
+    }
+
     goblin_element normalize() const
     {
         // no need to normalize, all goblin eccvm operations are returned normalized

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.hpp

@@ -130,8 +130,7 @@ element<C, Fq, Fr, G> element<C, Fq, Fr, G>::secp256k1_ecdsa_mul(const element&
 
         // when computing the wNAF we have already validated that positive_skew and negative_skew cannot both be true
         bool_ct skew_combined = positive_skew_bool ^ negative_skew_bool;
-        result.x = accumulator.x.conditional_select(result.x, skew_combined);
-        result.y = accumulator.y.conditional_select(result.y, skew_combined);
+        result = accumulator.conditional_select(result, skew_combined);
         return result;
     };
 

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.test.cpp

@@ -111,6 +111,61 @@ template <typename Curve> class stdlibBiggroupSecp256k1 : public testing::Test {
 
         EXPECT_CIRCUIT_CORRECTNESS(builder);
     }
+
+    static void test_secp256k1_ecdsa_mul_skew_handling_regression()
+    {
+        // The scalars s1, u1, u2 are chosen such that:
+        // Public key: P = (s1 * G)
+        //
+        // u1 * G + u2 * (s1 * G) = ø
+        //
+        // where ø is the point at infinity.
+        //
+        // The issue with such input was that we were not setting the point at infinity correctly
+        // while adding the skew points. For the cases when we reach the point at infinity and still have
+        // skew to add, we did not correctly set the flag _is_point_at_infinity. For this example, we have:
+        //
+        // u1_low skew:   0
+        // u1_high skew:  1
+        // u2_low skew:   1
+        // u2_high skew:  0
+        //
+        // After adding the u2_low skew (i.e., its base point), we get the point at infinity. Then we handle the
+        // u2 high skew as follows:
+        // result = acc ± u1_high_base_point
+        // result.x = u2_high_skew ? result.x : acc.x;
+        // result.y = u2_high_skew ? result.y : acc.y;
+        //
+        // However, we did not set the flag _is_point_at_infinity for result. We must copy the flag from the
+        // accumulator in this case, i.e., we must do:
+        // result.x = u2_high_skew ? result.x : acc.x;
+        // result.y = u2_high_skew ? result.y : acc.y;
+        // result._is_point_at_infinity = u2_high_skew ? result._is_point_at_infinity : acc._is_point_at_infinity;
+        //
+        // We define a new function `conditional_select` that does this operation and use it to handle the skew
+        // addition.
+        const uint256_t scalar_s1("0x66ad81e84534c20431c795de922fb592c3d8c68edcacfc6c5b52ab7ad10e47d3");
+        const uint256_t scalar_u1("0x37e0ba2e9c4dd42077fd751a7426a8484a8ff2928a6c85a651e4470b461c6215");
+        const uint256_t scalar_u2("0xdefbb9bbabde5b9f8d7175946e75babc2f11203a8bfb71beaeec1d7a2bff17dd");
+
+        // Check the assumptions
+        ASSERT(scalar_s1 < fr::modulus);
+        ASSERT(scalar_u1 < fr::modulus);
+        ASSERT(scalar_u2 < fr::modulus);
+        ASSERT((fr(scalar_s1) * fr(scalar_u2) + fr(scalar_u1)).is_zero());
+        ASSERT((g1::one * fr(scalar_u1) + (g1::one * fr(scalar_s1)) * fr(scalar_u2)).is_point_at_infinity());
+
+        Builder builder = Builder();
+        element_ct P_a = element_ct::from_witness(&builder, g1::one * fr(scalar_s1));
+        scalar_ct u1 = scalar_ct::from_witness(&builder, fr(scalar_u1));
+        scalar_ct u2 = scalar_ct::from_witness(&builder, fr(scalar_u2));
+        auto output = element_ct::secp256k1_ecdsa_mul(P_a, u1, u2);
+
+        // Check that the output is the point at infinity
+        EXPECT_EQ(output.is_point_at_infinity().get_value(), true);
+
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
 };
 
 // Then define the test types
@@ -133,3 +188,7 @@ TYPED_TEST(stdlibBiggroupSecp256k1, EcdsaMulSecp256k1)
 {
     TestFixture::test_ecdsa_mul_secp256k1();
 }
+TYPED_TEST(stdlibBiggroupSecp256k1, EcdsaMulSecp256k1SkewHandlingRegression)
+{
+    TestFixture::test_secp256k1_ecdsa_mul_skew_handling_regression();
+}