feat: biggroup_goblin handles points at infinity + 1.8x reduction in ECCVM size

barretenberg/cpp/src/barretenberg/eccvm/eccvm_builder_types.hpp

@@ -33,6 +33,17 @@ template <typename CycleGroup> struct VMOperation {
         return res;
     }
     bool operator==(const VMOperation<CycleGroup>& other) const = default;
+
+    std::array<uint256_t, 2> get_base_point_standard_form() const
+    {
+        uint256_t x(base_point.x);
+        uint256_t y(base_point.y);
+        if (base_point.is_point_at_infinity()) {
+            x = 0;
+            y = 0;
+        }
+        return { x, y };
+    }
 };
 template <typename CycleGroup> struct ScalarMul {
     uint32_t pc;

barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp

@@ -57,11 +57,11 @@ class GoblinMockCircuits {
         PROFILE_THIS();
 
         if (large) { // Results in circuit size 2^19
-            stdlib::generate_sha256_test_circuit(builder, 12);
+            stdlib::generate_sha256_test_circuit(builder, 11);
             stdlib::generate_ecdsa_verification_test_circuit(builder, 10);
             stdlib::generate_merkle_membership_test_circuit(builder, 12);
         } else { // Results in circuit size 2^17
-            stdlib::generate_sha256_test_circuit(builder, 9);
+            stdlib::generate_sha256_test_circuit(builder, 8);
             stdlib::generate_ecdsa_verification_test_circuit(builder, 2);
             stdlib::generate_merkle_membership_test_circuit(builder, 10);
         }
@@ -192,7 +192,7 @@ class GoblinMockCircuits {
 
         // Add operations representing general kernel logic e.g. state updates. Note: these are structured to make
         // the kernel "full" within the dyadic size 2^17
-        const size_t NUM_MERKLE_CHECKS = 20;
+        const size_t NUM_MERKLE_CHECKS = 19;
         const size_t NUM_ECDSA_VERIFICATIONS = 1;
         const size_t NUM_SHA_HASHES = 1;
         stdlib::generate_merkle_membership_test_circuit(builder, NUM_MERKLE_CHECKS);

barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/mega_arithmetization.hpp

@@ -74,14 +74,14 @@ template <typename FF_> class MegaArith {
         {
             this->ecc_op = 1 << 10;
             this->pub_inputs = 1 << 7;
-            this->busread = 1 << 7;
-            this->arithmetic = 201000;
+            this->arithmetic = 198000;
             this->delta_range = 90000;
             this->elliptic = 9000;
-            this->aux = 137000;
-            this->poseidon2_external = 2500;
-            this->poseidon2_internal = 11500;
+            this->aux = 136000;
             this->lookup = 72000;
+            this->busread = 1 << 7;
+            this->poseidon2_external = 2500;
+            this->poseidon2_internal = 14000;
         }
     };
 
@@ -228,6 +228,7 @@ template <typename FF_> class MegaArith {
         {
             for (auto block : this->get()) {
                 if (block.size() > block.get_fixed_size()) {
+
                     info("WARNING: Num gates in circuit block exceeds the specified fixed size - execution trace will "
                          "not be constructed correctly!");
                     summarize();

barretenberg/cpp/src/barretenberg/protogalaxy/protogalaxy_verifier.cpp

@@ -134,6 +134,15 @@ std::shared_ptr<typename DeciderVerificationKeys::DeciderVK> ProtogalaxyVerifier
         combination = linear_combination(to_combine, lagranges);
     }
 
+    // generate recursive folding challenges to ensure manifest matches with recursive verifier
+    // (in recursive verifier we use random challenges to convert Flavor::NUM_FOLDED_ENTITIES muls
+    //  into one large multiscalar multiplication)
+    std::array<std::string, Flavor::NUM_FOLDED_ENTITIES> args;
+    for (size_t idx = 0; idx < Flavor::NUM_FOLDED_ENTITIES; ++idx) {
+        args[idx] = "accumulator_combination_challenges" + std::to_string(idx);
+    }
+    transcript->template get_challenges<FF>(args);
+
     return next_accumulator;
 }
 

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

@@ -54,9 +54,10 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class goblin_ele
     static goblin_element from_witness(Builder* ctx, const typename NativeGroup::affine_element& input)
     {
         goblin_element out;
+        // ECCVM requires points at infinity to be represented by 0-value x/y coords
         if (input.is_point_at_infinity()) {
-            Fq x = Fq::from_witness(ctx, NativeGroup::affine_one.x);
-            Fq y = Fq::from_witness(ctx, NativeGroup::affine_one.y);
+            Fq x = Fq::from_witness(ctx, bb::fq(0));
+            Fq y = Fq::from_witness(ctx, bb::fq(0));
             out.x = x;
             out.y = y;
         } else {
@@ -92,6 +93,16 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class goblin_ele
         return goblin_element(x_fq, y_fq);
     }
 
+    static goblin_element point_at_infinity(Builder* ctx)
+    {
+        Fr zero = Fr::from_witness_index(ctx, ctx->zero_idx);
+        Fq x_fq(zero, zero);
+        Fq y_fq(zero, zero);
+        goblin_element result(x_fq, y_fq);
+        result.set_point_at_infinity(true);
+        return result;
+    }
+
     goblin_element checked_unconditional_add(const goblin_element& other) const
     {
         return goblin_element::operator+(*this, other);
@@ -107,10 +118,58 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class goblin_ele
     }
     goblin_element operator-(const goblin_element& other) const
     {
-        std::vector<goblin_element> points{ *this, other };
-        return batch_mul({ *this, other }, { Fr(1), -Fr(1) });
+        auto builder = get_context(other);
+        // Check that the internal accumulator is zero
+        ASSERT(builder->op_queue->get_accumulator().is_point_at_infinity());
+
+        // Compute the result natively, and validate that result + other == *this
+        typename NativeGroup::affine_element result_value = typename NativeGroup::affine_element(
+            typename NativeGroup::element(get_value()) - typename NativeGroup::element(other.get_value()));
+
+        ecc_op_tuple op_tuple;
+        op_tuple = builder->queue_ecc_add_accum(other.get_value());
+        {
+            auto x_lo = Fr::from_witness_index(builder, op_tuple.x_lo);
+            auto x_hi = Fr::from_witness_index(builder, op_tuple.x_hi);
+            auto y_lo = Fr::from_witness_index(builder, op_tuple.y_lo);
+            auto y_hi = Fr::from_witness_index(builder, op_tuple.y_hi);
+            x_lo.assert_equal(other.x.limbs[0]);
+            x_hi.assert_equal(other.x.limbs[1]);
+            y_lo.assert_equal(other.y.limbs[0]);
+            y_hi.assert_equal(other.y.limbs[1]);
+        }
+
+        ecc_op_tuple op_tuple2 = builder->queue_ecc_add_accum(result_value);
+        auto x_lo = Fr::from_witness_index(builder, op_tuple2.x_lo);
+        auto x_hi = Fr::from_witness_index(builder, op_tuple2.x_hi);
+        auto y_lo = Fr::from_witness_index(builder, op_tuple2.y_lo);
+        auto y_hi = Fr::from_witness_index(builder, op_tuple2.y_hi);
+
+        Fq result_x(x_lo, x_hi);
+        Fq result_y(y_lo, y_hi);
+        goblin_element result(result_x, result_y);
+
+        // if the output is at infinity, this is represented by x/y coordinates being zero
+        // because they are all 136-bit, we can do a cheap zerocheck by first summing the limbs
+        auto op2_is_infinity = (x_lo.add_two(x_hi, y_lo) + y_hi).is_zero();
+        result.set_point_at_infinity(op2_is_infinity);
+        {
+            ecc_op_tuple op_tuple3 = builder->queue_ecc_eq();
+            auto x_lo = Fr::from_witness_index(builder, op_tuple3.x_lo);
+            auto x_hi = Fr::from_witness_index(builder, op_tuple3.x_hi);
+            auto y_lo = Fr::from_witness_index(builder, op_tuple3.y_lo);
+            auto y_hi = Fr::from_witness_index(builder, op_tuple3.y_hi);
+
+            x_lo.assert_equal(x.limbs[0]);
+            x_hi.assert_equal(x.limbs[1]);
+            y_lo.assert_equal(y.limbs[0]);
+            y_hi.assert_equal(y.limbs[1]);
+        }
+        return result;
     }
-    goblin_element operator-() const { return batch_mul({ *this }, { -Fr(1) }); }
+
+    goblin_element operator-() const { return point_at_infinity(get_context()) - *this; }
+
     goblin_element operator+=(const goblin_element& other)
     {
         *this = *this + other;

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

@@ -42,6 +42,7 @@ template <typename Curve> class stdlib_biggroup_goblin : public testing::Test {
     static void test_goblin_style_batch_mul()
     {
         const size_t num_points = 5;
+        const size_t edge_case_points = 3;
         Builder builder;
 
         std::vector<affine_element> points;
@@ -50,10 +51,16 @@ template <typename Curve> class stdlib_biggroup_goblin : public testing::Test {
             points.push_back(affine_element(element::random_element()));
             scalars.push_back(fr::random_element());
         }
+        points.push_back(g1::affine_point_at_infinity);
+        scalars.push_back(fr::random_element());
+        points.push_back(g1::affine_point_at_infinity);
+        scalars.push_back(0);
+        points.push_back(element::random_element());
+        scalars.push_back(0);
 
         std::vector<element_ct> circuit_points;
         std::vector<scalar_ct> circuit_scalars;
-        for (size_t i = 0; i < num_points; ++i) {
+        for (size_t i = 0; i < num_points + edge_case_points; ++i) {
             circuit_points.push_back(element_ct::from_witness(&builder, points[i]));
             circuit_scalars.push_back(scalar_ct::from_witness(&builder, scalars[i]));
         }
@@ -62,7 +69,7 @@ template <typename Curve> class stdlib_biggroup_goblin : public testing::Test {
 
         element expected_point = g1::one;
         expected_point.self_set_infinity();
-        for (size_t i = 0; i < num_points; ++i) {
+        for (size_t i = 0; i < num_points + edge_case_points; ++i) {
             expected_point += (element(points[i]) * scalars[i]);
         }
 
@@ -75,13 +82,121 @@ template <typename Curve> class stdlib_biggroup_goblin : public testing::Test {
 
         EXPECT_CIRCUIT_CORRECTNESS(builder);
     }
+
+    static void test_goblin_style_batch_mul_to_zero()
+    {
+        const size_t num_points = 5;
+        Builder builder;
+
+        std::vector<affine_element> points;
+        std::vector<fr> scalars;
+        for (size_t i = 0; i < num_points; ++i) {
+            points.push_back(affine_element(element::random_element()));
+            scalars.push_back(fr::random_element());
+        }
+        for (size_t i = 0; i < num_points; ++i) {
+            points.push_back(points[i]);
+            scalars.push_back(-scalars[i]);
+        }
+        std::vector<element_ct> circuit_points;
+        std::vector<scalar_ct> circuit_scalars;
+        for (size_t i = 0; i < num_points * 2; ++i) {
+            circuit_points.push_back(element_ct::from_witness(&builder, points[i]));
+            circuit_scalars.push_back(scalar_ct::from_witness(&builder, scalars[i]));
+        }
+
+        element_ct result_point = element_ct::batch_mul(circuit_points, circuit_scalars);
+
+        EXPECT_EQ(result_point.get_value(), g1::affine_point_at_infinity);
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
+
+    /**
+     * @brief Test goblin-style sub
+     * @details Check that 1) Goblin-style batch sub returns correct value (esp. when infinities involved), and 2)
+     * resulting circuit is correct
+     */
+    static void test_goblin_style_sub()
+    {
+        Builder builder;
+
+        for (size_t i = 0; i < 100; ++i) {
+
+            affine_element lhs(element::random_element());
+            affine_element rhs(element::random_element());
+
+            affine_element expected = affine_element(element(lhs) - element(rhs));
+
+            element_ct lhs_ct = element_ct::from_witness(&builder, lhs);
+            element_ct lhs2_ct = element_ct::from_witness(&builder, lhs);
+
+            element_ct rhs_ct = element_ct::from_witness(&builder, rhs);
+            element_ct out_ct = lhs_ct - rhs_ct;
+            EXPECT_EQ(out_ct.get_value(), expected);
+
+            element_ct zero_ct = lhs_ct - lhs_ct;
+            EXPECT_TRUE(zero_ct.get_value().is_point_at_infinity());
+
+            element_ct zero_ct2 = lhs_ct - lhs2_ct;
+            EXPECT_TRUE(zero_ct2.get_value().is_point_at_infinity());
+
+            element_ct out2_ct = element_ct::point_at_infinity(&builder) - rhs_ct;
+            EXPECT_EQ(out2_ct.get_value(), -rhs);
+
+            element_ct out3_ct = lhs_ct - element_ct::point_at_infinity(&builder);
+            EXPECT_EQ(out3_ct.get_value(), lhs);
+
+            auto lhs_infinity_ct = element_ct::point_at_infinity(&builder);
+            auto rhs_infinity_ct = element_ct::point_at_infinity(&builder);
+            element_ct out4_ct = lhs_infinity_ct - rhs_infinity_ct;
+            EXPECT_TRUE(out4_ct.get_value().is_point_at_infinity());
+            EXPECT_TRUE(out4_ct.is_point_at_infinity().get_value());
+        }
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
+
+    /**
+     * @brief Check goblin-style negate works as intended, including with points at infinity
+     */
+    static void test_goblin_style_neg()
+    {
+        Builder builder;
+        affine_element lhs(element::random_element());
+
+        affine_element expected = -lhs;
+
+        element_ct lhs_ct = element_ct::from_witness(&builder, lhs);
+
+        element_ct result_ct = -lhs_ct;
+        EXPECT_EQ(result_ct.get_value(), expected);
+
+        element_ct infinity = element_ct::point_at_infinity(&builder);
+        element_ct result2_ct = -infinity;
+        EXPECT_EQ(result2_ct.get_value(), g1::affine_point_at_infinity);
+        EXPECT_CIRCUIT_CORRECTNESS(builder);
+    }
 };
 
 using TestTypes = testing::Types<stdlib::bn254<bb::MegaCircuitBuilder>>;
 
 TYPED_TEST_SUITE(stdlib_biggroup_goblin, TestTypes);
 
-HEAVY_TYPED_TEST(stdlib_biggroup_goblin, batch_mul)
+TYPED_TEST(stdlib_biggroup_goblin, batch_mul)
 {
     TestFixture::test_goblin_style_batch_mul();
 }
+
+TYPED_TEST(stdlib_biggroup_goblin, batch_mul_equals_zero)
+{
+    TestFixture::test_goblin_style_batch_mul_to_zero();
+}
+
+TYPED_TEST(stdlib_biggroup_goblin, sub)
+{
+    TestFixture::test_goblin_style_sub();
+}
+
+TYPED_TEST(stdlib_biggroup_goblin, neg)
+{
+    TestFixture::test_goblin_style_neg();
+}

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

@@ -90,11 +90,12 @@ goblin_element<C, Fq, Fr, G> goblin_element<C, Fq, Fr, G>::batch_mul(const std::
     Fq point_y(y_lo, y_hi);
     goblin_element result = goblin_element(point_x, point_y);
 
-    // NOTE: we can have an `if` statement here under the strict assumption that `return_is_infinity`
-    //       is produced from `eq_and_reset` opcode
-    if (op_tuple.return_is_infinity) {
-        result.set_point_at_infinity(bool_ct(builder, true));
-    };
+    // NOTE: this used to be set as a circuit constant from `op_tuple.return_is_infinity
+    // I do not see how this was secure as it meant a circuit constant could change depending on witness values
+    // e.g. x*[P] + y*[Q] where `x = y` and `[P] = -[Q]`
+    // TODO(@zac-williamson) what is op_queue.return_is_infinity actually used for? I don't see its value
+    auto op2_is_infinity = (x_lo.add_two(x_hi, y_lo) + y_hi).is_zero();
+    result.set_point_at_infinity(op2_is_infinity);
 
     return result;
 }