From 95142d58182baf64d3b7485ed14d375a413dcaf8 Mon Sep 17 00:00:00 2001 From: zac-williamson Date: Thu, 5 Sep 2024 18:52:48 +0000 Subject: [PATCH] comments, changes --- .../{biggroup => bigfield}/goblin_field.hpp | 26 +++++++--- .../stdlib/primitives/biggroup/biggroup.hpp | 23 ++++---- .../primitives/biggroup/biggroup_goblin.hpp | 52 +++++++------------ .../primitives/field/field_conversion.hpp | 29 +++-------- 4 files changed, 58 insertions(+), 72 deletions(-) rename barretenberg/cpp/src/barretenberg/stdlib/primitives/{biggroup => bigfield}/goblin_field.hpp (69%) diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/goblin_field.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/goblin_field.hpp similarity index 69% rename from barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/goblin_field.hpp rename to barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/goblin_field.hpp index 22ffa1ac582..b1d57a479b1 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/goblin_field.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/goblin_field.hpp @@ -4,6 +4,20 @@ #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 goblin_field { public: static constexpr uint1024_t DEFAULT_MAXIMUM_REMAINDER = @@ -16,10 +30,10 @@ template class goblin_field { using bool_ct = stdlib::bool_t; std::array 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)); @@ -29,8 +43,8 @@ template class goblin_field { goblin_field(bb::fq input) { uint256_t converted(input); - uint256_t lo_v = converted.slice(0, 136); - uint256_t hi_v = converted.slice(136, 254); + uint256_t lo_v = converted.slice(0, NUM_LIMBS * 2); + uint256_t hi_v = converted.slice(NUM_LIMBS * 2, NUM_LIMBS * 3 + NUM_LAST_LIMB_BITS); limbs = { bb::fr(lo_v), bb::fr(hi_v) }; } goblin_field(field_ct lo, field_ct hi) @@ -40,7 +54,7 @@ template class goblin_field { // 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) << 68), hilo + hihi * (uint256_t(1) << 68) } + : limbs{ lolo + lohi * (uint256_t(1) << NUM_LIMBS), hilo + hihi * (uint256_t(1) << NUM_LIMB_BITS) } {} void assert_equal(const goblin_field& other) const @@ -53,8 +67,8 @@ template class goblin_field { static goblin_field from_witness(Builder* ctx, bb::fq input) { uint256_t converted(input); - uint256_t lo_v = converted.slice(0, 136); - uint256_t hi_v = converted.slice(136, 254); + uint256_t lo_v = converted.slice(0, NUM_LIMBS * 2); + uint256_t hi_v = converted.slice(NUM_LIMBS * 2, NUM_LIMBS * 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); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp index 8125da0e2c8..ff538386143 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp @@ -1,29 +1,17 @@ #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" #include "../memory/rom_table.hpp" #include "../memory/twin_rom_table.hpp" -#include "./goblin_field.hpp" #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. -namespace bb::stdlib { -template -concept IsNotGoblinInefficiencyTrap = !(IsMegaBuilder && std::same_as); - -template -concept IsGoblinBigGroup = - IsMegaBuilder && std::same_as> && - std::same_as> && std::same_as; -} // namespace bb::stdlib namespace bb::stdlib::element_default { // ( ͡° ͜ʖ ͡°) @@ -953,6 +941,15 @@ namespace bb::stdlib { template concept IsBigGroup = std::is_same_v; +template +concept IsGoblinBigGroup = + IsMegaBuilder && std::same_as> && + std::same_as> && std::same_as; + +/** + * @brief element wraps either element_default::element or element_goblin::goblin_element depending on parametrisation + * @details if C = MegaBuilder, G = bn254, Fq = bigfield, Fr = field_t then we're cooking + */ template using element = std::conditional_t, element_goblin::goblin_element, Fr, G>, diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp index 274ea94059a..96fa5b85997 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp @@ -1,24 +1,33 @@ #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" #include "../memory/rom_table.hpp" #include "../memory/twin_rom_table.hpp" -#include "./goblin_field.hpp" #include "barretenberg/ecc/curves/bn254/g1.hpp" #include "barretenberg/ecc/curves/secp256k1/secp256k1.hpp" #include "barretenberg/ecc/curves/secp256r1/secp256r1.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 concept IsNotGoblinInefficiencyTrap = -// !(IsMegaBuilder && std::same_as); namespace bb::stdlib::element_goblin { -// ( ͡° ͜ʖ ͡°) +/** + * @brief Custom element class for when using goblin + * @details When using goblin (builder = MEGA and element = bn254), the assumptions and heuristics + * we apply vary considerably to the "default" case, justifying a separate class + * (we use a `using` declaration to make `element` map to `goblin_element` if the correct parametrisation is + * used, see the `IsGoblinBigGroup` concept for details) Differences between goblin and regular biggroup elements: + * 1. state model is different (x/y coordinates are 2 136-bit field_t members instead of 4 68-bit field_t + * members) + * 2. on-curve checks are not applied in-circuit (they are applied in the ECCVM circuit) + * 3. we do not need to range-constrain the coordinates to be 136-bits (applied in the Translator circuit) + * @tparam Builder + * @tparam Fq + * @tparam Fr + * @tparam NativeGroup + */ template class goblin_element { public: using BaseField = Fq; @@ -36,18 +45,12 @@ template class goblin_ele , y(y) , _is_infinity(false) {} - goblin_element(const goblin_element& other) = default; goblin_element(goblin_element&& other) noexcept = default; goblin_element& operator=(const goblin_element& other) = default; - goblin_element& operator=(goblin_element&& other) = default; - // static goblin_element from_witness_unsafe(Builder* ctx, const typename NativeGroup::affine_element& input) - // { - // // only valid usecase of this method is with a goblin builder - // ASSERT(IsMegaBuilder); - // goblin_element out; - // if (in) - // } + goblin_element& operator=(goblin_element&& other) noexcept = default; + ~goblin_element() = default; + static goblin_element from_witness(Builder* ctx, const typename NativeGroup::affine_element& input) { goblin_element out; @@ -80,14 +83,6 @@ template class goblin_ele return goblin_element(x_fq, y_fq); } - // byte_array to_byte_array() const - // { - // byte_array result(get_context()); - // result.write(y.to_byte_array()); - // result.write(x.to_byte_array()); - // return result; - // } - goblin_element checked_unconditional_add(const goblin_element& other) const { return goblin_element::operator+(*this, other); @@ -99,21 +94,14 @@ template class goblin_ele goblin_element operator+(const goblin_element& other) const { - // TODO(https://github.com/AztecProtocol/barretenberg/issues/707) Optimize - // Current gate count: 6398 return batch_mul({ *this, other }, { Fr(1), Fr(1) }); } goblin_element operator-(const goblin_element& other) const { - // TODO(https://github.com/AztecProtocol/barretenberg/issues/707) Optimize std::vector points{ *this, other }; return batch_mul({ *this, other }, { Fr(1), -Fr(1) }); } - goblin_element operator-() const - { - // TODO(https://github.com/AztecProtocol/barretenberg/issues/707) Optimize - return batch_mul({ *this }, { -Fr(1) }); - } + goblin_element operator-() const { return batch_mul({ *this }, { -Fr(1) }); } goblin_element operator+=(const goblin_element& other) { *this = *this + other; diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp index 85cf813cf2d..6be2d2990bc 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp @@ -2,7 +2,7 @@ #include "barretenberg/polynomials/univariate.hpp" #include "barretenberg/stdlib/primitives/bigfield/bigfield.hpp" -#include "barretenberg/stdlib/primitives/biggroup/goblin_field.hpp" +#include "barretenberg/stdlib/primitives/bigfield/goblin_field.hpp" #include "barretenberg/stdlib/primitives/curves/bn254.hpp" #include "barretenberg/stdlib/primitives/field/field.hpp" #include "barretenberg/stdlib/primitives/group/cycle_group.hpp" @@ -107,28 +107,15 @@ template T convert_from_bn254_frs(Builder& builde result.y = convert_from_bn254_frs( builder, fr_vec.subspan(BASE_FIELD_SCALAR_SIZE, BASE_FIELD_SCALAR_SIZE)); - auto sum = fr_vec[0].add_two(fr_vec[1], fr_vec[2]); - sum = sum + fr_vec[3]; + // We have a convention that the group element is at infinity if both x/y coordinates are 0. + // We also know that all bn254 field elements are 136-bit scalars. + // Therefore we can do a cheap "iszero" check by checking the vector sum is 0 + fr sum; + for (size_t i = 0; i < BASE_FIELD_SCALAR_SIZE; i += 1) { + sum = sum.add_two(fr_vec[2 * i], fr_vec[2 * i + 1]); + } result.set_point_at_infinity(sum.is_zero()); return result; - - // we know that all Field elements are constrained to 136 bits. - // therefore, if we add them all together, the only way the sum is zero is if all are zero - // 19 gates to 6 gates - // is zero - - // A * is_zero = 0 - // (1 - is_zero) * (A * I - 1) = 0 - // AI - 1 - is_zero * AI + is_zero = 0 - - // AZ + AI - 1 - ZAI + Z = 0 - - // Z + I - ZI = t0 - // A * t0 + Z - 1 = 0 - // we can reduce to 3 gates (2.25 if we fix bool checks) - // result.set_point_at_infinity(fr_vec[0].is_zero() && fr_vec[1].is_zero() && fr_vec[2].is_zero() && - // fr_vec[3].is_zero()); - // return result; } else if constexpr (IsAnyOf>) { using BaseField = fr; constexpr size_t BASE_FIELD_SCALAR_SIZE = calc_num_bn254_frs();