Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Moves add gate out of aux #8541

Merged
merged 8 commits into from
Sep 16, 2024
Merged

chore: Moves add gate out of aux #8541

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d8187d0
fix and add test
lucasxia01 Sep 13, 2024
859b300
actual fix oops
lucasxia01 Sep 13, 2024
5c2a1d5
add test to mega circuit builders that checks that the sorting is a c…
lucasxia01 Sep 13, 2024
87db6e2
updated aztec ivc bench structuring to account for new arithmetic gat…
lucasxia01 Sep 13, 2024
1727de5
fix test
lucasxia01 Sep 13, 2024
4f0421f
Merge branch 'master' into lx/move-add-gate-out-of-aux
codygunton Sep 16, 2024
a4b80bb
clean up and run all CI
lucasxia01 Sep 16, 2024
6165ae0
fix test
lucasxia01 Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 94 additions & 24 deletions barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace bb {
using plookup::ColumnIdx;
using plookup::MultiTableId;

TEST(ultra_circuit_constructor, copy_constructor)
TEST(UltraCircuitConstructor, CopyConstructor)
Copy link
Contributor Author

@lucasxia01 lucasxia01 Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was testing out ai editor to do these formatting changes

{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand Down Expand Up @@ -43,7 +43,7 @@ TEST(ultra_circuit_constructor, copy_constructor)
EXPECT_TRUE(CircuitChecker::check(duplicate_circuit_constructor));
}

TEST(ultra_circuit_constructor, create_gates_from_plookup_accumulators)
TEST(UltraCircuitConstructor, CreateGatesFromPlookupAccumulators)
{

UltraCircuitBuilder circuit_builder = UltraCircuitBuilder();
Expand Down Expand Up @@ -105,7 +105,7 @@ TEST(ultra_circuit_constructor, create_gates_from_plookup_accumulators)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, bad_lookup_failure)
TEST(UltraCircuitConstructor, BadLookupFailure)
{
UltraCircuitBuilder builder;
MockCircuits::add_lookup_gates(builder);
Expand All @@ -121,15 +121,15 @@ TEST(ultra_circuit_constructor, bad_lookup_failure)
EXPECT_FALSE(CircuitChecker::check(builder));
}

TEST(ultra_circuit_constructor, base_case)
TEST(UltraCircuitConstructor, BaseCase)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::one();
circuit_constructor.add_public_variable(a);
bool result = CircuitChecker::check(circuit_constructor);
EXPECT_EQ(result, true);
}
TEST(ultra_circuit_constructor, test_no_lookup_proof)
TEST(UltraCircuitConstructor, TestNoLookupProof)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand All @@ -152,7 +152,7 @@ TEST(ultra_circuit_constructor, test_no_lookup_proof)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, test_elliptic_gate)
TEST(UltraCircuitConstructor, TestEllipticGate)
{
typedef grumpkin::g1::affine_element affine_element;
typedef grumpkin::g1::element element;
Expand Down Expand Up @@ -180,7 +180,7 @@ TEST(ultra_circuit_constructor, test_elliptic_gate)
EXPECT_EQ(CircuitChecker::check(circuit_constructor), false);
}

TEST(ultra_circuit_constructor, test_elliptic_double_gate)
TEST(UltraCircuitConstructor, TestEllipticDoubleGate)
{
typedef grumpkin::g1::affine_element affine_element;
typedef grumpkin::g1::element element;
Expand All @@ -200,7 +200,7 @@ TEST(ultra_circuit_constructor, test_elliptic_double_gate)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, non_trivial_tag_permutation)
TEST(UltraCircuitConstructor, NonTrivialTagPermutation)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::random_element();
Expand Down Expand Up @@ -232,7 +232,7 @@ TEST(ultra_circuit_constructor, non_trivial_tag_permutation)
EXPECT_EQ(CircuitChecker::check(circuit_constructor), false);
}

TEST(ultra_circuit_constructor, non_trivial_tag_permutation_and_cycles)
TEST(UltraCircuitConstructor, NonTrivialTagPermutationAndCycles)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::random_element();
Expand Down Expand Up @@ -273,7 +273,7 @@ TEST(ultra_circuit_constructor, non_trivial_tag_permutation_and_cycles)
circuit_constructor.real_variable_tags[circuit_constructor.real_variable_index[a_idx]] = 2;
EXPECT_EQ(CircuitChecker::check(circuit_constructor), false);
}
TEST(ultra_circuit_constructor, bad_tag_permutation)
TEST(UltraCircuitConstructor, BadTagPermutation)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::random_element();
Expand Down Expand Up @@ -302,7 +302,7 @@ TEST(ultra_circuit_constructor, bad_tag_permutation)
EXPECT_EQ(result, false);
}

TEST(ultra_circuit_constructor, sort_widget)
TEST(UltraCircuitConstructor, SortWidget)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::one();
Expand All @@ -328,7 +328,7 @@ std::vector<uint32_t> add_variables(UltraCircuitBuilder& circuit_constructor, st
}
return res;
}
TEST(ultra_circuit_constructor, sort_with_edges_gate)
TEST(UltraCircuitConstructor, SortWithEdgesGate)
{
fr a = fr::one();
fr b = fr(2);
Expand Down Expand Up @@ -421,7 +421,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate)
}
}

TEST(ultra_circuit_constructor, range_constraint)
TEST(UltraCircuitConstructor, RangeConstraint)
{
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
Expand Down Expand Up @@ -490,7 +490,7 @@ TEST(ultra_circuit_constructor, range_constraint)
}
}

TEST(ultra_circuit_constructor, range_with_gates)
TEST(UltraCircuitConstructor, RangeWithGates)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
auto idx = add_variables(circuit_constructor, { 1, 2, 3, 4, 5, 6, 7, 8 });
Expand All @@ -510,7 +510,7 @@ TEST(ultra_circuit_constructor, range_with_gates)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, range_with_gates_where_range_is_not_a_power_of_two)
TEST(UltraCircuitConstructor, RangeWithGatesWhereRangeIsNotAPowerOfTwo)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
auto idx = add_variables(circuit_constructor, { 1, 2, 3, 4, 5, 6, 7, 8 });
Expand All @@ -530,7 +530,7 @@ TEST(ultra_circuit_constructor, range_with_gates_where_range_is_not_a_power_of_t
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, sort_widget_complex)
TEST(UltraCircuitConstructor, SortWidgetComplex)
{
{

Expand All @@ -557,7 +557,7 @@ TEST(ultra_circuit_constructor, sort_widget_complex)
EXPECT_EQ(result, false);
}
}
TEST(ultra_circuit_constructor, sort_widget_neg)
TEST(UltraCircuitConstructor, SortWidgetNeg)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
fr a = fr::one();
Expand All @@ -575,7 +575,7 @@ TEST(ultra_circuit_constructor, sort_widget_neg)
EXPECT_EQ(result, false);
}

TEST(ultra_circuit_constructor, composed_range_constraint)
TEST(UltraCircuitConstructor, ComposedRangeConstraint)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
auto c = fr::random_element();
Expand All @@ -590,7 +590,7 @@ TEST(ultra_circuit_constructor, composed_range_constraint)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, non_native_field_multiplication)
TEST(UltraCircuitConstructor, NonNativeFieldMultiplication)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand Down Expand Up @@ -646,7 +646,77 @@ TEST(ultra_circuit_constructor, non_native_field_multiplication)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, rom)
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test copies the previous test and uses the function that previously, but now doesn't, added a gate with q_arith turned on in the aux block. It checks that the aux block does not have any other selector turned on besides the aux one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have this, but you're only checking one block.

* @brief Test that each block only contains gates which have the corresponding selector turned on.
*
*/
TEST(UltraCircuitConstructor, NonNativeFieldMultiplicationSortCheck)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

fq a = fq::random_element();
fq b = fq::random_element();
uint256_t modulus = fq::modulus;

uint1024_t a_big = uint512_t(uint256_t(a));
uint1024_t b_big = uint512_t(uint256_t(b));
uint1024_t p_big = uint512_t(uint256_t(modulus));

uint1024_t q_big = (a_big * b_big) / p_big;
uint1024_t r_big = (a_big * b_big) % p_big;

uint256_t q(q_big.lo.lo);
uint256_t r(r_big.lo.lo);

const auto split_into_limbs = [&](const uint512_t& input) {
constexpr size_t NUM_BITS = 68;
std::array<fr, 5> limbs;
limbs[0] = input.slice(0, NUM_BITS).lo;
limbs[1] = input.slice(NUM_BITS * 1, NUM_BITS * 2).lo;
limbs[2] = input.slice(NUM_BITS * 2, NUM_BITS * 3).lo;
limbs[3] = input.slice(NUM_BITS * 3, NUM_BITS * 4).lo;
limbs[4] = fr(input.lo);
return limbs;
};

const auto get_limb_witness_indices = [&](const std::array<fr, 5>& limbs) {
std::array<uint32_t, 5> limb_indices;
limb_indices[0] = circuit_constructor.add_variable(limbs[0]);
limb_indices[1] = circuit_constructor.add_variable(limbs[1]);
limb_indices[2] = circuit_constructor.add_variable(limbs[2]);
limb_indices[3] = circuit_constructor.add_variable(limbs[3]);
limb_indices[4] = circuit_constructor.add_variable(limbs[4]);
return limb_indices;
};
const uint512_t BINARY_BASIS_MODULUS = uint512_t(1) << (68 * 4);
auto modulus_limbs = split_into_limbs(BINARY_BASIS_MODULUS - uint512_t(modulus));

const auto a_indices = get_limb_witness_indices(split_into_limbs(uint256_t(a)));
const auto b_indices = get_limb_witness_indices(split_into_limbs(uint256_t(b)));
const auto q_indices = get_limb_witness_indices(split_into_limbs(uint256_t(q)));
const auto r_indices = get_limb_witness_indices(split_into_limbs(uint256_t(r)));

non_native_field_witnesses<fr> inputs{
a_indices, b_indices, q_indices, r_indices, modulus_limbs, fr(uint256_t(modulus)),
};
const auto [lo_1_idx, hi_1_idx] = circuit_constructor.evaluate_non_native_field_multiplication(inputs);
circuit_constructor.range_constrain_two_limbs(lo_1_idx, hi_1_idx, 70, 70);

bool result = CircuitChecker::check(circuit_constructor);
EXPECT_EQ(result, true);

// Check that in the aux blocks, only the aux selector is nonzero, while the other selectors are zero
for (size_t i = 0; i < circuit_constructor.blocks.aux.size(); ++i) {
EXPECT_EQ(circuit_constructor.blocks.aux.q_arith()[i], 0);
EXPECT_EQ(circuit_constructor.blocks.aux.q_delta_range()[i], 0);
EXPECT_EQ(circuit_constructor.blocks.aux.q_elliptic()[i], 0);
EXPECT_EQ(circuit_constructor.blocks.aux.q_lookup_type()[i], 0);
EXPECT_EQ(circuit_constructor.blocks.aux.q_poseidon2_external()[i], 0);
EXPECT_EQ(circuit_constructor.blocks.aux.q_poseidon2_internal()[i], 0);
}
}

TEST(UltraCircuitConstructor, Rom)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand Down Expand Up @@ -692,7 +762,7 @@ TEST(ultra_circuit_constructor, rom)
* @brief A simple-as-possible RAM read test, for easier debugging
*
*/
TEST(ultra_circuit_constructor, ram_simple)
TEST(UltraCircuitConstructor, RamSimple)
{
UltraCircuitBuilder builder;

Expand Down Expand Up @@ -722,7 +792,7 @@ TEST(ultra_circuit_constructor, ram_simple)
EXPECT_TRUE(CircuitChecker::check(builder));
}

TEST(ultra_circuit_constructor, ram)
TEST(UltraCircuitConstructor, Ram)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand Down Expand Up @@ -793,7 +863,7 @@ TEST(ultra_circuit_constructor, ram)
EXPECT_TRUE(CircuitChecker::check(duplicate_circuit_constructor));
}

TEST(ultra_circuit_constructor, range_checks_on_duplicates)
TEST(UltraCircuitConstructor, RangeChecksOnDuplicates)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();

Expand Down Expand Up @@ -828,7 +898,7 @@ TEST(ultra_circuit_constructor, range_checks_on_duplicates)
EXPECT_EQ(result, true);
}

TEST(ultra_circuit_constructor, check_circuit_showcase)
TEST(UltraCircuitConstructor, CheckCircuitShowcase)
{
UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder();
// check_circuit allows us to check correctness on the go
Expand Down
45 changes: 11 additions & 34 deletions barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_circuit_builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1717,45 +1717,22 @@ std::array<uint32_t, 2> UltraCircuitBuilder_<Arithmetization>::evaluate_non_nati
range_constrain_two_limbs(input.q[2], input.q[3]);
}

// TODO(https://github.com/AztecProtocol/barretenberg/issues/913): Remove this arithmetic gate from the aux block
// and replace with the big_add + dummy below.
this->assert_valid_variables({ input.q[0], input.q[1], input.r[1], lo_1_idx });
blocks.aux.populate_wires(input.q[0], input.q[1], input.r[1], lo_1_idx);
blocks.aux.q_m().emplace_back(0);
blocks.aux.q_1().emplace_back(input.neg_modulus[0] + input.neg_modulus[1] * LIMB_SHIFT);
blocks.aux.q_2().emplace_back(input.neg_modulus[0] * LIMB_SHIFT);
blocks.aux.q_3().emplace_back(-LIMB_SHIFT);
blocks.aux.q_c().emplace_back(0);
blocks.aux.q_arith().emplace_back(2);
blocks.aux.q_4().emplace_back(-LIMB_SHIFT.sqr());
blocks.aux.q_delta_range().emplace_back(0);
blocks.aux.q_lookup_type().emplace_back(0);
blocks.aux.q_elliptic().emplace_back(0);
blocks.aux.q_aux().emplace_back(0);
blocks.aux.q_poseidon2_external().emplace_back(0);
blocks.aux.q_poseidon2_internal().emplace_back(0);
if constexpr (HasAdditionalSelectors<Arithmetization>) {
blocks.aux.pad_additional();
}
check_selector_length_consistency();
++this->num_gates;

// TODO(https://github.com/AztecProtocol/barretenberg/issues/879): Originally this was a single arithmetic gate.
// With trace sorting, we must add a dummy gate since the add gate would otherwise try to read into an aux gate that
// has been sorted out of sequence. (Note: temporarily disabled in favor of manual arith gate in aux block above).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this comment be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, great catch!

// product gate 1
// (lo_0 + q_0(p_0 + p_1*2^b) + q_1(p_0*2^b) - (r_1)2^b)2^-2b - lo_1 = 0
// create_big_add_gate({ input.q[0],
// input.q[1],
// input.r[1],
// lo_1_idx,
// input.neg_modulus[0] + input.neg_modulus[1] * LIMB_SHIFT,
// input.neg_modulus[0] * LIMB_SHIFT,
// -LIMB_SHIFT,
// -LIMB_SHIFT.sqr(),
// 0 },
// true);
// create_dummy_gate(blocks.arithmetic, this->zero_idx, this->zero_idx, this->zero_idx, lo_0_idx);
create_big_add_gate({ input.q[0],
input.q[1],
input.r[1],
lo_1_idx,
input.neg_modulus[0] + input.neg_modulus[1] * LIMB_SHIFT,
input.neg_modulus[0] * LIMB_SHIFT,
-LIMB_SHIFT,
-LIMB_SHIFT.sqr(),
0 },
true);
create_dummy_gate(blocks.arithmetic, this->zero_idx, this->zero_idx, this->zero_idx, lo_0_idx);

blocks.aux.populate_wires(input.a[1], input.b[1], input.r[0], lo_0_idx);
apply_aux_selectors(AUX_SELECTORS::NON_NATIVE_FIELD_1);
Expand Down
Loading