Skip to content

Commit

Permalink
comments, changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@zac-williamson
zac-williamson committed Sep 5, 2024
1 parent 418e1fe commit 95142d5
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 72 deletions.
26 changes: 20 additions & 6 deletions ...dlib/primitives/biggroup/goblin_field.hpp → ...dlib/primitives/bigfield/goblin_field.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 Builder> class goblin_field {
public:
static constexpr uint1024_t DEFAULT_MAXIMUM_REMAINDER =
Expand All @@ -16,10 +30,10 @@ template <class Builder> class goblin_field {
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));
Expand All @@ -29,8 +43,8 @@ template <class Builder> 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)
Expand All @@ -40,7 +54,7 @@ template <class Builder> 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
Expand All @@ -53,8 +67,8 @@ template <class Builder> 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);
Expand Down
23 changes: 10 additions & 13 deletions barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp
View file
Original file line number Diff line number Diff line change
@@ -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 <typename Builder, typename NativeGroup>
concept IsNotGoblinInefficiencyTrap = !(IsMegaBuilder<Builder> && std::same_as<NativeGroup, bb::g1>);

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>;

} // namespace bb::stdlib
namespace bb::stdlib::element_default {

// ( ͡° ͜ʖ ͡°)
Expand Down Expand Up @@ -953,6 +941,15 @@ 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>,
Expand Down
52 changes: 20 additions & 32 deletions barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp
View file
Original file line number Diff line number Diff line change
@@ -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 <typename Builder, typename NativeGroup> concept IsNotGoblinInefficiencyTrap =
// !(IsMegaBuilder<Builder> && std::same_as<NativeGroup, bb::g1>);

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 Builder, class Fq, class Fr, class NativeGroup> class goblin_element {
public:
using BaseField = Fq;
Expand All @@ -36,18 +45,12 @@ template <class Builder, class Fq, class Fr, class NativeGroup> 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<Builder>);
// 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;
Expand Down Expand Up @@ -80,14 +83,6 @@ template <class Builder, class Fq, class Fr, class NativeGroup> class goblin_ele
return goblin_element(x_fq, y_fq);
}

// byte_array<Builder> to_byte_array() const
// {
// byte_array<Builder> 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);
Expand All @@ -99,21 +94,14 @@ template <class Builder, class Fq, class Fr, class NativeGroup> 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<goblin_element> 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;
Expand Down
29 changes: 8 additions & 21 deletions barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -107,28 +107,15 @@ template <typename Builder, typename T> T convert_from_bn254_frs(Builder& builde
result.y = convert_from_bn254_frs<Builder, BaseField>(
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<Builder> 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<T, grumpkin_element<Builder>>) {
using BaseField = fr<Builder>;
constexpr size_t BASE_FIELD_SCALAR_SIZE = calc_num_bn254_frs<Builder, BaseField>();
Expand Down

0 comments on commit 95142d5

Please sign in to comment.