chore: cycle group cleanup #5

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp

@@ -1260,6 +1260,35 @@ template <typename Builder> field_t<Builder> field_t<Builder>::accumulate(const
     return total.normalize();
 }
 
+// WORKTODO: better name!
+template <typename Builder>
+std::pair<field_t<Builder>, field_t<Builder>> field_t<Builder>::split_at_unrestricted(const size_t lsb_index) const
+{
+    // WORKTODO: is this right? depends on how bb::fr is implemented. ideally this would be some predefined constant..
+    const size_t max_bits = 254;
+    ASSERT(lsb_index < max_bits);
+
+    const uint256_t value(this->get_value());
+    const uint256_t lo_val = value.slice(0, lsb_index);
+    const uint256_t hi_val = value.slice(lsb_index, max_bits);
+
+    // If `this` is constant, return constants based on the native hi/lo values
+    if (this->is_constant()) {
+        return { field_t(lo_val), field_t(hi_val) };
+    }
+
+    // Otherwise, create hi/lo witnesses and constrain them to reconstruct `this` as field elements
+    field_t lo = from_witness(get_context(), lo_val);
+    field_t hi = from_witness(get_context(), hi_val);
+    const uint256_t shift = uint256_t(1) << lsb_index;
+    (lo + (hi * shift)).assert_equal(*this);
+
+    // Set the origin tag for the limbs to be that of the original field element
+    lo.set_origin_tag(get_origin_tag());
+    hi.set_origin_tag(get_origin_tag());
+    return { lo, hi };
+}
+
 /**
  * @brief Splits the field element into (lo, hi), where:
  * - lo contains bits [0, lsb_index)

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

@@ -388,6 +388,8 @@ template <typename Builder_> class field_t {
 
     Builder* get_context() const { return context; }
 
+    std::pair<field_t<Builder>, field_t<Builder>> split_at_unrestricted(const size_t lsb_index) const;
+
     std::pair<field_t<Builder>, field_t<Builder>> split_at(
         const size_t lsb_index, const size_t num_bits = grumpkin::MAX_NO_WRAP_INTEGER_BIT_LENGTH) const;
 

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp

@@ -20,7 +20,8 @@
     using Group = typename Curve::Group;                                                                               \
     using bool_ct = stdlib::bool_t<Builder>;                                                                           \
     using witness_ct = stdlib::witness_t<Builder>;                                                                     \
-    using cycle_scalar_ct = cycle_group_ct::cycle_scalar;
+    using cycle_scalar_ct = cycle_group_ct::cycle_scalar;                                                              \
+    using BigScalarField = typename cycle_scalar_ct::BigScalarField;
 
 using namespace bb;
 
@@ -771,22 +772,26 @@ TYPED_TEST(CycleGroupTest, TestBatchMulGeneralMSM)
         // 1: add entry where point, scalar are witnesses
         expected += (element * scalar);
         points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar1(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar1));
 
         // 2: add entry where point is constant, scalar is witness
         expected += (element * scalar);
         points.emplace_back(cycle_group_ct(element));
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar2(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar2));
 
         // 3: add entry where point is witness, scalar is constant
         expected += (element * scalar);
         points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-        scalars.emplace_back(typename cycle_group_ct::cycle_scalar(scalar));
+        BigScalarField big_scalar3(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar3));
 
         // 4: add entry where point is constant, scalar is constant
         expected += (element * scalar);
         points.emplace_back(cycle_group_ct(element));
-        scalars.emplace_back(typename cycle_group_ct::cycle_scalar(scalar));
+        BigScalarField big_scalar4(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar4));
     }
 
     // Here and in the following cases assign different tags to points and scalars and get the union of them back
@@ -813,10 +818,12 @@ TYPED_TEST(CycleGroupTest, TestBatchMulProducesInfinity)
     auto element = TestFixture::generators[0];
     typename Group::Fr scalar = Group::Fr::random_element(&engine);
     points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-    scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+    BigScalarField big_scalar1(&builder, scalar);
+    scalars.emplace_back(cycle_scalar_ct(big_scalar1));
 
     points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-    scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, -scalar));
+    BigScalarField big_scalar2(&builder, -scalar);
+    scalars.emplace_back(cycle_scalar_ct(big_scalar2));
 
     const auto expected_tag = assign_and_merge_tags(points, scalars);
 
@@ -841,7 +848,8 @@ TYPED_TEST(CycleGroupTest, TestBatchMulMultiplyByZero)
     auto element = TestFixture::generators[0];
     typename Group::Fr scalar = 0;
     points.emplace_back(cycle_group_ct::from_witness(&builder, element));
-    scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+    BigScalarField big_scalar(&builder, scalar);
+    scalars.emplace_back(cycle_scalar_ct(big_scalar));
 
     const auto expected_tag = assign_and_merge_tags(points, scalars);
     auto result = cycle_group_ct::batch_mul(points, scalars);
@@ -869,14 +877,16 @@ TYPED_TEST(CycleGroupTest, TestBatchMulInputsAreInfinity)
         cycle_group_ct point = cycle_group_ct::from_witness(&builder, element);
         point.set_point_at_infinity(witness_ct(&builder, true));
         points.emplace_back(point);
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar1(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar1));
     }
     // is_infinity = constant
     {
         cycle_group_ct point = cycle_group_ct::from_witness(&builder, element);
         point.set_point_at_infinity(true);
         points.emplace_back(point);
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar2(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar2));
     }
 
     const auto expected_tag = assign_and_merge_tags(points, scalars);
@@ -907,14 +917,16 @@ TYPED_TEST(CycleGroupTest, TestBatchMulFixedBaseInLookupTable)
         // 1: add entry where point is constant, scalar is witness
         expected += (element * scalar);
         points.emplace_back(element);
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar));
         scalars_native.emplace_back(uint256_t(scalar));
 
         // 2: add entry where point is constant, scalar is constant
         element = plookup::fixed_base::table::rhs_generator_point();
         expected += (element * scalar);
         points.emplace_back(element);
-        scalars.emplace_back(typename cycle_group_ct::cycle_scalar(scalar));
+        BigScalarField big_scalar_const(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar_const));
         scalars_native.emplace_back(uint256_t(scalar));
     }
     const auto expected_tag = assign_and_merge_tags(points, scalars);
@@ -946,22 +958,25 @@ TYPED_TEST(CycleGroupTest, TestBatchMulFixedBaseSomeInLookupTable)
         // 1: add entry where point is constant, scalar is witness
         expected += (element * scalar);
         points.emplace_back(element);
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar));
         scalars_native.emplace_back(scalar);
 
         // 2: add entry where point is constant, scalar is constant
         element = plookup::fixed_base::table::rhs_generator_point();
         expected += (element * scalar);
         points.emplace_back(element);
-        scalars.emplace_back(typename cycle_group_ct::cycle_scalar(scalar));
+        BigScalarField big_scalar_const(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar_const));
         scalars_native.emplace_back(scalar);
 
         // 3: add entry where point is constant, scalar is witness
         scalar = Group::Fr::random_element(&engine);
         element = Group::one * Group::Fr::random_element(&engine);
         expected += (element * scalar);
         points.emplace_back(element);
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar2(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar2));
         scalars_native.emplace_back(scalar);
     }
     const auto expected_tag = assign_and_merge_tags(points, scalars);
@@ -989,11 +1004,13 @@ TYPED_TEST(CycleGroupTest, TestBatchMulFixedBaseZeroScalars)
 
         // 1: add entry where point is constant, scalar is witness
         points.emplace_back((element));
-        scalars.emplace_back(cycle_group_ct::cycle_scalar::from_witness(&builder, scalar));
+        BigScalarField big_scalar(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar));
 
         // 2: add entry where point is constant, scalar is constant
         points.emplace_back((element));
-        scalars.emplace_back(typename cycle_group_ct::cycle_scalar(scalar));
+        BigScalarField big_scalar_const(&builder, scalar);
+        scalars.emplace_back(cycle_scalar_ct(big_scalar_const));
     }
     const auto expected_tag = assign_and_merge_tags(points, scalars);
     auto result = cycle_group_ct::batch_mul(points, scalars);
@@ -1014,7 +1031,6 @@ TYPED_TEST(CycleGroupTest, TestMul)
     // case 1, general MSM with inputs that are combinations of constant and witnesses
     {
         cycle_group_ct point;
-        typename cycle_group_ct::cycle_scalar scalar;
         cycle_group_ct result;
         for (size_t i = 0; i < num_muls; ++i) {
             auto element = TestFixture::generators[i];
@@ -1023,7 +1039,8 @@ TYPED_TEST(CycleGroupTest, TestMul)
 
             // 1: add entry where point, scalar are witnesses
             point = (cycle_group_ct::from_witness(&builder, element));
-            scalar = (cycle_group_ct::cycle_scalar::from_witness(&builder, native_scalar));
+            BigScalarField big_scalar1(&builder, native_scalar);
+            cycle_scalar_ct scalar = (cycle_scalar_ct(big_scalar1));
             point.set_origin_tag(submitted_value_origin_tag);
             scalar.set_origin_tag(challenge_origin_tag);
             result = point * scalar;
@@ -1032,16 +1049,21 @@ TYPED_TEST(CycleGroupTest, TestMul)
 
             // 2: add entry where point is constant, scalar is witness
             point = (cycle_group_ct(element));
-            scalar = (cycle_group_ct::cycle_scalar::from_witness(&builder, native_scalar));
+            BigScalarField big_scalar2(&builder, native_scalar);
+            scalar = (cycle_scalar_ct(big_scalar2));
 
             EXPECT_EQ((result).get_value(), (expected_result));
 
             // 3: add entry where point is witness, scalar is constant
             point = (cycle_group_ct::from_witness(&builder, element));
+            BigScalarField big_scalar3(&builder, native_scalar);
+            scalar = (cycle_scalar_ct(big_scalar3));
             EXPECT_EQ((result).get_value(), (expected_result));
 
             // 4: add entry where point is constant, scalar is constant
             point = (cycle_group_ct(element));
+            BigScalarField big_scalar4(&builder, native_scalar);
+            scalar = (cycle_scalar_ct(big_scalar4));
             EXPECT_EQ((result).get_value(), (expected_result));
         }
     }
@@ -1156,7 +1178,8 @@ TYPED_TEST(CycleGroupTest, MixedLengthScalarsIsNotSupported)
 
     // First scalar: 256 bits
     uint256_t scalar1_value = uint256_t(123456789);
-    scalars.push_back(cycle_group_ct::cycle_scalar::from_witness(&builder, typename Curve::ScalarField(scalar1_value)));
+    BigScalarField big_scalar1(&builder, typename Curve::ScalarField(scalar1_value));
+    scalars.push_back(cycle_scalar_ct(big_scalar1));
 
     // Second scalar: 128 bits
     uint256_t scalar2_value = uint256_t(987654321);
@@ -1188,8 +1211,10 @@ TYPED_TEST(CycleGroupTest, TestFixedBaseBatchMul)
     auto scalar1_val = Group::Fr::random_element(&engine);
     auto scalar2_val = Group::Fr::random_element(&engine);
 
-    scalars.push_back(cycle_scalar_ct::from_witness(&builder, scalar1_val));
-    scalars.push_back(cycle_scalar_ct::from_witness(&builder, scalar2_val));
+    BigScalarField big_scalar1(&builder, scalar1_val);
+    BigScalarField big_scalar2(&builder, scalar2_val);
+    scalars.push_back(cycle_scalar_ct(big_scalar1));
+    scalars.push_back(cycle_scalar_ct(big_scalar2));
     points.push_back(cycle_group_ct(lhs_generator)); // constant point
     points.push_back(cycle_group_ct(rhs_generator)); // constant point
 

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_scalar.cpp

@@ -17,28 +17,6 @@ cycle_scalar<Builder>::cycle_scalar(const field_t& _lo, const field_t& _hi)
     , hi(_hi)
 {}
 
-template <typename Builder> cycle_scalar<Builder>::cycle_scalar(const field_t& in)
-{
-    const uint256_t value(in.get_value());
-    const uint256_t lo_v = value.slice(0, LO_BITS);
-    const uint256_t hi_v = value.slice(LO_BITS, HI_BITS);
-    constexpr uint256_t shift = uint256_t(1) << LO_BITS;
-    if (in.is_constant()) {
-        lo = lo_v;
-        hi = hi_v;
-    } else {
-        lo = witness_t<Builder>(in.get_context(), lo_v);
-        hi = witness_t<Builder>(in.get_context(), hi_v);
-        (lo + hi * shift).assert_equal(in);
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/1022): ensure lo and hi are in bb::fr modulus not
-        // bb::fq modulus otherwise we could have two representations for in
-        validate_scalar_is_in_field();
-    }
-    // We need to manually propagate the origin tag
-    lo.set_origin_tag(in.get_origin_tag());
-    hi.set_origin_tag(in.get_origin_tag());
-}
-
 template <typename Builder> cycle_scalar<Builder>::cycle_scalar(const ScalarField& in)
 {
     const uint256_t value(in);
@@ -79,44 +57,45 @@ cycle_scalar<Builder> cycle_scalar<Builder>::from_witness_bitstring(Builder* con
     BB_ASSERT_LT(bitstring.get_msb(), num_bits);
     const uint256_t lo_v = bitstring.slice(0, LO_BITS);
     const uint256_t hi_v = bitstring.slice(LO_BITS, HI_BITS);
-    field_t lo = witness_t<Builder>(context, lo_v);
-    field_t hi = witness_t<Builder>(context, hi_v);
-    lo.set_free_witness_tag();
-    hi.set_free_witness_tag();
-    cycle_scalar result{ lo, hi, num_bits, true, false };
-    return result;
+    auto lo = field_t::from_witness(context, lo_v);
+    auto hi = field_t::from_witness(context, hi_v);
+    return cycle_scalar{
+        lo, hi, num_bits, /*skip_primality_test=*/true, /*use_bn254_scalar_field_for_primality_test=*/false
+    };
 }
 
 /**
- * @brief Use when we want to multiply a group element by a string of bits of known size.
- *        N.B. using this constructor method will make our scalar multiplication methods not perform primality tests.
+ * @brief Construct a cycle scalar (grumpkin scalar field element) from a bn254 scalar field element
+ * @details This method ensures that the input is constrained to be less than the bn254 scalar field modulus.
  *
  * @tparam Builder
  * @param context
  * @param value
  * @param num_bits
  * @return cycle_scalar<Builder>
  */
-template <typename Builder>
-cycle_scalar<Builder> cycle_scalar<Builder>::create_from_bn254_scalar(const field_t& in, const bool skip_primality_test)
+template <typename Builder> cycle_scalar<Builder> cycle_scalar<Builder>::create_from_bn254_scalar(const field_t& in)
 {
     const uint256_t value_u256(in.get_value());
     const uint256_t lo_v = value_u256.slice(0, LO_BITS);
     const uint256_t hi_v = value_u256.slice(LO_BITS, HI_BITS);
+    const bool skip_primality_test = false;
+    const bool use_bn254_scalar_field_for_primality_test = true;
+    // WORKTODO: should be able to use field_t::split_at_unrestricted here
     if (in.is_constant()) {
-        cycle_scalar result{ field_t(lo_v), field_t(hi_v), NUM_BITS, false, true };
-        return result;
+        return cycle_scalar{
+            field_t(lo_v), field_t(hi_v), NUM_BITS, skip_primality_test, use_bn254_scalar_field_for_primality_test
+        };
     }
-    field_t lo = witness_t<Builder>(in.get_context(), lo_v);
-    field_t hi = witness_t<Builder>(in.get_context(), hi_v);
+    auto lo = field_t::from_witness(in.get_context(), lo_v);
+    auto hi = field_t::from_witness(in.get_context(), hi_v);
     lo.add_two(hi * (uint256_t(1) << LO_BITS), -in).assert_equal(0);
 
     // We need to manually propagate the origin tag
     lo.set_origin_tag(in.get_origin_tag());
     hi.set_origin_tag(in.get_origin_tag());
 
-    cycle_scalar result{ lo, hi, NUM_BITS, skip_primality_test, true };
-    return result;
+    return cycle_scalar{ lo, hi, NUM_BITS, skip_primality_test, use_bn254_scalar_field_for_primality_test };
 }
 /**
  * @brief Construct a new cycle scalar from a bigfield _value, over the same ScalarField Field. If  _value is a witness,

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_scalar.hpp

@@ -52,7 +52,6 @@ template <typename Builder> class cycle_scalar {
     // we want to validate the cycle_scalar < bn254::fr::modulus *not* grumpkin::fr::modulus
     bool _use_bn254_scalar_field_for_primality_test = false;
 
-  public:
     cycle_scalar(const field_t& _lo,
                  const field_t& _hi,
                  const size_t bits,
@@ -63,12 +62,16 @@ template <typename Builder> class cycle_scalar {
         , _num_bits(bits)
         , _skip_primality_test(skip_primality_test)
         , _use_bn254_scalar_field_for_primality_test(use_bn254_scalar_field_for_primality_test) {};
+
+  public:
+    // AUDITTODO: this is used only in the fuzzer.
     cycle_scalar(const ScalarField& _in = 0);
     cycle_scalar(const field_t& _lo, const field_t& _hi);
-    cycle_scalar(const field_t& _in);
+    // AUDITTODO: this is used only in the fuzzer. Its not inherently problematic, but perhaps the fuzzer should use a
+    // production entrypoint.
     static cycle_scalar from_witness(Builder* context, const ScalarField& value);
     static cycle_scalar from_witness_bitstring(Builder* context, const uint256_t& bitstring, size_t num_bits);
-    static cycle_scalar create_from_bn254_scalar(const field_t& _in, bool skip_primality_test = false);
+    static cycle_scalar create_from_bn254_scalar(const field_t& _in);
     [[nodiscard]] bool is_constant() const;
     ScalarField get_value() const;
     Builder* get_context() const { return lo.get_context() != nullptr ? lo.get_context() : hi.get_context(); }
@@ -115,4 +118,4 @@ template <typename Builder> class cycle_scalar {
     }
 };
 
-} // namespace bb::stdlib
+} // namespace bb::stdlib