feat: (bb) remove redundant constraints on field/group elements when using goblin plonk (#8409)

This PR adds distinct classes for goblin plonk group elements and
coordinate scalars in the stdlib so that we can refine the
implementation of these objects without proliferating edge cases in the
rest of our codebase

This Pr reduces the cost of
`ProtogalaxyRecursiveTests/0.RecursiveFoldingTest` from 24,630 gates to
14,106 gates.

`stdlib::element` is now a class alias that points to either the default
element class definition or the goblin plonk class definition depending
on whether goblin plonk is supported.

This allows us to apply the following improvements/useful restrictions:

1. goblin plonk group elements no longer apply `on_curve` checks when
created (performed in the eccvm)
2. goblin plonk coordinate field elements no longer have range
constraints applied to them (performed in the translator circuit)
3. goblin plonk coordinate field elements no longer generate constraints
when `assert_is_in_field` is applied (performed in the translator
circuit)
4. goblin plonk coordinate field elements do not have arithmetic
operations exposed (manipulation of goblin plonk group elements should
happen exclusively through the eccvm)

In addition, this PR improve the handling of checking whether bn254
points are at infinity when consuming points from a transcript via
`field_conversion`

Author: zac-williamson

barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/goblin_field.hpp

@@ -0,0 +1,114 @@
+#pragma once
+#include "../bigfield/bigfield.hpp"
+#include "../circuit_builders/circuit_builders_fwd.hpp"
+#include "../field/field.hpp"
+
+namespace bb::stdlib {
+
+/**
+ * @brief goblin_field wraps x/y coordinates of bn254 group elements when using goblin
+ * @details this class exists because we do not want to parametrise goblin bn254 coordinates with bigfield.
+ *          bigfield generates a large number of constraints to apply checks that are not needed for goblin coordinates
+ *          This is because, in the goblin context we can apply the following heuristics:
+ *          1. goblin coordinate field elements are range-constrained in the Translator circuit (no need to range
+ * constrain here)
+ *          2. field elements that come out of the ECCVM are well-formed, we do not need to call `assert_is_in_field`
+ *          3. there should be no need to apply arithmetic to goblin coordinate field elements in-circuit
+ *          Having a distinct class for `goblin_field` allows us to harvest these optimisations without a proliferation
+ * of edge cases and bloated logic in other classes
+ * @tparam Builder
+ */
+template <class Builder> class goblin_field {
+  public:
+    static constexpr uint1024_t DEFAULT_MAXIMUM_REMAINDER =
+        bigfield<Builder, bb::Bn254FqParams>::DEFAULT_MAXIMUM_REMAINDER;
+    static constexpr size_t NUM_LIMBS = bigfield<Builder, bb::Bn254FqParams>::NUM_LIMBS;
+    static constexpr size_t NUM_LIMB_BITS = bigfield<Builder, bb::Bn254FqParams>::NUM_LIMB_BITS;
+    static constexpr size_t NUM_LAST_LIMB_BITS = bigfield<Builder, bb::Bn254FqParams>::NUM_LAST_LIMB_BITS;
+
+    using field_ct = stdlib::field_t<Builder>;
+    using bool_ct = stdlib::bool_t<Builder>;
+    std::array<field_ct, 2> limbs;
+
+    // constructors mirror bigfield constructors
+    goblin_field()
+        : limbs{ 0, 0 }
+    {}
+    goblin_field(Builder* parent_context, const uint256_t& value)
+    {
+        (*this) = goblin_field(bb::fq(value));
+        limbs[0].context = parent_context;
+        limbs[1].context = parent_context;
+    }
+    goblin_field(bb::fq input)
+    {
+        uint256_t converted(input);
+        uint256_t lo_v = converted.slice(0, NUM_LIMB_BITS * 2);
+        uint256_t hi_v = converted.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 3 + NUM_LAST_LIMB_BITS);
+        limbs = { bb::fr(lo_v), bb::fr(hi_v) };
+    }
+    goblin_field(field_ct lo, field_ct hi)
+        : limbs{ lo, hi }
+    {}
+
+    // N.B. this method is because AggregationState expects group element coordinates to be split into 4 slices
+    // (we could update to only use 2 for Mega but that feels complex)
+    goblin_field(field_ct lolo, field_ct lohi, field_ct hilo, field_ct hihi, [[maybe_unused]] bool can_overflow = false)
+        : limbs{ lolo + lohi * (uint256_t(1) << NUM_LIMB_BITS), hilo + hihi * (uint256_t(1) << NUM_LIMB_BITS) }
+    {}
+
+    void assert_equal(const goblin_field& other) const
+    {
+        limbs[0].assert_equal(other.limbs[0]);
+        limbs[1].assert_equal(other.limbs[1]);
+    }
+    static goblin_field zero() { return goblin_field{ 0, 0 }; }
+
+    static goblin_field from_witness(Builder* ctx, bb::fq input)
+    {
+        uint256_t converted(input);
+        uint256_t lo_v = converted.slice(0, NUM_LIMB_BITS * 2);
+        uint256_t hi_v = converted.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 3 + NUM_LAST_LIMB_BITS);
+        field_ct lo = field_ct::from_witness(ctx, lo_v);
+        field_ct hi = field_ct::from_witness(ctx, hi_v);
+        return goblin_field(lo, hi);
+    }
+
+    static goblin_field conditional_assign(const bool_ct& predicate, const goblin_field& lhs, goblin_field& rhs)
+    {
+        goblin_field result;
+        result.limbs = {
+            field_ct::conditional_assign(predicate, lhs.limbs[0], rhs.limbs[0]),
+            field_ct::conditional_assign(predicate, lhs.limbs[1], rhs.limbs[1]),
+        };
+        return result;
+    }
+
+    // matches the interface for bigfield
+    uint512_t get_value() const
+    {
+        uint256_t lo = limbs[0].get_value();
+        uint256_t hi = limbs[1].get_value();
+        uint256_t result = lo + (hi << 136);
+        return result;
+    }
+
+    // matches the interface for bigfield
+    uint512_t get_maximum_value() const { return (*this).get_value(); }
+
+    Builder* get_context() const
+    {
+        if (limbs[0].get_context()) {
+            return limbs[0].get_context();
+        }
+        return limbs[1].get_context();
+    }
+
+    // done in the translator circuit
+    void assert_is_in_field(){};
+};
+template <typename C> inline std::ostream& operator<<(std::ostream& os, goblin_field<C> const& v)
+{
+    return os << "{ " << v.limbs[0] << " , " << v.limbs[1] << " }";
+}
+} // namespace bb::stdlib

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

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "../bigfield/bigfield.hpp"
+#include "../bigfield/goblin_field.hpp"
 #include "../byte_array/byte_array.hpp"
 #include "../circuit_builders/circuit_builders_fwd.hpp"
 #include "../field/field.hpp"
@@ -9,21 +10,16 @@
 #include "barretenberg/ecc/curves/bn254/g1.hpp"
 #include "barretenberg/ecc/curves/secp256k1/secp256k1.hpp"
 #include "barretenberg/ecc/curves/secp256r1/secp256r1.hpp"
+#include "barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp"
 
-// TODO(https://github.com/AztecProtocol/barretenberg/issues/707) If using a a circuit builder with Goblin, which is
-// designed to have efficient bb::g1 operations, a developer might accidentally write inefficient circuits
-// using biggroup functions that do not use the OpQueue. We use this concept to prevent compilation of such functions.
-template <typename Builder, typename NativeGroup>
-concept IsNotGoblinInefficiencyTrap = !(IsMegaBuilder<Builder> && std::same_as<NativeGroup, bb::g1>);
-
-namespace bb::stdlib {
+namespace bb::stdlib::element_default {
 
 // ( ͡° ͜ʖ ͡°)
 template <class Builder, class Fq, class Fr, class NativeGroup> class element {
   public:
     using bool_ct = stdlib::bool_t<Builder>;
     using biggroup_tag = element; // Facilitates a constexpr check IsBigGroup
-
+    using BaseField = Fq;
     struct secp256k1_wnaf {
         std::vector<field_t<Builder>> wnaf;
         field_t<Builder> positive_skew;
@@ -177,22 +173,13 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class element {
      * We can chain repeated point additions together, where we only require 2 non-native field multiplications per
      * point addition, instead of 3
      **/
-    static chain_add_accumulator chain_add_start(const element& p1, const element& p2)
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-    static chain_add_accumulator chain_add(const element& p1, const chain_add_accumulator& accumulator)
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-    static element chain_add_end(const chain_add_accumulator& accumulator)
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-
-    element montgomery_ladder(const element& other) const
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-    element montgomery_ladder(const chain_add_accumulator& accumulator)
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-    element multiple_montgomery_ladder(const std::vector<chain_add_accumulator>& to_add) const
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
-
-    element quadruple_and_add(const std::vector<element>& to_add) const
-        requires(IsNotGoblinInefficiencyTrap<Builder, NativeGroup>);
+    static chain_add_accumulator chain_add_start(const element& p1, const element& p2);
+    static chain_add_accumulator chain_add(const element& p1, const chain_add_accumulator& accumulator);
+    static element chain_add_end(const chain_add_accumulator& accumulator);
+    element montgomery_ladder(const element& other) const;
+    element montgomery_ladder(const chain_add_accumulator& accumulator);
+    element multiple_montgomery_ladder(const std::vector<chain_add_accumulator>& to_add) const;
+    element quadruple_and_add(const std::vector<element>& to_add) const;
 
     typename NativeGroup::affine_element get_value() const
     {
@@ -222,12 +209,6 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class element {
                              const size_t max_num_bits = 0,
                              const bool with_edgecases = false);
 
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/707) max_num_bits is unused; could implement and use
-    // this to optimize other operations.
-    static element goblin_batch_mul(const std::vector<element>& points,
-                                    const std::vector<Fr>& scalars,
-                                    const size_t max_num_bits = 0);
-
     // we want to conditionally compile this method iff our curve params are the BN254 curve.
     // This is a bit tricky to do with `std::enable_if`, because `bn254_endo_batch_mul` is a member function of a class
     // template
@@ -938,16 +919,31 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class element {
         typename std::conditional<HasPlookup<Builder>, batch_lookup_table_plookup<>, batch_lookup_table_base>::type;
 };
 
-template <typename T>
-concept IsBigGroup = std::is_same_v<typename T::biggroup_tag, T>;
-
 template <typename C, typename Fq, typename Fr, typename G>
 inline std::ostream& operator<<(std::ostream& os, element<C, Fq, Fr, G> const& v)
 {
     return os << "{ " << v.x << " , " << v.y << " }";
 }
-} // namespace bb::stdlib
+} // namespace bb::stdlib::element_default
 
+namespace bb::stdlib {
+template <typename T>
+concept IsBigGroup = std::is_same_v<typename T::biggroup_tag, T>;
+
+template <typename Builder, class Fq, class Fr, class NativeGroup>
+concept IsGoblinBigGroup =
+    IsMegaBuilder<Builder> && std::same_as<Fq, bb::stdlib::bigfield<Builder, bb::Bn254FqParams>> &&
+    std::same_as<Fr, bb::stdlib::field_t<Builder>> && std::same_as<NativeGroup, bb::g1>;
+
+/**
+ * @brief element wraps either element_default::element or element_goblin::goblin_element depending on parametrisation
+ * @details if C = MegaBuilder, G = bn254, Fq = bigfield<C, bb::Bn254FqParams>, Fr = field_t then we're cooking
+ */
+template <typename C, typename Fq, typename Fr, typename G>
+using element = std::conditional_t<IsGoblinBigGroup<C, Fq, Fr, G>,
+                                   element_goblin::goblin_element<C, goblin_field<C>, Fr, G>,
+                                   element_default::element<C, Fq, Fr, G>>;
+} // namespace bb::stdlib
 #include "biggroup_batch_mul.hpp"
 #include "biggroup_bn254.hpp"
 #include "biggroup_goblin.hpp"

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

@@ -16,6 +16,9 @@ namespace {
 auto& engine = numeric::get_debug_randomness();
 }
 
+template <typename T>
+concept HasGoblinBuilder = IsMegaBuilder<typename T::Curve::Builder>;
+
 // One can only define a TYPED_TEST with a single template paramter.
 // Our workaround is to pass parameters of the following type.
 template <typename _Curve, bool _use_bigfield = false> struct TestType {
@@ -1191,9 +1194,6 @@ using TestTypes = testing::Types<TestType<stdlib::bn254<bb::StandardCircuitBuild
 
 TYPED_TEST_SUITE(stdlib_biggroup, TestTypes);
 
-template <typename T>
-concept HasGoblinBuilder = IsMegaBuilder<typename T::Curve::Builder>;
-
 TYPED_TEST(stdlib_biggroup, add)
 {
 
@@ -1304,7 +1304,7 @@ HEAVY_TYPED_TEST(stdlib_biggroup, multiple_montgomery_ladder)
 HEAVY_TYPED_TEST(stdlib_biggroup, compute_naf)
 {
     // ULTRATODO: make this work for secp curves
-    if constexpr (TypeParam::Curve::type == CurveType::BN254) {
+    if constexpr ((TypeParam::Curve::type == CurveType::BN254) && !HasGoblinBuilder<TypeParam>) {
         size_t num_repetitions = 1;
         for (size_t i = 0; i < num_repetitions; i++) {
             TestFixture::test_compute_naf();
@@ -1318,8 +1318,8 @@ HEAVY_TYPED_TEST(stdlib_biggroup, compute_naf)
 HEAVY_TYPED_TEST(stdlib_biggroup, wnaf_batch_mul)
 {
     if constexpr (HasPlookup<typename TypeParam::Curve::Builder>) {
-        if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+        if constexpr (TypeParam::Curve::type == CurveType::BN254 && HasGoblinBuilder<TypeParam>) {
+            GTEST_SKIP();
         } else {
             TestFixture::test_compute_wnaf();
         };
@@ -1332,8 +1332,8 @@ HEAVY_TYPED_TEST(stdlib_biggroup, wnaf_batch_mul)
 HEAVY_TYPED_TEST(stdlib_biggroup, wnaf_batch_mul_edge_cases)
 {
     if constexpr (HasPlookup<typename TypeParam::Curve::Builder>) {
-        if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+        if constexpr (TypeParam::Curve::type == CurveType::BN254 && HasGoblinBuilder<TypeParam>) {
+            GTEST_SKIP();
         } else {
             TestFixture::test_compute_wnaf();
         };
@@ -1346,7 +1346,8 @@ HEAVY_TYPED_TEST(stdlib_biggroup, wnaf_batch_mul_edge_cases)
    case where Fr is a bigfield. */
 HEAVY_TYPED_TEST(stdlib_biggroup, compute_wnaf)
 {
-    if constexpr (!HasPlookup<typename TypeParam::Curve::Builder> && TypeParam::use_bigfield) {
+    if constexpr ((!HasPlookup<typename TypeParam::Curve::Builder> && TypeParam::use_bigfield) ||
+                  (TypeParam::Curve::type == CurveType::BN254 && HasGoblinBuilder<TypeParam>)) {
         GTEST_SKIP();
     } else {
         TestFixture::test_compute_wnaf();
@@ -1360,8 +1361,8 @@ HEAVY_TYPED_TEST(stdlib_biggroup, batch_mul_short_scalars)
     if constexpr (TypeParam::use_bigfield) {
         GTEST_SKIP();
     } else {
-        if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+        if constexpr (TypeParam::Curve::type == CurveType::BN254 && HasGoblinBuilder<TypeParam>) {
+            GTEST_SKIP();
         } else {
             TestFixture::test_batch_mul_short_scalars();
         };
@@ -1372,8 +1373,8 @@ HEAVY_TYPED_TEST(stdlib_biggroup, wnaf_batch_mul_128_bit)
     if constexpr (TypeParam::use_bigfield) {
         GTEST_SKIP();
     } else {
-        if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+        if constexpr (TypeParam::Curve::type == CurveType::BN254 && HasGoblinBuilder<TypeParam>) {
+            GTEST_SKIP();
         } else {
             TestFixture::test_wnaf_batch_mul_128_bit();
         };
@@ -1393,7 +1394,7 @@ HEAVY_TYPED_TEST(stdlib_biggroup, bn254_endo_batch_mul)
 {
     if constexpr (TypeParam::Curve::type == CurveType::BN254 && !TypeParam::use_bigfield) {
         if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+            GTEST_SKIP();
         } else {
             TestFixture::test_bn254_endo_batch_mul();
         };
@@ -1405,7 +1406,7 @@ HEAVY_TYPED_TEST(stdlib_biggroup, mixed_mul_bn254_endo)
 {
     if constexpr (TypeParam::Curve::type == CurveType::BN254 && !TypeParam::use_bigfield) {
         if constexpr (HasGoblinBuilder<TypeParam>) {
-            GTEST_SKIP() << "https://github.com/AztecProtocol/barretenberg/issues/707";
+            GTEST_SKIP();
         } else {
             TestFixture::test_mixed_mul_bn254_endo();
         };
@@ -1438,4 +1439,4 @@ HEAVY_TYPED_TEST(stdlib_biggroup, ecdsa_mul_secp256k1)
     } else {
         GTEST_SKIP();
     }
-}
+}

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

@@ -2,7 +2,7 @@
 
 #include "barretenberg/stdlib/primitives/biggroup/biggroup_edgecase_handling.hpp"
 #include <cstddef>
-namespace bb::stdlib {
+namespace bb::stdlib::element_default {
 
 /**
  * @brief Multiscalar multiplication that utilizes 4-bit wNAF lookup tables.
@@ -62,4 +62,4 @@ element<C, Fq, Fr, G> element<C, Fq, Fr, G>::wnaf_batch_mul(const std::vector<el
     accumulator -= offset_generators.second;
     return accumulator;
 }
-} // namespace bb::stdlib
+} // namespace bb::stdlib::element_default