From a9483cbee7d219fc9e9cec773da8fc3825211d5c Mon Sep 17 00:00:00 2001 From: codygunton Date: Fri, 30 Jun 2023 14:20:46 +0000 Subject: [PATCH 1/9] Add ECCOp and queue --- cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 4a810a37e8..742bc7d9db 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -114,4 +114,4 @@ class ECCOpQueue { } }; -} // namespace proof_system \ No newline at end of file +} // namespace proof_system From 072ffec2d2dd02f1b939ea324a16580a9c29ee52 Mon Sep 17 00:00:00 2001 From: Cody Gunton Date: Fri, 30 Jun 2023 18:30:09 +0200 Subject: [PATCH 2/9] feat: Add new ops, change curve, make const, document. (#568) --- cpp/.aztec-packages-commit | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/.aztec-packages-commit b/cpp/.aztec-packages-commit index 1f7391f92b..95a09114c0 100644 --- a/cpp/.aztec-packages-commit +++ b/cpp/.aztec-packages-commit @@ -1 +1 @@ -master +master From 95a124425952c03081bb3953cda8cfe0edeb093e Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Tue, 4 Jul 2023 13:12:01 +0000 Subject: [PATCH 3/9] basic ultragob builder functionality in place --- .../goblin_ultra_circuit_builder.cpp | 105 ++++++++++++++++++ .../goblin_ultra_circuit_builder.hpp | 40 +++++++ .../goblin_ultra_circuit_builder.test.cpp | 55 +++++++++ .../proof_system/op_queue/ecc_op_queue.hpp | 12 ++ 4 files changed, 212 insertions(+) create mode 100644 cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp create mode 100644 cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp create mode 100644 cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp new file mode 100644 index 0000000000..443e851124 --- /dev/null +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp @@ -0,0 +1,105 @@ +/** + * @file ultra_circuit_builder.cpp + * @author Luke (ledwards2225) and Kesha (Rumata888) + * @brief This file contains the implementation of UltraCircuitBuilder class that defines the logic of ultra-style + * circuits and is intended for the use in UltraHonk and UltraPlonk systems + * + * @todo 1) Replace barretenberg::fr with templated FF or Field + * + */ +#include "goblin_ultra_circuit_builder.hpp" +#include +#include +#include + +using namespace barretenberg; + +namespace proof_system { + +// WORKTODO: do we need diff methods for diff ops? maybe not + +void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affine_element& point) +{ + // Add raw op to queue + op_queue.add_accumulate(point); + + // Add ecc op gates + uint32_t op = 0; // WORKTODO: how to specify these values + add_ecc_op_gates(op, point); +} + +void GoblinUltraCircuitBuilder::queue_ecc_mul_accum(const barretenberg::g1::affine_element& point, + const barretenberg::fr& scalar) +{ + // Add raw op to op queue + op_queue.mul_accumulate(point, scalar); + + // Add ecc op gates + uint32_t op = 1; // WORKTODO: Enum? + add_ecc_op_gates(op, point, scalar); +} + +void GoblinUltraCircuitBuilder::queue_ecc_eq(const barretenberg::g1::affine_element& point) +{ + // Add raw op to op queue + op_queue.eq(point); + + // Add ecc op gates + uint32_t op = 2; + add_ecc_op_gates(op, point); +} + +void GoblinUltraCircuitBuilder::add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar) +{ + auto op_tuple = make_ecc_op_tuple(op, point, scalar); + + queue_ecc_op(op_tuple); +} + +ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, + const g1::affine_element& point, + const fr& scalar) +{ + auto x_256 = uint256_t(point.x); + auto y_256 = uint256_t(point.y); + auto x_lo_idx = add_variable(x_256.slice(0, NUM_LIMB_BITS * 2)); + auto x_hi_idx = add_variable(x_256.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 4)); + auto y_lo_idx = add_variable(y_256.slice(0, NUM_LIMB_BITS * 2)); + auto y_hi_idx = add_variable(y_256.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 4)); + + // Split scalar into 128 bit endomorphism scalars + fr z_1 = 0; + fr z_2 = 0; + // WORKTODO: do I need to do this montgomery conversion here? + // auto converted = scalar.from_montgomery_form(); + // fr::split_into_endomorphism_scalars(converted, z_1, z_2); + // z_1 = z_1.to_montgomery_form(); + // z_2 = z_2.to_montgomery_form(); + fr::split_into_endomorphism_scalars(scalar, z_1, z_2); + auto z_1_idx = add_variable(z_1); + auto z_2_idx = add_variable(z_2); + + return { op, x_lo_idx, x_hi_idx, y_lo_idx, y_hi_idx, z_1_idx, z_2_idx }; +} + +/** + * @brief Add ecc operation to queue + * + * @param in Variables array indices corresponding to operation inputs + * @note We dont increment the gate count here since these will simply be placed at the zeroth gate index inside a block + * of predetermined size + */ +void GoblinUltraCircuitBuilder::queue_ecc_op(const ecc_op_tuple& in) +{ + op_witness_1.emplace_back(in.op); + op_witness_2.emplace_back(in.x_lo); + op_witness_3.emplace_back(in.x_hi); + op_witness_4.emplace_back(in.y_lo); + + op_witness_1.emplace_back(in.op); // WORKTODO: op? 0? something else? + op_witness_2.emplace_back(in.y_hi); + op_witness_3.emplace_back(in.z_lo); + op_witness_4.emplace_back(in.z_hi); +}; + +} // namespace proof_system \ No newline at end of file diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp new file mode 100644 index 0000000000..f2137eee56 --- /dev/null +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp @@ -0,0 +1,40 @@ +#pragma once +#include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" +#include "barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp" + +namespace proof_system { + +using namespace barretenberg; + +class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { + public: + static constexpr std::string_view NAME_STRING = "GoblinUltraArithmetization"; + static constexpr CircuitType CIRCUIT_TYPE = CircuitType::ULTRA; + + const size_t NUM_LIMB_BITS = 68; + + ECCOpQueue op_queue; // WORKTODO: TBD. + + // WORKTODO: same as WireVector, probably dont need + using OpWitnessVector = std::vector>; + std::array op_witness; + + OpWitnessVector& op_witness_1 = std::get<0>(op_witness); + OpWitnessVector& op_witness_2 = std::get<1>(op_witness); + OpWitnessVector& op_witness_3 = std::get<2>(op_witness); + OpWitnessVector& op_witness_4 = std::get<3>(op_witness); + + /** + * ECC operations + **/ + + void queue_ecc_add_accum(const g1::affine_element& point); + void queue_ecc_mul_accum(const g1::affine_element& point, const fr& scalar); + void queue_ecc_eq(const g1::affine_element& point); + + private: + void queue_ecc_op(const ecc_op_tuple& in); + void add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); + ecc_op_tuple make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); +}; +} // namespace proof_system diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp new file mode 100644 index 0000000000..b465febe20 --- /dev/null +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -0,0 +1,55 @@ +#include "goblin_ultra_circuit_builder.hpp" +#include "barretenberg/crypto/generators/generator_data.hpp" +#include + +using namespace barretenberg; + +namespace { +auto& engine = numeric::random::get_debug_engine(); +} +namespace proof_system { + +TEST(goblin_ultra_circuit_constructor, base_case) +{ + auto circuit_constructor = GoblinUltraCircuitBuilder(); + fr a = fr::random_element(); + fr b = fr::random_element(); + + auto a_idx = circuit_constructor.add_variable(a); + auto b_idx = circuit_constructor.add_variable(b); + auto c_idx = circuit_constructor.add_variable(a + b); + + circuit_constructor.create_add_gate({ a_idx, b_idx, c_idx, fr::one(), fr::one(), -fr::one(), fr::zero() }); + + circuit_constructor.get_num_gates(); + + EXPECT_EQ(circuit_constructor.check_circuit(), true); +} + +TEST(goblin_ultra_circuit_constructor, simple) +{ + auto builder = GoblinUltraCircuitBuilder(); + + // Compute a simple point accumulation natively + auto P1 = g1::affine_element::random_element(); + auto P2 = g1::affine_element::random_element(); + auto z = fr::random_element(); + auto P_expected = P1 + P2 * z; + + // Add gates corresponding to the above operations + builder.queue_ecc_add_accum(P1); + builder.queue_ecc_mul_accum(P2, z); + + // Extract current accumulator point from the op queue and check the result + auto P_result = builder.op_queue.get_accumulator(); + EXPECT_EQ(P_result, P_expected); + + // Use the result to add equality op gates + builder.queue_ecc_eq(P_result); + + // Check that the accumulator in the op queue has been reset + auto accumulator = builder.op_queue.get_accumulator(); + EXPECT_EQ(accumulator, g1::affine_point_at_infinity); +} + +} // namespace proof_system \ No newline at end of file diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 742bc7d9db..f37e31c627 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -31,6 +31,9 @@ class ECCOpQueue { Point point_at_infinity = curve::BN254::Group::affine_point_at_infinity; using Fr = curve::BN254::ScalarField; using Fq = curve::BN254::BaseField; // Grumpkin's scalar field + + Point accumulator = point_at_infinity; // current accumulator point + public: std::vector raw_ops; std::vector> ultra_ops; @@ -52,8 +55,13 @@ class ECCOpQueue { return num_muls; } + Point get_accumulator() { return accumulator; } + void add_accumulate(const Point& to_add) { + accumulator.self_set_infinity(); + accumulator = accumulator + to_add; + raw_ops.emplace_back(ECCOp{ .add = true, .mul = false, @@ -68,6 +76,8 @@ class ECCOpQueue { void mul_accumulate(const Point& to_mul, const Fr& scalar) { + accumulator = accumulator + to_mul * scalar; + Fr scalar_1 = 0; Fr scalar_2 = 0; auto converted = scalar.from_montgomery_form(); @@ -87,6 +97,8 @@ class ECCOpQueue { } void eq(const Point& expected) { + accumulator.self_set_infinity(); // WORKTODO: is this always desired? + raw_ops.emplace_back(ECCOp{ .add = false, .mul = false, From e6e2139a0b473e993df0f726f9bbc40c0515c47e Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Tue, 4 Jul 2023 14:12:27 +0000 Subject: [PATCH 4/9] simple batch mul --- .../goblin_ultra_circuit_builder.cpp | 51 ++++++++++++++++++- .../goblin_ultra_circuit_builder.hpp | 1 + .../goblin_ultra_circuit_builder.test.cpp | 25 +++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp index 443e851124..d437d05b7f 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp @@ -16,8 +16,31 @@ using namespace barretenberg; namespace proof_system { -// WORKTODO: do we need diff methods for diff ops? maybe not +/** + * @brief Add gates corresponding to a batched mul + * + * @param points + * @param scalars + * @return g1::affine_element Result of batched mul + */ +g1::affine_element GoblinUltraCircuitBuilder::batch_mul(const std::vector& points, + const std::vector& scalars) +{ + // WORKTODO: This may need some checks, e.g. + ASSERT(op_queue.get_accumulator().is_point_at_infinity()); + + size_t num_muls = points.size(); + for (size_t idx = 0; idx < num_muls; ++idx) { + queue_ecc_mul_accum(points[idx], scalars[idx]); + } + return op_queue.get_accumulator(); +} +/** + * @brief Add gates for simple point addition without scalar and compute corresponding op natively + * + * @param point + */ void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affine_element& point) { // Add raw op to queue @@ -28,6 +51,12 @@ void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affi add_ecc_op_gates(op, point); } +/** + * @brief Add gates for point mul and add and compute corresponding op natively + * + * @param point + * @param scalar + */ void GoblinUltraCircuitBuilder::queue_ecc_mul_accum(const barretenberg::g1::affine_element& point, const barretenberg::fr& scalar) { @@ -39,6 +68,11 @@ void GoblinUltraCircuitBuilder::queue_ecc_mul_accum(const barretenberg::g1::affi add_ecc_op_gates(op, point, scalar); } +/** + * @brief Add point equality gates + * + * @param point + */ void GoblinUltraCircuitBuilder::queue_ecc_eq(const barretenberg::g1::affine_element& point) { // Add raw op to op queue @@ -49,6 +83,13 @@ void GoblinUltraCircuitBuilder::queue_ecc_eq(const barretenberg::g1::affine_elem add_ecc_op_gates(op, point); } +/** + * @brief Add ecc op gates for specified operation + * + * @param op Op code + * @param point + * @param scalar + */ void GoblinUltraCircuitBuilder::add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar) { auto op_tuple = make_ecc_op_tuple(op, point, scalar); @@ -56,6 +97,14 @@ void GoblinUltraCircuitBuilder::add_ecc_op_gates(uint32_t op, const g1::affine_e queue_ecc_op(op_tuple); } +/** + * @brief Decompose ecc operands into components, add corresponding variables, return ecc op index tuple + * + * @param op + * @param point + * @param scalar + * @return ecc_op_tuple Tuple of indices into variables array used to construct pair of ecc op gates + */ ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar) diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp index f2137eee56..2be43c505c 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp @@ -31,6 +31,7 @@ class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { void queue_ecc_add_accum(const g1::affine_element& point); void queue_ecc_mul_accum(const g1::affine_element& point, const fr& scalar); void queue_ecc_eq(const g1::affine_element& point); + g1::affine_element batch_mul(const std::vector& points, const std::vector& scalars); private: void queue_ecc_op(const ecc_op_tuple& in); diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp index b465febe20..dc34903926 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -52,4 +52,29 @@ TEST(goblin_ultra_circuit_constructor, simple) EXPECT_EQ(accumulator, g1::affine_point_at_infinity); } +TEST(goblin_ultra_circuit_constructor, batch_mul) +{ + using Point = g1::affine_element; + using Scalar = fr; + + auto builder = GoblinUltraCircuitBuilder(); + const size_t num_muls = 3; + + // Compute some points and scalars + std::vector points; + std::vector scalars; + auto batched_expected = Point::infinity(); + for (size_t i = 0; i < num_muls; ++i) { + points.emplace_back(Point::random_element()); + scalars.emplace_back(Scalar::random_element()); + batched_expected = batched_expected + points[i] * scalars[i]; + } + + // Add gates corresponding to batch mul + auto batched_result = builder.batch_mul(points, scalars); + + // Extract current accumulator point from the op queue and check the result + EXPECT_EQ(batched_result, batched_expected); +} + } // namespace proof_system \ No newline at end of file From d6606a11d0ed0f3a7fbe0486a8db790191cefd43 Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Mon, 17 Jul 2023 20:51:56 +0000 Subject: [PATCH 5/9] minor updates after rebase --- .../arithmetization/gate_data.hpp | 10 ++++++++ .../goblin_ultra_circuit_builder.cpp | 24 ++++++++++--------- .../goblin_ultra_circuit_builder.hpp | 18 ++++++++------ .../goblin_ultra_circuit_builder.test.cpp | 24 ++++++++++++++----- .../proof_system/op_queue/ecc_op_queue.hpp | 2 +- 5 files changed, 53 insertions(+), 25 deletions(-) diff --git a/cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp b/cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp index f47a033838..ba60a0dd0e 100644 --- a/cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp +++ b/cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp @@ -70,6 +70,16 @@ template struct poly_triple_ { using poly_triple = poly_triple_; +struct ecc_op_tuple { + uint32_t op; + uint32_t x_lo; + uint32_t x_hi; + uint32_t y_lo; + uint32_t y_hi; + uint32_t z_lo; + uint32_t z_hi; +}; + template inline void read(B& buf, poly_triple& constraint) { using serialize::read; diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp index d437d05b7f..4045c051c8 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp @@ -9,8 +9,8 @@ */ #include "goblin_ultra_circuit_builder.hpp" #include -#include #include +#include using namespace barretenberg; @@ -47,7 +47,7 @@ void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affi op_queue.add_accumulate(point); // Add ecc op gates - uint32_t op = 0; // WORKTODO: how to specify these values + uint32_t op = 0; // WORKTODO: how to specify these values. ENUM? add_ecc_op_gates(op, point); } @@ -140,15 +140,17 @@ ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, */ void GoblinUltraCircuitBuilder::queue_ecc_op(const ecc_op_tuple& in) { - op_witness_1.emplace_back(in.op); - op_witness_2.emplace_back(in.x_lo); - op_witness_3.emplace_back(in.x_hi); - op_witness_4.emplace_back(in.y_lo); - - op_witness_1.emplace_back(in.op); // WORKTODO: op? 0? something else? - op_witness_2.emplace_back(in.y_hi); - op_witness_3.emplace_back(in.z_lo); - op_witness_4.emplace_back(in.z_hi); + op_wire_1.emplace_back(in.op); + op_wire_2.emplace_back(in.x_lo); + op_wire_3.emplace_back(in.x_hi); + op_wire_4.emplace_back(in.y_lo); + + op_wire_1.emplace_back(in.op); // WORKTODO: sort of a dummy. is "op" ok? + op_wire_2.emplace_back(in.y_hi); + op_wire_3.emplace_back(in.z_lo); + op_wire_4.emplace_back(in.z_hi); + + num_ecc_op_gates += 2; }; } // namespace proof_system \ No newline at end of file diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp index 2be43c505c..2f384efe9a 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp @@ -1,6 +1,6 @@ #pragma once -#include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" #include "barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp" +#include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" namespace proof_system { @@ -11,18 +11,22 @@ class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { static constexpr std::string_view NAME_STRING = "GoblinUltraArithmetization"; static constexpr CircuitType CIRCUIT_TYPE = CircuitType::ULTRA; - const size_t NUM_LIMB_BITS = 68; + // Used for simulating big field; Equal to NUM_LIMB_BITS_IN_FIELD_SIMULATION + const size_t NUM_LIMB_BITS = 68; // WORKTODO: Set via NUM_LIMB_BITS_IN_FIELD_SIMULATION? ECCOpQueue op_queue; // WORKTODO: TBD. + // number of ecc op "gates"; these are placed at the start of the circuit + int num_ecc_op_gates = 0; + // WORKTODO: same as WireVector, probably dont need using OpWitnessVector = std::vector>; - std::array op_witness; + std::array op_wires; - OpWitnessVector& op_witness_1 = std::get<0>(op_witness); - OpWitnessVector& op_witness_2 = std::get<1>(op_witness); - OpWitnessVector& op_witness_3 = std::get<2>(op_witness); - OpWitnessVector& op_witness_4 = std::get<3>(op_witness); + OpWitnessVector& op_wire_1 = std::get<0>(op_wires); + OpWitnessVector& op_wire_2 = std::get<1>(op_wires); + OpWitnessVector& op_wire_3 = std::get<2>(op_wires); + OpWitnessVector& op_wire_4 = std::get<3>(op_wires); /** * ECC operations diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp index dc34903926..8007943326 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -1,5 +1,5 @@ -#include "goblin_ultra_circuit_builder.hpp" #include "barretenberg/crypto/generators/generator_data.hpp" +#include "goblin_ultra_circuit_builder.hpp" #include using namespace barretenberg; @@ -9,7 +9,11 @@ auto& engine = numeric::random::get_debug_engine(); } namespace proof_system { -TEST(goblin_ultra_circuit_constructor, base_case) +/** + * @brief Simple add gate to check basic Ultra functionality + * + */ +TEST(GoblinCircuitBuilder, BaseCase) { auto circuit_constructor = GoblinUltraCircuitBuilder(); fr a = fr::random_element(); @@ -26,7 +30,11 @@ TEST(goblin_ultra_circuit_constructor, base_case) EXPECT_EQ(circuit_constructor.check_circuit(), true); } -TEST(goblin_ultra_circuit_constructor, simple) +/** + * @brief Test correctness of native ecc computations performed behind the scenes when adding simple ecc op gates + * + */ +TEST(GoblinCircuitBuilder, Simple) { auto builder = GoblinUltraCircuitBuilder(); @@ -52,7 +60,11 @@ TEST(goblin_ultra_circuit_constructor, simple) EXPECT_EQ(accumulator, g1::affine_point_at_infinity); } -TEST(goblin_ultra_circuit_constructor, batch_mul) +/** + * @brief Test correctness of native ecc batch mul performed behind the scenes when adding ecc op gates for a batch mul + * + */ +TEST(GoblinCircuitBuilder, BatchMul) { using Point = g1::affine_element; using Scalar = fr; @@ -60,7 +72,7 @@ TEST(goblin_ultra_circuit_constructor, batch_mul) auto builder = GoblinUltraCircuitBuilder(); const size_t num_muls = 3; - // Compute some points and scalars + // Compute some random points and scalars to batch multiply std::vector points; std::vector scalars; auto batched_expected = Point::infinity(); @@ -70,7 +82,7 @@ TEST(goblin_ultra_circuit_constructor, batch_mul) batched_expected = batched_expected + points[i] * scalars[i]; } - // Add gates corresponding to batch mul + // Populate the batch mul operands in the op wires and natively compute the result auto batched_result = builder.batch_mul(points, scalars); // Extract current accumulator point from the op queue and check the result diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index f37e31c627..48bff78581 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -59,7 +59,7 @@ class ECCOpQueue { void add_accumulate(const Point& to_add) { - accumulator.self_set_infinity(); + accumulator.self_set_infinity(); // WORKTODO: this shouldnt be here should it? accumulator = accumulator + to_add; raw_ops.emplace_back(ECCOp{ From 89af2a3c207ae606468121e279c2a5293a8e2114 Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Mon, 17 Jul 2023 21:21:00 +0000 Subject: [PATCH 6/9] Updates to ecc op queue plus a test --- cpp/.aztec-packages-commit | 2 +- .../proof_system/op_queue/ecc_op_queue.hpp | 36 ++++++++++++++++--- .../op_queue/ecc_op_queue.test.cpp | 28 ++++++++++++++- 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/cpp/.aztec-packages-commit b/cpp/.aztec-packages-commit index 95a09114c0..1f7391f92b 100644 --- a/cpp/.aztec-packages-commit +++ b/cpp/.aztec-packages-commit @@ -1 +1 @@ -master +master diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 48bff78581..84fd6adb9b 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -32,7 +32,8 @@ class ECCOpQueue { using Fr = curve::BN254::ScalarField; using Fq = curve::BN254::BaseField; // Grumpkin's scalar field - Point accumulator = point_at_infinity; // current accumulator point + // The operations written to the queue are also performed natively; the result is stored in accumulator + Point accumulator = point_at_infinity; public: std::vector raw_ops; @@ -57,11 +58,17 @@ class ECCOpQueue { Point get_accumulator() { return accumulator; } + /** + * @brief Write point addition op to queue and natively perform addition + * + * @param to_add + */ void add_accumulate(const Point& to_add) { - accumulator.self_set_infinity(); // WORKTODO: this shouldnt be here should it? + // Update the accumulator natively accumulator = accumulator + to_add; + // Store the operation raw_ops.emplace_back(ECCOp{ .add = true, .mul = false, @@ -74,10 +81,17 @@ class ECCOpQueue { }); } + /** + * @brief Write multiply and add op to queue and natively perform operation + * + * @param to_add + */ void mul_accumulate(const Point& to_mul, const Fr& scalar) { + // Update the accumulator natively accumulator = accumulator + to_mul * scalar; + // Store the operation Fr scalar_1 = 0; Fr scalar_2 = 0; auto converted = scalar.from_montgomery_form(); @@ -95,9 +109,17 @@ class ECCOpQueue { .mul_scalar_full = scalar, }); } - void eq(const Point& expected) + + /** + * @brief Write equality op to queue and check equality with native accumulator + * + * @param expected + * @return whether accumulator point == expected point + */ + bool eq(const Point& expected) { - accumulator.self_set_infinity(); // WORKTODO: is this always desired? + bool accumulator_equals_expected = (accumulator == expected); + accumulator.self_set_infinity(); // TODO(luke): is this always desired? raw_ops.emplace_back(ECCOp{ .add = false, @@ -109,8 +131,14 @@ class ECCOpQueue { .scalar_2 = 0, .mul_scalar_full = 0, }); + + return accumulator_equals_expected; } + /** + * @brief Write empty row to queue + * + */ void empty_row() { raw_ops.emplace_back(ECCOp{ diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp index 7b1c160eb6..96b693355f 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp @@ -1,5 +1,5 @@ -#include #include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" +#include namespace proof_system::test_flavor { TEST(ECCOpQueueTest, Basic) @@ -11,4 +11,30 @@ TEST(ECCOpQueueTest, Basic) EXPECT_EQ(op_queue.raw_ops[1].add, false); } +TEST(ECCOpQueueTest, Accumulator) +{ + using point = barretenberg::g1::affine_element; + using scalar = barretenberg::fr; + + // Compute a simple point accumulation natively + auto P1 = point::random_element(); + auto P2 = point::random_element(); + auto z = scalar::random_element(); + auto P_expected = P1 + P2 * z; + + // Add the same operations to the ECC op queue; the native computation is performed under the hood. + ECCOpQueue op_queue; + op_queue.add_accumulate(P1); + op_queue.mul_accumulate(P2, z); + + // The correct result should now be stored in the accumulator within the op queue + EXPECT_EQ(op_queue.get_accumulator(), P_expected); + + // Equivalently, we can check that the equality op returns true + EXPECT_TRUE(op_queue.eq(P_expected)); + + // Adding an equality op should reset the accumulator to zero (the point at infinity) + EXPECT_TRUE(op_queue.get_accumulator().is_point_at_infinity()); +} + } // namespace proof_system::test_flavor From e0867382cf0504eb6ab657352373c9f1d0b78b24 Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Tue, 18 Jul 2023 17:29:20 +0000 Subject: [PATCH 7/9] add op wire checks to builder tests --- .../goblin_ultra_circuit_builder.cpp | 37 ++++++--------- .../goblin_ultra_circuit_builder.hpp | 26 ++++++----- .../goblin_ultra_circuit_builder.test.cpp | 46 ++++++++++++++++++- .../proof_system/op_queue/ecc_op_queue.hpp | 2 + 4 files changed, 74 insertions(+), 37 deletions(-) diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp index 4045c051c8..a0f808ee97 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp @@ -1,12 +1,3 @@ -/** - * @file ultra_circuit_builder.cpp - * @author Luke (ledwards2225) and Kesha (Rumata888) - * @brief This file contains the implementation of UltraCircuitBuilder class that defines the logic of ultra-style - * circuits and is intended for the use in UltraHonk and UltraPlonk systems - * - * @todo 1) Replace barretenberg::fr with templated FF or Field - * - */ #include "goblin_ultra_circuit_builder.hpp" #include #include @@ -26,7 +17,7 @@ namespace proof_system { g1::affine_element GoblinUltraCircuitBuilder::batch_mul(const std::vector& points, const std::vector& scalars) { - // WORKTODO: This may need some checks, e.g. + // TODO(luke): Do we necessarily want to check accum == 0? Other checks? ASSERT(op_queue.get_accumulator().is_point_at_infinity()); size_t num_muls = points.size(); @@ -47,8 +38,7 @@ void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affi op_queue.add_accumulate(point); // Add ecc op gates - uint32_t op = 0; // WORKTODO: how to specify these values. ENUM? - add_ecc_op_gates(op, point); + add_ecc_op_gates(EccOpCode::ADD_ACCUM, point); } /** @@ -64,8 +54,7 @@ void GoblinUltraCircuitBuilder::queue_ecc_mul_accum(const barretenberg::g1::affi op_queue.mul_accumulate(point, scalar); // Add ecc op gates - uint32_t op = 1; // WORKTODO: Enum? - add_ecc_op_gates(op, point, scalar); + add_ecc_op_gates(EccOpCode::MUL_ACCUM, point, scalar); } /** @@ -79,12 +68,11 @@ void GoblinUltraCircuitBuilder::queue_ecc_eq(const barretenberg::g1::affine_elem op_queue.eq(point); // Add ecc op gates - uint32_t op = 2; - add_ecc_op_gates(op, point); + add_ecc_op_gates(EccOpCode::EQUALITY, point); } /** - * @brief Add ecc op gates for specified operation + * @brief Add ecc op gates given an op code and its operands * * @param op Op code * @param point @@ -94,11 +82,11 @@ void GoblinUltraCircuitBuilder::add_ecc_op_gates(uint32_t op, const g1::affine_e { auto op_tuple = make_ecc_op_tuple(op, point, scalar); - queue_ecc_op(op_tuple); + record_ecc_op(op_tuple); } /** - * @brief Decompose ecc operands into components, add corresponding variables, return ecc op index tuple + * @brief Decompose ecc operands into components, add corresponding variables, return ecc op tuple * * @param op * @param point @@ -119,7 +107,7 @@ ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, // Split scalar into 128 bit endomorphism scalars fr z_1 = 0; fr z_2 = 0; - // WORKTODO: do I need to do this montgomery conversion here? + // TODO(luke): do this montgomery conversion here? // auto converted = scalar.from_montgomery_form(); // fr::split_into_endomorphism_scalars(converted, z_1, z_2); // z_1 = z_1.to_montgomery_form(); @@ -135,17 +123,18 @@ ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, * @brief Add ecc operation to queue * * @param in Variables array indices corresponding to operation inputs - * @note We dont increment the gate count here since these will simply be placed at the zeroth gate index inside a block - * of predetermined size + * @note We dont explicitly set values for the selectors here since their values are fully determined by + * num_ecc_op_gates. E.g. in the composer we can reconstruct q_ecc_op as the indicator on the first num_ecc_op_gates + * indices. All other selectors are simply 0 on this domain. */ -void GoblinUltraCircuitBuilder::queue_ecc_op(const ecc_op_tuple& in) +void GoblinUltraCircuitBuilder::record_ecc_op(const ecc_op_tuple& in) { op_wire_1.emplace_back(in.op); op_wire_2.emplace_back(in.x_lo); op_wire_3.emplace_back(in.x_hi); op_wire_4.emplace_back(in.y_lo); - op_wire_1.emplace_back(in.op); // WORKTODO: sort of a dummy. is "op" ok? + op_wire_1.emplace_back(in.op); // TODO(luke): second op val is sort of a dummy. use "op" again? op_wire_2.emplace_back(in.y_hi); op_wire_3.emplace_back(in.z_lo); op_wire_4.emplace_back(in.z_hi); diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp index 2f384efe9a..7cce13cb1d 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp @@ -6,27 +6,31 @@ namespace proof_system { using namespace barretenberg; +// TODO(luke): The Goblin builder requires all Ultra functionality plus a bit more. For now I've implented it as a +// standalone class that inherits from UltraBuilder. This is nice because it clearly separates out the "Goblin" +// functionlity, however, it is a break from our usual pattern. I may eventually simply incorporate the new +// functionality directly into the UltraBuilder. class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { public: static constexpr std::string_view NAME_STRING = "GoblinUltraArithmetization"; static constexpr CircuitType CIRCUIT_TYPE = CircuitType::ULTRA; // Used for simulating big field; Equal to NUM_LIMB_BITS_IN_FIELD_SIMULATION - const size_t NUM_LIMB_BITS = 68; // WORKTODO: Set via NUM_LIMB_BITS_IN_FIELD_SIMULATION? + const size_t NUM_LIMB_BITS = 68; // TODO(luke): Set via NUM_LIMB_BITS_IN_FIELD_SIMULATION? - ECCOpQueue op_queue; // WORKTODO: TBD. + // Stores record of ecc operations and performs corresponding native operations internally + ECCOpQueue op_queue; - // number of ecc op "gates"; these are placed at the start of the circuit + // number of ecc op "gates" (rows); these are placed at the start of the circuit int num_ecc_op_gates = 0; - // WORKTODO: same as WireVector, probably dont need - using OpWitnessVector = std::vector>; - std::array op_wires; + // Wires storing ecc op queue data; values are indices into the variables array + std::array op_wires; - OpWitnessVector& op_wire_1 = std::get<0>(op_wires); - OpWitnessVector& op_wire_2 = std::get<1>(op_wires); - OpWitnessVector& op_wire_3 = std::get<2>(op_wires); - OpWitnessVector& op_wire_4 = std::get<3>(op_wires); + WireVector& op_wire_1 = std::get<0>(op_wires); + WireVector& op_wire_2 = std::get<1>(op_wires); + WireVector& op_wire_3 = std::get<2>(op_wires); + WireVector& op_wire_4 = std::get<3>(op_wires); /** * ECC operations @@ -38,7 +42,7 @@ class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { g1::affine_element batch_mul(const std::vector& points, const std::vector& scalars); private: - void queue_ecc_op(const ecc_op_tuple& in); + void record_ecc_op(const ecc_op_tuple& in); void add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); ecc_op_tuple make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); }; diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp index 8007943326..54e00cca74 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -31,7 +31,10 @@ TEST(GoblinCircuitBuilder, BaseCase) } /** - * @brief Test correctness of native ecc computations performed behind the scenes when adding simple ecc op gates + * @brief Test the queueing of simple ecc ops via the Goblin builder + * @details There are two things to check here: 1) When ecc ops are queued by the builder, the corresponding native + * operations are performed correctly by the internal ecc op queue, and 2) The ecc op gate operands are correctly + * encoded in the op_wires, i.e. the operands can be reconstructed as expected. * */ TEST(GoblinCircuitBuilder, Simple) @@ -55,9 +58,48 @@ TEST(GoblinCircuitBuilder, Simple) // Use the result to add equality op gates builder.queue_ecc_eq(P_result); - // Check that the accumulator in the op queue has been reset + // Check that the accumulator in the op queue has been reset to 0 auto accumulator = builder.op_queue.get_accumulator(); EXPECT_EQ(accumulator, g1::affine_point_at_infinity); + + // Check number of ecc op "gates"/rows = 3 ops * 2 rows per op = 6 + EXPECT_EQ(builder.num_ecc_op_gates, 6); + + // Check that the expected op codes have been correctly recorded in the 1st op wire + EXPECT_EQ(builder.op_wire_1[0], EccOpCode::ADD_ACCUM); + EXPECT_EQ(builder.op_wire_1[2], EccOpCode::MUL_ACCUM); + EXPECT_EQ(builder.op_wire_1[4], EccOpCode::EQUALITY); + + // Check that we can reconstruct the coordinates of P1 from the op_wires + auto chunk_size = plonk::NUM_LIMB_BITS_IN_FIELD_SIMULATION * 2; + auto P1_x_lo = uint256_t(builder.variables[builder.op_wire_2[0]]); + auto P1_x_hi = uint256_t(builder.variables[builder.op_wire_3[0]]); + auto P1_x = P1_x_lo + (P1_x_hi << chunk_size); + EXPECT_EQ(P1_x, uint256_t(P1.x)); + auto P1_y_lo = uint256_t(builder.variables[builder.op_wire_4[0]]); + auto P1_y_hi = uint256_t(builder.variables[builder.op_wire_2[1]]); + auto P1_y = P1_y_lo + (P1_y_hi << chunk_size); + EXPECT_EQ(P1_y, uint256_t(P1.y)); + + // Check that we can reconstruct the coordinates of P2 from the op_wires + auto P2_x_lo = uint256_t(builder.variables[builder.op_wire_2[2]]); + auto P2_x_hi = uint256_t(builder.variables[builder.op_wire_3[2]]); + auto P2_x = P2_x_lo + (P2_x_hi << chunk_size); + EXPECT_EQ(P2_x, uint256_t(P2.x)); + auto P2_y_lo = uint256_t(builder.variables[builder.op_wire_4[2]]); + auto P2_y_hi = uint256_t(builder.variables[builder.op_wire_2[3]]); + auto P2_y = P2_y_lo + (P2_y_hi << chunk_size); + EXPECT_EQ(P2_y, uint256_t(P2.y)); + + // Check that we can reconstruct the coordinates of P_result from the op_wires + auto P_expected_x_lo = uint256_t(builder.variables[builder.op_wire_2[4]]); + auto P_expected_x_hi = uint256_t(builder.variables[builder.op_wire_3[4]]); + auto P_expected_x = P_expected_x_lo + (P_expected_x_hi << chunk_size); + EXPECT_EQ(P_expected_x, uint256_t(P_expected.x)); + auto P_expected_y_lo = uint256_t(builder.variables[builder.op_wire_4[4]]); + auto P_expected_y_hi = uint256_t(builder.variables[builder.op_wire_2[5]]); + auto P_expected_y = P_expected_y_lo + (P_expected_y_hi << chunk_size); + EXPECT_EQ(P_expected_y, uint256_t(P_expected.y)); } /** diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 84fd6adb9b..4b9a5f5555 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -4,6 +4,8 @@ namespace proof_system { +enum EccOpCode { ADD_ACCUM, MUL_ACCUM, EQUALITY, NULL_OP }; + /** * @brief Raw description of an ECC operation used to produce equivalent descriptions over different curves. */ From 09e33ba4988c90468960a28ed7748839b8873a39 Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Tue, 18 Jul 2023 20:04:36 +0000 Subject: [PATCH 8/9] Move goblin stuff to existing ultra builder --- .../goblin_ultra_circuit_builder.cpp | 145 ----------------- .../goblin_ultra_circuit_builder.hpp | 49 ------ .../goblin_ultra_circuit_builder.test.cpp | 61 +++----- .../circuit_builder/ultra_circuit_builder.cpp | 146 ++++++++++++++++++ .../circuit_builder/ultra_circuit_builder.hpp | 29 ++++ 5 files changed, 195 insertions(+), 235 deletions(-) delete mode 100644 cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp delete mode 100644 cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp deleted file mode 100644 index a0f808ee97..0000000000 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp +++ /dev/null @@ -1,145 +0,0 @@ -#include "goblin_ultra_circuit_builder.hpp" -#include -#include -#include - -using namespace barretenberg; - -namespace proof_system { - -/** - * @brief Add gates corresponding to a batched mul - * - * @param points - * @param scalars - * @return g1::affine_element Result of batched mul - */ -g1::affine_element GoblinUltraCircuitBuilder::batch_mul(const std::vector& points, - const std::vector& scalars) -{ - // TODO(luke): Do we necessarily want to check accum == 0? Other checks? - ASSERT(op_queue.get_accumulator().is_point_at_infinity()); - - size_t num_muls = points.size(); - for (size_t idx = 0; idx < num_muls; ++idx) { - queue_ecc_mul_accum(points[idx], scalars[idx]); - } - return op_queue.get_accumulator(); -} - -/** - * @brief Add gates for simple point addition without scalar and compute corresponding op natively - * - * @param point - */ -void GoblinUltraCircuitBuilder::queue_ecc_add_accum(const barretenberg::g1::affine_element& point) -{ - // Add raw op to queue - op_queue.add_accumulate(point); - - // Add ecc op gates - add_ecc_op_gates(EccOpCode::ADD_ACCUM, point); -} - -/** - * @brief Add gates for point mul and add and compute corresponding op natively - * - * @param point - * @param scalar - */ -void GoblinUltraCircuitBuilder::queue_ecc_mul_accum(const barretenberg::g1::affine_element& point, - const barretenberg::fr& scalar) -{ - // Add raw op to op queue - op_queue.mul_accumulate(point, scalar); - - // Add ecc op gates - add_ecc_op_gates(EccOpCode::MUL_ACCUM, point, scalar); -} - -/** - * @brief Add point equality gates - * - * @param point - */ -void GoblinUltraCircuitBuilder::queue_ecc_eq(const barretenberg::g1::affine_element& point) -{ - // Add raw op to op queue - op_queue.eq(point); - - // Add ecc op gates - add_ecc_op_gates(EccOpCode::EQUALITY, point); -} - -/** - * @brief Add ecc op gates given an op code and its operands - * - * @param op Op code - * @param point - * @param scalar - */ -void GoblinUltraCircuitBuilder::add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar) -{ - auto op_tuple = make_ecc_op_tuple(op, point, scalar); - - record_ecc_op(op_tuple); -} - -/** - * @brief Decompose ecc operands into components, add corresponding variables, return ecc op tuple - * - * @param op - * @param point - * @param scalar - * @return ecc_op_tuple Tuple of indices into variables array used to construct pair of ecc op gates - */ -ecc_op_tuple GoblinUltraCircuitBuilder::make_ecc_op_tuple(uint32_t op, - const g1::affine_element& point, - const fr& scalar) -{ - auto x_256 = uint256_t(point.x); - auto y_256 = uint256_t(point.y); - auto x_lo_idx = add_variable(x_256.slice(0, NUM_LIMB_BITS * 2)); - auto x_hi_idx = add_variable(x_256.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 4)); - auto y_lo_idx = add_variable(y_256.slice(0, NUM_LIMB_BITS * 2)); - auto y_hi_idx = add_variable(y_256.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 4)); - - // Split scalar into 128 bit endomorphism scalars - fr z_1 = 0; - fr z_2 = 0; - // TODO(luke): do this montgomery conversion here? - // auto converted = scalar.from_montgomery_form(); - // fr::split_into_endomorphism_scalars(converted, z_1, z_2); - // z_1 = z_1.to_montgomery_form(); - // z_2 = z_2.to_montgomery_form(); - fr::split_into_endomorphism_scalars(scalar, z_1, z_2); - auto z_1_idx = add_variable(z_1); - auto z_2_idx = add_variable(z_2); - - return { op, x_lo_idx, x_hi_idx, y_lo_idx, y_hi_idx, z_1_idx, z_2_idx }; -} - -/** - * @brief Add ecc operation to queue - * - * @param in Variables array indices corresponding to operation inputs - * @note We dont explicitly set values for the selectors here since their values are fully determined by - * num_ecc_op_gates. E.g. in the composer we can reconstruct q_ecc_op as the indicator on the first num_ecc_op_gates - * indices. All other selectors are simply 0 on this domain. - */ -void GoblinUltraCircuitBuilder::record_ecc_op(const ecc_op_tuple& in) -{ - op_wire_1.emplace_back(in.op); - op_wire_2.emplace_back(in.x_lo); - op_wire_3.emplace_back(in.x_hi); - op_wire_4.emplace_back(in.y_lo); - - op_wire_1.emplace_back(in.op); // TODO(luke): second op val is sort of a dummy. use "op" again? - op_wire_2.emplace_back(in.y_hi); - op_wire_3.emplace_back(in.z_lo); - op_wire_4.emplace_back(in.z_hi); - - num_ecc_op_gates += 2; -}; - -} // namespace proof_system \ No newline at end of file diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp deleted file mode 100644 index 7cce13cb1d..0000000000 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp +++ /dev/null @@ -1,49 +0,0 @@ -#pragma once -#include "barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp" -#include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" - -namespace proof_system { - -using namespace barretenberg; - -// TODO(luke): The Goblin builder requires all Ultra functionality plus a bit more. For now I've implented it as a -// standalone class that inherits from UltraBuilder. This is nice because it clearly separates out the "Goblin" -// functionlity, however, it is a break from our usual pattern. I may eventually simply incorporate the new -// functionality directly into the UltraBuilder. -class GoblinUltraCircuitBuilder : public UltraCircuitBuilder { - public: - static constexpr std::string_view NAME_STRING = "GoblinUltraArithmetization"; - static constexpr CircuitType CIRCUIT_TYPE = CircuitType::ULTRA; - - // Used for simulating big field; Equal to NUM_LIMB_BITS_IN_FIELD_SIMULATION - const size_t NUM_LIMB_BITS = 68; // TODO(luke): Set via NUM_LIMB_BITS_IN_FIELD_SIMULATION? - - // Stores record of ecc operations and performs corresponding native operations internally - ECCOpQueue op_queue; - - // number of ecc op "gates" (rows); these are placed at the start of the circuit - int num_ecc_op_gates = 0; - - // Wires storing ecc op queue data; values are indices into the variables array - std::array op_wires; - - WireVector& op_wire_1 = std::get<0>(op_wires); - WireVector& op_wire_2 = std::get<1>(op_wires); - WireVector& op_wire_3 = std::get<2>(op_wires); - WireVector& op_wire_4 = std::get<3>(op_wires); - - /** - * ECC operations - **/ - - void queue_ecc_add_accum(const g1::affine_element& point); - void queue_ecc_mul_accum(const g1::affine_element& point, const fr& scalar); - void queue_ecc_eq(const g1::affine_element& point); - g1::affine_element batch_mul(const std::vector& points, const std::vector& scalars); - - private: - void record_ecc_op(const ecc_op_tuple& in); - void add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); - ecc_op_tuple make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); -}; -} // namespace proof_system diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp index 54e00cca74..c9dbb98be4 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -1,5 +1,5 @@ #include "barretenberg/crypto/generators/generator_data.hpp" -#include "goblin_ultra_circuit_builder.hpp" +#include "ultra_circuit_builder.hpp" #include using namespace barretenberg; @@ -9,27 +9,6 @@ auto& engine = numeric::random::get_debug_engine(); } namespace proof_system { -/** - * @brief Simple add gate to check basic Ultra functionality - * - */ -TEST(GoblinCircuitBuilder, BaseCase) -{ - auto circuit_constructor = GoblinUltraCircuitBuilder(); - fr a = fr::random_element(); - fr b = fr::random_element(); - - auto a_idx = circuit_constructor.add_variable(a); - auto b_idx = circuit_constructor.add_variable(b); - auto c_idx = circuit_constructor.add_variable(a + b); - - circuit_constructor.create_add_gate({ a_idx, b_idx, c_idx, fr::one(), fr::one(), -fr::one(), fr::zero() }); - - circuit_constructor.get_num_gates(); - - EXPECT_EQ(circuit_constructor.check_circuit(), true); -} - /** * @brief Test the queueing of simple ecc ops via the Goblin builder * @details There are two things to check here: 1) When ecc ops are queued by the builder, the corresponding native @@ -37,9 +16,9 @@ TEST(GoblinCircuitBuilder, BaseCase) * encoded in the op_wires, i.e. the operands can be reconstructed as expected. * */ -TEST(GoblinCircuitBuilder, Simple) +TEST(GoblinUltraCircuitBuilder, Simple) { - auto builder = GoblinUltraCircuitBuilder(); + auto builder = UltraCircuitBuilder(); // Compute a simple point accumulation natively auto P1 = g1::affine_element::random_element(); @@ -66,38 +45,38 @@ TEST(GoblinCircuitBuilder, Simple) EXPECT_EQ(builder.num_ecc_op_gates, 6); // Check that the expected op codes have been correctly recorded in the 1st op wire - EXPECT_EQ(builder.op_wire_1[0], EccOpCode::ADD_ACCUM); - EXPECT_EQ(builder.op_wire_1[2], EccOpCode::MUL_ACCUM); - EXPECT_EQ(builder.op_wire_1[4], EccOpCode::EQUALITY); + EXPECT_EQ(builder.ecc_op_wire_1[0], EccOpCode::ADD_ACCUM); + EXPECT_EQ(builder.ecc_op_wire_1[2], EccOpCode::MUL_ACCUM); + EXPECT_EQ(builder.ecc_op_wire_1[4], EccOpCode::EQUALITY); // Check that we can reconstruct the coordinates of P1 from the op_wires auto chunk_size = plonk::NUM_LIMB_BITS_IN_FIELD_SIMULATION * 2; - auto P1_x_lo = uint256_t(builder.variables[builder.op_wire_2[0]]); - auto P1_x_hi = uint256_t(builder.variables[builder.op_wire_3[0]]); + auto P1_x_lo = uint256_t(builder.variables[builder.ecc_op_wire_2[0]]); + auto P1_x_hi = uint256_t(builder.variables[builder.ecc_op_wire_3[0]]); auto P1_x = P1_x_lo + (P1_x_hi << chunk_size); EXPECT_EQ(P1_x, uint256_t(P1.x)); - auto P1_y_lo = uint256_t(builder.variables[builder.op_wire_4[0]]); - auto P1_y_hi = uint256_t(builder.variables[builder.op_wire_2[1]]); + auto P1_y_lo = uint256_t(builder.variables[builder.ecc_op_wire_4[0]]); + auto P1_y_hi = uint256_t(builder.variables[builder.ecc_op_wire_2[1]]); auto P1_y = P1_y_lo + (P1_y_hi << chunk_size); EXPECT_EQ(P1_y, uint256_t(P1.y)); // Check that we can reconstruct the coordinates of P2 from the op_wires - auto P2_x_lo = uint256_t(builder.variables[builder.op_wire_2[2]]); - auto P2_x_hi = uint256_t(builder.variables[builder.op_wire_3[2]]); + auto P2_x_lo = uint256_t(builder.variables[builder.ecc_op_wire_2[2]]); + auto P2_x_hi = uint256_t(builder.variables[builder.ecc_op_wire_3[2]]); auto P2_x = P2_x_lo + (P2_x_hi << chunk_size); EXPECT_EQ(P2_x, uint256_t(P2.x)); - auto P2_y_lo = uint256_t(builder.variables[builder.op_wire_4[2]]); - auto P2_y_hi = uint256_t(builder.variables[builder.op_wire_2[3]]); + auto P2_y_lo = uint256_t(builder.variables[builder.ecc_op_wire_4[2]]); + auto P2_y_hi = uint256_t(builder.variables[builder.ecc_op_wire_2[3]]); auto P2_y = P2_y_lo + (P2_y_hi << chunk_size); EXPECT_EQ(P2_y, uint256_t(P2.y)); // Check that we can reconstruct the coordinates of P_result from the op_wires - auto P_expected_x_lo = uint256_t(builder.variables[builder.op_wire_2[4]]); - auto P_expected_x_hi = uint256_t(builder.variables[builder.op_wire_3[4]]); + auto P_expected_x_lo = uint256_t(builder.variables[builder.ecc_op_wire_2[4]]); + auto P_expected_x_hi = uint256_t(builder.variables[builder.ecc_op_wire_3[4]]); auto P_expected_x = P_expected_x_lo + (P_expected_x_hi << chunk_size); EXPECT_EQ(P_expected_x, uint256_t(P_expected.x)); - auto P_expected_y_lo = uint256_t(builder.variables[builder.op_wire_4[4]]); - auto P_expected_y_hi = uint256_t(builder.variables[builder.op_wire_2[5]]); + auto P_expected_y_lo = uint256_t(builder.variables[builder.ecc_op_wire_4[4]]); + auto P_expected_y_hi = uint256_t(builder.variables[builder.ecc_op_wire_2[5]]); auto P_expected_y = P_expected_y_lo + (P_expected_y_hi << chunk_size); EXPECT_EQ(P_expected_y, uint256_t(P_expected.y)); } @@ -106,12 +85,12 @@ TEST(GoblinCircuitBuilder, Simple) * @brief Test correctness of native ecc batch mul performed behind the scenes when adding ecc op gates for a batch mul * */ -TEST(GoblinCircuitBuilder, BatchMul) +TEST(GoblinUltraCircuitBuilder, BatchMul) { using Point = g1::affine_element; using Scalar = fr; - auto builder = GoblinUltraCircuitBuilder(); + auto builder = UltraCircuitBuilder(); const size_t num_muls = 3; // Compute some random points and scalars to batch multiply diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp index b8c65a780d..5277b7ed60 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp @@ -496,6 +496,152 @@ template uint32_t UltraCircuitBuilder_::put_constant_variable( } } +/** + * ** Goblin Methods ** + */ + +/** + * @brief Add gates corresponding to a batched mul + * + * @param points + * @param scalars + * @return g1::affine_element Result of batched mul + */ +template +g1::affine_element UltraCircuitBuilder_::batch_mul(const std::vector& points, + const std::vector& scalars) +{ + // TODO(luke): Do we necessarily want to check accum == 0? Other checks? + ASSERT(op_queue.get_accumulator().is_point_at_infinity()); + + size_t num_muls = points.size(); + for (size_t idx = 0; idx < num_muls; ++idx) { + queue_ecc_mul_accum(points[idx], scalars[idx]); + } + return op_queue.get_accumulator(); +} + +/** + * @brief Add gates for simple point addition without scalar and compute corresponding op natively + * + * @param point + */ +template void UltraCircuitBuilder_::queue_ecc_add_accum(const barretenberg::g1::affine_element& point) +{ + // Add raw op to queue + op_queue.add_accumulate(point); + + // Add ecc op gates + add_ecc_op_gates(EccOpCode::ADD_ACCUM, point); +} + +/** + * @brief Add gates for point mul and add and compute corresponding op natively + * + * @param point + * @param scalar + */ +template +void UltraCircuitBuilder_::queue_ecc_mul_accum(const barretenberg::g1::affine_element& point, + const barretenberg::fr& scalar) +{ + // Add raw op to op queue + op_queue.mul_accumulate(point, scalar); + + // Add ecc op gates + add_ecc_op_gates(EccOpCode::MUL_ACCUM, point, scalar); +} + +/** + * @brief Add point equality gates + * + * @param point + */ +template void UltraCircuitBuilder_::queue_ecc_eq(const barretenberg::g1::affine_element& point) +{ + // Add raw op to op queue + op_queue.eq(point); + + // Add ecc op gates + add_ecc_op_gates(EccOpCode::EQUALITY, point); +} + +/** + * @brief Add ecc op gates given an op code and its operands + * + * @param op Op code + * @param point + * @param scalar + */ +template +void UltraCircuitBuilder_::add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar) +{ + auto op_tuple = make_ecc_op_tuple(op, point, scalar); + + record_ecc_op(op_tuple); +} + +/** + * @brief Decompose ecc operands into components, add corresponding variables, return ecc op tuple + * + * @param op + * @param point + * @param scalar + * @return ecc_op_tuple Tuple of indices into variables array used to construct pair of ecc op gates + */ +template +ecc_op_tuple UltraCircuitBuilder_::make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar) +{ + const size_t CHUNK_SIZE = 2 * DEFAULT_NON_NATIVE_FIELD_LIMB_BITS; + auto x_256 = uint256_t(point.x); + auto y_256 = uint256_t(point.y); + auto x_lo_idx = this->add_variable(x_256.slice(0, CHUNK_SIZE)); + auto x_hi_idx = this->add_variable(x_256.slice(CHUNK_SIZE, CHUNK_SIZE * 2)); + auto y_lo_idx = this->add_variable(y_256.slice(0, CHUNK_SIZE)); + auto y_hi_idx = this->add_variable(y_256.slice(CHUNK_SIZE, CHUNK_SIZE * 2)); + + // Split scalar into 128 bit endomorphism scalars + fr z_1 = 0; + fr z_2 = 0; + // TODO(luke): do this montgomery conversion here? + // auto converted = scalar.from_montgomery_form(); + // fr::split_into_endomorphism_scalars(converted, z_1, z_2); + // z_1 = z_1.to_montgomery_form(); + // z_2 = z_2.to_montgomery_form(); + fr::split_into_endomorphism_scalars(scalar, z_1, z_2); + auto z_1_idx = this->add_variable(z_1); + auto z_2_idx = this->add_variable(z_2); + + return { op, x_lo_idx, x_hi_idx, y_lo_idx, y_hi_idx, z_1_idx, z_2_idx }; +} + +/** + * @brief Add ecc operation to queue + * + * @param in Variables array indices corresponding to operation inputs + * @note We dont explicitly set values for the selectors here since their values are fully determined by + * num_ecc_op_gates. E.g. in the composer we can reconstruct q_ecc_op as the indicator on the first num_ecc_op_gates + * indices. All other selectors are simply 0 on this domain. + */ +template void UltraCircuitBuilder_::record_ecc_op(const ecc_op_tuple& in) +{ + ecc_op_wire_1.emplace_back(in.op); + ecc_op_wire_2.emplace_back(in.x_lo); + ecc_op_wire_3.emplace_back(in.x_hi); + ecc_op_wire_4.emplace_back(in.y_lo); + + ecc_op_wire_1.emplace_back(in.op); // TODO(luke): second op val is sort of a dummy. use "op" again? + ecc_op_wire_2.emplace_back(in.y_hi); + ecc_op_wire_3.emplace_back(in.z_lo); + ecc_op_wire_4.emplace_back(in.z_hi); + + num_ecc_op_gates += 2; +}; + +/** + * End of Goblin Methods + */ + template plookup::BasicTable& UltraCircuitBuilder_::get_table(const plookup::BasicTableId id) { for (plookup::BasicTable& table : lookup_tables) { diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp index 5fd3e06da1..602b6952b7 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp @@ -4,6 +4,7 @@ #include "barretenberg/plonk/proof_system/types/prover_settings.hpp" #include "barretenberg/polynomials/polynomial.hpp" #include "barretenberg/proof_system/arithmetization/arithmetization.hpp" +#include "barretenberg/proof_system/op_queue/ecc_op_queue.hpp" #include "barretenberg/proof_system/plookup_tables/plookup_tables.hpp" #include "barretenberg/proof_system/plookup_tables/types.hpp" #include "barretenberg/proof_system/types/merkle_hash_type.hpp" @@ -35,6 +36,12 @@ template class UltraCircuitBuilder_ : public CircuitBuilderBase class UltraCircuitBuilder_ : public CircuitBuilderBase(this->wires); WireVector& w_4 = std::get<3>(this->wires); + // Wires storing ecc op queue data; values are indices into the variables array + std::array::NUM_WIRES> ecc_op_wires; + + WireVector& ecc_op_wire_1 = std::get<0>(ecc_op_wires); + WireVector& ecc_op_wire_2 = std::get<1>(ecc_op_wires); + WireVector& ecc_op_wire_3 = std::get<2>(ecc_op_wires); + WireVector& ecc_op_wire_4 = std::get<3>(ecc_op_wires); + SelectorVector& q_m = this->selectors.q_m; SelectorVector& q_c = this->selectors.q_c; SelectorVector& q_1 = this->selectors.q_1; @@ -688,6 +703,20 @@ template class UltraCircuitBuilder_ : public CircuitBuilderBase& points, const std::vector& scalars); + + private: + void record_ecc_op(const ecc_op_tuple& in); + void add_ecc_op_gates(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); + ecc_op_tuple make_ecc_op_tuple(uint32_t op, const g1::affine_element& point, const fr& scalar = fr::zero()); + + public: size_t get_num_constant_gates() const override { return 0; } /** From 016aea53f1aed2319e5e90ceef1b99a004fcf528 Mon Sep 17 00:00:00 2001 From: ledwards2225 Date: Wed, 19 Jul 2023 15:00:25 +0000 Subject: [PATCH 9/9] implicitly use internal accumulator in equality op --- .../goblin_ultra_circuit_builder.test.cpp | 13 ++++++------- .../circuit_builder/ultra_circuit_builder.cpp | 8 +++++--- .../circuit_builder/ultra_circuit_builder.hpp | 2 +- .../proof_system/op_queue/ecc_op_queue.hpp | 11 +++++------ .../proof_system/op_queue/ecc_op_queue.test.cpp | 6 +++--- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp index c9dbb98be4..6e229f7ba6 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.test.cpp @@ -16,7 +16,7 @@ namespace proof_system { * encoded in the op_wires, i.e. the operands can be reconstructed as expected. * */ -TEST(GoblinUltraCircuitBuilder, Simple) +TEST(UltraCircuitBuilder, GoblinSimple) { auto builder = UltraCircuitBuilder(); @@ -30,12 +30,11 @@ TEST(GoblinUltraCircuitBuilder, Simple) builder.queue_ecc_add_accum(P1); builder.queue_ecc_mul_accum(P2, z); - // Extract current accumulator point from the op queue and check the result - auto P_result = builder.op_queue.get_accumulator(); - EXPECT_EQ(P_result, P_expected); + // Add equality op gates based on the internal accumulator + auto P_result = builder.queue_ecc_eq(); - // Use the result to add equality op gates - builder.queue_ecc_eq(P_result); + // Check that value returned from internal accumulator is correct + EXPECT_EQ(P_result, P_expected); // Check that the accumulator in the op queue has been reset to 0 auto accumulator = builder.op_queue.get_accumulator(); @@ -85,7 +84,7 @@ TEST(GoblinUltraCircuitBuilder, Simple) * @brief Test correctness of native ecc batch mul performed behind the scenes when adding ecc op gates for a batch mul * */ -TEST(GoblinUltraCircuitBuilder, BatchMul) +TEST(UltraCircuitBuilder, GoblinBatchMul) { using Point = g1::affine_element; using Scalar = fr; diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp index 5277b7ed60..0273773a2f 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.cpp @@ -555,15 +555,17 @@ void UltraCircuitBuilder_::queue_ecc_mul_accum(const barretenberg::g1::affin /** * @brief Add point equality gates * - * @param point + * @return point to which equality has been asserted */ -template void UltraCircuitBuilder_::queue_ecc_eq(const barretenberg::g1::affine_element& point) +template barretenberg::g1::affine_element UltraCircuitBuilder_::queue_ecc_eq() { // Add raw op to op queue - op_queue.eq(point); + auto point = op_queue.eq(); // Add ecc op gates add_ecc_op_gates(EccOpCode::EQUALITY, point); + + return point; } /** diff --git a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp index 602b6952b7..29220028f6 100644 --- a/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp +++ b/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp @@ -708,7 +708,7 @@ template class UltraCircuitBuilder_ : public CircuitBuilderBase& points, const std::vector& scalars); private: diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 4b9a5f5555..a5c582e602 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -113,14 +113,13 @@ class ECCOpQueue { } /** - * @brief Write equality op to queue and check equality with native accumulator + * @brief Write equality op using internal accumulator point * - * @param expected - * @return whether accumulator point == expected point + * @return current internal accumulator point (prior to reset to 0) */ - bool eq(const Point& expected) + Point eq() { - bool accumulator_equals_expected = (accumulator == expected); + auto expected = accumulator; accumulator.self_set_infinity(); // TODO(luke): is this always desired? raw_ops.emplace_back(ECCOp{ @@ -134,7 +133,7 @@ class ECCOpQueue { .mul_scalar_full = 0, }); - return accumulator_equals_expected; + return expected; } /** diff --git a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp index 96b693355f..2b41923ecd 100644 --- a/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp +++ b/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.test.cpp @@ -11,7 +11,7 @@ TEST(ECCOpQueueTest, Basic) EXPECT_EQ(op_queue.raw_ops[1].add, false); } -TEST(ECCOpQueueTest, Accumulator) +TEST(ECCOpQueueTest, InternalAccumulatorCorrectness) { using point = barretenberg::g1::affine_element; using scalar = barretenberg::fr; @@ -30,8 +30,8 @@ TEST(ECCOpQueueTest, Accumulator) // The correct result should now be stored in the accumulator within the op queue EXPECT_EQ(op_queue.get_accumulator(), P_expected); - // Equivalently, we can check that the equality op returns true - EXPECT_TRUE(op_queue.eq(P_expected)); + // Equivalently, we can check that the equality op returns the correct point + EXPECT_EQ(op_queue.eq(), P_expected); // Adding an equality op should reset the accumulator to zero (the point at infinity) EXPECT_TRUE(op_queue.get_accumulator().is_point_at_infinity());